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]>
---
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
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 <[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().
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 <[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().
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
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".