| Message ID | 5425961A.5000604@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 26/09/14 17:36, Chen Gang wrote: > When xenbus_switch_state() fails, it will call xenbus_switch_fatal() > internally, so need not return any status value, then use 'void' instead > of 'int' for xenbus_switch_state() and __xenbus_switch_state(). > > Also need be sure that all callers which check the return value must let > 'err' be 0. I've rewritten the commit message as: xen/xenbus: don't return errors from xenbus_switch_state() Most users of xenbus_switch_state() weren't handling the failure of xenbus_switch_state() correctly. They either called xenbus_dev_fatal() (which xenbus_switch_state() has effectively already tried to do), or ignored errors. xenbus_switch_state() may fail because: a) The device is being unplugged by the toolstack. The device will shortly be removed and the error does not need to be handled. b) Xenstore is broken. There isn't much the driver can do in this case since xenstore is required to signal failure to the toolstack. So, don't report any errors from xenbus_switch_state() which removes some unnecessary error handling in some of the drivers. I'd appreciate a review from some of the other front/backend driver maintainers on whether this is sensible reasoning. David > > And also need change the related comments for xenbus_switch_state(). > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > drivers/block/xen-blkback/xenbus.c | 9 ++------- > drivers/net/xen-netback/xenbus.c | 5 +---- > drivers/pci/xen-pcifront.c | 15 ++++++--------- > drivers/xen/xen-pciback/xenbus.c | 21 ++++----------------- > drivers/xen/xen-scsiback.c | 5 +---- > drivers/xen/xenbus/xenbus_client.c | 16 ++++++++-------- > include/xen/xenbus.h | 3 ++- > 7 files changed, 24 insertions(+), 50 deletions(-) > > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c > index 3a8b810..fdcc584 100644 > --- a/drivers/block/xen-blkback/xenbus.c > +++ b/drivers/block/xen-blkback/xenbus.c > @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev, > if (err) > goto fail; > > - err = xenbus_switch_state(dev, XenbusStateInitWait); > - if (err) > - goto fail; > + xenbus_switch_state(dev, XenbusStateInitWait); > > return 0; > > @@ -837,10 +835,7 @@ again: > if (err) > xenbus_dev_fatal(dev, err, "ending transaction"); > > - err = xenbus_switch_state(dev, XenbusStateConnected); > - if (err) > - xenbus_dev_fatal(dev, err, "%s: switching to Connected state", > - dev->nodename); > + xenbus_switch_state(dev, XenbusStateConnected); > > return; > abort: > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c > index 9c47b89..b5c3d47 100644 > --- a/drivers/net/xen-netback/xenbus.c > +++ b/drivers/net/xen-netback/xenbus.c > @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev, > if (err) > pr_debug("Error writing multi-queue-max-queues\n"); > > - err = xenbus_switch_state(dev, XenbusStateInitWait); > - if (err) > - goto fail; > - > + xenbus_switch_state(dev, XenbusStateInitWait); > be->state = XenbusStateInitWait; > > /* This kicks hotplug scripts, so do it immediately. */ > diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c > index 53df39a..c1d73b7 100644 > --- a/drivers/pci/xen-pcifront.c > +++ b/drivers/pci/xen-pcifront.c > @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev) > } > } > > - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected); > - > + xenbus_switch_state(pdev->xdev, XenbusStateConnected); > + return 0; > out: > return err; > } > > static int pcifront_try_disconnect(struct pcifront_device *pdev) > { > - int err = 0; > enum xenbus_state prev_state; > > - > prev_state = xenbus_read_driver_state(pdev->xdev->nodename); > > if (prev_state >= XenbusStateClosing) > @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev) > pcifront_disconnect(pdev); > } > > - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed); > + xenbus_switch_state(pdev->xdev, XenbusStateClosed); > > out: > - > - return err; > + return 0; > } > > static int pcifront_attach_devices(struct pcifront_device *pdev) > @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) > domain, bus, slot, func); > } > > - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring); > - > + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring); > + return 0; > out: > return err; > } > diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c > index c214daa..630a215 100644 > --- a/drivers/xen/xen-pciback/xenbus.c > +++ b/drivers/xen/xen-pciback/xenbus.c > @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev) > > dev_dbg(&pdev->xdev->dev, "Connecting...\n"); > > - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected); > - if (err) > - xenbus_dev_fatal(pdev->xdev, err, > - "Error switching to connected state!"); > + xenbus_switch_state(pdev->xdev, XenbusStateConnected); > > dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err); > out: > @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev) > } > } > > - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured); > - if (err) { > - xenbus_dev_fatal(pdev->xdev, err, > - "Error switching to reconfigured state!"); > - goto out; > - } > + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured); > > out: > mutex_unlock(&pdev->dev_lock); > @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev) > goto out; > } > > - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised); > - if (err) > - xenbus_dev_fatal(pdev->xdev, err, > - "Error switching to initialised state!"); > + xenbus_switch_state(pdev->xdev, XenbusStateInitialised); > > out: > mutex_unlock(&pdev->dev_lock); > @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev, > } > > /* wait for xend to configure us */ > - err = xenbus_switch_state(dev, XenbusStateInitWait); > - if (err) > - goto out; > + xenbus_switch_state(dev, XenbusStateInitWait); > > /* watch the backend node for backend configuration information */ > err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch, > diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c > index ad11258..847bc9c 100644 > --- a/drivers/xen/xen-scsiback.c > +++ b/drivers/xen/xen-scsiback.c > @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev, > if (err) > xenbus_dev_error(dev, err, "writing feature-sg-grant"); > > - err = xenbus_switch_state(dev, XenbusStateInitWait); > - if (err) > - goto fail; > - > + xenbus_switch_state(dev, XenbusStateInitWait); > return 0; > > fail: > diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c > index ca74410..e2bcd1d 100644 > --- a/drivers/xen/xenbus/xenbus_client.c > +++ b/drivers/xen/xenbus/xenbus_client.c > @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt); > static void xenbus_switch_fatal(struct xenbus_device *, int, int, > const char *, ...); > > -static int > +static void > __xenbus_switch_state(struct xenbus_device *dev, > enum xenbus_state state, int depth) > { > @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev, > int err, abort; > > if (state == dev->state) > - return 0; > + return; > > again: > abort = 1; > @@ -196,7 +196,7 @@ again: > err = xenbus_transaction_start(&xbt); > if (err) { > xenbus_switch_fatal(dev, depth, err, "starting transaction"); > - return 0; > + return; > } > > err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state); > @@ -219,7 +219,7 @@ abort: > } else > dev->state = state; > > - return 0; > + return; > } > > /** > @@ -228,12 +228,12 @@ abort: > * @state: new state > * > * Advertise in the store a change of the given driver to the given new_state. > - * Return 0 on success, or -errno on error. On error, the device will switch > - * to XenbusStateClosing, and the error will be saved in the store. > + * When failure occurs, it will call xenbus_switch_fatal() internally, so need > + * not return value to notify upper caller. > */ > -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) > +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) > { > - return __xenbus_switch_state(dev, state, 0); > + __xenbus_switch_state(dev, state, 0); > } > > EXPORT_SYMBOL_GPL(xenbus_switch_state); > diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h > index 0324c6d..587c53d 100644 > --- a/include/xen/xenbus.h > +++ b/include/xen/xenbus.h > @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch, > const char **, unsigned int), > const char *pathfmt, ...); > > -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state); > +void xenbus_switch_state(struct xenbus_device *dev, > + enum xenbus_state new_state); > int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn); > int xenbus_map_ring_valloc(struct xenbus_device *dev, > int gnt_ref, void **vaddr); > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/27/2014 02:07 AM, David Vrabel wrote: > On 26/09/14 17:36, Chen Gang wrote: >> When xenbus_switch_state() fails, it will call xenbus_switch_fatal() >> internally, so need not return any status value, then use 'void' instead >> of 'int' for xenbus_switch_state() and __xenbus_switch_state(). >> >> Also need be sure that all callers which check the return value must let >> 'err' be 0. > > I've rewritten the commit message as: > > xen/xenbus: don't return errors from xenbus_switch_state() > > Most users of xenbus_switch_state() weren't handling the failure of > xenbus_switch_state() correctly. They either called > xenbus_dev_fatal() (which xenbus_switch_state() has effectively > already tried to do), or ignored errors. > > xenbus_switch_state() may fail because: > > a) The device is being unplugged by the toolstack. The device will > shortly be removed and the error does not need to be handled. > > b) Xenstore is broken. There isn't much the driver can do in this > case since xenstore is required to signal failure to the toolstack. > > So, don't report any errors from xenbus_switch_state() which removes > some unnecessary error handling in some of the drivers. > > I'd appreciate a review from some of the other front/backend driver > maintainers on whether this is sensible reasoning. > At least for me, what your changes is necessary, and also necessary to let other related members to have a look. And one thing I did not mention: after the patch, it reports a warning, which I have sent the other followed patch for it (it is for a bug fix, but also can fix this warning). The other followed patch is "xen/xen-scsiback: Need go to fail after xenbus_dev_error()" Please help check, too. Thanks. > David > >> >> And also need change the related comments for xenbus_switch_state(). >> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >> --- >> drivers/block/xen-blkback/xenbus.c | 9 ++------- >> drivers/net/xen-netback/xenbus.c | 5 +---- >> drivers/pci/xen-pcifront.c | 15 ++++++--------- >> drivers/xen/xen-pciback/xenbus.c | 21 ++++----------------- >> drivers/xen/xen-scsiback.c | 5 +---- >> drivers/xen/xenbus/xenbus_client.c | 16 ++++++++-------- >> include/xen/xenbus.h | 3 ++- >> 7 files changed, 24 insertions(+), 50 deletions(-) >> >> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c >> index 3a8b810..fdcc584 100644 >> --- a/drivers/block/xen-blkback/xenbus.c >> +++ b/drivers/block/xen-blkback/xenbus.c >> @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev, >> if (err) >> goto fail; >> >> - err = xenbus_switch_state(dev, XenbusStateInitWait); >> - if (err) >> - goto fail; >> + xenbus_switch_state(dev, XenbusStateInitWait); >> >> return 0; >> >> @@ -837,10 +835,7 @@ again: >> if (err) >> xenbus_dev_fatal(dev, err, "ending transaction"); >> >> - err = xenbus_switch_state(dev, XenbusStateConnected); >> - if (err) >> - xenbus_dev_fatal(dev, err, "%s: switching to Connected state", >> - dev->nodename); >> + xenbus_switch_state(dev, XenbusStateConnected); >> >> return; >> abort: >> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c >> index 9c47b89..b5c3d47 100644 >> --- a/drivers/net/xen-netback/xenbus.c >> +++ b/drivers/net/xen-netback/xenbus.c >> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev, >> if (err) >> pr_debug("Error writing multi-queue-max-queues\n"); >> >> - err = xenbus_switch_state(dev, XenbusStateInitWait); >> - if (err) >> - goto fail; >> - >> + xenbus_switch_state(dev, XenbusStateInitWait); >> be->state = XenbusStateInitWait; >> >> /* This kicks hotplug scripts, so do it immediately. */ >> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c >> index 53df39a..c1d73b7 100644 >> --- a/drivers/pci/xen-pcifront.c >> +++ b/drivers/pci/xen-pcifront.c >> @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev) >> } >> } >> >> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected); >> - >> + xenbus_switch_state(pdev->xdev, XenbusStateConnected); >> + return 0; >> out: >> return err; >> } >> >> static int pcifront_try_disconnect(struct pcifront_device *pdev) >> { >> - int err = 0; >> enum xenbus_state prev_state; >> >> - >> prev_state = xenbus_read_driver_state(pdev->xdev->nodename); >> >> if (prev_state >= XenbusStateClosing) >> @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev) >> pcifront_disconnect(pdev); >> } >> >> - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed); >> + xenbus_switch_state(pdev->xdev, XenbusStateClosed); >> >> out: >> - >> - return err; >> + return 0; >> } >> >> static int pcifront_attach_devices(struct pcifront_device *pdev) >> @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) >> domain, bus, slot, func); >> } >> >> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring); >> - >> + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring); >> + return 0; >> out: >> return err; >> } >> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c >> index c214daa..630a215 100644 >> --- a/drivers/xen/xen-pciback/xenbus.c >> +++ b/drivers/xen/xen-pciback/xenbus.c >> @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev) >> >> dev_dbg(&pdev->xdev->dev, "Connecting...\n"); >> >> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected); >> - if (err) >> - xenbus_dev_fatal(pdev->xdev, err, >> - "Error switching to connected state!"); >> + xenbus_switch_state(pdev->xdev, XenbusStateConnected); >> >> dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err); >> out: >> @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev) >> } >> } >> >> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured); >> - if (err) { >> - xenbus_dev_fatal(pdev->xdev, err, >> - "Error switching to reconfigured state!"); >> - goto out; >> - } >> + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured); >> >> out: >> mutex_unlock(&pdev->dev_lock); >> @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev) >> goto out; >> } >> >> - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised); >> - if (err) >> - xenbus_dev_fatal(pdev->xdev, err, >> - "Error switching to initialised state!"); >> + xenbus_switch_state(pdev->xdev, XenbusStateInitialised); >> >> out: >> mutex_unlock(&pdev->dev_lock); >> @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev, >> } >> >> /* wait for xend to configure us */ >> - err = xenbus_switch_state(dev, XenbusStateInitWait); >> - if (err) >> - goto out; >> + xenbus_switch_state(dev, XenbusStateInitWait); >> >> /* watch the backend node for backend configuration information */ >> err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch, >> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c >> index ad11258..847bc9c 100644 >> --- a/drivers/xen/xen-scsiback.c >> +++ b/drivers/xen/xen-scsiback.c >> @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev, >> if (err) >> xenbus_dev_error(dev, err, "writing feature-sg-grant"); >> >> - err = xenbus_switch_state(dev, XenbusStateInitWait); >> - if (err) >> - goto fail; >> - >> + xenbus_switch_state(dev, XenbusStateInitWait); >> return 0; >> >> fail: >> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c >> index ca74410..e2bcd1d 100644 >> --- a/drivers/xen/xenbus/xenbus_client.c >> +++ b/drivers/xen/xenbus/xenbus_client.c >> @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt); >> static void xenbus_switch_fatal(struct xenbus_device *, int, int, >> const char *, ...); >> >> -static int >> +static void >> __xenbus_switch_state(struct xenbus_device *dev, >> enum xenbus_state state, int depth) >> { >> @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev, >> int err, abort; >> >> if (state == dev->state) >> - return 0; >> + return; >> >> again: >> abort = 1; >> @@ -196,7 +196,7 @@ again: >> err = xenbus_transaction_start(&xbt); >> if (err) { >> xenbus_switch_fatal(dev, depth, err, "starting transaction"); >> - return 0; >> + return; >> } >> >> err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state); >> @@ -219,7 +219,7 @@ abort: >> } else >> dev->state = state; >> >> - return 0; >> + return; >> } >> >> /** >> @@ -228,12 +228,12 @@ abort: >> * @state: new state >> * >> * Advertise in the store a change of the given driver to the given new_state. >> - * Return 0 on success, or -errno on error. On error, the device will switch >> - * to XenbusStateClosing, and the error will be saved in the store. >> + * When failure occurs, it will call xenbus_switch_fatal() internally, so need >> + * not return value to notify upper caller. >> */ >> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) >> +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) >> { >> - return __xenbus_switch_state(dev, state, 0); >> + __xenbus_switch_state(dev, state, 0); >> } >> >> EXPORT_SYMBOL_GPL(xenbus_switch_state); >> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h >> index 0324c6d..587c53d 100644 >> --- a/include/xen/xenbus.h >> +++ b/include/xen/xenbus.h >> @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch, >> const char **, unsigned int), >> const char *pathfmt, ...); >> >> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state); >> +void xenbus_switch_state(struct xenbus_device *dev, >> + enum xenbus_state new_state); >> int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn); >> int xenbus_map_ring_valloc(struct xenbus_device *dev, >> int gnt_ref, void **vaddr); >> >
On 09/26/2014 06:36 PM, Chen Gang wrote: > When xenbus_switch_state() fails, it will call xenbus_switch_fatal() > internally, so need not return any status value, then use 'void' instead > of 'int' for xenbus_switch_state() and __xenbus_switch_state(). > > Also need be sure that all callers which check the return value must let > 'err' be 0. > > And also need change the related comments for xenbus_switch_state(). > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> Acked-by: Juergen Gross <jgross@suse.com> > --- > drivers/block/xen-blkback/xenbus.c | 9 ++------- > drivers/net/xen-netback/xenbus.c | 5 +---- > drivers/pci/xen-pcifront.c | 15 ++++++--------- > drivers/xen/xen-pciback/xenbus.c | 21 ++++----------------- > drivers/xen/xen-scsiback.c | 5 +---- > drivers/xen/xenbus/xenbus_client.c | 16 ++++++++-------- > include/xen/xenbus.h | 3 ++- > 7 files changed, 24 insertions(+), 50 deletions(-) > > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c > index 3a8b810..fdcc584 100644 > --- a/drivers/block/xen-blkback/xenbus.c > +++ b/drivers/block/xen-blkback/xenbus.c > @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev, > if (err) > goto fail; > > - err = xenbus_switch_state(dev, XenbusStateInitWait); > - if (err) > - goto fail; > + xenbus_switch_state(dev, XenbusStateInitWait); > > return 0; > > @@ -837,10 +835,7 @@ again: > if (err) > xenbus_dev_fatal(dev, err, "ending transaction"); > > - err = xenbus_switch_state(dev, XenbusStateConnected); > - if (err) > - xenbus_dev_fatal(dev, err, "%s: switching to Connected state", > - dev->nodename); > + xenbus_switch_state(dev, XenbusStateConnected); > > return; > abort: > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c > index 9c47b89..b5c3d47 100644 > --- a/drivers/net/xen-netback/xenbus.c > +++ b/drivers/net/xen-netback/xenbus.c > @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev, > if (err) > pr_debug("Error writing multi-queue-max-queues\n"); > > - err = xenbus_switch_state(dev, XenbusStateInitWait); > - if (err) > - goto fail; > - > + xenbus_switch_state(dev, XenbusStateInitWait); > be->state = XenbusStateInitWait; > > /* This kicks hotplug scripts, so do it immediately. */ > diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c > index 53df39a..c1d73b7 100644 > --- a/drivers/pci/xen-pcifront.c > +++ b/drivers/pci/xen-pcifront.c > @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev) > } > } > > - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected); > - > + xenbus_switch_state(pdev->xdev, XenbusStateConnected); > + return 0; > out: > return err; > } > > static int pcifront_try_disconnect(struct pcifront_device *pdev) > { > - int err = 0; > enum xenbus_state prev_state; > > - > prev_state = xenbus_read_driver_state(pdev->xdev->nodename); > > if (prev_state >= XenbusStateClosing) > @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev) > pcifront_disconnect(pdev); > } > > - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed); > + xenbus_switch_state(pdev->xdev, XenbusStateClosed); > > out: > - > - return err; > + return 0; > } > > static int pcifront_attach_devices(struct pcifront_device *pdev) > @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) > domain, bus, slot, func); > } > > - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring); > - > + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring); > + return 0; > out: > return err; > } > diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c > index c214daa..630a215 100644 > --- a/drivers/xen/xen-pciback/xenbus.c > +++ b/drivers/xen/xen-pciback/xenbus.c > @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev) > > dev_dbg(&pdev->xdev->dev, "Connecting...\n"); > > - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected); > - if (err) > - xenbus_dev_fatal(pdev->xdev, err, > - "Error switching to connected state!"); > + xenbus_switch_state(pdev->xdev, XenbusStateConnected); > > dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err); > out: > @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev) > } > } > > - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured); > - if (err) { > - xenbus_dev_fatal(pdev->xdev, err, > - "Error switching to reconfigured state!"); > - goto out; > - } > + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured); > > out: > mutex_unlock(&pdev->dev_lock); > @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev) > goto out; > } > > - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised); > - if (err) > - xenbus_dev_fatal(pdev->xdev, err, > - "Error switching to initialised state!"); > + xenbus_switch_state(pdev->xdev, XenbusStateInitialised); > > out: > mutex_unlock(&pdev->dev_lock); > @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev, > } > > /* wait for xend to configure us */ > - err = xenbus_switch_state(dev, XenbusStateInitWait); > - if (err) > - goto out; > + xenbus_switch_state(dev, XenbusStateInitWait); > > /* watch the backend node for backend configuration information */ > err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch, > diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c > index ad11258..847bc9c 100644 > --- a/drivers/xen/xen-scsiback.c > +++ b/drivers/xen/xen-scsiback.c > @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev, > if (err) > xenbus_dev_error(dev, err, "writing feature-sg-grant"); > > - err = xenbus_switch_state(dev, XenbusStateInitWait); > - if (err) > - goto fail; > - > + xenbus_switch_state(dev, XenbusStateInitWait); > return 0; > > fail: > diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c > index ca74410..e2bcd1d 100644 > --- a/drivers/xen/xenbus/xenbus_client.c > +++ b/drivers/xen/xenbus/xenbus_client.c > @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt); > static void xenbus_switch_fatal(struct xenbus_device *, int, int, > const char *, ...); > > -static int > +static void > __xenbus_switch_state(struct xenbus_device *dev, > enum xenbus_state state, int depth) > { > @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev, > int err, abort; > > if (state == dev->state) > - return 0; > + return; > > again: > abort = 1; > @@ -196,7 +196,7 @@ again: > err = xenbus_transaction_start(&xbt); > if (err) { > xenbus_switch_fatal(dev, depth, err, "starting transaction"); > - return 0; > + return; > } > > err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state); > @@ -219,7 +219,7 @@ abort: > } else > dev->state = state; > > - return 0; > + return; > } > > /** > @@ -228,12 +228,12 @@ abort: > * @state: new state > * > * Advertise in the store a change of the given driver to the given new_state. > - * Return 0 on success, or -errno on error. On error, the device will switch > - * to XenbusStateClosing, and the error will be saved in the store. > + * When failure occurs, it will call xenbus_switch_fatal() internally, so need > + * not return value to notify upper caller. > */ > -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) > +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) > { > - return __xenbus_switch_state(dev, state, 0); > + __xenbus_switch_state(dev, state, 0); > } > > EXPORT_SYMBOL_GPL(xenbus_switch_state); > diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h > index 0324c6d..587c53d 100644 > --- a/include/xen/xenbus.h > +++ b/include/xen/xenbus.h > @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch, > const char **, unsigned int), > const char *pathfmt, ...); > > -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state); > +void xenbus_switch_state(struct xenbus_device *dev, > + enum xenbus_state new_state); > int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn); > int xenbus_map_ring_valloc(struct xenbus_device *dev, > int gnt_ref, void **vaddr); > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 26, 2014 at 07:07:19PM +0100, David Vrabel wrote: > On 26/09/14 17:36, Chen Gang wrote: > > When xenbus_switch_state() fails, it will call xenbus_switch_fatal() > > internally, so need not return any status value, then use 'void' instead > > of 'int' for xenbus_switch_state() and __xenbus_switch_state(). > > > > Also need be sure that all callers which check the return value must let > > 'err' be 0. > > I've rewritten the commit message as: > > xen/xenbus: don't return errors from xenbus_switch_state() > > Most users of xenbus_switch_state() weren't handling the failure of > xenbus_switch_state() correctly. They either called > xenbus_dev_fatal() (which xenbus_switch_state() has effectively > already tried to do), or ignored errors. > > xenbus_switch_state() may fail because: > > a) The device is being unplugged by the toolstack. The device will > shortly be removed and the error does not need to be handled. > > b) Xenstore is broken. There isn't much the driver can do in this > case since xenstore is required to signal failure to the toolstack. > > So, don't report any errors from xenbus_switch_state() which removes > some unnecessary error handling in some of the drivers. > > I'd appreciate a review from some of the other front/backend driver > maintainers on whether this is sensible reasoning. > Looks reasonable to me. Wei. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 26, 2014 at 10:36 AM, Chen Gang <gang.chen.5i5j@gmail.com> wrote: > When xenbus_switch_state() fails, it will call xenbus_switch_fatal() > internally, so need not return any status value, then use 'void' instead > of 'int' for xenbus_switch_state() and __xenbus_switch_state(). > > Also need be sure that all callers which check the return value must let > 'err' be 0. > > And also need change the related comments for xenbus_switch_state(). > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > drivers/block/xen-blkback/xenbus.c | 9 ++------- > drivers/net/xen-netback/xenbus.c | 5 +---- > drivers/pci/xen-pcifront.c | 15 ++++++--------- > drivers/xen/xen-pciback/xenbus.c | 21 ++++----------------- > drivers/xen/xen-scsiback.c | 5 +---- > drivers/xen/xenbus/xenbus_client.c | 16 ++++++++-------- > include/xen/xenbus.h | 3 ++- > 7 files changed, 24 insertions(+), 50 deletions(-) I don't know enough to review this, but as far as I'm concerned, the Xen folks own drivers/pci/xen-pcifront.c, so whatever they want to do is fine with me. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 26, 2014 at 07:07:19PM +0100, David Vrabel wrote: > On 26/09/14 17:36, Chen Gang wrote: > > When xenbus_switch_state() fails, it will call xenbus_switch_fatal() > > internally, so need not return any status value, then use 'void' instead > > of 'int' for xenbus_switch_state() and __xenbus_switch_state(). > > > > Also need be sure that all callers which check the return value must let > > 'err' be 0. > > I've rewritten the commit message as: > > xen/xenbus: don't return errors from xenbus_switch_state() > > Most users of xenbus_switch_state() weren't handling the failure of > xenbus_switch_state() correctly. They either called > xenbus_dev_fatal() (which xenbus_switch_state() has effectively > already tried to do), or ignored errors. > > xenbus_switch_state() may fail because: > > a) The device is being unplugged by the toolstack. The device will > shortly be removed and the error does not need to be handled. > > b) Xenstore is broken. There isn't much the driver can do in this > case since xenstore is required to signal failure to the toolstack. > > So, don't report any errors from xenbus_switch_state() which removes > some unnecessary error handling in some of the drivers. > > I'd appreciate a review from some of the other front/backend driver > maintainers on whether this is sensible reasoning. Done that but I am not too keen about this patch - as I think it will cause memory leaks on failure paths. > > David > > > > > And also need change the related comments for xenbus_switch_state(). > > > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > > --- > > drivers/block/xen-blkback/xenbus.c | 9 ++------- > > drivers/net/xen-netback/xenbus.c | 5 +---- > > drivers/pci/xen-pcifront.c | 15 ++++++--------- > > drivers/xen/xen-pciback/xenbus.c | 21 ++++----------------- > > drivers/xen/xen-scsiback.c | 5 +---- > > drivers/xen/xenbus/xenbus_client.c | 16 ++++++++-------- > > include/xen/xenbus.h | 3 ++- > > 7 files changed, 24 insertions(+), 50 deletions(-) > > > > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c > > index 3a8b810..fdcc584 100644 > > --- a/drivers/block/xen-blkback/xenbus.c > > +++ b/drivers/block/xen-blkback/xenbus.c > > @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev, > > if (err) > > goto fail; > > > > - err = xenbus_switch_state(dev, XenbusStateInitWait); > > - if (err) > > - goto fail; > > + xenbus_switch_state(dev, XenbusStateInitWait); > > > > return 0; > > > > @@ -837,10 +835,7 @@ again: > > if (err) > > xenbus_dev_fatal(dev, err, "ending transaction"); > > > > - err = xenbus_switch_state(dev, XenbusStateConnected); > > - if (err) > > - xenbus_dev_fatal(dev, err, "%s: switching to Connected state", > > - dev->nodename); > > + xenbus_switch_state(dev, XenbusStateConnected); > > > > return; > > abort: > > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c > > index 9c47b89..b5c3d47 100644 > > --- a/drivers/net/xen-netback/xenbus.c > > +++ b/drivers/net/xen-netback/xenbus.c > > @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev, > > if (err) > > pr_debug("Error writing multi-queue-max-queues\n"); > > > > - err = xenbus_switch_state(dev, XenbusStateInitWait); > > - if (err) > > - goto fail; > > - > > + xenbus_switch_state(dev, XenbusStateInitWait); > > be->state = XenbusStateInitWait; > > > > /* This kicks hotplug scripts, so do it immediately. */ > > diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c > > index 53df39a..c1d73b7 100644 > > --- a/drivers/pci/xen-pcifront.c > > +++ b/drivers/pci/xen-pcifront.c > > @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev) > > } > > } > > > > - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected); > > - > > + xenbus_switch_state(pdev->xdev, XenbusStateConnected); > > + return 0; > > out: > > return err; > > } > > > > static int pcifront_try_disconnect(struct pcifront_device *pdev) > > { > > - int err = 0; > > enum xenbus_state prev_state; > > > > - > > prev_state = xenbus_read_driver_state(pdev->xdev->nodename); > > > > if (prev_state >= XenbusStateClosing) > > @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev) > > pcifront_disconnect(pdev); > > } > > > > - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed); > > + xenbus_switch_state(pdev->xdev, XenbusStateClosed); > > > > out: > > - > > - return err; > > + return 0; > > } > > > > static int pcifront_attach_devices(struct pcifront_device *pdev) > > @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) > > domain, bus, slot, func); > > } > > > > - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring); > > - > > + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring); > > + return 0; > > out: > > return err; > > } > > diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c > > index c214daa..630a215 100644 > > --- a/drivers/xen/xen-pciback/xenbus.c > > +++ b/drivers/xen/xen-pciback/xenbus.c > > @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev) > > > > dev_dbg(&pdev->xdev->dev, "Connecting...\n"); > > > > - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected); > > - if (err) > > - xenbus_dev_fatal(pdev->xdev, err, > > - "Error switching to connected state!"); > > + xenbus_switch_state(pdev->xdev, XenbusStateConnected); > > > > dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err); > > out: > > @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev) > > } > > } > > > > - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured); > > - if (err) { > > - xenbus_dev_fatal(pdev->xdev, err, > > - "Error switching to reconfigured state!"); > > - goto out; > > - } > > + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured); > > > > out: > > mutex_unlock(&pdev->dev_lock); > > @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev) > > goto out; > > } > > > > - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised); > > - if (err) > > - xenbus_dev_fatal(pdev->xdev, err, > > - "Error switching to initialised state!"); > > + xenbus_switch_state(pdev->xdev, XenbusStateInitialised); > > > > out: > > mutex_unlock(&pdev->dev_lock); > > @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev, > > } > > > > /* wait for xend to configure us */ > > - err = xenbus_switch_state(dev, XenbusStateInitWait); > > - if (err) > > - goto out; > > + xenbus_switch_state(dev, XenbusStateInitWait); > > > > /* watch the backend node for backend configuration information */ > > err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch, > > diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c > > index ad11258..847bc9c 100644 > > --- a/drivers/xen/xen-scsiback.c > > +++ b/drivers/xen/xen-scsiback.c > > @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev, > > if (err) > > xenbus_dev_error(dev, err, "writing feature-sg-grant"); > > > > - err = xenbus_switch_state(dev, XenbusStateInitWait); > > - if (err) > > - goto fail; > > - > > + xenbus_switch_state(dev, XenbusStateInitWait); > > return 0; > > > > fail: > > diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c > > index ca74410..e2bcd1d 100644 > > --- a/drivers/xen/xenbus/xenbus_client.c > > +++ b/drivers/xen/xenbus/xenbus_client.c > > @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt); > > static void xenbus_switch_fatal(struct xenbus_device *, int, int, > > const char *, ...); > > > > -static int > > +static void > > __xenbus_switch_state(struct xenbus_device *dev, > > enum xenbus_state state, int depth) > > { > > @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev, > > int err, abort; > > > > if (state == dev->state) > > - return 0; > > + return; > > > > again: > > abort = 1; > > @@ -196,7 +196,7 @@ again: > > err = xenbus_transaction_start(&xbt); > > if (err) { > > xenbus_switch_fatal(dev, depth, err, "starting transaction"); > > - return 0; > > + return; > > } > > > > err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state); > > @@ -219,7 +219,7 @@ abort: > > } else > > dev->state = state; > > > > - return 0; > > + return; > > } > > > > /** > > @@ -228,12 +228,12 @@ abort: > > * @state: new state > > * > > * Advertise in the store a change of the given driver to the given new_state. > > - * Return 0 on success, or -errno on error. On error, the device will switch > > - * to XenbusStateClosing, and the error will be saved in the store. > > + * When failure occurs, it will call xenbus_switch_fatal() internally, so need > > + * not return value to notify upper caller. > > */ > > -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) > > +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) > > { > > - return __xenbus_switch_state(dev, state, 0); > > + __xenbus_switch_state(dev, state, 0); > > } > > > > EXPORT_SYMBOL_GPL(xenbus_switch_state); > > diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h > > index 0324c6d..587c53d 100644 > > --- a/include/xen/xenbus.h > > +++ b/include/xen/xenbus.h > > @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch, > > const char **, unsigned int), > > const char *pathfmt, ...); > > > > -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state); > > +void xenbus_switch_state(struct xenbus_device *dev, > > + enum xenbus_state new_state); > > int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn); > > int xenbus_map_ring_valloc(struct xenbus_device *dev, > > int gnt_ref, void **vaddr); > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26/09/14 17:36, Chen Gang wrote: > When xenbus_switch_state() fails, it will call xenbus_switch_fatal() > internally, so need not return any status value, then use 'void' instead > of 'int' for xenbus_switch_state() and __xenbus_switch_state(). > > Also need be sure that all callers which check the return value must let > 'err' be 0. > > And also need change the related comments for xenbus_switch_state(). Since this patch does not fix a bug and there is no unanimous agreement on the API change I'm not going to apply it (nor the previous version). David -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9/30/14 17:55, David Vrabel wrote: > On 26/09/14 17:36, Chen Gang wrote: >> When xenbus_switch_state() fails, it will call xenbus_switch_fatal() >> internally, so need not return any status value, then use 'void' instead >> of 'int' for xenbus_switch_state() and __xenbus_switch_state(). >> >> Also need be sure that all callers which check the return value must let >> 'err' be 0. >> >> And also need change the related comments for xenbus_switch_state(). > > Since this patch does not fix a bug and there is no unanimous agreement > on the API change I'm not going to apply it (nor the previous version). > OK, at least for me, it is no problems. But I still recommend to improve it in the future, it is not a good idea to let all related things remain in current condition (for me, at lease need some related code comments for it). Thanks.
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c index 3a8b810..fdcc584 100644 --- a/drivers/block/xen-blkback/xenbus.c +++ b/drivers/block/xen-blkback/xenbus.c @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev, if (err) goto fail; - err = xenbus_switch_state(dev, XenbusStateInitWait); - if (err) - goto fail; + xenbus_switch_state(dev, XenbusStateInitWait); return 0; @@ -837,10 +835,7 @@ again: if (err) xenbus_dev_fatal(dev, err, "ending transaction"); - err = xenbus_switch_state(dev, XenbusStateConnected); - if (err) - xenbus_dev_fatal(dev, err, "%s: switching to Connected state", - dev->nodename); + xenbus_switch_state(dev, XenbusStateConnected); return; abort: diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c index 9c47b89..b5c3d47 100644 --- a/drivers/net/xen-netback/xenbus.c +++ b/drivers/net/xen-netback/xenbus.c @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev, if (err) pr_debug("Error writing multi-queue-max-queues\n"); - err = xenbus_switch_state(dev, XenbusStateInitWait); - if (err) - goto fail; - + xenbus_switch_state(dev, XenbusStateInitWait); be->state = XenbusStateInitWait; /* This kicks hotplug scripts, so do it immediately. */ diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c index 53df39a..c1d73b7 100644 --- a/drivers/pci/xen-pcifront.c +++ b/drivers/pci/xen-pcifront.c @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev) } } - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected); - + xenbus_switch_state(pdev->xdev, XenbusStateConnected); + return 0; out: return err; } static int pcifront_try_disconnect(struct pcifront_device *pdev) { - int err = 0; enum xenbus_state prev_state; - prev_state = xenbus_read_driver_state(pdev->xdev->nodename); if (prev_state >= XenbusStateClosing) @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev) pcifront_disconnect(pdev); } - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed); + xenbus_switch_state(pdev->xdev, XenbusStateClosed); out: - - return err; + return 0; } static int pcifront_attach_devices(struct pcifront_device *pdev) @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) domain, bus, slot, func); } - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring); - + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring); + return 0; out: return err; } diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c index c214daa..630a215 100644 --- a/drivers/xen/xen-pciback/xenbus.c +++ b/drivers/xen/xen-pciback/xenbus.c @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev) dev_dbg(&pdev->xdev->dev, "Connecting...\n"); - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected); - if (err) - xenbus_dev_fatal(pdev->xdev, err, - "Error switching to connected state!"); + xenbus_switch_state(pdev->xdev, XenbusStateConnected); dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err); out: @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev) } } - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured); - if (err) { - xenbus_dev_fatal(pdev->xdev, err, - "Error switching to reconfigured state!"); - goto out; - } + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured); out: mutex_unlock(&pdev->dev_lock); @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev) goto out; } - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised); - if (err) - xenbus_dev_fatal(pdev->xdev, err, - "Error switching to initialised state!"); + xenbus_switch_state(pdev->xdev, XenbusStateInitialised); out: mutex_unlock(&pdev->dev_lock); @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev, } /* wait for xend to configure us */ - err = xenbus_switch_state(dev, XenbusStateInitWait); - if (err) - goto out; + xenbus_switch_state(dev, XenbusStateInitWait); /* watch the backend node for backend configuration information */ err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch, diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c index ad11258..847bc9c 100644 --- a/drivers/xen/xen-scsiback.c +++ b/drivers/xen/xen-scsiback.c @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev, if (err) xenbus_dev_error(dev, err, "writing feature-sg-grant"); - err = xenbus_switch_state(dev, XenbusStateInitWait); - if (err) - goto fail; - + xenbus_switch_state(dev, XenbusStateInitWait); return 0; fail: diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c index ca74410..e2bcd1d 100644 --- a/drivers/xen/xenbus/xenbus_client.c +++ b/drivers/xen/xenbus/xenbus_client.c @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt); static void xenbus_switch_fatal(struct xenbus_device *, int, int, const char *, ...); -static int +static void __xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state, int depth) { @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev, int err, abort; if (state == dev->state) - return 0; + return; again: abort = 1; @@ -196,7 +196,7 @@ again: err = xenbus_transaction_start(&xbt); if (err) { xenbus_switch_fatal(dev, depth, err, "starting transaction"); - return 0; + return; } err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state); @@ -219,7 +219,7 @@ abort: } else dev->state = state; - return 0; + return; } /** @@ -228,12 +228,12 @@ abort: * @state: new state * * Advertise in the store a change of the given driver to the given new_state. - * Return 0 on success, or -errno on error. On error, the device will switch - * to XenbusStateClosing, and the error will be saved in the store. + * When failure occurs, it will call xenbus_switch_fatal() internally, so need + * not return value to notify upper caller. */ -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) { - return __xenbus_switch_state(dev, state, 0); + __xenbus_switch_state(dev, state, 0); } EXPORT_SYMBOL_GPL(xenbus_switch_state); diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h index 0324c6d..587c53d 100644 --- a/include/xen/xenbus.h +++ b/include/xen/xenbus.h @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch, const char **, unsigned int), const char *pathfmt, ...); -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state); +void xenbus_switch_state(struct xenbus_device *dev, + enum xenbus_state new_state); int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn); int xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref, void **vaddr);
When xenbus_switch_state() fails, it will call xenbus_switch_fatal() internally, so need not return any status value, then use 'void' instead of 'int' for xenbus_switch_state() and __xenbus_switch_state(). Also need be sure that all callers which check the return value must let 'err' be 0. And also need change the related comments for xenbus_switch_state(). Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- drivers/block/xen-blkback/xenbus.c | 9 ++------- drivers/net/xen-netback/xenbus.c | 5 +---- drivers/pci/xen-pcifront.c | 15 ++++++--------- drivers/xen/xen-pciback/xenbus.c | 21 ++++----------------- drivers/xen/xen-scsiback.c | 5 +---- drivers/xen/xenbus/xenbus_client.c | 16 ++++++++-------- include/xen/xenbus.h | 3 ++- 7 files changed, 24 insertions(+), 50 deletions(-)