[PATCH] nvme-pci: Avoid use of goto in nvme_reset_work()

[PATCH] nvme-pci: Avoid use of goto in nvme_reset_work()

[Alexandru Gagniuc]

This patch started as a challenge from Keith relating to code
structuring with goto vs return. I think the final result improves
readability on two counts:
First, it clarifies the separation between work struct and nvme_dev.
Second, it makes it clearer what error is being passed on:
'return -ENODEV' vs 'goto out', where 'result' happens to be -ENODEV

CC: Keith Busch <keith.busch@intel.com>
Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/nvme/host/pci.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index fbc71fac6f1e..bac7d30a10a7 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2286,16 +2286,14 @@ static void nvme_remove_dead_ctrl(struct nvme_dev *dev, int status)
 		nvme_put_ctrl(&dev->ctrl);
 }
 
-static void nvme_reset_work(struct work_struct *work)
+static int do_nvme_reset_work(struct nvme_dev *dev)
 {
-	struct nvme_dev *dev =
-		container_of(work, struct nvme_dev, ctrl.reset_work);
 	bool was_suspend = !!(dev->ctrl.ctrl_config & NVME_CC_SHN_NORMAL);
 	int result = -ENODEV;
 	enum nvme_ctrl_state new_state = NVME_CTRL_LIVE;
 
 	if (WARN_ON(dev->ctrl.state != NVME_CTRL_RESETTING))
-		goto out;
+		return -ENODEV;
 
 	/*
 	 * If we're called to reset a live controller first shut it down before
@@ -2310,30 +2308,30 @@ static void nvme_reset_work(struct work_struct *work)
 	 */
 	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_CONNECTING)) {
 		dev_warn(dev->ctrl.device,
-			"failed to mark controller CONNECTING\n");
-		goto out;
+			 "failed to mark controller CONNECTING\n");
+		return -ENODEV;
 	}
 
 	result = nvme_pci_enable(dev);
 	if (result)
-		goto out;
+		return result;
 
 	result = nvme_pci_configure_admin_queue(dev);
 	if (result)
-		goto out;
+		return result;
 
 	result = nvme_alloc_admin_tags(dev);
 	if (result)
-		goto out;
+		return result;
 
 	result = nvme_init_identify(&dev->ctrl);
 	if (result)
-		goto out;
+		return result;
 
 	if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) {
 		if (!dev->ctrl.opal_dev)
 			dev->ctrl.opal_dev =
-				init_opal_dev(&dev->ctrl, &nvme_sec_submit);
+			init_opal_dev(&dev->ctrl, &nvme_sec_submit);
 		else if (was_suspend)
 			opal_unlock_from_suspend(dev->ctrl.opal_dev);
 	} else {
@@ -2351,12 +2349,12 @@ static void nvme_reset_work(struct work_struct *work)
 	if (dev->ctrl.hmpre) {
 		result = nvme_setup_host_mem(dev);
 		if (result < 0)
-			goto out;
+			return result;
 	}
 
 	result = nvme_setup_io_queues(dev);
 	if (result)
-		goto out;
+		return result;
 
 	/*
 	 * Keep the controller around but remove all namespaces if we don't have
@@ -2382,15 +2380,23 @@ static void nvme_reset_work(struct work_struct *work)
 	 */
 	if (!nvme_change_ctrl_state(&dev->ctrl, new_state)) {
 		dev_warn(dev->ctrl.device,
-			"failed to mark controller state %d\n", new_state);
-		goto out;
+			 "failed to mark controller state %d\n", new_state);
+		return 0;
 	}
 
 	nvme_start_ctrl(&dev->ctrl);
-	return;
+	return 0;
+
+}
+static void nvme_reset_work(struct work_struct *work)
+{
+	struct nvme_dev *dev =
+		container_of(work, struct nvme_dev, ctrl.reset_work);
+	int result;
 
- out:
-	nvme_remove_dead_ctrl(dev, result);
+	result = do_nvme_reset_work(dev);
+	if (result < 0)
+		nvme_remove_dead_ctrl(dev, result);
 }
 
 static void nvme_remove_dead_ctrl_work(struct work_struct *work)
-- 
2.14.3

Re: [PATCH] nvme-pci: Avoid use of goto in nvme_reset_work()

[Keith Busch]

On Thu, May 10, 2018 at 11:46:33AM -0500, Alexandru Gagniuc wrote:
> This patch started as a challenge from Keith relating to code
> structuring with goto vs return. I think the final result improves
> readability on two counts:
> First, it clarifies the separation between work struct and nvme_dev.
> Second, it makes it clearer what error is being passed on:
> 'return -ENODEV' vs 'goto out', where 'result' happens to be -ENODEV
> 
> CC: Keith Busch <keith.busch@intel.com>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>

Ah, that's just wrapping a function that has a single out. The challenge
is to find a better mechanism than 'goto' to unwind a failure that has
multiple outs, like nvme_probe().

Re: [PATCH] nvme-pci: Avoid use of goto in nvme_reset_work()

[Alex G.]

On 05/10/2018 12:00 PM, Keith Busch wrote:
> On Thu, May 10, 2018 at 11:46:33AM -0500, Alexandru Gagniuc wrote:
>> This patch started as a challenge from Keith relating to code
>> structuring with goto vs return. I think the final result improves
>> readability on two counts:
>> First, it clarifies the separation between work struct and nvme_dev.
>> Second, it makes it clearer what error is being passed on:
>> 'return -ENODEV' vs 'goto out', where 'result' happens to be -ENODEV
>>
>> CC: Keith Busch <keith.busch@intel.com>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> 
> Ah, that's just wrapping a function that has a single out. The challenge
> is to find a better mechanism than 'goto' to unwind a failure that has
> multiple outs, like nvme_probe().

The same principle applies there too. It might have to be wrapped in
several chunks, and it will likely be more readable. Am I supposed to
bite and refactor that too?

Alex

Re: [PATCH] nvme-pci: Avoid use of goto in nvme_reset_work()

[Christoph Hellwig]

On Thu, May 10, 2018 at 11:46:33AM -0500, Alexandru Gagniuc wrote:
> This patch started as a challenge from Keith relating to code
> structuring with goto vs return. I think the final result improves
> readability on two counts:
> First, it clarifies the separation between work struct and nvme_dev.
> Second, it makes it clearer what error is being passed on:
> 'return -ENODEV' vs 'goto out', where 'result' happens to be -ENODEV

I think this actually makes the code much less readable.  The only real
improvement the code needs is to replace the "out" label name with
something more descriptive like "remove_controller".