* [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
@ 2014-09-26 16:36 Chen Gang
2014-09-26 18:07 ` David Vrabel
` (9 more replies)
0 siblings, 10 replies; 32+ messages in thread
From: Chen Gang @ 2014-09-26 16:36 UTC (permalink / raw)
To: David Vrabel, konrad.wilk, ian.campbell, wei.liu2,
boris.ostrovsky, bhelgaas, jgross, yongjun_wei, mukesh.rathor
Cc: xen-devel, netdev, linux-pci, linux-scsi, linux-kernel
When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
internally, so need not return any status value, then use 'void' instead
of 'int' for xenbus_switch_state() and __xenbus_switch_state().
Also need be sure that all callers which check the return value must let
'err' be 0.
And also need change the related comments for xenbus_switch_state().
Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
drivers/block/xen-blkback/xenbus.c | 9 ++-------
drivers/net/xen-netback/xenbus.c | 5 +----
drivers/pci/xen-pcifront.c | 15 ++++++---------
drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
drivers/xen/xen-scsiback.c | 5 +----
drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
include/xen/xenbus.h | 3 ++-
7 files changed, 24 insertions(+), 50 deletions(-)
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 3a8b810..fdcc584 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
if (err)
goto fail;
- err = xenbus_switch_state(dev, XenbusStateInitWait);
- if (err)
- goto fail;
+ xenbus_switch_state(dev, XenbusStateInitWait);
return 0;
@@ -837,10 +835,7 @@ again:
if (err)
xenbus_dev_fatal(dev, err, "ending transaction");
- err = xenbus_switch_state(dev, XenbusStateConnected);
- if (err)
- xenbus_dev_fatal(dev, err, "%s: switching to Connected state",
- dev->nodename);
+ xenbus_switch_state(dev, XenbusStateConnected);
return;
abort:
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 9c47b89..b5c3d47 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
if (err)
pr_debug("Error writing multi-queue-max-queues\n");
- err = xenbus_switch_state(dev, XenbusStateInitWait);
- if (err)
- goto fail;
-
+ xenbus_switch_state(dev, XenbusStateInitWait);
be->state = XenbusStateInitWait;
/* This kicks hotplug scripts, so do it immediately. */
diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index 53df39a..c1d73b7 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
}
}
- err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
-
+ xenbus_switch_state(pdev->xdev, XenbusStateConnected);
+ return 0;
out:
return err;
}
static int pcifront_try_disconnect(struct pcifront_device *pdev)
{
- int err = 0;
enum xenbus_state prev_state;
-
prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
if (prev_state >= XenbusStateClosing)
@@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
pcifront_disconnect(pdev);
}
- err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
+ xenbus_switch_state(pdev->xdev, XenbusStateClosed);
out:
-
- return err;
+ return 0;
}
static int pcifront_attach_devices(struct pcifront_device *pdev)
@@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
domain, bus, slot, func);
}
- err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
-
+ xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
+ return 0;
out:
return err;
}
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index c214daa..630a215 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
dev_dbg(&pdev->xdev->dev, "Connecting...\n");
- err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
- if (err)
- xenbus_dev_fatal(pdev->xdev, err,
- "Error switching to connected state!");
+ xenbus_switch_state(pdev->xdev, XenbusStateConnected);
dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err);
out:
@@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
}
}
- err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
- if (err) {
- xenbus_dev_fatal(pdev->xdev, err,
- "Error switching to reconfigured state!");
- goto out;
- }
+ xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
out:
mutex_unlock(&pdev->dev_lock);
@@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev)
goto out;
}
- err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
- if (err)
- xenbus_dev_fatal(pdev->xdev, err,
- "Error switching to initialised state!");
+ xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
out:
mutex_unlock(&pdev->dev_lock);
@@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
}
/* wait for xend to configure us */
- err = xenbus_switch_state(dev, XenbusStateInitWait);
- if (err)
- goto out;
+ xenbus_switch_state(dev, XenbusStateInitWait);
/* watch the backend node for backend configuration information */
err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index ad11258..847bc9c 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
if (err)
xenbus_dev_error(dev, err, "writing feature-sg-grant");
- err = xenbus_switch_state(dev, XenbusStateInitWait);
- if (err)
- goto fail;
-
+ xenbus_switch_state(dev, XenbusStateInitWait);
return 0;
fail:
diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index ca74410..e2bcd1d 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
static void xenbus_switch_fatal(struct xenbus_device *, int, int,
const char *, ...);
-static int
+static void
__xenbus_switch_state(struct xenbus_device *dev,
enum xenbus_state state, int depth)
{
@@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev,
int err, abort;
if (state == dev->state)
- return 0;
+ return;
again:
abort = 1;
@@ -196,7 +196,7 @@ again:
err = xenbus_transaction_start(&xbt);
if (err) {
xenbus_switch_fatal(dev, depth, err, "starting transaction");
- return 0;
+ return;
}
err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state);
@@ -219,7 +219,7 @@ abort:
} else
dev->state = state;
- return 0;
+ return;
}
/**
@@ -228,12 +228,12 @@ abort:
* @state: new state
*
* Advertise in the store a change of the given driver to the given new_state.
- * Return 0 on success, or -errno on error. On error, the device will switch
- * to XenbusStateClosing, and the error will be saved in the store.
+ * When failure occurs, it will call xenbus_switch_fatal() internally, so need
+ * not return value to notify upper caller.
*/
-int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
+void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
{
- return __xenbus_switch_state(dev, state, 0);
+ __xenbus_switch_state(dev, state, 0);
}
EXPORT_SYMBOL_GPL(xenbus_switch_state);
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index 0324c6d..587c53d 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
const char **, unsigned int),
const char *pathfmt, ...);
-int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
+void xenbus_switch_state(struct xenbus_device *dev,
+ enum xenbus_state new_state);
int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn);
int xenbus_map_ring_valloc(struct xenbus_device *dev,
int gnt_ref, void **vaddr);
--
1.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 16:36 [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state() Chen Gang
@ 2014-09-26 18:07 ` David Vrabel
2014-09-26 18:07 ` David Vrabel
` (8 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: David Vrabel @ 2014-09-26 18:07 UTC (permalink / raw)
To: Chen Gang, David Vrabel, konrad.wilk, ian.campbell, wei.liu2,
boris.ostrovsky, bhelgaas, jgross, yongjun_wei, mukesh.rathor
Cc: xen-devel, linux-pci, linux-kernel, linux-scsi, netdev
On 26/09/14 17:36, Chen Gang wrote:
> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> internally, so need not return any status value, then use 'void' instead
> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>
> Also need be sure that all callers which check the return value must let
> 'err' be 0.
I've rewritten the commit message as:
xen/xenbus: don't return errors from xenbus_switch_state()
Most users of xenbus_switch_state() weren't handling the failure of
xenbus_switch_state() correctly. They either called
xenbus_dev_fatal() (which xenbus_switch_state() has effectively
already tried to do), or ignored errors.
xenbus_switch_state() may fail because:
a) The device is being unplugged by the toolstack. The device will
shortly be removed and the error does not need to be handled.
b) Xenstore is broken. There isn't much the driver can do in this
case since xenstore is required to signal failure to the toolstack.
So, don't report any errors from xenbus_switch_state() which removes
some unnecessary error handling in some of the drivers.
I'd appreciate a review from some of the other front/backend driver
maintainers on whether this is sensible reasoning.
David
>
> And also need change the related comments for xenbus_switch_state().
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
> drivers/block/xen-blkback/xenbus.c | 9 ++-------
> drivers/net/xen-netback/xenbus.c | 5 +----
> drivers/pci/xen-pcifront.c | 15 ++++++---------
> drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
> drivers/xen/xen-scsiback.c | 5 +----
> drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
> include/xen/xenbus.h | 3 ++-
> 7 files changed, 24 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index 3a8b810..fdcc584 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
> if (err)
> goto fail;
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> return 0;
>
> @@ -837,10 +835,7 @@ again:
> if (err)
> xenbus_dev_fatal(dev, err, "ending transaction");
>
> - err = xenbus_switch_state(dev, XenbusStateConnected);
> - if (err)
> - xenbus_dev_fatal(dev, err, "%s: switching to Connected state",
> - dev->nodename);
> + xenbus_switch_state(dev, XenbusStateConnected);
>
> return;
> abort:
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 9c47b89..b5c3d47 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
> if (err)
> pr_debug("Error writing multi-queue-max-queues\n");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
> be->state = XenbusStateInitWait;
>
> /* This kicks hotplug scripts, so do it immediately. */
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 53df39a..c1d73b7 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> }
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> + return 0;
> out:
> return err;
> }
>
> static int pcifront_try_disconnect(struct pcifront_device *pdev)
> {
> - int err = 0;
> enum xenbus_state prev_state;
>
> -
> prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
>
> if (prev_state >= XenbusStateClosing)
> @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
> pcifront_disconnect(pdev);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
> + xenbus_switch_state(pdev->xdev, XenbusStateClosed);
>
> out:
> -
> - return err;
> + return 0;
> }
>
> static int pcifront_attach_devices(struct pcifront_device *pdev)
> @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> domain, bus, slot, func);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> + return 0;
> out:
> return err;
> }
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c214daa..630a215 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
>
> dev_dbg(&pdev->xdev->dev, "Connecting...\n");
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> - if (err)
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to connected state!");
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>
> dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err);
> out:
> @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
> }
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
> - if (err) {
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to reconfigured state!");
> - goto out;
> - }
> + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
>
> out:
> mutex_unlock(&pdev->dev_lock);
> @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev)
> goto out;
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
> - if (err)
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to initialised state!");
> + xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
>
> out:
> mutex_unlock(&pdev->dev_lock);
> @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
> }
>
> /* wait for xend to configure us */
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto out;
> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> /* watch the backend node for backend configuration information */
> err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index ad11258..847bc9c 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
> if (err)
> xenbus_dev_error(dev, err, "writing feature-sg-grant");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
> return 0;
>
> fail:
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index ca74410..e2bcd1d 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
> static void xenbus_switch_fatal(struct xenbus_device *, int, int,
> const char *, ...);
>
> -static int
> +static void
> __xenbus_switch_state(struct xenbus_device *dev,
> enum xenbus_state state, int depth)
> {
> @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev,
> int err, abort;
>
> if (state == dev->state)
> - return 0;
> + return;
>
> again:
> abort = 1;
> @@ -196,7 +196,7 @@ again:
> err = xenbus_transaction_start(&xbt);
> if (err) {
> xenbus_switch_fatal(dev, depth, err, "starting transaction");
> - return 0;
> + return;
> }
>
> err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state);
> @@ -219,7 +219,7 @@ abort:
> } else
> dev->state = state;
>
> - return 0;
> + return;
> }
>
> /**
> @@ -228,12 +228,12 @@ abort:
> * @state: new state
> *
> * Advertise in the store a change of the given driver to the given new_state.
> - * Return 0 on success, or -errno on error. On error, the device will switch
> - * to XenbusStateClosing, and the error will be saved in the store.
> + * When failure occurs, it will call xenbus_switch_fatal() internally, so need
> + * not return value to notify upper caller.
> */
> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> {
> - return __xenbus_switch_state(dev, state, 0);
> + __xenbus_switch_state(dev, state, 0);
> }
>
> EXPORT_SYMBOL_GPL(xenbus_switch_state);
> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> index 0324c6d..587c53d 100644
> --- a/include/xen/xenbus.h
> +++ b/include/xen/xenbus.h
> @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
> const char **, unsigned int),
> const char *pathfmt, ...);
>
> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
> +void xenbus_switch_state(struct xenbus_device *dev,
> + enum xenbus_state new_state);
> int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn);
> int xenbus_map_ring_valloc(struct xenbus_device *dev,
> int gnt_ref, void **vaddr);
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
@ 2014-09-26 18:07 ` David Vrabel
0 siblings, 0 replies; 32+ messages in thread
From: David Vrabel @ 2014-09-26 18:07 UTC (permalink / raw)
To: Chen Gang, David Vrabel, konrad.wilk, ian.campbell, wei.liu2,
boris.ostrovsky, bhelgaas, jgross, yongjun_wei, mukesh.rathor
Cc: xen-devel, linux-pci, linux-kernel, linux-scsi, netdev
On 26/09/14 17:36, Chen Gang wrote:
> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> internally, so need not return any status value, then use 'void' instead
> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>
> Also need be sure that all callers which check the return value must let
> 'err' be 0.
I've rewritten the commit message as:
xen/xenbus: don't return errors from xenbus_switch_state()
Most users of xenbus_switch_state() weren't handling the failure of
xenbus_switch_state() correctly. They either called
xenbus_dev_fatal() (which xenbus_switch_state() has effectively
already tried to do), or ignored errors.
xenbus_switch_state() may fail because:
a) The device is being unplugged by the toolstack. The device will
shortly be removed and the error does not need to be handled.
b) Xenstore is broken. There isn't much the driver can do in this
case since xenstore is required to signal failure to the toolstack.
So, don't report any errors from xenbus_switch_state() which removes
some unnecessary error handling in some of the drivers.
I'd appreciate a review from some of the other front/backend driver
maintainers on whether this is sensible reasoning.
David
>
> And also need change the related comments for xenbus_switch_state().
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
> drivers/block/xen-blkback/xenbus.c | 9 ++-------
> drivers/net/xen-netback/xenbus.c | 5 +----
> drivers/pci/xen-pcifront.c | 15 ++++++---------
> drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
> drivers/xen/xen-scsiback.c | 5 +----
> drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
> include/xen/xenbus.h | 3 ++-
> 7 files changed, 24 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index 3a8b810..fdcc584 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
> if (err)
> goto fail;
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> return 0;
>
> @@ -837,10 +835,7 @@ again:
> if (err)
> xenbus_dev_fatal(dev, err, "ending transaction");
>
> - err = xenbus_switch_state(dev, XenbusStateConnected);
> - if (err)
> - xenbus_dev_fatal(dev, err, "%s: switching to Connected state",
> - dev->nodename);
> + xenbus_switch_state(dev, XenbusStateConnected);
>
> return;
> abort:
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 9c47b89..b5c3d47 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
> if (err)
> pr_debug("Error writing multi-queue-max-queues\n");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
> be->state = XenbusStateInitWait;
>
> /* This kicks hotplug scripts, so do it immediately. */
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 53df39a..c1d73b7 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> }
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> + return 0;
> out:
> return err;
> }
>
> static int pcifront_try_disconnect(struct pcifront_device *pdev)
> {
> - int err = 0;
> enum xenbus_state prev_state;
>
> -
> prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
>
> if (prev_state >= XenbusStateClosing)
> @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
> pcifront_disconnect(pdev);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
> + xenbus_switch_state(pdev->xdev, XenbusStateClosed);
>
> out:
> -
> - return err;
> + return 0;
> }
>
> static int pcifront_attach_devices(struct pcifront_device *pdev)
> @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> domain, bus, slot, func);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> + return 0;
> out:
> return err;
> }
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c214daa..630a215 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
>
> dev_dbg(&pdev->xdev->dev, "Connecting...\n");
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> - if (err)
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to connected state!");
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>
> dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err);
> out:
> @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
> }
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
> - if (err) {
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to reconfigured state!");
> - goto out;
> - }
> + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
>
> out:
> mutex_unlock(&pdev->dev_lock);
> @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev)
> goto out;
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
> - if (err)
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to initialised state!");
> + xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
>
> out:
> mutex_unlock(&pdev->dev_lock);
> @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
> }
>
> /* wait for xend to configure us */
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto out;
> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> /* watch the backend node for backend configuration information */
> err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index ad11258..847bc9c 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
> if (err)
> xenbus_dev_error(dev, err, "writing feature-sg-grant");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
> return 0;
>
> fail:
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index ca74410..e2bcd1d 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
> static void xenbus_switch_fatal(struct xenbus_device *, int, int,
> const char *, ...);
>
> -static int
> +static void
> __xenbus_switch_state(struct xenbus_device *dev,
> enum xenbus_state state, int depth)
> {
> @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev,
> int err, abort;
>
> if (state == dev->state)
> - return 0;
> + return;
>
> again:
> abort = 1;
> @@ -196,7 +196,7 @@ again:
> err = xenbus_transaction_start(&xbt);
> if (err) {
> xenbus_switch_fatal(dev, depth, err, "starting transaction");
> - return 0;
> + return;
> }
>
> err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state);
> @@ -219,7 +219,7 @@ abort:
> } else
> dev->state = state;
>
> - return 0;
> + return;
> }
>
> /**
> @@ -228,12 +228,12 @@ abort:
> * @state: new state
> *
> * Advertise in the store a change of the given driver to the given new_state.
> - * Return 0 on success, or -errno on error. On error, the device will switch
> - * to XenbusStateClosing, and the error will be saved in the store.
> + * When failure occurs, it will call xenbus_switch_fatal() internally, so need
> + * not return value to notify upper caller.
> */
> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> {
> - return __xenbus_switch_state(dev, state, 0);
> + __xenbus_switch_state(dev, state, 0);
> }
>
> EXPORT_SYMBOL_GPL(xenbus_switch_state);
> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> index 0324c6d..587c53d 100644
> --- a/include/xen/xenbus.h
> +++ b/include/xen/xenbus.h
> @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
> const char **, unsigned int),
> const char *pathfmt, ...);
>
> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
> +void xenbus_switch_state(struct xenbus_device *dev,
> + enum xenbus_state new_state);
> int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn);
> int xenbus_map_ring_valloc(struct xenbus_device *dev,
> int gnt_ref, void **vaddr);
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 16:36 [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state() Chen Gang
2014-09-26 18:07 ` David Vrabel
@ 2014-09-26 18:07 ` David Vrabel
2014-09-29 5:09 ` [Xen-devel] " Juergen Gross
` (7 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: David Vrabel @ 2014-09-26 18:07 UTC (permalink / raw)
To: Chen Gang, David Vrabel, konrad.wilk, ian.campbell, wei.liu2,
boris.ostrovsky, bhelgaas, jgross, yongjun_wei, mukesh.rathor
Cc: xen-devel, netdev, linux-kernel, linux-scsi, linux-pci
On 26/09/14 17:36, Chen Gang wrote:
> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> internally, so need not return any status value, then use 'void' instead
> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>
> Also need be sure that all callers which check the return value must let
> 'err' be 0.
I've rewritten the commit message as:
xen/xenbus: don't return errors from xenbus_switch_state()
Most users of xenbus_switch_state() weren't handling the failure of
xenbus_switch_state() correctly. They either called
xenbus_dev_fatal() (which xenbus_switch_state() has effectively
already tried to do), or ignored errors.
xenbus_switch_state() may fail because:
a) The device is being unplugged by the toolstack. The device will
shortly be removed and the error does not need to be handled.
b) Xenstore is broken. There isn't much the driver can do in this
case since xenstore is required to signal failure to the toolstack.
So, don't report any errors from xenbus_switch_state() which removes
some unnecessary error handling in some of the drivers.
I'd appreciate a review from some of the other front/backend driver
maintainers on whether this is sensible reasoning.
David
>
> And also need change the related comments for xenbus_switch_state().
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
> drivers/block/xen-blkback/xenbus.c | 9 ++-------
> drivers/net/xen-netback/xenbus.c | 5 +----
> drivers/pci/xen-pcifront.c | 15 ++++++---------
> drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
> drivers/xen/xen-scsiback.c | 5 +----
> drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
> include/xen/xenbus.h | 3 ++-
> 7 files changed, 24 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index 3a8b810..fdcc584 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
> if (err)
> goto fail;
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> return 0;
>
> @@ -837,10 +835,7 @@ again:
> if (err)
> xenbus_dev_fatal(dev, err, "ending transaction");
>
> - err = xenbus_switch_state(dev, XenbusStateConnected);
> - if (err)
> - xenbus_dev_fatal(dev, err, "%s: switching to Connected state",
> - dev->nodename);
> + xenbus_switch_state(dev, XenbusStateConnected);
>
> return;
> abort:
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 9c47b89..b5c3d47 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
> if (err)
> pr_debug("Error writing multi-queue-max-queues\n");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
> be->state = XenbusStateInitWait;
>
> /* This kicks hotplug scripts, so do it immediately. */
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 53df39a..c1d73b7 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> }
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> + return 0;
> out:
> return err;
> }
>
> static int pcifront_try_disconnect(struct pcifront_device *pdev)
> {
> - int err = 0;
> enum xenbus_state prev_state;
>
> -
> prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
>
> if (prev_state >= XenbusStateClosing)
> @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
> pcifront_disconnect(pdev);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
> + xenbus_switch_state(pdev->xdev, XenbusStateClosed);
>
> out:
> -
> - return err;
> + return 0;
> }
>
> static int pcifront_attach_devices(struct pcifront_device *pdev)
> @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> domain, bus, slot, func);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> + return 0;
> out:
> return err;
> }
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c214daa..630a215 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
>
> dev_dbg(&pdev->xdev->dev, "Connecting...\n");
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> - if (err)
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to connected state!");
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>
> dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err);
> out:
> @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
> }
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
> - if (err) {
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to reconfigured state!");
> - goto out;
> - }
> + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
>
> out:
> mutex_unlock(&pdev->dev_lock);
> @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev)
> goto out;
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
> - if (err)
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to initialised state!");
> + xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
>
> out:
> mutex_unlock(&pdev->dev_lock);
> @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
> }
>
> /* wait for xend to configure us */
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto out;
> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> /* watch the backend node for backend configuration information */
> err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index ad11258..847bc9c 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
> if (err)
> xenbus_dev_error(dev, err, "writing feature-sg-grant");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
> return 0;
>
> fail:
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index ca74410..e2bcd1d 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
> static void xenbus_switch_fatal(struct xenbus_device *, int, int,
> const char *, ...);
>
> -static int
> +static void
> __xenbus_switch_state(struct xenbus_device *dev,
> enum xenbus_state state, int depth)
> {
> @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev,
> int err, abort;
>
> if (state == dev->state)
> - return 0;
> + return;
>
> again:
> abort = 1;
> @@ -196,7 +196,7 @@ again:
> err = xenbus_transaction_start(&xbt);
> if (err) {
> xenbus_switch_fatal(dev, depth, err, "starting transaction");
> - return 0;
> + return;
> }
>
> err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state);
> @@ -219,7 +219,7 @@ abort:
> } else
> dev->state = state;
>
> - return 0;
> + return;
> }
>
> /**
> @@ -228,12 +228,12 @@ abort:
> * @state: new state
> *
> * Advertise in the store a change of the given driver to the given new_state.
> - * Return 0 on success, or -errno on error. On error, the device will switch
> - * to XenbusStateClosing, and the error will be saved in the store.
> + * When failure occurs, it will call xenbus_switch_fatal() internally, so need
> + * not return value to notify upper caller.
> */
> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> {
> - return __xenbus_switch_state(dev, state, 0);
> + __xenbus_switch_state(dev, state, 0);
> }
>
> EXPORT_SYMBOL_GPL(xenbus_switch_state);
> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> index 0324c6d..587c53d 100644
> --- a/include/xen/xenbus.h
> +++ b/include/xen/xenbus.h
> @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
> const char **, unsigned int),
> const char *pathfmt, ...);
>
> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
> +void xenbus_switch_state(struct xenbus_device *dev,
> + enum xenbus_state new_state);
> int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn);
> int xenbus_map_ring_valloc(struct xenbus_device *dev,
> int gnt_ref, void **vaddr);
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 18:07 ` David Vrabel
(?)
@ 2014-09-27 9:20 ` Chen Gang
-1 siblings, 0 replies; 32+ messages in thread
From: Chen Gang @ 2014-09-27 9:20 UTC (permalink / raw)
To: David Vrabel
Cc: konrad.wilk, ian.campbell, wei.liu2, boris.ostrovsky, bhelgaas,
jgross, yongjun_wei, mukesh.rathor, xen-devel, linux-pci,
linux-kernel, linux-scsi, netdev
On 09/27/2014 02:07 AM, David Vrabel wrote:
> On 26/09/14 17:36, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
>> internally, so need not return any status value, then use 'void' instead
>> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>>
>> Also need be sure that all callers which check the return value must let
>> 'err' be 0.
>
> I've rewritten the commit message as:
>
> xen/xenbus: don't return errors from xenbus_switch_state()
>
> Most users of xenbus_switch_state() weren't handling the failure of
> xenbus_switch_state() correctly. They either called
> xenbus_dev_fatal() (which xenbus_switch_state() has effectively
> already tried to do), or ignored errors.
>
> xenbus_switch_state() may fail because:
>
> a) The device is being unplugged by the toolstack. The device will
> shortly be removed and the error does not need to be handled.
>
> b) Xenstore is broken. There isn't much the driver can do in this
> case since xenstore is required to signal failure to the toolstack.
>
> So, don't report any errors from xenbus_switch_state() which removes
> some unnecessary error handling in some of the drivers.
>
> I'd appreciate a review from some of the other front/backend driver
> maintainers on whether this is sensible reasoning.
>
At least for me, what your changes is necessary, and also necessary to
let other related members to have a look.
And one thing I did not mention: after the patch, it reports a warning,
which I have sent the other followed patch for it (it is for a bug fix,
but also can fix this warning). The other followed patch is
"xen/xen-scsiback: Need go to fail after xenbus_dev_error()"
Please help check, too.
Thanks.
> David
>
>>
>> And also need change the related comments for xenbus_switch_state().
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>> drivers/block/xen-blkback/xenbus.c | 9 ++-------
>> drivers/net/xen-netback/xenbus.c | 5 +----
>> drivers/pci/xen-pcifront.c | 15 ++++++---------
>> drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
>> drivers/xen/xen-scsiback.c | 5 +----
>> drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
>> include/xen/xenbus.h | 3 ++-
>> 7 files changed, 24 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>> index 3a8b810..fdcc584 100644
>> --- a/drivers/block/xen-blkback/xenbus.c
>> +++ b/drivers/block/xen-blkback/xenbus.c
>> @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
>> if (err)
>> goto fail;
>>
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto fail;
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>>
>> return 0;
>>
>> @@ -837,10 +835,7 @@ again:
>> if (err)
>> xenbus_dev_fatal(dev, err, "ending transaction");
>>
>> - err = xenbus_switch_state(dev, XenbusStateConnected);
>> - if (err)
>> - xenbus_dev_fatal(dev, err, "%s: switching to Connected state",
>> - dev->nodename);
>> + xenbus_switch_state(dev, XenbusStateConnected);
>>
>> return;
>> abort:
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>> index 9c47b89..b5c3d47 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>> if (err)
>> pr_debug("Error writing multi-queue-max-queues\n");
>>
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto fail;
>> -
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>> be->state = XenbusStateInitWait;
>>
>> /* This kicks hotplug scripts, so do it immediately. */
>> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
>> index 53df39a..c1d73b7 100644
>> --- a/drivers/pci/xen-pcifront.c
>> +++ b/drivers/pci/xen-pcifront.c
>> @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
>> }
>> }
>>
>> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>> -
>> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>> + return 0;
>> out:
>> return err;
>> }
>>
>> static int pcifront_try_disconnect(struct pcifront_device *pdev)
>> {
>> - int err = 0;
>> enum xenbus_state prev_state;
>>
>> -
>> prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
>>
>> if (prev_state >= XenbusStateClosing)
>> @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
>> pcifront_disconnect(pdev);
>> }
>>
>> - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
>> + xenbus_switch_state(pdev->xdev, XenbusStateClosed);
>>
>> out:
>> -
>> - return err;
>> + return 0;
>> }
>>
>> static int pcifront_attach_devices(struct pcifront_device *pdev)
>> @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
>> domain, bus, slot, func);
>> }
>>
>> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
>> -
>> + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
>> + return 0;
>> out:
>> return err;
>> }
>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>> index c214daa..630a215 100644
>> --- a/drivers/xen/xen-pciback/xenbus.c
>> +++ b/drivers/xen/xen-pciback/xenbus.c
>> @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
>>
>> dev_dbg(&pdev->xdev->dev, "Connecting...\n");
>>
>> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>> - if (err)
>> - xenbus_dev_fatal(pdev->xdev, err,
>> - "Error switching to connected state!");
>> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>>
>> dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err);
>> out:
>> @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
>> }
>> }
>>
>> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
>> - if (err) {
>> - xenbus_dev_fatal(pdev->xdev, err,
>> - "Error switching to reconfigured state!");
>> - goto out;
>> - }
>> + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
>>
>> out:
>> mutex_unlock(&pdev->dev_lock);
>> @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev)
>> goto out;
>> }
>>
>> - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
>> - if (err)
>> - xenbus_dev_fatal(pdev->xdev, err,
>> - "Error switching to initialised state!");
>> + xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
>>
>> out:
>> mutex_unlock(&pdev->dev_lock);
>> @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>> }
>>
>> /* wait for xend to configure us */
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto out;
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>>
>> /* watch the backend node for backend configuration information */
>> err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
>> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
>> index ad11258..847bc9c 100644
>> --- a/drivers/xen/xen-scsiback.c
>> +++ b/drivers/xen/xen-scsiback.c
>> @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
>> if (err)
>> xenbus_dev_error(dev, err, "writing feature-sg-grant");
>>
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto fail;
>> -
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>> return 0;
>>
>> fail:
>> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
>> index ca74410..e2bcd1d 100644
>> --- a/drivers/xen/xenbus/xenbus_client.c
>> +++ b/drivers/xen/xenbus/xenbus_client.c
>> @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
>> static void xenbus_switch_fatal(struct xenbus_device *, int, int,
>> const char *, ...);
>>
>> -static int
>> +static void
>> __xenbus_switch_state(struct xenbus_device *dev,
>> enum xenbus_state state, int depth)
>> {
>> @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev,
>> int err, abort;
>>
>> if (state == dev->state)
>> - return 0;
>> + return;
>>
>> again:
>> abort = 1;
>> @@ -196,7 +196,7 @@ again:
>> err = xenbus_transaction_start(&xbt);
>> if (err) {
>> xenbus_switch_fatal(dev, depth, err, "starting transaction");
>> - return 0;
>> + return;
>> }
>>
>> err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state);
>> @@ -219,7 +219,7 @@ abort:
>> } else
>> dev->state = state;
>>
>> - return 0;
>> + return;
>> }
>>
>> /**
>> @@ -228,12 +228,12 @@ abort:
>> * @state: new state
>> *
>> * Advertise in the store a change of the given driver to the given new_state.
>> - * Return 0 on success, or -errno on error. On error, the device will switch
>> - * to XenbusStateClosing, and the error will be saved in the store.
>> + * When failure occurs, it will call xenbus_switch_fatal() internally, so need
>> + * not return value to notify upper caller.
>> */
>> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
>> +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
>> {
>> - return __xenbus_switch_state(dev, state, 0);
>> + __xenbus_switch_state(dev, state, 0);
>> }
>>
>> EXPORT_SYMBOL_GPL(xenbus_switch_state);
>> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
>> index 0324c6d..587c53d 100644
>> --- a/include/xen/xenbus.h
>> +++ b/include/xen/xenbus.h
>> @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
>> const char **, unsigned int),
>> const char *pathfmt, ...);
>>
>> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
>> +void xenbus_switch_state(struct xenbus_device *dev,
>> + enum xenbus_state new_state);
>> int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn);
>> int xenbus_map_ring_valloc(struct xenbus_device *dev,
>> int gnt_ref, void **vaddr);
>>
>
--
Chen Gang
Open share and attitude like air water and life which God blessed
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 18:07 ` David Vrabel
(?)
(?)
@ 2014-09-27 9:20 ` Chen Gang
-1 siblings, 0 replies; 32+ messages in thread
From: Chen Gang @ 2014-09-27 9:20 UTC (permalink / raw)
To: David Vrabel
Cc: jgross, wei.liu2, ian.campbell, linux-scsi, linux-pci,
linux-kernel, yongjun_wei, netdev, bhelgaas, xen-devel,
boris.ostrovsky
On 09/27/2014 02:07 AM, David Vrabel wrote:
> On 26/09/14 17:36, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
>> internally, so need not return any status value, then use 'void' instead
>> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>>
>> Also need be sure that all callers which check the return value must let
>> 'err' be 0.
>
> I've rewritten the commit message as:
>
> xen/xenbus: don't return errors from xenbus_switch_state()
>
> Most users of xenbus_switch_state() weren't handling the failure of
> xenbus_switch_state() correctly. They either called
> xenbus_dev_fatal() (which xenbus_switch_state() has effectively
> already tried to do), or ignored errors.
>
> xenbus_switch_state() may fail because:
>
> a) The device is being unplugged by the toolstack. The device will
> shortly be removed and the error does not need to be handled.
>
> b) Xenstore is broken. There isn't much the driver can do in this
> case since xenstore is required to signal failure to the toolstack.
>
> So, don't report any errors from xenbus_switch_state() which removes
> some unnecessary error handling in some of the drivers.
>
> I'd appreciate a review from some of the other front/backend driver
> maintainers on whether this is sensible reasoning.
>
At least for me, what your changes is necessary, and also necessary to
let other related members to have a look.
And one thing I did not mention: after the patch, it reports a warning,
which I have sent the other followed patch for it (it is for a bug fix,
but also can fix this warning). The other followed patch is
"xen/xen-scsiback: Need go to fail after xenbus_dev_error()"
Please help check, too.
Thanks.
> David
>
>>
>> And also need change the related comments for xenbus_switch_state().
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>> drivers/block/xen-blkback/xenbus.c | 9 ++-------
>> drivers/net/xen-netback/xenbus.c | 5 +----
>> drivers/pci/xen-pcifront.c | 15 ++++++---------
>> drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
>> drivers/xen/xen-scsiback.c | 5 +----
>> drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
>> include/xen/xenbus.h | 3 ++-
>> 7 files changed, 24 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>> index 3a8b810..fdcc584 100644
>> --- a/drivers/block/xen-blkback/xenbus.c
>> +++ b/drivers/block/xen-blkback/xenbus.c
>> @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
>> if (err)
>> goto fail;
>>
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto fail;
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>>
>> return 0;
>>
>> @@ -837,10 +835,7 @@ again:
>> if (err)
>> xenbus_dev_fatal(dev, err, "ending transaction");
>>
>> - err = xenbus_switch_state(dev, XenbusStateConnected);
>> - if (err)
>> - xenbus_dev_fatal(dev, err, "%s: switching to Connected state",
>> - dev->nodename);
>> + xenbus_switch_state(dev, XenbusStateConnected);
>>
>> return;
>> abort:
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>> index 9c47b89..b5c3d47 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>> if (err)
>> pr_debug("Error writing multi-queue-max-queues\n");
>>
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto fail;
>> -
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>> be->state = XenbusStateInitWait;
>>
>> /* This kicks hotplug scripts, so do it immediately. */
>> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
>> index 53df39a..c1d73b7 100644
>> --- a/drivers/pci/xen-pcifront.c
>> +++ b/drivers/pci/xen-pcifront.c
>> @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
>> }
>> }
>>
>> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>> -
>> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>> + return 0;
>> out:
>> return err;
>> }
>>
>> static int pcifront_try_disconnect(struct pcifront_device *pdev)
>> {
>> - int err = 0;
>> enum xenbus_state prev_state;
>>
>> -
>> prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
>>
>> if (prev_state >= XenbusStateClosing)
>> @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
>> pcifront_disconnect(pdev);
>> }
>>
>> - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
>> + xenbus_switch_state(pdev->xdev, XenbusStateClosed);
>>
>> out:
>> -
>> - return err;
>> + return 0;
>> }
>>
>> static int pcifront_attach_devices(struct pcifront_device *pdev)
>> @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
>> domain, bus, slot, func);
>> }
>>
>> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
>> -
>> + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
>> + return 0;
>> out:
>> return err;
>> }
>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>> index c214daa..630a215 100644
>> --- a/drivers/xen/xen-pciback/xenbus.c
>> +++ b/drivers/xen/xen-pciback/xenbus.c
>> @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
>>
>> dev_dbg(&pdev->xdev->dev, "Connecting...\n");
>>
>> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>> - if (err)
>> - xenbus_dev_fatal(pdev->xdev, err,
>> - "Error switching to connected state!");
>> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>>
>> dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err);
>> out:
>> @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
>> }
>> }
>>
>> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
>> - if (err) {
>> - xenbus_dev_fatal(pdev->xdev, err,
>> - "Error switching to reconfigured state!");
>> - goto out;
>> - }
>> + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
>>
>> out:
>> mutex_unlock(&pdev->dev_lock);
>> @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev)
>> goto out;
>> }
>>
>> - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
>> - if (err)
>> - xenbus_dev_fatal(pdev->xdev, err,
>> - "Error switching to initialised state!");
>> + xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
>>
>> out:
>> mutex_unlock(&pdev->dev_lock);
>> @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
>> }
>>
>> /* wait for xend to configure us */
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto out;
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>>
>> /* watch the backend node for backend configuration information */
>> err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
>> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
>> index ad11258..847bc9c 100644
>> --- a/drivers/xen/xen-scsiback.c
>> +++ b/drivers/xen/xen-scsiback.c
>> @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
>> if (err)
>> xenbus_dev_error(dev, err, "writing feature-sg-grant");
>>
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto fail;
>> -
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>> return 0;
>>
>> fail:
>> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
>> index ca74410..e2bcd1d 100644
>> --- a/drivers/xen/xenbus/xenbus_client.c
>> +++ b/drivers/xen/xenbus/xenbus_client.c
>> @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
>> static void xenbus_switch_fatal(struct xenbus_device *, int, int,
>> const char *, ...);
>>
>> -static int
>> +static void
>> __xenbus_switch_state(struct xenbus_device *dev,
>> enum xenbus_state state, int depth)
>> {
>> @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev,
>> int err, abort;
>>
>> if (state == dev->state)
>> - return 0;
>> + return;
>>
>> again:
>> abort = 1;
>> @@ -196,7 +196,7 @@ again:
>> err = xenbus_transaction_start(&xbt);
>> if (err) {
>> xenbus_switch_fatal(dev, depth, err, "starting transaction");
>> - return 0;
>> + return;
>> }
>>
>> err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state);
>> @@ -219,7 +219,7 @@ abort:
>> } else
>> dev->state = state;
>>
>> - return 0;
>> + return;
>> }
>>
>> /**
>> @@ -228,12 +228,12 @@ abort:
>> * @state: new state
>> *
>> * Advertise in the store a change of the given driver to the given new_state.
>> - * Return 0 on success, or -errno on error. On error, the device will switch
>> - * to XenbusStateClosing, and the error will be saved in the store.
>> + * When failure occurs, it will call xenbus_switch_fatal() internally, so need
>> + * not return value to notify upper caller.
>> */
>> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
>> +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
>> {
>> - return __xenbus_switch_state(dev, state, 0);
>> + __xenbus_switch_state(dev, state, 0);
>> }
>>
>> EXPORT_SYMBOL_GPL(xenbus_switch_state);
>> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
>> index 0324c6d..587c53d 100644
>> --- a/include/xen/xenbus.h
>> +++ b/include/xen/xenbus.h
>> @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
>> const char **, unsigned int),
>> const char *pathfmt, ...);
>>
>> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
>> +void xenbus_switch_state(struct xenbus_device *dev,
>> + enum xenbus_state new_state);
>> int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn);
>> int xenbus_map_ring_valloc(struct xenbus_device *dev,
>> int gnt_ref, void **vaddr);
>>
>
--
Chen Gang
Open share and attitude like air water and life which God blessed
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 16:36 [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state() Chen Gang
2014-09-26 18:07 ` David Vrabel
2014-09-26 18:07 ` David Vrabel
@ 2014-09-29 5:09 ` Juergen Gross
2014-09-29 5:09 ` Juergen Gross
` (6 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Juergen Gross @ 2014-09-29 5:09 UTC (permalink / raw)
To: Chen Gang, David Vrabel, konrad.wilk, ian.campbell, wei.liu2,
boris.ostrovsky, bhelgaas, yongjun_wei, mukesh.rathor
Cc: xen-devel, linux-pci, linux-kernel, linux-scsi, netdev
On 09/26/2014 06:36 PM, Chen Gang wrote:
> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> internally, so need not return any status value, then use 'void' instead
> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>
> Also need be sure that all callers which check the return value must let
> 'err' be 0.
>
> And also need change the related comments for xenbus_switch_state().
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
Acked-by: Juergen Gross <jgross@suse.com>
> ---
> drivers/block/xen-blkback/xenbus.c | 9 ++-------
> drivers/net/xen-netback/xenbus.c | 5 +----
> drivers/pci/xen-pcifront.c | 15 ++++++---------
> drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
> drivers/xen/xen-scsiback.c | 5 +----
> drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
> include/xen/xenbus.h | 3 ++-
> 7 files changed, 24 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index 3a8b810..fdcc584 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
> if (err)
> goto fail;
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> return 0;
>
> @@ -837,10 +835,7 @@ again:
> if (err)
> xenbus_dev_fatal(dev, err, "ending transaction");
>
> - err = xenbus_switch_state(dev, XenbusStateConnected);
> - if (err)
> - xenbus_dev_fatal(dev, err, "%s: switching to Connected state",
> - dev->nodename);
> + xenbus_switch_state(dev, XenbusStateConnected);
>
> return;
> abort:
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 9c47b89..b5c3d47 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
> if (err)
> pr_debug("Error writing multi-queue-max-queues\n");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
> be->state = XenbusStateInitWait;
>
> /* This kicks hotplug scripts, so do it immediately. */
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 53df39a..c1d73b7 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> }
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> + return 0;
> out:
> return err;
> }
>
> static int pcifront_try_disconnect(struct pcifront_device *pdev)
> {
> - int err = 0;
> enum xenbus_state prev_state;
>
> -
> prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
>
> if (prev_state >= XenbusStateClosing)
> @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
> pcifront_disconnect(pdev);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
> + xenbus_switch_state(pdev->xdev, XenbusStateClosed);
>
> out:
> -
> - return err;
> + return 0;
> }
>
> static int pcifront_attach_devices(struct pcifront_device *pdev)
> @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> domain, bus, slot, func);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> + return 0;
> out:
> return err;
> }
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c214daa..630a215 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
>
> dev_dbg(&pdev->xdev->dev, "Connecting...\n");
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> - if (err)
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to connected state!");
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>
> dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err);
> out:
> @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
> }
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
> - if (err) {
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to reconfigured state!");
> - goto out;
> - }
> + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
>
> out:
> mutex_unlock(&pdev->dev_lock);
> @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev)
> goto out;
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
> - if (err)
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to initialised state!");
> + xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
>
> out:
> mutex_unlock(&pdev->dev_lock);
> @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
> }
>
> /* wait for xend to configure us */
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto out;
> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> /* watch the backend node for backend configuration information */
> err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index ad11258..847bc9c 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
> if (err)
> xenbus_dev_error(dev, err, "writing feature-sg-grant");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
> return 0;
>
> fail:
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index ca74410..e2bcd1d 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
> static void xenbus_switch_fatal(struct xenbus_device *, int, int,
> const char *, ...);
>
> -static int
> +static void
> __xenbus_switch_state(struct xenbus_device *dev,
> enum xenbus_state state, int depth)
> {
> @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev,
> int err, abort;
>
> if (state == dev->state)
> - return 0;
> + return;
>
> again:
> abort = 1;
> @@ -196,7 +196,7 @@ again:
> err = xenbus_transaction_start(&xbt);
> if (err) {
> xenbus_switch_fatal(dev, depth, err, "starting transaction");
> - return 0;
> + return;
> }
>
> err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state);
> @@ -219,7 +219,7 @@ abort:
> } else
> dev->state = state;
>
> - return 0;
> + return;
> }
>
> /**
> @@ -228,12 +228,12 @@ abort:
> * @state: new state
> *
> * Advertise in the store a change of the given driver to the given new_state.
> - * Return 0 on success, or -errno on error. On error, the device will switch
> - * to XenbusStateClosing, and the error will be saved in the store.
> + * When failure occurs, it will call xenbus_switch_fatal() internally, so need
> + * not return value to notify upper caller.
> */
> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> {
> - return __xenbus_switch_state(dev, state, 0);
> + __xenbus_switch_state(dev, state, 0);
> }
>
> EXPORT_SYMBOL_GPL(xenbus_switch_state);
> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> index 0324c6d..587c53d 100644
> --- a/include/xen/xenbus.h
> +++ b/include/xen/xenbus.h
> @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
> const char **, unsigned int),
> const char *pathfmt, ...);
>
> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
> +void xenbus_switch_state(struct xenbus_device *dev,
> + enum xenbus_state new_state);
> int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn);
> int xenbus_map_ring_valloc(struct xenbus_device *dev,
> int gnt_ref, void **vaddr);
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 16:36 [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state() Chen Gang
` (2 preceding siblings ...)
2014-09-29 5:09 ` [Xen-devel] " Juergen Gross
@ 2014-09-29 5:09 ` Juergen Gross
2014-09-29 13:35 ` Bjorn Helgaas
` (5 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Juergen Gross @ 2014-09-29 5:09 UTC (permalink / raw)
To: Chen Gang, David Vrabel, konrad.wilk, ian.campbell, wei.liu2,
boris.ostrovsky, bhelgaas, yongjun_wei, mukesh.rathor
Cc: xen-devel, netdev, linux-kernel, linux-scsi, linux-pci
On 09/26/2014 06:36 PM, Chen Gang wrote:
> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> internally, so need not return any status value, then use 'void' instead
> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>
> Also need be sure that all callers which check the return value must let
> 'err' be 0.
>
> And also need change the related comments for xenbus_switch_state().
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
Acked-by: Juergen Gross <jgross@suse.com>
> ---
> drivers/block/xen-blkback/xenbus.c | 9 ++-------
> drivers/net/xen-netback/xenbus.c | 5 +----
> drivers/pci/xen-pcifront.c | 15 ++++++---------
> drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
> drivers/xen/xen-scsiback.c | 5 +----
> drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
> include/xen/xenbus.h | 3 ++-
> 7 files changed, 24 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index 3a8b810..fdcc584 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
> if (err)
> goto fail;
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> return 0;
>
> @@ -837,10 +835,7 @@ again:
> if (err)
> xenbus_dev_fatal(dev, err, "ending transaction");
>
> - err = xenbus_switch_state(dev, XenbusStateConnected);
> - if (err)
> - xenbus_dev_fatal(dev, err, "%s: switching to Connected state",
> - dev->nodename);
> + xenbus_switch_state(dev, XenbusStateConnected);
>
> return;
> abort:
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 9c47b89..b5c3d47 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
> if (err)
> pr_debug("Error writing multi-queue-max-queues\n");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
> be->state = XenbusStateInitWait;
>
> /* This kicks hotplug scripts, so do it immediately. */
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 53df39a..c1d73b7 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> }
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> + return 0;
> out:
> return err;
> }
>
> static int pcifront_try_disconnect(struct pcifront_device *pdev)
> {
> - int err = 0;
> enum xenbus_state prev_state;
>
> -
> prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
>
> if (prev_state >= XenbusStateClosing)
> @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
> pcifront_disconnect(pdev);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
> + xenbus_switch_state(pdev->xdev, XenbusStateClosed);
>
> out:
> -
> - return err;
> + return 0;
> }
>
> static int pcifront_attach_devices(struct pcifront_device *pdev)
> @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> domain, bus, slot, func);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> + return 0;
> out:
> return err;
> }
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c214daa..630a215 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
>
> dev_dbg(&pdev->xdev->dev, "Connecting...\n");
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> - if (err)
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to connected state!");
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
>
> dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err);
> out:
> @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
> }
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
> - if (err) {
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to reconfigured state!");
> - goto out;
> - }
> + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
>
> out:
> mutex_unlock(&pdev->dev_lock);
> @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev)
> goto out;
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
> - if (err)
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to initialised state!");
> + xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
>
> out:
> mutex_unlock(&pdev->dev_lock);
> @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
> }
>
> /* wait for xend to configure us */
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto out;
> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> /* watch the backend node for backend configuration information */
> err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index ad11258..847bc9c 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
> if (err)
> xenbus_dev_error(dev, err, "writing feature-sg-grant");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
> return 0;
>
> fail:
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index ca74410..e2bcd1d 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
> static void xenbus_switch_fatal(struct xenbus_device *, int, int,
> const char *, ...);
>
> -static int
> +static void
> __xenbus_switch_state(struct xenbus_device *dev,
> enum xenbus_state state, int depth)
> {
> @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev,
> int err, abort;
>
> if (state == dev->state)
> - return 0;
> + return;
>
> again:
> abort = 1;
> @@ -196,7 +196,7 @@ again:
> err = xenbus_transaction_start(&xbt);
> if (err) {
> xenbus_switch_fatal(dev, depth, err, "starting transaction");
> - return 0;
> + return;
> }
>
> err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state);
> @@ -219,7 +219,7 @@ abort:
> } else
> dev->state = state;
>
> - return 0;
> + return;
> }
>
> /**
> @@ -228,12 +228,12 @@ abort:
> * @state: new state
> *
> * Advertise in the store a change of the given driver to the given new_state.
> - * Return 0 on success, or -errno on error. On error, the device will switch
> - * to XenbusStateClosing, and the error will be saved in the store.
> + * When failure occurs, it will call xenbus_switch_fatal() internally, so need
> + * not return value to notify upper caller.
> */
> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> {
> - return __xenbus_switch_state(dev, state, 0);
> + __xenbus_switch_state(dev, state, 0);
> }
>
> EXPORT_SYMBOL_GPL(xenbus_switch_state);
> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> index 0324c6d..587c53d 100644
> --- a/include/xen/xenbus.h
> +++ b/include/xen/xenbus.h
> @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
> const char **, unsigned int),
> const char *pathfmt, ...);
>
> -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
> +void xenbus_switch_state(struct xenbus_device *dev,
> + enum xenbus_state new_state);
> int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn);
> int xenbus_map_ring_valloc(struct xenbus_device *dev,
> int gnt_ref, void **vaddr);
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 18:07 ` David Vrabel
@ 2014-09-29 8:41 ` Wei Liu
-1 siblings, 0 replies; 32+ messages in thread
From: Wei Liu @ 2014-09-29 8:41 UTC (permalink / raw)
To: David Vrabel
Cc: Chen Gang, konrad.wilk, ian.campbell, wei.liu2, boris.ostrovsky,
bhelgaas, jgross, yongjun_wei, mukesh.rathor, xen-devel,
linux-pci, linux-kernel, linux-scsi, netdev
On Fri, Sep 26, 2014 at 07:07:19PM +0100, David Vrabel wrote:
> On 26/09/14 17:36, Chen Gang wrote:
> > When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> > internally, so need not return any status value, then use 'void' instead
> > of 'int' for xenbus_switch_state() and __xenbus_switch_state().
> >
> > Also need be sure that all callers which check the return value must let
> > 'err' be 0.
>
> I've rewritten the commit message as:
>
> xen/xenbus: don't return errors from xenbus_switch_state()
>
> Most users of xenbus_switch_state() weren't handling the failure of
> xenbus_switch_state() correctly. They either called
> xenbus_dev_fatal() (which xenbus_switch_state() has effectively
> already tried to do), or ignored errors.
>
> xenbus_switch_state() may fail because:
>
> a) The device is being unplugged by the toolstack. The device will
> shortly be removed and the error does not need to be handled.
>
> b) Xenstore is broken. There isn't much the driver can do in this
> case since xenstore is required to signal failure to the toolstack.
>
> So, don't report any errors from xenbus_switch_state() which removes
> some unnecessary error handling in some of the drivers.
>
> I'd appreciate a review from some of the other front/backend driver
> maintainers on whether this is sensible reasoning.
>
Looks reasonable to me.
Wei.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
@ 2014-09-29 8:41 ` Wei Liu
0 siblings, 0 replies; 32+ messages in thread
From: Wei Liu @ 2014-09-29 8:41 UTC (permalink / raw)
To: David Vrabel
Cc: Chen Gang, konrad.wilk, ian.campbell, wei.liu2, boris.ostrovsky,
bhelgaas, jgross, yongjun_wei, mukesh.rathor, xen-devel,
linux-pci, linux-kernel, linux-scsi, netdev
On Fri, Sep 26, 2014 at 07:07:19PM +0100, David Vrabel wrote:
> On 26/09/14 17:36, Chen Gang wrote:
> > When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> > internally, so need not return any status value, then use 'void' instead
> > of 'int' for xenbus_switch_state() and __xenbus_switch_state().
> >
> > Also need be sure that all callers which check the return value must let
> > 'err' be 0.
>
> I've rewritten the commit message as:
>
> xen/xenbus: don't return errors from xenbus_switch_state()
>
> Most users of xenbus_switch_state() weren't handling the failure of
> xenbus_switch_state() correctly. They either called
> xenbus_dev_fatal() (which xenbus_switch_state() has effectively
> already tried to do), or ignored errors.
>
> xenbus_switch_state() may fail because:
>
> a) The device is being unplugged by the toolstack. The device will
> shortly be removed and the error does not need to be handled.
>
> b) Xenstore is broken. There isn't much the driver can do in this
> case since xenstore is required to signal failure to the toolstack.
>
> So, don't report any errors from xenbus_switch_state() which removes
> some unnecessary error handling in some of the drivers.
>
> I'd appreciate a review from some of the other front/backend driver
> maintainers on whether this is sensible reasoning.
>
Looks reasonable to me.
Wei.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 18:07 ` David Vrabel
` (2 preceding siblings ...)
(?)
@ 2014-09-29 8:41 ` Wei Liu
-1 siblings, 0 replies; 32+ messages in thread
From: Wei Liu @ 2014-09-29 8:41 UTC (permalink / raw)
To: David Vrabel
Cc: jgross, wei.liu2, ian.campbell, linux-scsi, linux-pci,
linux-kernel, yongjun_wei, netdev, bhelgaas, xen-devel,
boris.ostrovsky, Chen Gang
On Fri, Sep 26, 2014 at 07:07:19PM +0100, David Vrabel wrote:
> On 26/09/14 17:36, Chen Gang wrote:
> > When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> > internally, so need not return any status value, then use 'void' instead
> > of 'int' for xenbus_switch_state() and __xenbus_switch_state().
> >
> > Also need be sure that all callers which check the return value must let
> > 'err' be 0.
>
> I've rewritten the commit message as:
>
> xen/xenbus: don't return errors from xenbus_switch_state()
>
> Most users of xenbus_switch_state() weren't handling the failure of
> xenbus_switch_state() correctly. They either called
> xenbus_dev_fatal() (which xenbus_switch_state() has effectively
> already tried to do), or ignored errors.
>
> xenbus_switch_state() may fail because:
>
> a) The device is being unplugged by the toolstack. The device will
> shortly be removed and the error does not need to be handled.
>
> b) Xenstore is broken. There isn't much the driver can do in this
> case since xenstore is required to signal failure to the toolstack.
>
> So, don't report any errors from xenbus_switch_state() which removes
> some unnecessary error handling in some of the drivers.
>
> I'd appreciate a review from some of the other front/backend driver
> maintainers on whether this is sensible reasoning.
>
Looks reasonable to me.
Wei.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 16:36 [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state() Chen Gang
` (3 preceding siblings ...)
2014-09-29 5:09 ` Juergen Gross
@ 2014-09-29 13:35 ` Bjorn Helgaas
2014-09-29 13:35 ` Bjorn Helgaas
` (4 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Bjorn Helgaas @ 2014-09-29 13:35 UTC (permalink / raw)
To: Chen Gang
Cc: David Vrabel, Konrad Rzeszutek Wilk, Ian Campbell, wei.liu2,
Boris Ostrovsky, jgross, Wei Yongjun, mukesh.rathor, xen-devel,
netdev, linux-pci, linux-scsi, linux-kernel
On Fri, Sep 26, 2014 at 10:36 AM, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> internally, so need not return any status value, then use 'void' instead
> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>
> Also need be sure that all callers which check the return value must let
> 'err' be 0.
>
> And also need change the related comments for xenbus_switch_state().
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
> drivers/block/xen-blkback/xenbus.c | 9 ++-------
> drivers/net/xen-netback/xenbus.c | 5 +----
> drivers/pci/xen-pcifront.c | 15 ++++++---------
> drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
> drivers/xen/xen-scsiback.c | 5 +----
> drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
> include/xen/xenbus.h | 3 ++-
> 7 files changed, 24 insertions(+), 50 deletions(-)
I don't know enough to review this, but as far as I'm concerned, the
Xen folks own drivers/pci/xen-pcifront.c, so whatever they want to do
is fine with me.
Bjorn
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 16:36 [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state() Chen Gang
` (4 preceding siblings ...)
2014-09-29 13:35 ` Bjorn Helgaas
@ 2014-09-29 13:35 ` Bjorn Helgaas
2014-09-29 14:02 ` Konrad Rzeszutek Wilk
` (3 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Bjorn Helgaas @ 2014-09-29 13:35 UTC (permalink / raw)
To: Chen Gang
Cc: jgross, wei.liu2, Ian Campbell, linux-scsi, netdev, linux-kernel,
Wei Yongjun, David Vrabel, linux-pci, xen-devel, Boris Ostrovsky
On Fri, Sep 26, 2014 at 10:36 AM, Chen Gang <gang.chen.5i5j@gmail.com> wrote:
> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> internally, so need not return any status value, then use 'void' instead
> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>
> Also need be sure that all callers which check the return value must let
> 'err' be 0.
>
> And also need change the related comments for xenbus_switch_state().
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
> drivers/block/xen-blkback/xenbus.c | 9 ++-------
> drivers/net/xen-netback/xenbus.c | 5 +----
> drivers/pci/xen-pcifront.c | 15 ++++++---------
> drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
> drivers/xen/xen-scsiback.c | 5 +----
> drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
> include/xen/xenbus.h | 3 ++-
> 7 files changed, 24 insertions(+), 50 deletions(-)
I don't know enough to review this, but as far as I'm concerned, the
Xen folks own drivers/pci/xen-pcifront.c, so whatever they want to do
is fine with me.
Bjorn
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 16:36 [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state() Chen Gang
` (5 preceding siblings ...)
2014-09-29 13:35 ` Bjorn Helgaas
@ 2014-09-29 14:02 ` Konrad Rzeszutek Wilk
2014-09-29 14:17 ` David Vrabel
` (3 more replies)
2014-09-29 14:02 ` Konrad Rzeszutek Wilk
` (2 subsequent siblings)
9 siblings, 4 replies; 32+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-09-29 14:02 UTC (permalink / raw)
To: Chen Gang
Cc: David Vrabel, ian.campbell, wei.liu2, boris.ostrovsky, bhelgaas,
jgross, yongjun_wei, mukesh.rathor, xen-devel, netdev, linux-pci,
linux-scsi, linux-kernel
On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
> When xenbus_switch_state() fails, it will call xenbus_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).
> 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:
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index c214daa..f7399fd 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -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;
>
> 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..
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 9c47b89..b5c3d47 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
> if (err)
> pr_debug("Error writing multi-queue-max-queues\n");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
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.
> be->state = XenbusStateInitWait;
>
> /* This kicks hotplug scripts, so do it immediately. */
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 53df39a..c1d73b7 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> }
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> + return 0;
> out:
> return err;
> }
>
> static int pcifront_try_disconnect(struct pcifront_device *pdev)
> {
> - int err = 0;
> enum xenbus_state prev_state;
>
> -
> prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
>
> if (prev_state >= XenbusStateClosing)
> @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
> pcifront_disconnect(pdev);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
> + xenbus_switch_state(pdev->xdev, XenbusStateClosed);
>
> out:
> -
> - return err;
> + return 0;
> }
>
> static int pcifront_attach_devices(struct pcifront_device *pdev)
> @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> domain, bus, slot, func);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> + return 0;
> out:
> return err;
> }
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c214daa..630a215 100644
The xen-pcifront are OK, this one:
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
>
> dev_dbg(&pdev->xdev->dev, "Connecting...\n");
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> - if (err)
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to connected state!");
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
This one is OK
>
> 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);
That is OKish, thought I am not too happy about us holding the lock.
>
> 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);
And this one needs that comment above about the mutex.
>
> 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);
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.
>
> /* watch the backend node for backend configuration information */
> err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index ad11258..847bc9c 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
> if (err)
> xenbus_dev_error(dev, err, "writing feature-sg-grant");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
> return 0;
>
> fail:
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 16:36 [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state() Chen Gang
` (6 preceding siblings ...)
2014-09-29 14:02 ` Konrad Rzeszutek Wilk
@ 2014-09-29 14:02 ` Konrad Rzeszutek Wilk
2014-09-30 9:55 ` David Vrabel
2014-09-30 9:55 ` David Vrabel
9 siblings, 0 replies; 32+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-09-29 14:02 UTC (permalink / raw)
To: Chen Gang
Cc: jgross, wei.liu2, ian.campbell, linux-scsi, netdev, linux-kernel,
yongjun_wei, David Vrabel, linux-pci, bhelgaas, xen-devel,
boris.ostrovsky
On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
> When xenbus_switch_state() fails, it will call xenbus_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).
> 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:
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index c214daa..f7399fd 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -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;
>
> 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..
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 9c47b89..b5c3d47 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
> if (err)
> pr_debug("Error writing multi-queue-max-queues\n");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
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.
> be->state = XenbusStateInitWait;
>
> /* This kicks hotplug scripts, so do it immediately. */
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 53df39a..c1d73b7 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> }
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> + return 0;
> out:
> return err;
> }
>
> static int pcifront_try_disconnect(struct pcifront_device *pdev)
> {
> - int err = 0;
> enum xenbus_state prev_state;
>
> -
> prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
>
> if (prev_state >= XenbusStateClosing)
> @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
> pcifront_disconnect(pdev);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
> + xenbus_switch_state(pdev->xdev, XenbusStateClosed);
>
> out:
> -
> - return err;
> + return 0;
> }
>
> static int pcifront_attach_devices(struct pcifront_device *pdev)
> @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> domain, bus, slot, func);
> }
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> -
> + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> + return 0;
> out:
> return err;
> }
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c214daa..630a215 100644
The xen-pcifront are OK, this one:
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
>
> dev_dbg(&pdev->xdev->dev, "Connecting...\n");
>
> - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> - if (err)
> - xenbus_dev_fatal(pdev->xdev, err,
> - "Error switching to connected state!");
> + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
This one is OK
>
> 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);
That is OKish, thought I am not too happy about us holding the lock.
>
> 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);
And this one needs that comment above about the mutex.
>
> 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);
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.
>
> /* watch the backend node for backend configuration information */
> err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index ad11258..847bc9c 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
> if (err)
> xenbus_dev_error(dev, err, "writing feature-sg-grant");
>
> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> - if (err)
> - goto fail;
> -
> + xenbus_switch_state(dev, XenbusStateInitWait);
> return 0;
>
> fail:
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [Xen-devel] [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 18:07 ` David Vrabel
` (5 preceding siblings ...)
(?)
@ 2014-09-29 14:03 ` Konrad Rzeszutek Wilk
-1 siblings, 0 replies; 32+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-09-29 14:03 UTC (permalink / raw)
To: David Vrabel
Cc: Chen Gang, ian.campbell, wei.liu2, boris.ostrovsky, bhelgaas,
jgross, yongjun_wei, mukesh.rathor, xen-devel, linux-pci,
linux-kernel, linux-scsi, netdev
On Fri, Sep 26, 2014 at 07:07:19PM +0100, David Vrabel wrote:
> On 26/09/14 17:36, Chen Gang wrote:
> > When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> > internally, so need not return any status value, then use 'void' instead
> > of 'int' for xenbus_switch_state() and __xenbus_switch_state().
> >
> > Also need be sure that all callers which check the return value must let
> > 'err' be 0.
>
> I've rewritten the commit message as:
>
> xen/xenbus: don't return errors from xenbus_switch_state()
>
> Most users of xenbus_switch_state() weren't handling the failure of
> xenbus_switch_state() correctly. They either called
> xenbus_dev_fatal() (which xenbus_switch_state() has effectively
> already tried to do), or ignored errors.
>
> xenbus_switch_state() may fail because:
>
> a) The device is being unplugged by the toolstack. The device will
> shortly be removed and the error does not need to be handled.
>
> b) Xenstore is broken. There isn't much the driver can do in this
> case since xenstore is required to signal failure to the toolstack.
>
> So, don't report any errors from xenbus_switch_state() which removes
> some unnecessary error handling in some of the drivers.
>
> I'd appreciate a review from some of the other front/backend driver
> maintainers on whether this is sensible reasoning.
Done that but I am not too keen about this patch - as I think it will
cause memory leaks on failure paths.
>
> David
>
> >
> > And also need change the related comments for xenbus_switch_state().
> >
> > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> > ---
> > drivers/block/xen-blkback/xenbus.c | 9 ++-------
> > drivers/net/xen-netback/xenbus.c | 5 +----
> > drivers/pci/xen-pcifront.c | 15 ++++++---------
> > drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
> > drivers/xen/xen-scsiback.c | 5 +----
> > drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
> > include/xen/xenbus.h | 3 ++-
> > 7 files changed, 24 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> > index 3a8b810..fdcc584 100644
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
> > if (err)
> > goto fail;
> >
> > - err = xenbus_switch_state(dev, XenbusStateInitWait);
> > - if (err)
> > - goto fail;
> > + xenbus_switch_state(dev, XenbusStateInitWait);
> >
> > return 0;
> >
> > @@ -837,10 +835,7 @@ again:
> > if (err)
> > xenbus_dev_fatal(dev, err, "ending transaction");
> >
> > - err = xenbus_switch_state(dev, XenbusStateConnected);
> > - if (err)
> > - xenbus_dev_fatal(dev, err, "%s: switching to Connected state",
> > - dev->nodename);
> > + xenbus_switch_state(dev, XenbusStateConnected);
> >
> > return;
> > abort:
> > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> > index 9c47b89..b5c3d47 100644
> > --- a/drivers/net/xen-netback/xenbus.c
> > +++ b/drivers/net/xen-netback/xenbus.c
> > @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
> > if (err)
> > pr_debug("Error writing multi-queue-max-queues\n");
> >
> > - err = xenbus_switch_state(dev, XenbusStateInitWait);
> > - if (err)
> > - goto fail;
> > -
> > + xenbus_switch_state(dev, XenbusStateInitWait);
> > be->state = XenbusStateInitWait;
> >
> > /* This kicks hotplug scripts, so do it immediately. */
> > diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> > index 53df39a..c1d73b7 100644
> > --- a/drivers/pci/xen-pcifront.c
> > +++ b/drivers/pci/xen-pcifront.c
> > @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> > }
> > }
> >
> > - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> > -
> > + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> > + return 0;
> > out:
> > return err;
> > }
> >
> > static int pcifront_try_disconnect(struct pcifront_device *pdev)
> > {
> > - int err = 0;
> > enum xenbus_state prev_state;
> >
> > -
> > prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
> >
> > if (prev_state >= XenbusStateClosing)
> > @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
> > pcifront_disconnect(pdev);
> > }
> >
> > - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
> > + xenbus_switch_state(pdev->xdev, XenbusStateClosed);
> >
> > out:
> > -
> > - return err;
> > + return 0;
> > }
> >
> > static int pcifront_attach_devices(struct pcifront_device *pdev)
> > @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> > domain, bus, slot, func);
> > }
> >
> > - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> > -
> > + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> > + return 0;
> > out:
> > return err;
> > }
> > diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> > index c214daa..630a215 100644
> > --- a/drivers/xen/xen-pciback/xenbus.c
> > +++ b/drivers/xen/xen-pciback/xenbus.c
> > @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
> >
> > dev_dbg(&pdev->xdev->dev, "Connecting...\n");
> >
> > - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> > - if (err)
> > - xenbus_dev_fatal(pdev->xdev, err,
> > - "Error switching to connected state!");
> > + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> >
> > dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err);
> > out:
> > @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
> > }
> > }
> >
> > - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
> > - if (err) {
> > - xenbus_dev_fatal(pdev->xdev, err,
> > - "Error switching to reconfigured state!");
> > - goto out;
> > - }
> > + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
> >
> > out:
> > mutex_unlock(&pdev->dev_lock);
> > @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev)
> > goto out;
> > }
> >
> > - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
> > - if (err)
> > - xenbus_dev_fatal(pdev->xdev, err,
> > - "Error switching to initialised state!");
> > + xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
> >
> > out:
> > mutex_unlock(&pdev->dev_lock);
> > @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
> > }
> >
> > /* wait for xend to configure us */
> > - err = xenbus_switch_state(dev, XenbusStateInitWait);
> > - if (err)
> > - goto out;
> > + xenbus_switch_state(dev, XenbusStateInitWait);
> >
> > /* watch the backend node for backend configuration information */
> > err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
> > diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> > index ad11258..847bc9c 100644
> > --- a/drivers/xen/xen-scsiback.c
> > +++ b/drivers/xen/xen-scsiback.c
> > @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
> > if (err)
> > xenbus_dev_error(dev, err, "writing feature-sg-grant");
> >
> > - err = xenbus_switch_state(dev, XenbusStateInitWait);
> > - if (err)
> > - goto fail;
> > -
> > + xenbus_switch_state(dev, XenbusStateInitWait);
> > return 0;
> >
> > fail:
> > diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> > index ca74410..e2bcd1d 100644
> > --- a/drivers/xen/xenbus/xenbus_client.c
> > +++ b/drivers/xen/xenbus/xenbus_client.c
> > @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
> > static void xenbus_switch_fatal(struct xenbus_device *, int, int,
> > const char *, ...);
> >
> > -static int
> > +static void
> > __xenbus_switch_state(struct xenbus_device *dev,
> > enum xenbus_state state, int depth)
> > {
> > @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev,
> > int err, abort;
> >
> > if (state == dev->state)
> > - return 0;
> > + return;
> >
> > again:
> > abort = 1;
> > @@ -196,7 +196,7 @@ again:
> > err = xenbus_transaction_start(&xbt);
> > if (err) {
> > xenbus_switch_fatal(dev, depth, err, "starting transaction");
> > - return 0;
> > + return;
> > }
> >
> > err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state);
> > @@ -219,7 +219,7 @@ abort:
> > } else
> > dev->state = state;
> >
> > - return 0;
> > + return;
> > }
> >
> > /**
> > @@ -228,12 +228,12 @@ abort:
> > * @state: new state
> > *
> > * Advertise in the store a change of the given driver to the given new_state.
> > - * Return 0 on success, or -errno on error. On error, the device will switch
> > - * to XenbusStateClosing, and the error will be saved in the store.
> > + * When failure occurs, it will call xenbus_switch_fatal() internally, so need
> > + * not return value to notify upper caller.
> > */
> > -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> > +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> > {
> > - return __xenbus_switch_state(dev, state, 0);
> > + __xenbus_switch_state(dev, state, 0);
> > }
> >
> > EXPORT_SYMBOL_GPL(xenbus_switch_state);
> > diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> > index 0324c6d..587c53d 100644
> > --- a/include/xen/xenbus.h
> > +++ b/include/xen/xenbus.h
> > @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
> > const char **, unsigned int),
> > const char *pathfmt, ...);
> >
> > -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
> > +void xenbus_switch_state(struct xenbus_device *dev,
> > + enum xenbus_state new_state);
> > int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn);
> > int xenbus_map_ring_valloc(struct xenbus_device *dev,
> > int gnt_ref, void **vaddr);
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 18:07 ` David Vrabel
` (4 preceding siblings ...)
(?)
@ 2014-09-29 14:03 ` Konrad Rzeszutek Wilk
-1 siblings, 0 replies; 32+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-09-29 14:03 UTC (permalink / raw)
To: David Vrabel
Cc: jgross, wei.liu2, Chen Gang, linux-scsi, linux-pci, linux-kernel,
yongjun_wei, netdev, bhelgaas, xen-devel, boris.ostrovsky,
ian.campbell
On Fri, Sep 26, 2014 at 07:07:19PM +0100, David Vrabel wrote:
> On 26/09/14 17:36, Chen Gang wrote:
> > When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> > internally, so need not return any status value, then use 'void' instead
> > of 'int' for xenbus_switch_state() and __xenbus_switch_state().
> >
> > Also need be sure that all callers which check the return value must let
> > 'err' be 0.
>
> I've rewritten the commit message as:
>
> xen/xenbus: don't return errors from xenbus_switch_state()
>
> Most users of xenbus_switch_state() weren't handling the failure of
> xenbus_switch_state() correctly. They either called
> xenbus_dev_fatal() (which xenbus_switch_state() has effectively
> already tried to do), or ignored errors.
>
> xenbus_switch_state() may fail because:
>
> a) The device is being unplugged by the toolstack. The device will
> shortly be removed and the error does not need to be handled.
>
> b) Xenstore is broken. There isn't much the driver can do in this
> case since xenstore is required to signal failure to the toolstack.
>
> So, don't report any errors from xenbus_switch_state() which removes
> some unnecessary error handling in some of the drivers.
>
> I'd appreciate a review from some of the other front/backend driver
> maintainers on whether this is sensible reasoning.
Done that but I am not too keen about this patch - as I think it will
cause memory leaks on failure paths.
>
> David
>
> >
> > And also need change the related comments for xenbus_switch_state().
> >
> > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> > ---
> > drivers/block/xen-blkback/xenbus.c | 9 ++-------
> > drivers/net/xen-netback/xenbus.c | 5 +----
> > drivers/pci/xen-pcifront.c | 15 ++++++---------
> > drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
> > drivers/xen/xen-scsiback.c | 5 +----
> > drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
> > include/xen/xenbus.h | 3 ++-
> > 7 files changed, 24 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> > index 3a8b810..fdcc584 100644
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
> > if (err)
> > goto fail;
> >
> > - err = xenbus_switch_state(dev, XenbusStateInitWait);
> > - if (err)
> > - goto fail;
> > + xenbus_switch_state(dev, XenbusStateInitWait);
> >
> > return 0;
> >
> > @@ -837,10 +835,7 @@ again:
> > if (err)
> > xenbus_dev_fatal(dev, err, "ending transaction");
> >
> > - err = xenbus_switch_state(dev, XenbusStateConnected);
> > - if (err)
> > - xenbus_dev_fatal(dev, err, "%s: switching to Connected state",
> > - dev->nodename);
> > + xenbus_switch_state(dev, XenbusStateConnected);
> >
> > return;
> > abort:
> > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> > index 9c47b89..b5c3d47 100644
> > --- a/drivers/net/xen-netback/xenbus.c
> > +++ b/drivers/net/xen-netback/xenbus.c
> > @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
> > if (err)
> > pr_debug("Error writing multi-queue-max-queues\n");
> >
> > - err = xenbus_switch_state(dev, XenbusStateInitWait);
> > - if (err)
> > - goto fail;
> > -
> > + xenbus_switch_state(dev, XenbusStateInitWait);
> > be->state = XenbusStateInitWait;
> >
> > /* This kicks hotplug scripts, so do it immediately. */
> > diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> > index 53df39a..c1d73b7 100644
> > --- a/drivers/pci/xen-pcifront.c
> > +++ b/drivers/pci/xen-pcifront.c
> > @@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device *pdev)
> > }
> > }
> >
> > - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> > -
> > + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> > + return 0;
> > out:
> > return err;
> > }
> >
> > static int pcifront_try_disconnect(struct pcifront_device *pdev)
> > {
> > - int err = 0;
> > enum xenbus_state prev_state;
> >
> > -
> > prev_state = xenbus_read_driver_state(pdev->xdev->nodename);
> >
> > if (prev_state >= XenbusStateClosing)
> > @@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
> > pcifront_disconnect(pdev);
> > }
> >
> > - err = xenbus_switch_state(pdev->xdev, XenbusStateClosed);
> > + xenbus_switch_state(pdev->xdev, XenbusStateClosed);
> >
> > out:
> > -
> > - return err;
> > + return 0;
> > }
> >
> > static int pcifront_attach_devices(struct pcifront_device *pdev)
> > @@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> > domain, bus, slot, func);
> > }
> >
> > - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> > -
> > + xenbus_switch_state(pdev->xdev, XenbusStateReconfiguring);
> > + return 0;
> > out:
> > return err;
> > }
> > diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> > index c214daa..630a215 100644
> > --- a/drivers/xen/xen-pciback/xenbus.c
> > +++ b/drivers/xen/xen-pciback/xenbus.c
> > @@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)
> >
> > dev_dbg(&pdev->xdev->dev, "Connecting...\n");
> >
> > - err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> > - if (err)
> > - xenbus_dev_fatal(pdev->xdev, err,
> > - "Error switching to connected state!");
> > + xenbus_switch_state(pdev->xdev, XenbusStateConnected);
> >
> > dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err);
> > out:
> > @@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device *pdev)
> > }
> > }
> >
> > - err = xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
> > - if (err) {
> > - xenbus_dev_fatal(pdev->xdev, err,
> > - "Error switching to reconfigured state!");
> > - goto out;
> > - }
> > + xenbus_switch_state(pdev->xdev, XenbusStateReconfigured);
> >
> > out:
> > mutex_unlock(&pdev->dev_lock);
> > @@ -640,10 +632,7 @@ static int xen_pcibk_setup_backend(struct xen_pcibk_device *pdev)
> > goto out;
> > }
> >
> > - err = xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
> > - if (err)
> > - xenbus_dev_fatal(pdev->xdev, err,
> > - "Error switching to initialised state!");
> > + xenbus_switch_state(pdev->xdev, XenbusStateInitialised);
> >
> > out:
> > mutex_unlock(&pdev->dev_lock);
> > @@ -683,9 +672,7 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
> > }
> >
> > /* wait for xend to configure us */
> > - err = xenbus_switch_state(dev, XenbusStateInitWait);
> > - if (err)
> > - goto out;
> > + xenbus_switch_state(dev, XenbusStateInitWait);
> >
> > /* watch the backend node for backend configuration information */
> > err = xenbus_watch_path(dev, dev->nodename, &pdev->be_watch,
> > diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> > index ad11258..847bc9c 100644
> > --- a/drivers/xen/xen-scsiback.c
> > +++ b/drivers/xen/xen-scsiback.c
> > @@ -1225,10 +1225,7 @@ static int scsiback_probe(struct xenbus_device *dev,
> > if (err)
> > xenbus_dev_error(dev, err, "writing feature-sg-grant");
> >
> > - err = xenbus_switch_state(dev, XenbusStateInitWait);
> > - if (err)
> > - goto fail;
> > -
> > + xenbus_switch_state(dev, XenbusStateInitWait);
> > return 0;
> >
> > fail:
> > diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> > index ca74410..e2bcd1d 100644
> > --- a/drivers/xen/xenbus/xenbus_client.c
> > +++ b/drivers/xen/xenbus/xenbus_client.c
> > @@ -166,7 +166,7 @@ EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
> > static void xenbus_switch_fatal(struct xenbus_device *, int, int,
> > const char *, ...);
> >
> > -static int
> > +static void
> > __xenbus_switch_state(struct xenbus_device *dev,
> > enum xenbus_state state, int depth)
> > {
> > @@ -188,7 +188,7 @@ __xenbus_switch_state(struct xenbus_device *dev,
> > int err, abort;
> >
> > if (state == dev->state)
> > - return 0;
> > + return;
> >
> > again:
> > abort = 1;
> > @@ -196,7 +196,7 @@ again:
> > err = xenbus_transaction_start(&xbt);
> > if (err) {
> > xenbus_switch_fatal(dev, depth, err, "starting transaction");
> > - return 0;
> > + return;
> > }
> >
> > err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state);
> > @@ -219,7 +219,7 @@ abort:
> > } else
> > dev->state = state;
> >
> > - return 0;
> > + return;
> > }
> >
> > /**
> > @@ -228,12 +228,12 @@ abort:
> > * @state: new state
> > *
> > * Advertise in the store a change of the given driver to the given new_state.
> > - * Return 0 on success, or -errno on error. On error, the device will switch
> > - * to XenbusStateClosing, and the error will be saved in the store.
> > + * When failure occurs, it will call xenbus_switch_fatal() internally, so need
> > + * not return value to notify upper caller.
> > */
> > -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> > +void xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
> > {
> > - return __xenbus_switch_state(dev, state, 0);
> > + __xenbus_switch_state(dev, state, 0);
> > }
> >
> > EXPORT_SYMBOL_GPL(xenbus_switch_state);
> > diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> > index 0324c6d..587c53d 100644
> > --- a/include/xen/xenbus.h
> > +++ b/include/xen/xenbus.h
> > @@ -195,7 +195,8 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
> > const char **, unsigned int),
> > const char *pathfmt, ...);
> >
> > -int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
> > +void xenbus_switch_state(struct xenbus_device *dev,
> > + enum xenbus_state new_state);
> > int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn);
> > int xenbus_map_ring_valloc(struct xenbus_device *dev,
> > int gnt_ref, void **vaddr);
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-29 14:02 ` Konrad Rzeszutek Wilk
@ 2014-09-29 14:17 ` David Vrabel
2014-09-29 14:17 ` David Vrabel
` (2 subsequent siblings)
3 siblings, 0 replies; 32+ messages in thread
From: David Vrabel @ 2014-09-29 14:17 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Chen Gang
Cc: ian.campbell, wei.liu2, boris.ostrovsky, bhelgaas, jgross,
yongjun_wei, mukesh.rathor, xen-devel, netdev, linux-pci,
linux-scsi, linux-kernel
On 29/09/14 15:02, Konrad Rzeszutek Wilk wrote:
> On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_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).
>
>> 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:
>
>
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c214daa..f7399fd 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -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).
> xen_pcibk_setup_backend(pdev);
> break;
>
>>
>> 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..
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>> index 9c47b89..b5c3d47 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>> if (err)
>> pr_debug("Error writing multi-queue-max-queues\n");
>>
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto fail;
>> -
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> 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.
It's not leaking the memory. All resources will be recovered when the
device is removed.
> The same is for xen-blkback mechanism in the probe function.
David
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
@ 2014-09-29 14:17 ` David Vrabel
0 siblings, 0 replies; 32+ messages in thread
From: David Vrabel @ 2014-09-29 14:17 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Chen Gang
Cc: ian.campbell, wei.liu2, boris.ostrovsky, bhelgaas, jgross,
yongjun_wei, mukesh.rathor, xen-devel, netdev, linux-pci,
linux-scsi, linux-kernel
On 29/09/14 15:02, Konrad Rzeszutek Wilk wrote:
> On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_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).
>
>> 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:
>
>
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c214daa..f7399fd 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -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).
> xen_pcibk_setup_backend(pdev);
> break;
>
>>
>> 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..
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>> index 9c47b89..b5c3d47 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>> if (err)
>> pr_debug("Error writing multi-queue-max-queues\n");
>>
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto fail;
>> -
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> 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.
It's not leaking the memory. All resources will be recovered when the
device is removed.
> The same is for xen-blkback mechanism in the probe function.
David
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-29 14:02 ` Konrad Rzeszutek Wilk
2014-09-29 14:17 ` David Vrabel
@ 2014-09-29 14:17 ` David Vrabel
2014-09-30 8:04 ` Chen Gang
2014-09-30 8:04 ` Chen Gang
3 siblings, 0 replies; 32+ messages in thread
From: David Vrabel @ 2014-09-29 14:17 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Chen Gang
Cc: jgross, wei.liu2, ian.campbell, linux-scsi, netdev, linux-kernel,
yongjun_wei, linux-pci, bhelgaas, xen-devel, boris.ostrovsky
On 29/09/14 15:02, Konrad Rzeszutek Wilk wrote:
> On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_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).
>
>> 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:
>
>
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c214daa..f7399fd 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -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).
> xen_pcibk_setup_backend(pdev);
> break;
>
>>
>> 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..
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>> index 9c47b89..b5c3d47 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>> if (err)
>> pr_debug("Error writing multi-queue-max-queues\n");
>>
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto fail;
>> -
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> 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.
It's not leaking the memory. All resources will be recovered when the
device is removed.
> The same is for xen-blkback mechanism in the probe function.
David
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-29 14:17 ` David Vrabel
(?)
@ 2014-09-29 15:40 ` Konrad Rzeszutek Wilk
2014-09-29 17:14 ` David Vrabel
2014-09-29 17:14 ` David Vrabel
-1 siblings, 2 replies; 32+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-09-29 15:40 UTC (permalink / raw)
To: David Vrabel
Cc: Chen Gang, ian.campbell, wei.liu2, boris.ostrovsky, bhelgaas,
jgross, yongjun_wei, mukesh.rathor, xen-devel, netdev, linux-pci,
linux-scsi, linux-kernel
On Mon, Sep 29, 2014 at 03:17:10PM +0100, David Vrabel wrote:
> On 29/09/14 15:02, Konrad Rzeszutek Wilk wrote:
> > On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
> >> When xenbus_switch_state() fails, it will call xenbus_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).
> >
> >> 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:
> >
> >
> > diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> > index c214daa..f7399fd 100644
> > --- a/drivers/xen/xen-pciback/xenbus.c
> > +++ b/drivers/xen/xen-pciback/xenbus.c
> > @@ -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).
>
> > xen_pcibk_setup_backend(pdev);
> > break;
> >
> >>
> >> 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..
> >> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> >> index 9c47b89..b5c3d47 100644
> >> --- a/drivers/net/xen-netback/xenbus.c
> >> +++ b/drivers/net/xen-netback/xenbus.c
> >> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
> >> if (err)
> >> pr_debug("Error writing multi-queue-max-queues\n");
> >>
> >> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> >> - if (err)
> >> - goto fail;
> >> -
> >> + xenbus_switch_state(dev, XenbusStateInitWait);
> >
> > 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.
>
> 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.
>
> > The same is for xen-blkback mechanism in the probe function.
>
> David
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-29 14:17 ` David Vrabel
(?)
(?)
@ 2014-09-29 15:40 ` Konrad Rzeszutek Wilk
-1 siblings, 0 replies; 32+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-09-29 15:40 UTC (permalink / raw)
To: David Vrabel
Cc: jgross, wei.liu2, Chen Gang, linux-scsi, netdev, linux-kernel,
yongjun_wei, linux-pci, bhelgaas, xen-devel, boris.ostrovsky,
ian.campbell
On Mon, Sep 29, 2014 at 03:17:10PM +0100, David Vrabel wrote:
> On 29/09/14 15:02, Konrad Rzeszutek Wilk wrote:
> > On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
> >> When xenbus_switch_state() fails, it will call xenbus_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).
> >
> >> 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:
> >
> >
> > diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> > index c214daa..f7399fd 100644
> > --- a/drivers/xen/xen-pciback/xenbus.c
> > +++ b/drivers/xen/xen-pciback/xenbus.c
> > @@ -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).
>
> > xen_pcibk_setup_backend(pdev);
> > break;
> >
> >>
> >> 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..
> >> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> >> index 9c47b89..b5c3d47 100644
> >> --- a/drivers/net/xen-netback/xenbus.c
> >> +++ b/drivers/net/xen-netback/xenbus.c
> >> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
> >> if (err)
> >> pr_debug("Error writing multi-queue-max-queues\n");
> >>
> >> - err = xenbus_switch_state(dev, XenbusStateInitWait);
> >> - if (err)
> >> - goto fail;
> >> -
> >> + xenbus_switch_state(dev, XenbusStateInitWait);
> >
> > 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.
>
> 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.
>
> > The same is for xen-blkback mechanism in the probe function.
>
> David
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-29 15:40 ` Konrad Rzeszutek Wilk
@ 2014-09-29 17:14 ` David Vrabel
2014-09-29 17:14 ` David Vrabel
1 sibling, 0 replies; 32+ messages in thread
From: David Vrabel @ 2014-09-29 17:14 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Chen Gang, ian.campbell, wei.liu2, boris.ostrovsky, bhelgaas,
jgross, yongjun_wei, mukesh.rathor, xen-devel, netdev, linux-pci,
linux-scsi, linux-kernel
On 29/09/14 16:40, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 29, 2014 at 03:17:10PM +0100, David Vrabel wrote:
>> On 29/09/14 15:02, Konrad Rzeszutek Wilk wrote:
>>> On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
>>>> When xenbus_switch_state() fails, it will call xenbus_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).
>>>
>>>> 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:
>>>
>>>
>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>>> index c214daa..f7399fd 100644
>>> --- a/drivers/xen/xen-pciback/xenbus.c
>>> +++ b/drivers/xen/xen-pciback/xenbus.c
>>> @@ -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).
>>
>>> xen_pcibk_setup_backend(pdev);
>>> break;
>>>
>>>>
>>>> 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..
>>>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>>>> index 9c47b89..b5c3d47 100644
>>>> --- a/drivers/net/xen-netback/xenbus.c
>>>> +++ b/drivers/net/xen-netback/xenbus.c
>>>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>>>> if (err)
>>>> pr_debug("Error writing multi-queue-max-queues\n");
>>>>
>>>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>>>> - if (err)
>>>> - goto fail;
>>>> -
>>>> + xenbus_switch_state(dev, XenbusStateInitWait);
>>>
>>> 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.
>>
>> 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
@ 2014-09-29 17:14 ` David Vrabel
0 siblings, 0 replies; 32+ messages in thread
From: David Vrabel @ 2014-09-29 17:14 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Chen Gang, ian.campbell, wei.liu2, boris.ostrovsky, bhelgaas,
jgross, yongjun_wei, mukesh.rathor, xen-devel, netdev, linux-pci,
linux-scsi, linux-kernel
On 29/09/14 16:40, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 29, 2014 at 03:17:10PM +0100, David Vrabel wrote:
>> On 29/09/14 15:02, Konrad Rzeszutek Wilk wrote:
>>> On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
>>>> When xenbus_switch_state() fails, it will call xenbus_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).
>>>
>>>> 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:
>>>
>>>
>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>>> index c214daa..f7399fd 100644
>>> --- a/drivers/xen/xen-pciback/xenbus.c
>>> +++ b/drivers/xen/xen-pciback/xenbus.c
>>> @@ -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).
>>
>>> xen_pcibk_setup_backend(pdev);
>>> break;
>>>
>>>>
>>>> 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..
>>>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>>>> index 9c47b89..b5c3d47 100644
>>>> --- a/drivers/net/xen-netback/xenbus.c
>>>> +++ b/drivers/net/xen-netback/xenbus.c
>>>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>>>> if (err)
>>>> pr_debug("Error writing multi-queue-max-queues\n");
>>>>
>>>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>>>> - if (err)
>>>> - goto fail;
>>>> -
>>>> + xenbus_switch_state(dev, XenbusStateInitWait);
>>>
>>> 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.
>>
>> 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-29 15:40 ` Konrad Rzeszutek Wilk
@ 2014-09-29 17:14 ` David Vrabel
2014-09-29 17:14 ` David Vrabel
1 sibling, 0 replies; 32+ messages in thread
From: David Vrabel @ 2014-09-29 17:14 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: jgross, wei.liu2, Chen Gang, linux-scsi, netdev, linux-kernel,
yongjun_wei, linux-pci, bhelgaas, xen-devel, boris.ostrovsky,
ian.campbell
On 29/09/14 16:40, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 29, 2014 at 03:17:10PM +0100, David Vrabel wrote:
>> On 29/09/14 15:02, Konrad Rzeszutek Wilk wrote:
>>> On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
>>>> When xenbus_switch_state() fails, it will call xenbus_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).
>>>
>>>> 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:
>>>
>>>
>>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>>> index c214daa..f7399fd 100644
>>> --- a/drivers/xen/xen-pciback/xenbus.c
>>> +++ b/drivers/xen/xen-pciback/xenbus.c
>>> @@ -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).
>>
>>> xen_pcibk_setup_backend(pdev);
>>> break;
>>>
>>>>
>>>> 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..
>>>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>>>> index 9c47b89..b5c3d47 100644
>>>> --- a/drivers/net/xen-netback/xenbus.c
>>>> +++ b/drivers/net/xen-netback/xenbus.c
>>>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>>>> if (err)
>>>> pr_debug("Error writing multi-queue-max-queues\n");
>>>>
>>>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>>>> - if (err)
>>>> - goto fail;
>>>> -
>>>> + xenbus_switch_state(dev, XenbusStateInitWait);
>>>
>>> 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.
>>
>> 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-29 14:02 ` Konrad Rzeszutek Wilk
` (2 preceding siblings ...)
2014-09-30 8:04 ` Chen Gang
@ 2014-09-30 8:04 ` Chen Gang
3 siblings, 0 replies; 32+ messages in thread
From: Chen Gang @ 2014-09-30 8:04 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: David Vrabel, ian.campbell, wei.liu2, boris.ostrovsky, bhelgaas,
jgross, yongjun_wei, mukesh.rathor, xen-devel, netdev, linux-pci,
linux-scsi, linux-kernel
On 9/29/14 22:02, Konrad Rzeszutek Wilk wrote:
> On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_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).
>
Yeah, I guess you want to give more completion for this comment, do not
mean the original comments is incorrect.
[...]
>>
>> 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..
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>> index 9c47b89..b5c3d47 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>> if (err)
>> pr_debug("Error writing multi-queue-max-queues\n");
>>
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto fail;
>> -
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> Which if it fails it won't call:
>
> 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.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-29 14:02 ` Konrad Rzeszutek Wilk
2014-09-29 14:17 ` David Vrabel
2014-09-29 14:17 ` David Vrabel
@ 2014-09-30 8:04 ` Chen Gang
2014-09-30 8:04 ` Chen Gang
3 siblings, 0 replies; 32+ messages in thread
From: Chen Gang @ 2014-09-30 8:04 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: jgross, wei.liu2, ian.campbell, linux-scsi, netdev, linux-kernel,
yongjun_wei, David Vrabel, linux-pci, bhelgaas, xen-devel,
boris.ostrovsky
On 9/29/14 22:02, Konrad Rzeszutek Wilk wrote:
> On Sat, Sep 27, 2014 at 12:36:42AM +0800, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_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).
>
Yeah, I guess you want to give more completion for this comment, do not
mean the original comments is incorrect.
[...]
>>
>> 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..
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>> index 9c47b89..b5c3d47 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
>> if (err)
>> pr_debug("Error writing multi-queue-max-queues\n");
>>
>> - err = xenbus_switch_state(dev, XenbusStateInitWait);
>> - if (err)
>> - goto fail;
>> -
>> + xenbus_switch_state(dev, XenbusStateInitWait);
>
> Which if it fails it won't call:
>
> 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.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 16:36 [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state() Chen Gang
@ 2014-09-30 9:55 ` David Vrabel
2014-09-26 18:07 ` David Vrabel
` (8 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: David Vrabel @ 2014-09-30 9:55 UTC (permalink / raw)
To: Chen Gang, konrad.wilk, ian.campbell, wei.liu2, boris.ostrovsky,
bhelgaas, jgross, yongjun_wei, mukesh.rathor
Cc: xen-devel, netdev, linux-pci, linux-scsi, linux-kernel
On 26/09/14 17:36, Chen Gang wrote:
> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> internally, so need not return any status value, then use 'void' instead
> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>
> Also need be sure that all callers which check the return value must let
> 'err' be 0.
>
> And also need change the related comments for xenbus_switch_state().
Since this patch does not fix a bug and there is no unanimous agreement
on the API change I'm not going to apply it (nor the previous version).
David
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
@ 2014-09-30 9:55 ` David Vrabel
0 siblings, 0 replies; 32+ messages in thread
From: David Vrabel @ 2014-09-30 9:55 UTC (permalink / raw)
To: Chen Gang, konrad.wilk, ian.campbell, wei.liu2, boris.ostrovsky,
bhelgaas, jgross, yongjun_wei, mukesh.rathor
Cc: xen-devel, netdev, linux-pci, linux-scsi, linux-kernel
On 26/09/14 17:36, Chen Gang wrote:
> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> internally, so need not return any status value, then use 'void' instead
> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>
> Also need be sure that all callers which check the return value must let
> 'err' be 0.
>
> And also need change the related comments for xenbus_switch_state().
Since this patch does not fix a bug and there is no unanimous agreement
on the API change I'm not going to apply it (nor the previous version).
David
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-26 16:36 [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state() Chen Gang
` (8 preceding siblings ...)
2014-09-30 9:55 ` David Vrabel
@ 2014-09-30 9:55 ` David Vrabel
9 siblings, 0 replies; 32+ messages in thread
From: David Vrabel @ 2014-09-30 9:55 UTC (permalink / raw)
To: Chen Gang, konrad.wilk, ian.campbell, wei.liu2, boris.ostrovsky,
bhelgaas, jgross, yongjun_wei, mukesh.rathor
Cc: xen-devel, linux-pci, linux-kernel, linux-scsi, netdev
On 26/09/14 17:36, Chen Gang wrote:
> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> internally, so need not return any status value, then use 'void' instead
> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>
> Also need be sure that all callers which check the return value must let
> 'err' be 0.
>
> And also need change the related comments for xenbus_switch_state().
Since this patch does not fix a bug and there is no unanimous agreement
on the API change I'm not going to apply it (nor the previous version).
David
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-30 9:55 ` David Vrabel
(?)
(?)
@ 2014-09-30 10:10 ` Chen Gang
-1 siblings, 0 replies; 32+ messages in thread
From: Chen Gang @ 2014-09-30 10:10 UTC (permalink / raw)
To: David Vrabel, konrad.wilk, ian.campbell, wei.liu2,
boris.ostrovsky, bhelgaas, jgross, yongjun_wei, mukesh.rathor
Cc: xen-devel, netdev, linux-pci, linux-scsi, linux-kernel
On 9/30/14 17:55, David Vrabel wrote:
> On 26/09/14 17:36, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
>> internally, so need not return any status value, then use 'void' instead
>> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>>
>> Also need be sure that all callers which check the return value must let
>> 'err' be 0.
>>
>> And also need change the related comments for xenbus_switch_state().
>
> Since this patch does not fix a bug and there is no unanimous agreement
> on the API change I'm not going to apply it (nor the previous version).
>
OK, at least for me, it is no problems.
But I still recommend to improve it in the future, it is not a good idea
to let all related things remain in current condition (for me, at lease
need some related code comments for it).
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
2014-09-30 9:55 ` David Vrabel
(?)
@ 2014-09-30 10:10 ` Chen Gang
-1 siblings, 0 replies; 32+ messages in thread
From: Chen Gang @ 2014-09-30 10:10 UTC (permalink / raw)
To: David Vrabel, konrad.wilk, ian.campbell, wei.liu2,
boris.ostrovsky, bhelgaas, jgross, yongjun_wei, mukesh.rathor
Cc: xen-devel, linux-pci, linux-kernel, linux-scsi, netdev
On 9/30/14 17:55, David Vrabel wrote:
> On 26/09/14 17:36, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
>> internally, so need not return any status value, then use 'void' instead
>> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>>
>> Also need be sure that all callers which check the return value must let
>> 'err' be 0.
>>
>> And also need change the related comments for xenbus_switch_state().
>
> Since this patch does not fix a bug and there is no unanimous agreement
> on the API change I'm not going to apply it (nor the previous version).
>
OK, at least for me, it is no problems.
But I still recommend to improve it in the future, it is not a good idea
to let all related things remain in current condition (for me, at lease
need some related code comments for it).
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2014-09-30 10:05 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-26 16:36 [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state() Chen Gang
2014-09-26 18:07 ` [Xen-devel] " David Vrabel
2014-09-26 18:07 ` David Vrabel
2014-09-27 9:20 ` Chen Gang
2014-09-27 9:20 ` Chen Gang
2014-09-29 8:41 ` Wei Liu
2014-09-29 8:41 ` [Xen-devel] " Wei Liu
2014-09-29 8:41 ` Wei Liu
2014-09-29 14:03 ` Konrad Rzeszutek Wilk
2014-09-29 14:03 ` [Xen-devel] " Konrad Rzeszutek Wilk
2014-09-26 18:07 ` David Vrabel
2014-09-29 5:09 ` [Xen-devel] " Juergen Gross
2014-09-29 5:09 ` Juergen Gross
2014-09-29 13:35 ` Bjorn Helgaas
2014-09-29 13:35 ` Bjorn Helgaas
2014-09-29 14:02 ` Konrad Rzeszutek Wilk
2014-09-29 14:17 ` David Vrabel
2014-09-29 14:17 ` David Vrabel
2014-09-29 15:40 ` Konrad Rzeszutek Wilk
2014-09-29 17:14 ` David Vrabel
2014-09-29 17:14 ` David Vrabel
2014-09-29 17:14 ` David Vrabel
2014-09-29 15:40 ` Konrad Rzeszutek Wilk
2014-09-29 14:17 ` David Vrabel
2014-09-30 8:04 ` Chen Gang
2014-09-30 8:04 ` Chen Gang
2014-09-29 14:02 ` Konrad Rzeszutek Wilk
2014-09-30 9:55 ` David Vrabel
2014-09-30 9:55 ` David Vrabel
2014-09-30 10:10 ` Chen Gang
2014-09-30 10:10 ` Chen Gang
2014-09-30 9:55 ` David Vrabel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.