[PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()

[Chen Gang]

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 <***@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", &current_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);

[David Vrabel]

Post by Chen Gang
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

Post by Chen Gang
And also need change the related comments for xenbus_switch_state().
---
drivers/block/xen-blkback/xenbus.c | 9 ++-------
drivers/net/xen-netback/xenbus.c | 5 +----
drivers/pci/xen-pcifront.c | 15 ++++++---------
drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
drivers/xen/xen-scsiback.c | 5 +----
drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
include/xen/xenbus.h | 3 ++-
7 files changed, 24 insertions(+), 50 deletions(-)
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;
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;
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;
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);
-
- 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;
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);
@@ -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);
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);
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;
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;
abort = 1;
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", &current_state);
} else
dev->state = state;
- return 0;
+ return;
}
/**
*
* 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]

Post by David Vrabel

Post by Chen Gang
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.

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.
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.

Post by David Vrabel
David

Post by Chen Gang
And also need change the related comments for xenbus_switch_state().
---
drivers/block/xen-blkback/xenbus.c | 9 ++-------
drivers/net/xen-netback/xenbus.c | 5 +----
drivers/pci/xen-pcifront.c | 15 ++++++---------
drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
drivers/xen/xen-scsiback.c | 5 +----
drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
include/xen/xenbus.h | 3 ++-
7 files changed, 24 insertions(+), 50 deletions(-)
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;
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;
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;
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);
-
- 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;
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);
@@ -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);
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);
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;
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;
abort = 1;
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", &current_state);
} else
dev->state = state;
- return 0;
+ return;
}
/**
*
* 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);

[Wei Liu]

Post by David Vrabel

Post by Chen Gang
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.

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.
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.

[Konrad Rzeszutek Wilk]

Post by David Vrabel

Post by Chen Gang
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.

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.
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.

Post by David Vrabel
David

Post by Chen Gang
And also need change the related comments for xenbus_switch_state().
---
drivers/block/xen-blkback/xenbus.c | 9 ++-------
drivers/net/xen-netback/xenbus.c | 5 +----
drivers/pci/xen-pcifront.c | 15 ++++++---------
drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
drivers/xen/xen-scsiback.c | 5 +----
drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
include/xen/xenbus.h | 3 ++-
7 files changed, 24 insertions(+), 50 deletions(-)
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;
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;
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;
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);
-
- 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;
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);
@@ -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);
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);
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;
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;
abort = 1;
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", &current_state);
} else
dev->state = state;
- return 0;
+ return;
}
/**
*
* 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);

[Juergen Gross]

Post by Chen Gang
When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
internally, so need not return any status value, then use 'void' instead
of 'int' for xenbus_switch_state() and __xenbus_switch_state().
Also need be sure that all callers which check the return value must let
'err' be 0.
And also need change the related comments for xenbus_switch_state().
---
drivers/block/xen-blkback/xenbus.c | 9 ++-------
drivers/net/xen-netback/xenbus.c | 5 +----
drivers/pci/xen-pcifront.c | 15 ++++++---------
drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
drivers/xen/xen-scsiback.c | 5 +----
drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
include/xen/xenbus.h | 3 ++-
7 files changed, 24 insertions(+), 50 deletions(-)
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;
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;
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;
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);
-
- 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;
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);
@@ -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);
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);
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;
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;
abort = 1;
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", &current_state);
} else
dev->state = state;
- return 0;
+ return;
}
/**
*
* 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);

[Bjorn Helgaas]

Post by Chen Gang
When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
internally, so need not return any status value, then use 'void' instead
of 'int' for xenbus_switch_state() and __xenbus_switch_state().
Also need be sure that all callers which check the return value must let
'err' be 0.
And also need change the related comments for xenbus_switch_state().
---
drivers/block/xen-blkback/xenbus.c | 9 ++-------
drivers/net/xen-netback/xenbus.c | 5 +----
drivers/pci/xen-pcifront.c | 15 ++++++---------
drivers/xen/xen-pciback/xenbus.c | 21 ++++-----------------
drivers/xen/xen-scsiback.c | 5 +----
drivers/xen/xenbus/xenbus_client.c | 16 ++++++++--------
include/xen/xenbus.h | 3 ++-
7 files changed, 24 insertions(+), 50 deletions(-)

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

[Konrad Rzeszutek Wilk]

Post by Chen Gang
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).

Post by Chen Gang
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;

Post by Chen Gang
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..

Post by Chen Gang
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.

Post by Chen Gang
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;
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);
-
- 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;
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);

This one is OK

Post by Chen Gang
dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err);
@@ -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.

Post by Chen Gang
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.

Post by Chen Gang
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.

Post by Chen Gang
/* 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;

[David Vrabel]

Post by Konrad Rzeszutek Wilk

Post by Chen Gang
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).

Post by Chen Gang
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.
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)) {
+ /*
+ * 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).

Post by Konrad Rzeszutek Wilk
xen_pcibk_setup_backend(pdev);
break;

Post by Chen Gang
Also need be sure that all callers which check the return value must let
'err' be 0.

.. snip..

Post by Chen Gang
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);

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.

Post by Konrad Rzeszutek Wilk
The same is for xen-blkback mechanism in the probe function.

David

[Konrad Rzeszutek Wilk]

Post by David Vrabel

Post by Konrad Rzeszutek Wilk

Post by Chen Gang
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).

Post by Chen Gang
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.
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)) {
+ /*
+ * 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).

Post by Konrad Rzeszutek Wilk
xen_pcibk_setup_backend(pdev);
break;

Post by Chen Gang
Also need be sure that all callers which check the return value must let
'err' be 0.

.. snip..

Post by Chen Gang
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);

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.

Post by David Vrabel

Post by Konrad Rzeszutek Wilk
The same is for xen-blkback mechanism in the probe function.

David

[David Vrabel]

Post by Konrad Rzeszutek Wilk

Post by David Vrabel

Post by Konrad Rzeszutek Wilk

Post by Chen Gang
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).

Post by Chen Gang
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.
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)) {
+ /*
+ * 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).

Post by Konrad Rzeszutek Wilk
xen_pcibk_setup_backend(pdev);
break;

Post by Chen Gang
Also need be sure that all callers which check the return value must let
'err' be 0.

.. snip..

Post by Chen Gang
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);

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

[Chen Gang]

Post by Konrad Rzeszutek Wilk

Post by Chen Gang
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.

[...]

Post by Konrad Rzeszutek Wilk

Post by Chen Gang
Also need be sure that all callers which check the return value must let
'err' be 0.

.. snip..

Post by Chen Gang
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);

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.

[David Vrabel]

Post by Chen Gang
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

[Chen Gang]

Post by David Vrabel

Post by Chen Gang
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.