xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
Subsystems (10)
PATCH 1/1 xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
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().
--- 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(-)
drivers/block/xen-blkback/xenbus.c +2 -7
@@ -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:
drivers/net/xen-netback/xenbus.c +1 -4
@@ -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. */
drivers/pci/xen-pcifront.c +6 -9
@@ -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; }
drivers/xen/xen-pciback/xenbus.c +4 -17
@@ -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,
drivers/xen/xen-scsiback.c +1 -4
@@ -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:
drivers/xen/xenbus/xenbus_client.c +8 -8
@@ -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);
include/xen/xenbus.h +2 -1
@@ -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);
Signature · 1 line
1.9.3
Quoted 6 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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
Quoted 248 lines
And also need change the related comments for
xenbus_switch_state().Signed-off-by Chen Gang <[email protected]>--- 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(-)drivers/block/xen-blkback/xenbus.c +2
-7@@ -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:drivers/net/xen-netback/xenbus.c +1
-4@@ -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. */ drivers/pci/xen-pcifront.c +6
-9@@ -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; }drivers/xen/xen-pciback/xenbus.c +4
-17@@ -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,drivers/xen/xen-scsiback.c +1
-4@@ -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:drivers/xen/xenbus/xenbus_client.c +8
-8@@ -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);include/xen/xenbus.h +2
-1@@ -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);
Quoted 22 lines · David Vrabel
Quoted 6 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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 ofxenbus_switch_state()correctly. They either calledxenbus_dev_fatal()(whichxenbus_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.
Quoted 9 lines
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.
Quoted 251 lines
David
Quoted 248 lines
And also need change the related comments for
xenbus_switch_state().Signed-off-by Chen Gang <[email protected]>--- 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(-)drivers/block/xen-blkback/xenbus.c +2
-7@@ -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:drivers/net/xen-netback/xenbus.c +1
-4@@ -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. */ drivers/pci/xen-pcifront.c +6
-9@@ -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; }drivers/xen/xen-pciback/xenbus.c +4
-17@@ -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,drivers/xen/xen-scsiback.c +1
-4@@ -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:drivers/xen/xenbus/xenbus_client.c +8
-8@@ -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);include/xen/xenbus.h +2
-1@@ -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);
Signature · 3 lines
Chen Gang Open share and attitude like air water and life which God blessed
Quoted 22 lines · David Vrabel
Quoted 6 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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 ofxenbus_switch_state()correctly. They either calledxenbus_dev_fatal()(whichxenbus_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.
Quoted 9 lines
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.
Quoted 251 lines
David
Quoted 248 lines
And also need change the related comments for
xenbus_switch_state().Signed-off-by Chen Gang <[email protected]>--- 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(-)drivers/block/xen-blkback/xenbus.c +2
-7@@ -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:drivers/net/xen-netback/xenbus.c +1
-4@@ -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. */ drivers/pci/xen-pcifront.c +6
-9@@ -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; }drivers/xen/xen-pciback/xenbus.c +4
-17@@ -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,drivers/xen/xen-scsiback.c +1
-4@@ -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:drivers/xen/xenbus/xenbus_client.c +8
-8@@ -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);include/xen/xenbus.h +2
-1@@ -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);
Signature · 3 lines
Chen Gang Open share and attitude like air water and life which God blessed
Quoted 22 lines · David Vrabel
Quoted 6 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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 ofxenbus_switch_state()correctly. They either calledxenbus_dev_fatal()(whichxenbus_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.
Quoted 9 lines
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.
Quoted 22 lines · David Vrabel
Quoted 6 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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 ofxenbus_switch_state()correctly. They either calledxenbus_dev_fatal()(whichxenbus_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.
Quoted 9 lines
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.
Quoted 22 lines · David Vrabel
Quoted 6 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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 ofxenbus_switch_state()correctly. They either calledxenbus_dev_fatal()(whichxenbus_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.
Quoted 8 lines
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.
Quoted 252 lines
David
Quoted 248 lines
And also need change the related comments for
xenbus_switch_state().Signed-off-by Chen Gang <[email protected]>--- 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(-)drivers/block/xen-blkback/xenbus.c +2
-7@@ -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:drivers/net/xen-netback/xenbus.c +1
-4@@ -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. */ drivers/pci/xen-pcifront.c +6
-9@@ -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; }drivers/xen/xen-pciback/xenbus.c +4
-17@@ -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,drivers/xen/xen-scsiback.c +1
-4@@ -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:drivers/xen/xenbus/xenbus_client.c +8
-8@@ -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);include/xen/xenbus.h +2
-1@@ -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);
Quoted 22 lines · David Vrabel
Quoted 6 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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 ofxenbus_switch_state()correctly. They either calledxenbus_dev_fatal()(whichxenbus_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.
Quoted 8 lines
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.
Quoted 252 lines
David
Quoted 248 lines
And also need change the related comments for
xenbus_switch_state().Signed-off-by Chen Gang <[email protected]>--- 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(-)drivers/block/xen-blkback/xenbus.c +2
-7@@ -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:drivers/net/xen-netback/xenbus.c +1
-4@@ -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. */ drivers/pci/xen-pcifront.c +6
-9@@ -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; }drivers/xen/xen-pciback/xenbus.c +4
-17@@ -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,drivers/xen/xen-scsiback.c +1
-4@@ -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:drivers/xen/xenbus/xenbus_client.c +8
-8@@ -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);include/xen/xenbus.h +2
-1@@ -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);
Quoted 6 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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
Quoted 248 lines
And also need change the related comments for
xenbus_switch_state().Signed-off-by Chen Gang <[email protected]>--- 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(-)drivers/block/xen-blkback/xenbus.c +2
-7@@ -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:drivers/net/xen-netback/xenbus.c +1
-4@@ -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. */ drivers/pci/xen-pcifront.c +6
-9@@ -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; }drivers/xen/xen-pciback/xenbus.c +4
-17@@ -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,drivers/xen/xen-scsiback.c +1
-4@@ -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:drivers/xen/xenbus/xenbus_client.c +8
-8@@ -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);include/xen/xenbus.h +2
-1@@ -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);
Quoted 10 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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 <[email protected]>
Quoted 244 lines
--- 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(-)drivers/block/xen-blkback/xenbus.c +2
-7@@ -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:drivers/net/xen-netback/xenbus.c +1
-4@@ -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. */ drivers/pci/xen-pcifront.c +6
-9@@ -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; }drivers/xen/xen-pciback/xenbus.c +4
-17@@ -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,drivers/xen/xen-scsiback.c +1
-4@@ -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:drivers/xen/xenbus/xenbus_client.c +8
-8@@ -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);include/xen/xenbus.h +2
-1@@ -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);
Quoted 10 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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 <[email protected]>
Quoted 244 lines
--- 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(-)drivers/block/xen-blkback/xenbus.c +2
-7@@ -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:drivers/net/xen-netback/xenbus.c +1
-4@@ -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. */ drivers/pci/xen-pcifront.c +6
-9@@ -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; }drivers/xen/xen-pciback/xenbus.c +4
-17@@ -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,drivers/xen/xen-scsiback.c +1
-4@@ -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:drivers/xen/xenbus/xenbus_client.c +8
-8@@ -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);include/xen/xenbus.h +2
-1@@ -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);
Bjorn Helgaas
maintainerQuoted 10 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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 <[email protected]>
Quoted 9 lines
--- 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
Bjorn Helgaas
maintainerQuoted 10 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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 <[email protected]>
Quoted 9 lines
--- 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
Quoted 1 line · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()
Only on the first depth, not on the subsequent ones (as in if the first xenbus_switch_fail fails, it won't try to call xenbus_switch_state again and again).
Quoted 2 lines
internally, so need not return any status value, then use 'void' instead of 'int' for
xenbus_switch_state()and__xenbus_switch_state().
When that switch occurs (to XenbusStateConnected) won't the watches fire - meaning we MUST make sure that the watch functions - if they use the xenbus_switch_state() they MUST not hold any locks - because they could be executed once more?
Oh wait, we don't have to worry about that right now as the callbacks that pick up the messages from the XenBus are all gated on one mutex anyhow.
Hm, anyhow, I would add this extra piece of information to the patch:
drivers/xen/xen-pciback/xenbus.c +6 -0
@@ -661,6 +661,12 @@ static void xen_pcibk_be_watch(struct xenbus_watch *watch,switch (xenbus_read_driver_state (pdev->xdev->nodename)) {case XenbusStateInitWait: +/* +* xenbus_switch_state can call xenbus_switch_fatal which will +* immediately set the state to XenbusStateClosing which +* means if we were reading for it here we MUST drop any +* locks so that we don't dead-lock. +*/ xen_pcibk_setup_backend (pdev);break ;
Quoted 3 lines
Also need be sure that all callers which check the return value must let 'err' be 0.
I am bit uncomfortable with that, that is due to:
.. snip..
drivers/net/xen-netback/xenbus.c static int netback_probe(struct xenbus_device *dev, · line 340
Quoted 13 lines
drivers/net/xen-netback/xenbus.c +1
-4@@ -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);
Which if it fails it won't call:
354 fail:
355 pr_debug("failed\n");
356 netback_remove(dev);
357 return err; And since there is no watch on the backend state to go in Closing it won't ever call those and we leak memory.
The same is for xen-blkback mechanism in the probe function.
drivers/xen/xen-pciback/xenbus.c
Quoted 55 lines
drivers/net/xen-netback/xenbus.c +0
-0be->state = XenbusStateInitWait;/* This kicks hotplug scripts, so do it immediately. */ drivers/pci/xen-pcifront.c +6
-9@@ -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; }drivers/xen/xen-pciback/xenbus.c +0
-0
The xen-pcifront are OK, this one:
drivers/xen/xen-pciback/xenbus.c static int xen_pcibk_attach(struct xen_pcibk_device *pdev) · line 187
Quoted 11 lines
drivers/xen/xen-pciback/xenbus.c +1
-4@@ -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);
This one is OK
Quoted 4 lines
drivers/xen/xen-pciback/xenbus.c +0
-0dev_dbg (&pdev->xdev->dev,"Connected? %d\n" , err); out:
drivers/xen/xen-pciback/xenbus.c static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev) · line 500
Quoted 10 lines
drivers/xen/xen-pciback/xenbus.c +1
-6@@ -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);
That is OKish, thought I am not too happy about us holding the lock.
drivers/xen/xen-pciback/xenbus.c static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev) · line 635
Quoted 12 lines
drivers/xen/xen-pciback/xenbus.c +1
-4out: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);
And this one needs that comment above about the mutex.
drivers/xen/xen-pciback/xenbus.c static int xen_pcibk_xenbus_probe(struct xenbus_device *dev, · line 675
Quoted 11 lines
drivers/xen/xen-pciback/xenbus.c +1
-3out: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);
That means the error that is returned on 'probe' is always zero. I don't think that is right as we did fail to establish a connection.
Quoted 19 lines
drivers/xen/xen-pciback/xenbus.c +0
-0/* watch the backend node for backend configuration information */ err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,drivers/xen/xen-scsiback.c +1
-4@@ -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:
Quoted 27 lines · Konrad Rzeszutek Wilk
Quoted 1 line · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()Only on the first depth, not on the subsequent ones (as in if the first
xenbus_switch_failfails, it won't try to callxenbus_switch_stateagain and again).Quoted 2 lines
internally, so need not return any status value, then use 'void' instead of 'int' for
xenbus_switch_state()and__xenbus_switch_state().When that switch occurs (to XenbusStateConnected) won't the watches fire - meaning we MUST make sure that the watch functions - if they use the
xenbus_switch_state()they MUST not hold any locks - because they could be executed once more?Oh wait, we don't have to worry about that right now as the callbacks that pick up the messages from the XenBus are all gated on one mutex anyhow.
Hm, anyhow, I would add this extra piece of information to the patch:
drivers/xen/xen-pciback/xenbus.c +0
-0
drivers/xen/xen-pciback/xenbus.c static void xen_pcibk_be_watch(struct xenbus_watch *watch, · line 669
Quoted 9 lines
drivers/xen/xen-pciback/xenbus.c +6
-0@@ -661,6 +661,12 @@ static void xen_pcibk_be_watch(struct xenbus_watch *watch,switch (xenbus_read_driver_state (pdev->xdev->nodename)) {case XenbusStateInitWait: +/* +* xenbus_switch_state can call xenbus_switch_fatal which will +* immediately set the state to XenbusStateClosing which +* means if we were reading for it here we MUST drop any +* locks so that we don't dead-lock. +*/
Watches are asynchronous and serialised by the xenwatch thread. I can't see what deadlock you're talking about here. Particularly since the backend doesn't watch its own state node (it watches the frontend one).
Quoted 27 lines
drivers/xen/xen-pciback/xenbus.c +0
-0xen_pcibk_setup_backend (pdev);break ;Quoted 3 lines
Also need be sure that all callers which check the return value must let 'err' be 0.
I am bit uncomfortable with that, that is due to:
.. snip..
drivers/net/xen-netback/xenbus.c static int netback_probe(struct xenbus_device *dev, · line 340
Quoted 13 lines
drivers/net/xen-netback/xenbus.c +1
-4@@ -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);Which if it fails it won't call:
Quoted 8 lines
354 fail:
355 pr_debug("failed\n"); 356 netback_remove(dev); 357 return err;And since there is no watch on the backend state to go in Closing it won't ever call those and we leak memory.
It's not leaking the memory. All resources will be recovered when the device is removed.
Quoted 1 line
The same is for xen-blkback mechanism in the probe function.
David
Quoted 75 lines · David Vrabel
Quoted 27 lines · Konrad Rzeszutek Wilk
Quoted 1 line · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()Only on the first depth, not on the subsequent ones (as in if the first
xenbus_switch_failfails, it won't try to callxenbus_switch_stateagain and again).Quoted 2 lines
internally, so need not return any status value, then use 'void' instead of 'int' for
xenbus_switch_state()and__xenbus_switch_state().When that switch occurs (to XenbusStateConnected) won't the watches fire - meaning we MUST make sure that the watch functions - if they use the
xenbus_switch_state()they MUST not hold any locks - because they could be executed once more?Oh wait, we don't have to worry about that right now as the callbacks that pick up the messages from the XenBus are all gated on one mutex anyhow.
Hm, anyhow, I would add this extra piece of information to the patch:
drivers/xen/xen-pciback/xenbus.c +0
-0drivers/xen/xen-pciback/xenbus.c static void xen_pcibk_be_watch(struct xenbus_watch *watch, · line 669
Quoted 9 lines
drivers/xen/xen-pciback/xenbus.c +6
-0@@ -661,6 +661,12 @@ static void xen_pcibk_be_watch(struct xenbus_watch *watch,switch (xenbus_read_driver_state (pdev->xdev->nodename)) {case XenbusStateInitWait: +/* +* xenbus_switch_state can call xenbus_switch_fatal which will +* immediately set the state to XenbusStateClosing which +* means if we were reading for it here we MUST drop any +* locks so that we don't dead-lock. +*/ Watches are asynchronous and serialised by the xenwatch thread. I can't see what deadlock you're talking about here. Particularly since the backend doesn't watch its own state node (it watches the frontend one).
Quoted 33 lines
drivers/xen/xen-pciback/xenbus.c +0
-0xen_pcibk_setup_backend (pdev);break ;Quoted 3 lines
Also need be sure that all callers which check the return value must let 'err' be 0.
I am bit uncomfortable with that, that is due to:
.. snip..
drivers/net/xen-netback/xenbus.c static int netback_probe(struct xenbus_device *dev, · line 340
Quoted 13 lines
drivers/net/xen-netback/xenbus.c +1
-4@@ -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);Which if it fails it won't call:
354 fail:
355 pr_debug("failed\n"); 356 netback_remove(dev); 357 return err;
Quoted 5 lines
Quoted 2 lines
And since there is no watch on the backend state to go in Closing it won't ever call those and we leak memory.
It's not leaking the memory. All resources will be recovered when the device is removed.
I presume you mean when the XenBus entries are torn down? It does look like it would call the .remove functionality. That should take care of that.
In which case we can just remove all of the 'netback_remove()' and also remove some of the labels.
Quoted 5 lines
Quoted 2 lines
The same is for xen-blkback mechanism in the probe function.
David
Quoted 79 lines · Konrad Rzeszutek Wilk
Quoted 78 lines · David Vrabel
Quoted 27 lines · Konrad Rzeszutek Wilk
Quoted 1 line · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()Only on the first depth, not on the subsequent ones (as in if the first
xenbus_switch_failfails, it won't try to callxenbus_switch_stateagain and again).Quoted 2 lines
internally, so need not return any status value, then use 'void' instead of 'int' for
xenbus_switch_state()and__xenbus_switch_state().When that switch occurs (to XenbusStateConnected) won't the watches fire - meaning we MUST make sure that the watch functions - if they use the
xenbus_switch_state()they MUST not hold any locks - because they could be executed once more?Oh wait, we don't have to worry about that right now as the callbacks that pick up the messages from the XenBus are all gated on one mutex anyhow.
Hm, anyhow, I would add this extra piece of information to the patch:
drivers/xen/xen-pciback/xenbus.c +0
-0drivers/xen/xen-pciback/xenbus.c static void xen_pcibk_be_watch(struct xenbus_watch *watch, · line 669
Quoted 9 lines
drivers/xen/xen-pciback/xenbus.c +6
-0@@ -661,6 +661,12 @@ static void xen_pcibk_be_watch(struct xenbus_watch *watch,switch (xenbus_read_driver_state (pdev->xdev->nodename)) {case XenbusStateInitWait: +/* +* xenbus_switch_state can call xenbus_switch_fatal which will +* immediately set the state to XenbusStateClosing which +* means if we were reading for it here we MUST drop any +* locks so that we don't dead-lock. +*/ Watches are asynchronous and serialised by the xenwatch thread. I can't see what deadlock you're talking about here. Particularly since the backend doesn't watch its own state node (it watches the frontend one).
Quoted 35 lines
drivers/xen/xen-pciback/xenbus.c +0
-0xen_pcibk_setup_backend (pdev);break ;Quoted 3 lines
Also need be sure that all callers which check the return value must let 'err' be 0.
I am bit uncomfortable with that, that is due to:
.. snip..
drivers/net/xen-netback/xenbus.c static int netback_probe(struct xenbus_device *dev, · line 340
Quoted 13 lines
drivers/net/xen-netback/xenbus.c +1
-4@@ -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);Which if it fails it won't call:
354 fail:
355 pr_debug("failed\n"); 356 netback_remove(dev); 357 return err;And since there is no watch on the backend state to go in Closing it won't ever call those and we leak memory.
Quoted 8 lines
Quoted 2 lines
It's not leaking the memory. All resources will be recovered when the device is removed.
I presume you mean when the XenBus entries are torn down? It does look like it would call the .remove functionality. That should take care of that.
In which case we can just remove all of the '
netback_remove()' and also remove some of the labels.
No. If the final xenbus_switch_state() fails then at least the device is in a consistent state, waiting for the other end to notice.
We don't want to return success from a probe with a half-setup device.
David
Quoted 79 lines · Konrad Rzeszutek Wilk
Quoted 78 lines · David Vrabel
Quoted 27 lines · Konrad Rzeszutek Wilk
Quoted 1 line · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()Only on the first depth, not on the subsequent ones (as in if the first
xenbus_switch_failfails, it won't try to callxenbus_switch_stateagain and again).Quoted 2 lines
internally, so need not return any status value, then use 'void' instead of 'int' for
xenbus_switch_state()and__xenbus_switch_state().When that switch occurs (to XenbusStateConnected) won't the watches fire - meaning we MUST make sure that the watch functions - if they use the
xenbus_switch_state()they MUST not hold any locks - because they could be executed once more?Oh wait, we don't have to worry about that right now as the callbacks that pick up the messages from the XenBus are all gated on one mutex anyhow.
Hm, anyhow, I would add this extra piece of information to the patch:
drivers/xen/xen-pciback/xenbus.c +0
-0drivers/xen/xen-pciback/xenbus.c static void xen_pcibk_be_watch(struct xenbus_watch *watch, · line 669
Quoted 9 lines
drivers/xen/xen-pciback/xenbus.c +6
-0@@ -661,6 +661,12 @@ static void xen_pcibk_be_watch(struct xenbus_watch *watch,switch (xenbus_read_driver_state (pdev->xdev->nodename)) {case XenbusStateInitWait: +/* +* xenbus_switch_state can call xenbus_switch_fatal which will +* immediately set the state to XenbusStateClosing which +* means if we were reading for it here we MUST drop any +* locks so that we don't dead-lock. +*/ Watches are asynchronous and serialised by the xenwatch thread. I can't see what deadlock you're talking about here. Particularly since the backend doesn't watch its own state node (it watches the frontend one).
Quoted 35 lines
drivers/xen/xen-pciback/xenbus.c +0
-0xen_pcibk_setup_backend (pdev);break ;Quoted 3 lines
Also need be sure that all callers which check the return value must let 'err' be 0.
I am bit uncomfortable with that, that is due to:
.. snip..
drivers/net/xen-netback/xenbus.c static int netback_probe(struct xenbus_device *dev, · line 340
Quoted 13 lines
drivers/net/xen-netback/xenbus.c +1
-4@@ -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);Which if it fails it won't call:
354 fail:
355 pr_debug("failed\n"); 356 netback_remove(dev); 357 return err;And since there is no watch on the backend state to go in Closing it won't ever call those and we leak memory.
Quoted 8 lines
Quoted 2 lines
It's not leaking the memory. All resources will be recovered when the device is removed.
I presume you mean when the XenBus entries are torn down? It does look like it would call the .remove functionality. That should take care of that.
In which case we can just remove all of the '
netback_remove()' and also remove some of the labels.
No. If the final xenbus_switch_state() fails then at least the device is in a consistent state, waiting for the other end to notice.
We don't want to return success from a probe with a half-setup device.
David
Quoted 75 lines · David Vrabel
Quoted 27 lines · Konrad Rzeszutek Wilk
Quoted 1 line · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()Only on the first depth, not on the subsequent ones (as in if the first
xenbus_switch_failfails, it won't try to callxenbus_switch_stateagain and again).Quoted 2 lines
internally, so need not return any status value, then use 'void' instead of 'int' for
xenbus_switch_state()and__xenbus_switch_state().When that switch occurs (to XenbusStateConnected) won't the watches fire - meaning we MUST make sure that the watch functions - if they use the
xenbus_switch_state()they MUST not hold any locks - because they could be executed once more?Oh wait, we don't have to worry about that right now as the callbacks that pick up the messages from the XenBus are all gated on one mutex anyhow.
Hm, anyhow, I would add this extra piece of information to the patch:
drivers/xen/xen-pciback/xenbus.c +0
-0drivers/xen/xen-pciback/xenbus.c static void xen_pcibk_be_watch(struct xenbus_watch *watch, · line 669
Quoted 9 lines
drivers/xen/xen-pciback/xenbus.c +6
-0@@ -661,6 +661,12 @@ static void xen_pcibk_be_watch(struct xenbus_watch *watch,switch (xenbus_read_driver_state (pdev->xdev->nodename)) {case XenbusStateInitWait: +/* +* xenbus_switch_state can call xenbus_switch_fatal which will +* immediately set the state to XenbusStateClosing which +* means if we were reading for it here we MUST drop any +* locks so that we don't dead-lock. +*/ Watches are asynchronous and serialised by the xenwatch thread. I can't see what deadlock you're talking about here. Particularly since the backend doesn't watch its own state node (it watches the frontend one).
Quoted 33 lines
drivers/xen/xen-pciback/xenbus.c +0
-0xen_pcibk_setup_backend (pdev);break ;Quoted 3 lines
Also need be sure that all callers which check the return value must let 'err' be 0.
I am bit uncomfortable with that, that is due to:
.. snip..
drivers/net/xen-netback/xenbus.c static int netback_probe(struct xenbus_device *dev, · line 340
Quoted 13 lines
drivers/net/xen-netback/xenbus.c +1
-4@@ -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);Which if it fails it won't call:
354 fail:
355 pr_debug("failed\n"); 356 netback_remove(dev); 357 return err;
Quoted 5 lines
Quoted 2 lines
And since there is no watch on the backend state to go in Closing it won't ever call those and we leak memory.
It's not leaking the memory. All resources will be recovered when the device is removed.
I presume you mean when the XenBus entries are torn down? It does look like it would call the .remove functionality. That should take care of that.
In which case we can just remove all of the 'netback_remove()' and also remove some of the labels.
Quoted 5 lines
Quoted 2 lines
The same is for xen-blkback mechanism in the probe function.
David
Quoted 27 lines · Konrad Rzeszutek Wilk
Quoted 1 line · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()Only on the first depth, not on the subsequent ones (as in if the first
xenbus_switch_failfails, it won't try to callxenbus_switch_stateagain and again).Quoted 2 lines
internally, so need not return any status value, then use 'void' instead of 'int' for
xenbus_switch_state()and__xenbus_switch_state().When that switch occurs (to XenbusStateConnected) won't the watches fire - meaning we MUST make sure that the watch functions - if they use the
xenbus_switch_state()they MUST not hold any locks - because they could be executed once more?Oh wait, we don't have to worry about that right now as the callbacks that pick up the messages from the XenBus are all gated on one mutex anyhow.
Hm, anyhow, I would add this extra piece of information to the patch:
drivers/xen/xen-pciback/xenbus.c +0
-0
drivers/xen/xen-pciback/xenbus.c static void xen_pcibk_be_watch(struct xenbus_watch *watch, · line 669
Quoted 9 lines
drivers/xen/xen-pciback/xenbus.c +6
-0@@ -661,6 +661,12 @@ static void xen_pcibk_be_watch(struct xenbus_watch *watch,switch (xenbus_read_driver_state (pdev->xdev->nodename)) {case XenbusStateInitWait: +/* +* xenbus_switch_state can call xenbus_switch_fatal which will +* immediately set the state to XenbusStateClosing which +* means if we were reading for it here we MUST drop any +* locks so that we don't dead-lock. +*/
Watches are asynchronous and serialised by the xenwatch thread. I can't see what deadlock you're talking about here. Particularly since the backend doesn't watch its own state node (it watches the frontend one).
Quoted 27 lines
drivers/xen/xen-pciback/xenbus.c +0
-0xen_pcibk_setup_backend (pdev);break ;Quoted 3 lines
Also need be sure that all callers which check the return value must let 'err' be 0.
I am bit uncomfortable with that, that is due to:
.. snip..
drivers/net/xen-netback/xenbus.c static int netback_probe(struct xenbus_device *dev, · line 340
Quoted 13 lines
drivers/net/xen-netback/xenbus.c +1
-4@@ -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);Which if it fails it won't call:
Quoted 8 lines
354 fail:
355 pr_debug("failed\n"); 356 netback_remove(dev); 357 return err;And since there is no watch on the backend state to go in Closing it won't ever call those and we leak memory.
It's not leaking the memory. All resources will be recovered when the device is removed.
Quoted 1 line
The same is for xen-blkback mechanism in the probe function.
David
Quoted 7 lines · Konrad Rzeszutek Wilk
Quoted 1 line · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()Only on the first depth, not on the subsequent ones (as in if the first
xenbus_switch_failfails, it won't try to callxenbus_switch_stateagain and again).
Yeah, I guess you want to give more completion for this comment, do not mean the original comments is incorrect.
[...]
Quoted 24 lines
Quoted 3 lines
Also need be sure that all callers which check the return value must let 'err' be 0.
I am bit uncomfortable with that, that is due to:
.. snip..
drivers/net/xen-netback/xenbus.c static int netback_probe(struct xenbus_device *dev, · line 340
Quoted 13 lines
drivers/net/xen-netback/xenbus.c +1
-4@@ -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);Which if it fails it won't call:
Quoted 5 lines
354 fail:
355 pr_debug("failed\n"); 356 netback_remove(dev); 357 return err;
Originally, I intended left 'fail' code block for the next patch (which I thought the next patch would need it).
Hmm... but any way, originally, I really need give additional comments for it.
[...]
I skip all other contents which have already discussed by maintainers, if I really miss something, please let me know.
Thanks.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed
Quoted 7 lines · Konrad Rzeszutek Wilk
Quoted 1 line · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()Only on the first depth, not on the subsequent ones (as in if the first
xenbus_switch_failfails, it won't try to callxenbus_switch_stateagain and again).
Yeah, I guess you want to give more completion for this comment, do not mean the original comments is incorrect.
[...]
Quoted 24 lines
Quoted 3 lines
Also need be sure that all callers which check the return value must let 'err' be 0.
I am bit uncomfortable with that, that is due to:
.. snip..
drivers/net/xen-netback/xenbus.c static int netback_probe(struct xenbus_device *dev, · line 340
Quoted 13 lines
drivers/net/xen-netback/xenbus.c +1
-4@@ -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);Which if it fails it won't call:
Quoted 5 lines
354 fail:
355 pr_debug("failed\n"); 356 netback_remove(dev); 357 return err;
Originally, I intended left 'fail' code block for the next patch (which I thought the next patch would need it).
Hmm... but any way, originally, I really need give additional comments for it.
[...]
I skip all other contents which have already discussed by maintainers, if I really miss something, please let me know.
Thanks.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed
Quoted 1 line · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()
Only on the first depth, not on the subsequent ones (as in if the first xenbus_switch_fail fails, it won't try to call xenbus_switch_state again and again).
Quoted 2 lines
internally, so need not return any status value, then use 'void' instead of 'int' for
xenbus_switch_state()and__xenbus_switch_state().
When that switch occurs (to XenbusStateConnected) won't the watches fire - meaning we MUST make sure that the watch functions - if they use the xenbus_switch_state() they MUST not hold any locks - because they could be executed once more?
Oh wait, we don't have to worry about that right now as the callbacks that pick up the messages from the XenBus are all gated on one mutex anyhow.
Hm, anyhow, I would add this extra piece of information to the patch:
drivers/xen/xen-pciback/xenbus.c +6 -0
@@ -661,6 +661,12 @@ static void xen_pcibk_be_watch(struct xenbus_watch *watch,switch (xenbus_read_driver_state (pdev->xdev->nodename)) {case XenbusStateInitWait: +/* +* xenbus_switch_state can call xenbus_switch_fatal which will +* immediately set the state to XenbusStateClosing which +* means if we were reading for it here we MUST drop any +* locks so that we don't dead-lock. +*/ xen_pcibk_setup_backend (pdev);break ;
Quoted 3 lines
Also need be sure that all callers which check the return value must let 'err' be 0.
I am bit uncomfortable with that, that is due to:
.. snip..
drivers/net/xen-netback/xenbus.c static int netback_probe(struct xenbus_device *dev, · line 340
Quoted 13 lines
drivers/net/xen-netback/xenbus.c +1
-4@@ -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);
Which if it fails it won't call:
354 fail:
355 pr_debug("failed\n");
356 netback_remove(dev);
357 return err; And since there is no watch on the backend state to go in Closing it won't ever call those and we leak memory.
The same is for xen-blkback mechanism in the probe function.
drivers/xen/xen-pciback/xenbus.c
Quoted 55 lines
drivers/net/xen-netback/xenbus.c +0
-0be->state = XenbusStateInitWait;/* This kicks hotplug scripts, so do it immediately. */ drivers/pci/xen-pcifront.c +6
-9@@ -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; }drivers/xen/xen-pciback/xenbus.c +0
-0
The xen-pcifront are OK, this one:
drivers/xen/xen-pciback/xenbus.c static int xen_pcibk_attach(struct xen_pcibk_device *pdev) · line 187
Quoted 11 lines
drivers/xen/xen-pciback/xenbus.c +1
-4@@ -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);
This one is OK
Quoted 4 lines
drivers/xen/xen-pciback/xenbus.c +0
-0dev_dbg (&pdev->xdev->dev,"Connected? %d\n" , err); out:
drivers/xen/xen-pciback/xenbus.c static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev) · line 500
Quoted 10 lines
drivers/xen/xen-pciback/xenbus.c +1
-6@@ -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);
That is OKish, thought I am not too happy about us holding the lock.
drivers/xen/xen-pciback/xenbus.c static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev) · line 635
Quoted 12 lines
drivers/xen/xen-pciback/xenbus.c +1
-4out: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);
And this one needs that comment above about the mutex.
drivers/xen/xen-pciback/xenbus.c static int xen_pcibk_xenbus_probe(struct xenbus_device *dev, · line 675
Quoted 11 lines
drivers/xen/xen-pciback/xenbus.c +1
-3out: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);
That means the error that is returned on 'probe' is always zero. I don't think that is right as we did fail to establish a connection.
Quoted 19 lines
drivers/xen/xen-pciback/xenbus.c +0
-0/* watch the backend node for backend configuration information */ err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,drivers/xen/xen-scsiback.c +1
-4@@ -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:
Quoted 8 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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
Quoted 5 lines · David Vrabel
Quoted 4 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_switch_state()and__xenbus_switch_state().
Quoted 8 lines
Quoted 4 lines
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.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed
Quoted 5 lines · David Vrabel
Quoted 4 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_switch_state()and__xenbus_switch_state().
Quoted 8 lines
Quoted 4 lines
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.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed
Quoted 8 lines · Chen Gang
When
xenbus_switch_state()fails, it will callxenbus_switch_fatal()internally, so need not return any status value, then use 'void' instead of 'int' forxenbus_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