| Message ID | 1473768687-27384-1-git-send-email-filipe.manco@neclab.eu |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
CC xen-devel as well. On Tue, Sep 13, 2016 at 02:11:27PM +0200, Filipe Manco wrote: > In case of error during netback_probe() (e.g. an entry missing on the > xenstore) netback_remove() is called on the new device, which will set > the device backend state to XenbusStateClosed by calling > set_backend_state(). However, the backend state wasn't initialized by > netback_probe() at this point, which will cause and invalid transaction > and set_backend_state() to BUG(). > > Initialize the backend state at the beginning of netback_probe() to > XenbusStateInitialising, and create a new valid state transaction on > set_backend_state(), from XenbusStateInitialising to XenbusStateClosed. > > Signed-off-by: Filipe Manco <filipe.manco@neclab.eu> There is a state machine right before set_backend_state. You would also need to update that. According to the definition of XenbusStateInitialising, this patch looks plausible to me. Wei. > --- > drivers/net/xen-netback/xenbus.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c > index 6a31f2610c23..c0e5f6994d01 100644 > --- a/drivers/net/xen-netback/xenbus.c > +++ b/drivers/net/xen-netback/xenbus.c > @@ -270,6 +270,7 @@ static int netback_probe(struct xenbus_device *dev, > > be->dev = dev; > dev_set_drvdata(&dev->dev, be); > + be->state = XenbusStateInitialising; > > sg = 1; > > @@ -515,6 +516,15 @@ static void set_backend_state(struct backend_info *be, > { > while (be->state != state) { > switch (be->state) { > + case XenbusStateInitialising: > + switch (state) { > + case XenbusStateClosed: > + backend_switch_state(be, XenbusStateClosed); > + break; > + default: > + BUG(); > + } > + break; > case XenbusStateClosed: > switch (state) { > case XenbusStateInitWait: > -- > 2.7.4 >
On 14-09-2016 12:10, Wei Liu wrote: > CC xen-devel as well. > > On Tue, Sep 13, 2016 at 02:11:27PM +0200, Filipe Manco wrote: >> In case of error during netback_probe() (e.g. an entry missing on the >> xenstore) netback_remove() is called on the new device, which will set >> the device backend state to XenbusStateClosed by calling >> set_backend_state(). However, the backend state wasn't initialized by >> netback_probe() at this point, which will cause and invalid transaction >> and set_backend_state() to BUG(). >> >> Initialize the backend state at the beginning of netback_probe() to >> XenbusStateInitialising, and create a new valid state transaction on >> set_backend_state(), from XenbusStateInitialising to XenbusStateClosed. >> >> Signed-off-by: Filipe Manco <filipe.manco@neclab.eu> > There is a state machine right before set_backend_state. You would also > need to update that. Good point I'll update the diagram. After looking at the diagram and for consistency, shouldn't the transition Initialising -> InitWait be handled using set_backend_state()? Currently it is done directly in netback_probe() code. If you agree I'll submit a v2 with these two changes. > According to the definition of XenbusStateInitialising, this patch looks > plausible to me. > > Wei. > Filipe >> --- >> drivers/net/xen-netback/xenbus.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c >> index 6a31f2610c23..c0e5f6994d01 100644 >> --- a/drivers/net/xen-netback/xenbus.c >> +++ b/drivers/net/xen-netback/xenbus.c >> @@ -270,6 +270,7 @@ static int netback_probe(struct xenbus_device *dev, >> >> be->dev = dev; >> dev_set_drvdata(&dev->dev, be); >> + be->state = XenbusStateInitialising; >> >> sg = 1; >> >> @@ -515,6 +516,15 @@ static void set_backend_state(struct backend_info *be, >> { >> while (be->state != state) { >> switch (be->state) { >> + case XenbusStateInitialising: >> + switch (state) { >> + case XenbusStateClosed: >> + backend_switch_state(be, XenbusStateClosed); >> + break; >> + default: >> + BUG(); >> + } >> + break; >> case XenbusStateClosed: >> switch (state) { >> case XenbusStateInitWait: >> -- >> 2.7.4 >> > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
On Thu, Sep 15, 2016 at 04:05:17PM +0200, Filipe Manco wrote: > On 14-09-2016 12:10, Wei Liu wrote: > >CC xen-devel as well. > > > >On Tue, Sep 13, 2016 at 02:11:27PM +0200, Filipe Manco wrote: > >>In case of error during netback_probe() (e.g. an entry missing on the > >>xenstore) netback_remove() is called on the new device, which will set > >>the device backend state to XenbusStateClosed by calling > >>set_backend_state(). However, the backend state wasn't initialized by > >>netback_probe() at this point, which will cause and invalid transaction > >>and set_backend_state() to BUG(). > >> > >>Initialize the backend state at the beginning of netback_probe() to > >>XenbusStateInitialising, and create a new valid state transaction on > >>set_backend_state(), from XenbusStateInitialising to XenbusStateClosed. > >> > >>Signed-off-by: Filipe Manco <filipe.manco@neclab.eu> > >There is a state machine right before set_backend_state. You would also > >need to update that. > Good point I'll update the diagram. > > After looking at the diagram and for consistency, shouldn't the transition > Initialising -> InitWait be handled using set_backend_state()? Currently it > is done directly in netback_probe() code. If you agree I'll submit a v2 with > these two changes. That's fine with me. Wei.
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c index 6a31f2610c23..c0e5f6994d01 100644 --- a/drivers/net/xen-netback/xenbus.c +++ b/drivers/net/xen-netback/xenbus.c @@ -270,6 +270,7 @@ static int netback_probe(struct xenbus_device *dev, be->dev = dev; dev_set_drvdata(&dev->dev, be); + be->state = XenbusStateInitialising; sg = 1; @@ -515,6 +516,15 @@ static void set_backend_state(struct backend_info *be, { while (be->state != state) { switch (be->state) { + case XenbusStateInitialising: + switch (state) { + case XenbusStateClosed: + backend_switch_state(be, XenbusStateClosed); + break; + default: + BUG(); + } + break; case XenbusStateClosed: switch (state) { case XenbusStateInitWait:
In case of error during netback_probe() (e.g. an entry missing on the xenstore) netback_remove() is called on the new device, which will set the device backend state to XenbusStateClosed by calling set_backend_state(). However, the backend state wasn't initialized by netback_probe() at this point, which will cause and invalid transaction and set_backend_state() to BUG(). Initialize the backend state at the beginning of netback_probe() to XenbusStateInitialising, and create a new valid state transaction on set_backend_state(), from XenbusStateInitialising to XenbusStateClosed. Signed-off-by: Filipe Manco <filipe.manco@neclab.eu> --- drivers/net/xen-netback/xenbus.c | 10 ++++++++++ 1 file changed, 10 insertions(+)