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

PATCH In review Alexandru Gagniuc 3 participants 4 messages May 10, 2018 – May 11, 2018 NVMe

Share & tools

Alexandru Gagniuc

author

2018-05-10 16:46 UTC #0 lore raw

PATCH 1/1 nvme-pci: Avoid use of goto in nvme_reset_work()

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 at intel.com>

Signed-off-by Alexandru Gagniuc <mr.nuke.me at gmail.com>

---
 drivers/nvme/host/pci.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

drivers/nvme/host/pci.c +24 ~~-18~~

index fbc71fac6f1e..bac7d30a10a7 100644
@@ -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)

Signature · 1 line

2.14.3

Keith Busch

2018-05-10 16:58 UTC #1 lore raw

Quoted 9 lines · 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 <[email protected]>
Signed-off-by Alexandru Gagniuc <[email protected]>

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

Alex G.

author

2018-05-10 17:01 UTC #2 lore raw

Quoted 8 lines · Keith Busch

Quoted 7 lines · 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

Quoted 6 lines

Quoted 2 lines
CC Keith Busch <keith.busch at intel.com>
Signed-off-by Alexandru Gagniuc <mr.nuke.me at 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

Christoph Hellwig

maintainer

2018-05-11 06:33 UTC #3 lore raw

In reply to Alexandru Gagniuc

Quoted 6 lines · 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

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