cxlflash: Simplify attach path error cleanup

The cxlflash_disk_attach() routine currently uses a cascading error
gate strategy for its error cleanup path. While this strategy is
commonly used to handle cleanup scenarios, it is too restrictive when
function callouts need to be restructured. Problems range from
inserting error path bugs in previously 'good' code to the cleanup
path imposing design changes to how the normal path is structured.
A less restrictive approach is needed to support ordering changes
that come about when operating in different environments.

To overcome this restriction, the error cleanup path is modified to
have a single entrypoint and use conditional logic to cleanup where
necessary. Entities that require multiple cleanup steps must be
carefully vetted to ensure their APIs support state. In cases where
they do not (none as of this commit) additional local variables can
be used to maintain state on their behalf.

Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
Reviewed-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index b30b362318fa17975ecb7fbe5c39d15d860333d0..7ec0b7a92876cfbf4cc1eb1e162a4e0942253023 100644 (file)
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -1315,9 +1315,9 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
        u32 perms;
        int ctxid = -1;
        u64 rctxid = 0UL;
-       struct file *file;
+       struct file *file = NULL;
 
-       struct cxl_context *ctx;
+       struct cxl_context *ctx = NULL;
 
        int fd = -1;
 
@@ -1371,7 +1371,7 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
        if (unlikely(!lun_access)) {
                dev_err(dev, "%s: Unable to allocate lun_access!\n", __func__);
                rc = -ENOMEM;
-               goto err0;
+               goto err;
        }
 
        lun_access->lli = lli;
@@ -1391,21 +1391,21 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
                dev_err(dev, "%s: Could not initialize context %p\n",
                        __func__, ctx);
                rc = -ENODEV;
-               goto err1;
+               goto err;
        }
 
        ctxid = cxl_process_element(ctx);
        if (unlikely((ctxid >= MAX_CONTEXT) || (ctxid < 0))) {
                dev_err(dev, "%s: ctxid (%d) invalid!\n", __func__, ctxid);
                rc = -EPERM;
-               goto err2;
+               goto err;
        }
 
        file = cxl_get_fd(ctx, &cfg->cxl_fops, &fd);
        if (unlikely(fd < 0)) {
                rc = -ENODEV;
                dev_err(dev, "%s: Could not get file descriptor\n", __func__);
-               goto err2;
+               goto err;
        }
 
        /* Translate read/write O_* flags from fcntl.h to AFU permission bits */
@@ -1415,7 +1415,7 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
        if (unlikely(!ctxi)) {
                dev_err(dev, "%s: Failed to create context! (%d)\n",
                        __func__, ctxid);
-               goto err3;
+               goto err;
        }
 
        /* Context mutex is locked upon return */
@@ -1429,13 +1429,13 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
        if (unlikely(rc)) {
                dev_dbg(dev, "%s: Could not start context rc=%d\n",
                        __func__, rc);
-               goto err4;
+               goto err;
        }
 
        rc = afu_attach(cfg, ctxi);
        if (unlikely(rc)) {
                dev_err(dev, "%s: Could not attach AFU rc %d\n", __func__, rc);
-               goto err5;
+               goto err;
        }
 
        /*
@@ -1471,13 +1471,14 @@ out:
                __func__, ctxid, fd, attach->block_size, rc, attach->last_lba);
        return rc;
 
-err5:
-       cxl_stop_context(ctx);
-err4:
-       put_context(ctxi);
-       destroy_context(cfg, ctxi);
-       ctxi = NULL;
-err3:
+err:
+       /* Cleanup CXL context; okay to 'stop' even if it was not started */
+       if (!IS_ERR_OR_NULL(ctx)) {
+               cxl_stop_context(ctx);
+               cxl_release_context(ctx);
+               ctx = NULL;
+       }
+
        /*
         * Here, we're overriding the fops with a dummy all-NULL fops because
         * fput() calls the release fop, which will cause us to mistakenly
@@ -1485,15 +1486,21 @@ err3:
         * to that routine (cxlflash_cxl_release) we should try to fix the
         * issue here.
         */
-       file->f_op = &null_fops;
-       fput(file);
-       put_unused_fd(fd);
-       fd = -1;
-err2:
-       cxl_release_context(ctx);
-err1:
+       if (fd > 0) {
+               file->f_op = &null_fops;
+               fput(file);
+               put_unused_fd(fd);
+               fd = -1;
+               file = NULL;
+       }
+
+       /* Cleanup our context; safe to call even with mutex locked */
+       if (ctxi) {
+               destroy_context(cfg, ctxi);
+               ctxi = NULL;
+       }
+
        kfree(lun_access);
-err0:
        scsi_device_put(sdev);
        goto out;
 }