[PATCH 0/6] cxlflash: Improvements and cleanup

[PATCH 0/6] cxlflash: Improvements and cleanup

[Matthew R. Ochs]

This patch set contains various code improvements and cleanups that
were inspired by Al Viro upon reviewing the cxlflash driver. The core
improvement is that the driver will no longer cache the adapter file
descriptor associated with a context. This results in a user API change
that is documented alongside the modifications.

The series is based upon 4.8-rc1, intended for 4.9, and is bisectable.

Matthew R. Ochs (6):
  cxlflash: Avoid mutex when destroying context
  cxlflash: Cache owning adapter within context
  cxlflash: Add kref to context
  cxlflash: Transition to application close model
  cxlflash: Remove adapter file descriptor cache
  cxlflash: Update documentation

 Documentation/powerpc/cxlflash.txt |  44 ++++++++-
 drivers/scsi/cxlflash/superpipe.c  | 181 ++++++++++++++++---------------------
 drivers/scsi/cxlflash/superpipe.h  |   3 +-
 drivers/scsi/cxlflash/vlun.c       |  13 +--
 include/uapi/scsi/cxlflash_ioctl.h |  19 +++-
 5 files changed, 135 insertions(+), 125 deletions(-)

-- 
2.1.0

[PATCH 1/6] cxlflash: Avoid mutex when destroying context

[Matthew R. Ochs]

Context information structures are protected by a mutex that is held
when accessing/manipulating the context. When the code that manages
these structures was authored, a decision was made to include taking
the mutex as part of the allocation/initialization sequence and also
handle the scenario where the mutex was already held when freeing the
context.

While not a problem outright, this design decision has been deemed as
too flexible and the code should be made more rigid to avoid future bugs.
In addition, further review of the code yields that the existing mutex
manipulations in both of these context management paths are superfluous.

This commit removes the obtaining of the context mutex in the context
initialization routine and assumes the mutex is not held in the context
free path.

Inspired-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/superpipe.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index ce15070..ab5c893 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -709,14 +709,13 @@ int cxlflash_disk_release(struct scsi_device *sdev,
  * @cfg:	Internal structure associated with the host.
  * @ctxi:	Context to release.
  *
- * This routine is safe to be called with a a non-initialized context
- * and is tolerant of being called with the context's mutex held (it
- * will be unlocked if necessary before freeing). Also note that the
- * routine conditionally checks for the existence of the context control
- * map before clearing the RHT registers and context capabilities because
- * it is possible to destroy a context while the context is in the error
- * state (previous mapping was removed [so there is no need to worry about
- * clearing] and context is waiting for a new mapping).
+ * This routine is safe to be called with a a non-initialized context.
+ * Also note that the routine conditionally checks for the existence
+ * of the context control map before clearing the RHT registers and
+ * context capabilities because it is possible to destroy a context
+ * while the context is in the error state (previous mapping was
+ * removed [so there is no need to worry about clearing] and context
+ * is waiting for a new mapping).
  */
 static void destroy_context(struct cxlflash_cfg *cfg,
 			    struct ctx_info *ctxi)
@@ -732,9 +731,6 @@ static void destroy_context(struct cxlflash_cfg *cfg,
 			writeq_be(0, &ctxi->ctrl_map->rht_cnt_id);
 			writeq_be(0, &ctxi->ctrl_map->ctx_cap);
 		}
-
-		if (mutex_is_locked(&ctxi->mutex))
-			mutex_unlock(&ctxi->mutex);
 	}
 
 	/* Free memory associated with context */
@@ -795,9 +791,6 @@ err:
  * @adap_fd:	Previously obtained adapter fd associated with CXL context.
  * @file:	Previously obtained file associated with CXL context.
  * @perms:	User-specified permissions.
- *
- * Upon return, the context is marked as initialized and the context's mutex
- * is locked.
  */
 static void init_context(struct ctx_info *ctxi, struct cxlflash_cfg *cfg,
 			 struct cxl_context *ctx, int ctxid, int adap_fd,
@@ -816,8 +809,6 @@ static void init_context(struct ctx_info *ctxi, struct cxlflash_cfg *cfg,
 	mutex_init(&ctxi->mutex);
 	INIT_LIST_HEAD(&ctxi->luns);
 	INIT_LIST_HEAD(&ctxi->list); /* initialize for list_empty() */
-
-	mutex_lock(&ctxi->mutex);
 }
 
 /**
@@ -1445,7 +1436,6 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 	 * knows about us yet; we can be the only one holding our mutex.
 	 */
 	list_add(&lun_access->list, &ctxi->luns);
-	mutex_unlock(&ctxi->mutex);
 	mutex_lock(&cfg->ctx_tbl_list_mutex);
 	mutex_lock(&ctxi->mutex);
 	cfg->ctx_tbl[ctxid] = ctxi;
@@ -1494,7 +1484,7 @@ err:
 		file = NULL;
 	}
 
-	/* Cleanup our context; safe to call even with mutex locked */
+	/* Cleanup our context */
 	if (ctxi) {
 		destroy_context(cfg, ctxi);
 		ctxi = NULL;
-- 
2.1.0

Re: [PATCH 1/6] cxlflash: Avoid mutex when destroying context

[Manoj Kumar]

Acked-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>

On 8/9/2016 6:39 PM, Matthew R. Ochs wrote:
> Context information structures are protected by a mutex that is held
> when accessing/manipulating the context. When the code that manages
> these structures was authored, a decision was made to include taking
> the mutex as part of the allocation/initialization sequence and also
> handle the scenario where the mutex was already held when freeing the
> context.
>
> While not a problem outright, this design decision has been deemed as
> too flexible and the code should be made more rigid to avoid future bugs.
> In addition, further review of the code yields that the existing mutex
> manipulations in both of these context management paths are superfluous.
>
> This commit removes the obtaining of the context mutex in the context
> initialization routine and assumes the mutex is not held in the context
> free path.
>
> Inspired-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>

[PATCH 2/6] cxlflash: Cache owning adapter within context

[Matthew R. Ochs]

The context removal routine requires access to the owning adapter
structure to reset the context within the AFU as part of the tear
down sequence. In order to support kref adoption, the owning adapter
must be accessible from the release handler. As the kref framework
only provides the kref reference as the sole parameter, another means
is needed to derive the owning adapter.

As a remedy, the owning adapter reference is saved off within the
context during initialization.

Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/superpipe.c | 1 +
 drivers/scsi/cxlflash/superpipe.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index ab5c893..640c3a2 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -804,6 +804,7 @@ static void init_context(struct ctx_info *ctxi, struct cxlflash_cfg *cfg,
 	ctxi->lfd = adap_fd;
 	ctxi->pid = current->tgid; /* tgid = pid */
 	ctxi->ctx = ctx;
+	ctxi->cfg = cfg;
 	ctxi->file = file;
 	ctxi->initialized = true;
 	mutex_init(&ctxi->mutex);
diff --git a/drivers/scsi/cxlflash/superpipe.h b/drivers/scsi/cxlflash/superpipe.h
index 5f9a091..61404f2 100644
--- a/drivers/scsi/cxlflash/superpipe.h
+++ b/drivers/scsi/cxlflash/superpipe.h
@@ -107,6 +107,7 @@ struct ctx_info {
 	bool err_recovery_active;
 	struct mutex mutex; /* Context protection */
 	struct cxl_context *ctx;
+	struct cxlflash_cfg *cfg;
 	struct list_head luns;	/* LUNs attached to this context */
 	const struct vm_operations_struct *cxl_mmap_vmops;
 	struct file *file;
-- 
2.1.0

Re: [PATCH 2/6] cxlflash: Cache owning adapter within context

[Manoj Kumar]

Acked-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>

On 8/9/2016 6:39 PM, Matthew R. Ochs wrote:
> The context removal routine requires access to the owning adapter
> structure to reset the context within the AFU as part of the tear
> down sequence. In order to support kref adoption, the owning adapter
> must be accessible from the release handler. As the kref framework
> only provides the kref reference as the sole parameter, another means
> is needed to derive the owning adapter.
>
> As a remedy, the owning adapter reference is saved off within the
> context during initialization.
>
> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
> ---
>  drivers/scsi/cxlflash/superpipe.c | 1 +
>  drivers/scsi/cxlflash/superpipe.h | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
> index ab5c893..640c3a2 100644
> --- a/drivers/scsi/cxlflash/superpipe.c
> +++ b/drivers/scsi/cxlflash/superpipe.c
> @@ -804,6 +804,7 @@ static void init_context(struct ctx_info *ctxi, struct cxlflash_cfg *cfg,
>  	ctxi->lfd = adap_fd;
>  	ctxi->pid = current->tgid; /* tgid = pid */
>  	ctxi->ctx = ctx;
> +	ctxi->cfg = cfg;
>  	ctxi->file = file;
>  	ctxi->initialized = true;
>  	mutex_init(&ctxi->mutex);
> diff --git a/drivers/scsi/cxlflash/superpipe.h b/drivers/scsi/cxlflash/superpipe.h
> index 5f9a091..61404f2 100644
> --- a/drivers/scsi/cxlflash/superpipe.h
> +++ b/drivers/scsi/cxlflash/superpipe.h
> @@ -107,6 +107,7 @@ struct ctx_info {
>  	bool err_recovery_active;
>  	struct mutex mutex; /* Context protection */
>  	struct cxl_context *ctx;
> +	struct cxlflash_cfg *cfg;
>  	struct list_head luns;	/* LUNs attached to this context */
>  	const struct vm_operations_struct *cxl_mmap_vmops;
>  	struct file *file;
>

[PATCH 3/6] cxlflash: Add kref to context

[Matthew R. Ochs]

Currently, context user references are tracked via the list of LUNs
that have attached to the context. While convenient, this is not
intuitive without a deep study of the code and is inconsistent with
the existing reference tracking patterns within the kernel. This design
choice can lead to future bug injection.

To improve code comprehension and better protect against future bugs, add
explicit reference counting to contexts and migrate the context removal
code to the kref release handler.

Inspired-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/superpipe.c | 87 +++++++++++++++++++++++----------------
 drivers/scsi/cxlflash/superpipe.h |  1 +
 2 files changed, 53 insertions(+), 35 deletions(-)

diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index 640c3a2..be7522a 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -808,11 +808,56 @@ static void init_context(struct ctx_info *ctxi, struct cxlflash_cfg *cfg,
 	ctxi->file = file;
 	ctxi->initialized = true;
 	mutex_init(&ctxi->mutex);
+	kref_init(&ctxi->kref);
 	INIT_LIST_HEAD(&ctxi->luns);
 	INIT_LIST_HEAD(&ctxi->list); /* initialize for list_empty() */
 }
 
 /**
+ * remove_context() - context kref release handler
+ * @kref:	Kernel reference associated with context to be removed.
+ *
+ * When a context no longer has any references it can safely be removed
+ * from global access and destroyed. Note that it is assumed the thread
+ * relinquishing access to the context holds its mutex.
+ */
+static void remove_context(struct kref *kref)
+{
+	struct ctx_info *ctxi = container_of(kref, struct ctx_info, kref);
+	struct cxlflash_cfg *cfg = ctxi->cfg;
+	int lfd;
+	u64 ctxid = DECODE_CTXID(ctxi->ctxid);
+
+	/* Remove context from table/error list */
+	WARN_ON(!mutex_is_locked(&ctxi->mutex));
+	ctxi->unavail = true;
+	mutex_unlock(&ctxi->mutex);
+	mutex_lock(&cfg->ctx_tbl_list_mutex);
+	mutex_lock(&ctxi->mutex);
+
+	if (!list_empty(&ctxi->list))
+		list_del(&ctxi->list);
+	cfg->ctx_tbl[ctxid] = NULL;
+	mutex_unlock(&cfg->ctx_tbl_list_mutex);
+	mutex_unlock(&ctxi->mutex);
+
+	/* Context now completely uncoupled/unreachable */
+	lfd = ctxi->lfd;
+	destroy_context(cfg, ctxi);
+
+	/*
+	 * As a last step, clean up external resources when not
+	 * already on an external cleanup thread, i.e.: close(adap_fd).
+	 *
+	 * NOTE: this will free up the context from the CXL services,
+	 * allowing it to dole out the same context_id on a future
+	 * (or even currently in-flight) disk_attach operation.
+	 */
+	if (lfd != -1)
+		sys_close(lfd);
+}
+
+/**
  * _cxlflash_disk_detach() - detaches a LUN from a context
  * @sdev:	SCSI device associated with LUN.
  * @ctxi:	Context owning resources.
@@ -837,7 +882,6 @@ static int _cxlflash_disk_detach(struct scsi_device *sdev,
 
 	int i;
 	int rc = 0;
-	int lfd;
 	u64 ctxid = DECODE_CTXID(detach->context_id),
 	    rctxid = detach->context_id;
 
@@ -879,40 +923,12 @@ static int _cxlflash_disk_detach(struct scsi_device *sdev,
 			break;
 		}
 
-	/* Tear down context following last LUN cleanup */
-	if (list_empty(&ctxi->luns)) {
-		ctxi->unavail = true;
-		mutex_unlock(&ctxi->mutex);
-		mutex_lock(&cfg->ctx_tbl_list_mutex);
-		mutex_lock(&ctxi->mutex);
-
-		/* Might not have been in error list so conditionally remove */
-		if (!list_empty(&ctxi->list))
-			list_del(&ctxi->list);
-		cfg->ctx_tbl[ctxid] = NULL;
-		mutex_unlock(&cfg->ctx_tbl_list_mutex);
-		mutex_unlock(&ctxi->mutex);
-
-		lfd = ctxi->lfd;
-		destroy_context(cfg, ctxi);
-		ctxi = NULL;
-		put_ctx = false;
-
-		/*
-		 * As a last step, clean up external resources when not
-		 * already on an external cleanup thread, i.e.: close(adap_fd).
-		 *
-		 * NOTE: this will free up the context from the CXL services,
-		 * allowing it to dole out the same context_id on a future
-		 * (or even currently in-flight) disk_attach operation.
-		 */
-		if (lfd != -1)
-			sys_close(lfd);
-	}
-
-	/* Release the sdev reference that bound this LUN to the context */
+	/*
+	 * Release the context reference and the sdev reference that
+	 * bound this LUN to the context.
+	 */
+	put_ctx = !kref_put(&ctxi->kref, remove_context);
 	scsi_device_put(sdev);
-
 out:
 	if (put_ctx)
 		put_context(ctxi);
@@ -1369,10 +1385,11 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 	lun_access->lli = lli;
 	lun_access->sdev = sdev;
 
-	/* Non-NULL context indicates reuse */
+	/* Non-NULL context indicates reuse (another context reference) */
 	if (ctxi) {
 		dev_dbg(dev, "%s: Reusing context for LUN! (%016llX)\n",
 			__func__, rctxid);
+		kref_get(&ctxi->kref);
 		list_add(&lun_access->list, &ctxi->luns);
 		fd = ctxi->lfd;
 		goto out_attach;
diff --git a/drivers/scsi/cxlflash/superpipe.h b/drivers/scsi/cxlflash/superpipe.h
index 61404f2..5bda8b5 100644
--- a/drivers/scsi/cxlflash/superpipe.h
+++ b/drivers/scsi/cxlflash/superpipe.h
@@ -106,6 +106,7 @@ struct ctx_info {
 	bool unavail;
 	bool err_recovery_active;
 	struct mutex mutex; /* Context protection */
+	struct kref kref;
 	struct cxl_context *ctx;
 	struct cxlflash_cfg *cfg;
 	struct list_head luns;	/* LUNs attached to this context */
-- 
2.1.0

Re: [PATCH 3/6] cxlflash: Add kref to context

[Manoj Kumar]

Acked-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>

On 8/9/2016 6:39 PM, Matthew R. Ochs wrote:
> Currently, context user references are tracked via the list of LUNs
> that have attached to the context. While convenient, this is not
> intuitive without a deep study of the code and is inconsistent with
> the existing reference tracking patterns within the kernel. This design
> choice can lead to future bug injection.
>
> To improve code comprehension and better protect against future bugs, add
> explicit reference counting to contexts and migrate the context removal
> code to the kref release handler.
>
> Inspired-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>

[PATCH 4/6] cxlflash: Transition to application close model

[Matthew R. Ochs]

Caching the adapter file descriptor and performing a close on behalf
of an application is a poor design. This is due to the fact that once
a file descriptor in installed, it is free to be altered without the
knowledge of the cxlflash driver. This can lead to inconsistencies
between the application and kernel. Furthermore, the nature of the
former design is more exploitable and thus should be abandoned.

To support applications performing a close on the adapter file that
is associated with a context, a new flag is introduced to the user
API to indicate to applications that they are responsible for the
close following the cleanup (detach) of a context. The documentation
is also updated to reflect this change in behavior.

Inspired-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
 Documentation/powerpc/cxlflash.txt | 42 +++++++++++++++++++---
 drivers/scsi/cxlflash/superpipe.c  | 71 ++++++++++----------------------------
 drivers/scsi/cxlflash/vlun.c       | 13 ++-----
 include/uapi/scsi/cxlflash_ioctl.h | 19 +++++++---
 4 files changed, 73 insertions(+), 72 deletions(-)

diff --git a/Documentation/powerpc/cxlflash.txt b/Documentation/powerpc/cxlflash.txt
index 4202d1b..f4c1190 100644
--- a/Documentation/powerpc/cxlflash.txt
+++ b/Documentation/powerpc/cxlflash.txt
@@ -171,11 +171,30 @@ DK_CXLFLASH_ATTACH
           destroyed, the tokens are to be considered stale and subsequent
           usage will result in errors.
 
+	- A valid adapter file descriptor (fd2 >= 0) is only returned on
+	  the initial attach for a context. Subsequent attaches to an
+	  existing context (DK_CXLFLASH_ATTACH_REUSE_CONTEXT flag present)
+	  do not provide the adapter file descriptor as it was previously
+	  made known to the application.
+
         - When a context is no longer needed, the user shall detach from
-          the context via the DK_CXLFLASH_DETACH ioctl.
+          the context via the DK_CXLFLASH_DETACH ioctl. When this ioctl
+	  returns with a valid adapter file descriptor and the return flag
+	  DK_CXLFLASH_APP_CLOSE_ADAP_FD is present, the application _must_
+	  close the adapter file descriptor following a successful detach.
+
+	- When this ioctl returns with a valid fd2 and the return flag
+	  DK_CXLFLASH_APP_CLOSE_ADAP_FD is present, the application _must_
+	  close fd2 in the following circumstances:
+
+		+ Following a successful detach of the last user of the context
+		+ Following a successful recovery on the context's original fd2
+		+ In the child process of a fork(), following a clone ioctl,
+		  on the fd2 associated with the source context
 
-        - A close on fd2 will invalidate the tokens. This operation is not
-          required by the user.
+        - At any time, a close on fd2 will invalidate the tokens. Applications
+	  should exercise caution to only close fd2 when appropriate (outlined
+	  in the previous bullet) to avoid premature loss of I/O.
 
 DK_CXLFLASH_USER_DIRECT
 -----------------------
@@ -254,6 +273,10 @@ DK_CXLFLASH_DETACH
     success, all "tokens" which had been provided to the user from the
     DK_CXLFLASH_ATTACH onward are no longer valid.
 
+    When the DK_CXLFLASH_APP_CLOSE_ADAP_FD flag was returned on a successful
+    attach, the application _must_ close the fd2 associated with the context
+    following the detach of the final user of the context.
+
 DK_CXLFLASH_VLUN_CLONE
 ----------------------
     This ioctl is responsible for cloning a previously created
@@ -261,7 +284,7 @@ DK_CXLFLASH_VLUN_CLONE
     support maintaining user space access to storage after a process
     forks. Upon success, the child process (which invoked the ioctl)
     will have access to the same LUNs via the same resource handle(s)
-    and fd2 as the parent, but under a different context.
+    as the parent, but under a different context.
 
     Context sharing across processes is not supported with CXL and
     therefore each fork must be met with establishing a new context
@@ -275,6 +298,12 @@ DK_CXLFLASH_VLUN_CLONE
     translation tables are copied from the parent context to the child's
     and then synced with the AFU.
 
+    When the DK_CXLFLASH_APP_CLOSE_ADAP_FD flag was returned on a successful
+    attach, the application _must_ close the fd2 associated with the source
+    context (still resident/accessible in the parent process) following the
+    clone. This is to avoid a stale entry in the file descriptor table of the
+    child process.
+
 DK_CXLFLASH_VERIFY
 ------------------
     This ioctl is used to detect various changes such as the capacity of
@@ -309,6 +338,11 @@ DK_CXLFLASH_RECOVER_AFU
     at which time the context/resources they held will be freed as part of
     the release fop.
 
+    When the DK_CXLFLASH_APP_CLOSE_ADAP_FD flag was returned on a successful
+    attach, the application _must_ unmap and close the fd2 associated with the
+    original context following this ioctl returning success and indicating that
+    the context was recovered (DK_CXLFLASH_RECOVER_AFU_CONTEXT_RESET).
+
 DK_CXLFLASH_MANAGE_LUN
 ----------------------
     This ioctl is used to switch a LUN from a mode where it is available
diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index be7522a..b3bb90d 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -825,7 +825,6 @@ static void remove_context(struct kref *kref)
 {
 	struct ctx_info *ctxi = container_of(kref, struct ctx_info, kref);
 	struct cxlflash_cfg *cfg = ctxi->cfg;
-	int lfd;
 	u64 ctxid = DECODE_CTXID(ctxi->ctxid);
 
 	/* Remove context from table/error list */
@@ -842,19 +841,7 @@ static void remove_context(struct kref *kref)
 	mutex_unlock(&ctxi->mutex);
 
 	/* Context now completely uncoupled/unreachable */
-	lfd = ctxi->lfd;
 	destroy_context(cfg, ctxi);
-
-	/*
-	 * As a last step, clean up external resources when not
-	 * already on an external cleanup thread, i.e.: close(adap_fd).
-	 *
-	 * NOTE: this will free up the context from the CXL services,
-	 * allowing it to dole out the same context_id on a future
-	 * (or even currently in-flight) disk_attach operation.
-	 */
-	if (lfd != -1)
-		sys_close(lfd);
 }
 
 /**
@@ -949,34 +936,18 @@ static int cxlflash_disk_detach(struct scsi_device *sdev,
  *
  * This routine is the release handler for the fops registered with
  * the CXL services on an initial attach for a context. It is called
- * when a close is performed on the adapter file descriptor returned
- * to the user. Programmatically, the user is not required to perform
- * the close, as it is handled internally via the detach ioctl when
- * a context is being removed. Note that nothing prevents the user
- * from performing a close, but the user should be aware that doing
- * so is considered catastrophic and subsequent usage of the superpipe
- * API with previously saved off tokens will fail.
+ * when a close (explicity by the user or as part of a process tear
+ * down) is performed on the adapter file descriptor returned to the
+ * user. The user should be aware that explicitly performing a close
+ * considered catastrophic and subsequent usage of the superpipe API
+ * with previously saved off tokens will fail.
  *
- * When initiated from an external close (either by the user or via
- * a process tear down), the routine derives the context reference
- * and calls detach for each LUN associated with the context. The
- * final detach operation will cause the context itself to be freed.
- * Note that the saved off lfd is reset prior to calling detach to
- * signify that the final detach should not perform a close.
- *
- * When initiated from a detach operation as part of the tear down
- * of a context, the context is first completely freed and then the
- * close is performed. This routine will fail to derive the context
- * reference (due to the context having already been freed) and then
- * call into the CXL release entry point.
- *
- * Thus, with exception to when the CXL process element (context id)
- * lookup fails (a case that should theoretically never occur), every
- * call into this routine results in a complete freeing of a context.
- *
- * As part of the detach, all per-context resources associated with the LUN
- * are cleaned up. When detaching the last LUN for a context, the context
- * itself is cleaned up and released.
+ * This routine derives the context reference and calls detach for
+ * each LUN associated with the context.The final detach operation
+ * causes the context itself to be freed. With exception to when the
+ * CXL process element (context id) lookup fails (a case that should
+ * theoretically never occur), every call into this routine results
+ * in a complete freeing of a context.
  *
  * Return: 0 on success
  */
@@ -1014,11 +985,8 @@ static int cxlflash_cxl_release(struct inode *inode, struct file *file)
 		goto out;
 	}
 
-	dev_dbg(dev, "%s: close(%d) for context %d\n",
-		__func__, ctxi->lfd, ctxid);
+	dev_dbg(dev, "%s: close for context %d\n", __func__, ctxid);
 
-	/* Reset the file descriptor to indicate we're on a close() thread */
-	ctxi->lfd = -1;
 	detach.context_id = ctxi->ctxid;
 	list_for_each_entry_safe(lun_access, t, &ctxi->luns, list)
 		_cxlflash_disk_detach(lun_access->sdev, ctxi, &detach);
@@ -1391,7 +1359,6 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 			__func__, rctxid);
 		kref_get(&ctxi->kref);
 		list_add(&lun_access->list, &ctxi->luns);
-		fd = ctxi->lfd;
 		goto out_attach;
 	}
 
@@ -1461,7 +1428,11 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 	fd_install(fd, file);
 
 out_attach:
-	attach->hdr.return_flags = 0;
+	if (fd != -1)
+		attach->hdr.return_flags = DK_CXLFLASH_APP_CLOSE_ADAP_FD;
+	else
+		attach->hdr.return_flags = 0;
+
 	attach->context_id = ctxi->ctxid;
 	attach->block_size = gli->blk_len;
 	attach->mmio_size = sizeof(afu->afu_map->hosts[0].harea);
@@ -1526,7 +1497,7 @@ static int recover_context(struct cxlflash_cfg *cfg, struct ctx_info *ctxi)
 {
 	struct device *dev = &cfg->dev->dev;
 	int rc = 0;
-	int old_fd, fd = -1;
+	int fd = -1;
 	int ctxid = -1;
 	struct file *file;
 	struct cxl_context *ctx;
@@ -1574,7 +1545,6 @@ static int recover_context(struct cxlflash_cfg *cfg, struct ctx_info *ctxi)
 	 * No error paths after this point. Once the fd is installed it's
 	 * visible to user space and can't be undone safely on this thread.
 	 */
-	old_fd = ctxi->lfd;
 	ctxi->ctxid = ENCODE_CTXID(ctxi, ctxid);
 	ctxi->lfd = fd;
 	ctxi->ctx = ctx;
@@ -1593,9 +1563,6 @@ static int recover_context(struct cxlflash_cfg *cfg, struct ctx_info *ctxi)
 	cfg->ctx_tbl[ctxid] = ctxi;
 	mutex_unlock(&cfg->ctx_tbl_list_mutex);
 	fd_install(fd, file);
-
-	/* Release the original adapter fd and associated CXL resources */
-	sys_close(old_fd);
 out:
 	dev_dbg(dev, "%s: returning ctxid=%d fd=%d rc=%d\n",
 		__func__, ctxid, fd, rc);
@@ -1707,7 +1674,7 @@ retry_recover:
 		recover->context_id = ctxi->ctxid;
 		recover->adap_fd = ctxi->lfd;
 		recover->mmio_size = sizeof(afu->afu_map->hosts[0].harea);
-		recover->hdr.return_flags |=
+		recover->hdr.return_flags = DK_CXLFLASH_APP_CLOSE_ADAP_FD |
 			DK_CXLFLASH_RECOVER_AFU_CONTEXT_RESET;
 		goto out;
 	}
diff --git a/drivers/scsi/cxlflash/vlun.c b/drivers/scsi/cxlflash/vlun.c
index 50f8e93..90c5d7f 100644
--- a/drivers/scsi/cxlflash/vlun.c
+++ b/drivers/scsi/cxlflash/vlun.c
@@ -1135,14 +1135,13 @@ int cxlflash_disk_clone(struct scsi_device *sdev,
 	    ctxid_dst = DECODE_CTXID(clone->context_id_dst),
 	    rctxid_src = clone->context_id_src,
 	    rctxid_dst = clone->context_id_dst;
-	int adap_fd_src = clone->adap_fd_src;
 	int i, j;
 	int rc = 0;
 	bool found;
 	LIST_HEAD(sidecar);
 
-	pr_debug("%s: ctxid_src=%llu ctxid_dst=%llu adap_fd_src=%d\n",
-		 __func__, ctxid_src, ctxid_dst, adap_fd_src);
+	pr_debug("%s: ctxid_src=%llu ctxid_dst=%llu\n",
+		 __func__, ctxid_src, ctxid_dst);
 
 	/* Do not clone yourself */
 	if (unlikely(rctxid_src == rctxid_dst)) {
@@ -1166,13 +1165,6 @@ int cxlflash_disk_clone(struct scsi_device *sdev,
 		goto out;
 	}
 
-	if (unlikely(adap_fd_src != ctxi_src->lfd)) {
-		pr_debug("%s: Invalid source adapter fd! (%d)\n",
-			 __func__, adap_fd_src);
-		rc = -EINVAL;
-		goto out;
-	}
-
 	/* Verify there is no open resource handle in the destination context */
 	for (i = 0; i < MAX_RHT_PER_CONTEXT; i++)
 		if (ctxi_dst->rht_start[i].nmask != 0) {
@@ -1257,7 +1249,6 @@ int cxlflash_disk_clone(struct scsi_device *sdev,
 
 out_success:
 	list_splice(&sidecar, &ctxi_dst->luns);
-	sys_close(adap_fd_src);
 
 	/* fall through */
 out:
diff --git a/include/uapi/scsi/cxlflash_ioctl.h b/include/uapi/scsi/cxlflash_ioctl.h
index 2302f3c..6bf1f8a 100644
--- a/include/uapi/scsi/cxlflash_ioctl.h
+++ b/include/uapi/scsi/cxlflash_ioctl.h
@@ -39,19 +39,28 @@ struct dk_cxlflash_hdr {
  * at this time, this provides future flexibility.
  */
 #define DK_CXLFLASH_ALL_PORTS_ACTIVE	0x0000000000000001ULL
+#define DK_CXLFLASH_APP_CLOSE_ADAP_FD	0x0000000000000002ULL
 
 /*
- * Notes:
- * -----
+ * General Notes:
+ * -------------
  * The 'context_id' field of all ioctl structures contains the context
  * identifier for a context in the lower 32-bits (upper 32-bits are not
  * to be used when identifying a context to the AFU). That said, the value
  * in its entirety (all 64-bits) is to be treated as an opaque cookie and
  * should be presented as such when issuing ioctls.
+ */
+
+/*
+ * DK_CXLFLASH_ATTACH Notes:
+ * ------------------------
+ * Read/write access permissions are specified via the O_RDONLY, O_WRONLY,
+ * and O_RDWR flags defined in the fcntl.h header file.
  *
- * For DK_CXLFLASH_ATTACH ioctl, user specifies read/write access
- * permissions via the O_RDONLY, O_WRONLY, and O_RDWR flags defined in
- * the fcntl.h header file.
+ * A valid adapter file descriptor (fd >= 0) is only returned on the initial
+ * attach (successful) of a context. When a context is shared(reused), the user
+ * is expected to already 'know' the adapter file descriptor associated with the
+ * context.
  */
 #define DK_CXLFLASH_ATTACH_REUSE_CONTEXT	0x8000000000000000ULL
 
-- 
2.1.0

Re: [PATCH 4/6] cxlflash: Transition to application close model

[Manoj Kumar]

Acked-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>


On 8/9/2016 6:39 PM, Matthew R. Ochs wrote:
> Caching the adapter file descriptor and performing a close on behalf
> of an application is a poor design. This is due to the fact that once
> a file descriptor in installed, it is free to be altered without the
> knowledge of the cxlflash driver. This can lead to inconsistencies
> between the application and kernel. Furthermore, the nature of the
> former design is more exploitable and thus should be abandoned.
>
> To support applications performing a close on the adapter file that
> is associated with a context, a new flag is introduced to the user
> API to indicate to applications that they are responsible for the
> close following the cleanup (detach) of a context. The documentation
> is also updated to reflect this change in behavior.
>
> Inspired-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>

Re: [PATCH 4/6] cxlflash: Transition to application close model

[Martin K. Petersen]

>>>>> "Matthew" == Matthew R Ochs <mrochs@linux.vnet.ibm.com> writes:

Matthew> Caching the adapter file descriptor and performing a close on
Matthew> behalf of an application is a poor design. This is due to the
Matthew> fact that once a file descriptor in installed, it is free to be
Matthew> altered without the knowledge of the cxlflash driver. This can
Matthew> lead to inconsistencies between the application and
Matthew> kernel. Furthermore, the nature of the former design is more
Matthew> exploitable and thus should be abandoned.

Matthew> To support applications performing a close on the adapter file
Matthew> that is associated with a context, a new flag is introduced to
Matthew> the user API to indicate to applications that they are
Matthew> responsible for the close following the cleanup (detach) of a
Matthew> context. The documentation is also updated to reflect this
Matthew> change in behavior.

Applied patches 4 through 6 to 4.9/scsi-queue.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 5/6] cxlflash: Remove adapter file descriptor cache

[Matthew R. Ochs]

The adapter file descriptor was previously cached within the kernel
for a given context in order to support performing a close on behalf
of an application. This is no longer needed as applications are now
required to perform a close on the adapter file descriptor.

Inspired-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/superpipe.c | 26 +++++++++++++-------------
 drivers/scsi/cxlflash/superpipe.h |  1 -
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index b3bb90d..c91fe6f 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -788,20 +788,18 @@ err:
  * @cfg:	Internal structure associated with the host.
  * @ctx:	Previously obtained CXL context reference.
  * @ctxid:	Previously obtained process element associated with CXL context.
- * @adap_fd:	Previously obtained adapter fd associated with CXL context.
  * @file:	Previously obtained file associated with CXL context.
  * @perms:	User-specified permissions.
  */
 static void init_context(struct ctx_info *ctxi, struct cxlflash_cfg *cfg,
-			 struct cxl_context *ctx, int ctxid, int adap_fd,
-			 struct file *file, u32 perms)
+			 struct cxl_context *ctx, int ctxid, struct file *file,
+			 u32 perms)
 {
 	struct afu *afu = cfg->afu;
 
 	ctxi->rht_perms = perms;
 	ctxi->ctrl_map = &afu->afu_map->ctrls[ctxid].ctrl;
 	ctxi->ctxid = ENCODE_CTXID(ctxi, ctxid);
-	ctxi->lfd = adap_fd;
 	ctxi->pid = current->tgid; /* tgid = pid */
 	ctxi->ctx = ctx;
 	ctxi->cfg = cfg;
@@ -1086,8 +1084,7 @@ static int cxlflash_mmap_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 		goto err;
 	}
 
-	dev_dbg(dev, "%s: fault(%d) for context %d\n",
-		__func__, ctxi->lfd, ctxid);
+	dev_dbg(dev, "%s: fault for context %d\n", __func__, ctxid);
 
 	if (likely(!ctxi->err_recovery_active)) {
 		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
@@ -1162,8 +1159,7 @@ static int cxlflash_cxl_mmap(struct file *file, struct vm_area_struct *vma)
 		goto out;
 	}
 
-	dev_dbg(dev, "%s: mmap(%d) for context %d\n",
-		__func__, ctxi->lfd, ctxid);
+	dev_dbg(dev, "%s: mmap for context %d\n", __func__, ctxid);
 
 	rc = cxl_fd_mmap(file, vma);
 	if (likely(!rc)) {
@@ -1406,7 +1402,7 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 	perms = SISL_RHT_PERM(attach->hdr.flags + 1);
 
 	/* Context mutex is locked upon return */
-	init_context(ctxi, cfg, ctx, ctxid, fd, file, perms);
+	init_context(ctxi, cfg, ctx, ctxid, file, perms);
 
 	rc = afu_attach(cfg, ctxi);
 	if (unlikely(rc)) {
@@ -1488,12 +1484,15 @@ err:
  * recover_context() - recovers a context in error
  * @cfg:	Internal structure associated with the host.
  * @ctxi:	Context to release.
+ * @adap_fd:	Adapter file descriptor associated with new/recovered context.
  *
  * Restablishes the state for a context-in-error.
  *
  * Return: 0 on success, -errno on failure
  */
-static int recover_context(struct cxlflash_cfg *cfg, struct ctx_info *ctxi)
+static int recover_context(struct cxlflash_cfg *cfg,
+			   struct ctx_info *ctxi,
+			   int *adap_fd)
 {
 	struct device *dev = &cfg->dev->dev;
 	int rc = 0;
@@ -1546,7 +1545,6 @@ static int recover_context(struct cxlflash_cfg *cfg, struct ctx_info *ctxi)
 	 * visible to user space and can't be undone safely on this thread.
 	 */
 	ctxi->ctxid = ENCODE_CTXID(ctxi, ctxid);
-	ctxi->lfd = fd;
 	ctxi->ctx = ctx;
 	ctxi->file = file;
 
@@ -1563,6 +1561,7 @@ static int recover_context(struct cxlflash_cfg *cfg, struct ctx_info *ctxi)
 	cfg->ctx_tbl[ctxid] = ctxi;
 	mutex_unlock(&cfg->ctx_tbl_list_mutex);
 	fd_install(fd, file);
+	*adap_fd = fd;
 out:
 	dev_dbg(dev, "%s: returning ctxid=%d fd=%d rc=%d\n",
 		__func__, ctxid, fd, rc);
@@ -1621,6 +1620,7 @@ static int cxlflash_afu_recover(struct scsi_device *sdev,
 	    rctxid = recover->context_id;
 	long reg;
 	int lretry = 20; /* up to 2 seconds */
+	int new_adap_fd = -1;
 	int rc = 0;
 
 	atomic_inc(&cfg->recovery_threads);
@@ -1650,7 +1650,7 @@ retry:
 
 	if (ctxi->err_recovery_active) {
 retry_recover:
-		rc = recover_context(cfg, ctxi);
+		rc = recover_context(cfg, ctxi, &new_adap_fd);
 		if (unlikely(rc)) {
 			dev_err(dev, "%s: Recovery failed for context %llu (rc=%d)\n",
 				__func__, ctxid, rc);
@@ -1672,7 +1672,7 @@ retry_recover:
 
 		ctxi->err_recovery_active = false;
 		recover->context_id = ctxi->ctxid;
-		recover->adap_fd = ctxi->lfd;
+		recover->adap_fd = new_adap_fd;
 		recover->mmio_size = sizeof(afu->afu_map->hosts[0].harea);
 		recover->hdr.return_flags = DK_CXLFLASH_APP_CLOSE_ADAP_FD |
 			DK_CXLFLASH_RECOVER_AFU_CONTEXT_RESET;
diff --git a/drivers/scsi/cxlflash/superpipe.h b/drivers/scsi/cxlflash/superpipe.h
index 5bda8b5..9e62ff3 100644
--- a/drivers/scsi/cxlflash/superpipe.h
+++ b/drivers/scsi/cxlflash/superpipe.h
@@ -100,7 +100,6 @@ struct ctx_info {
 
 	struct cxl_ioctl_start_work work;
 	u64 ctxid;
-	int lfd;
 	pid_t pid;
 	bool initialized;
 	bool unavail;
-- 
2.1.0

Re: [PATCH 5/6] cxlflash: Remove adapter file descriptor cache

[Manoj Kumar]

Acked-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>


On 8/9/2016 6:40 PM, Matthew R. Ochs wrote:
> The adapter file descriptor was previously cached within the kernel
> for a given context in order to support performing a close on behalf
> of an application. This is no longer needed as applications are now
> required to perform a close on the adapter file descriptor.
>
> Inspired-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>

[PATCH 6/6] cxlflash: Update documentation

[Matthew R. Ochs]

Update the block library link in the API documentation.

Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
 Documentation/powerpc/cxlflash.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/powerpc/cxlflash.txt b/Documentation/powerpc/cxlflash.txt
index f4c1190..6d9a2ed 100644
--- a/Documentation/powerpc/cxlflash.txt
+++ b/Documentation/powerpc/cxlflash.txt
@@ -121,7 +121,7 @@ Block library API
     below.
 
     The block library can be found on GitHub:
-    http://www.github.com/mikehollinger/ibmcapikv
+    http://github.com/open-power/capiflash
 
 
 CXL Flash Driver IOCTLs
-- 
2.1.0

Re: [PATCH 6/6] cxlflash: Update documentation

[Manoj Kumar]

Acked-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>


On 8/9/2016 6:40 PM, Matthew R. Ochs wrote:
> Update the block library link in the API documentation.
>
> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
> ---
>  Documentation/powerpc/cxlflash.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/powerpc/cxlflash.txt b/Documentation/powerpc/cxlflash.txt
> index f4c1190..6d9a2ed 100644
> --- a/Documentation/powerpc/cxlflash.txt
> +++ b/Documentation/powerpc/cxlflash.txt
> @@ -121,7 +121,7 @@ Block library API
>      below.
>
>      The block library can be found on GitHub:
> -    http://www.github.com/mikehollinger/ibmcapikv
> +    http://github.com/open-power/capiflash
>
>
>  CXL Flash Driver IOCTLs
>

Re: [PATCH 0/6] cxlflash: Improvements and cleanup

[Martin K. Petersen]

>>>>> "Matthew" == Matthew R Ochs <mrochs@linux.vnet.ibm.com> writes:

Matthew> This patch set contains various code improvements and cleanups
Matthew> that were inspired by Al Viro upon reviewing the cxlflash
Matthew> driver. The core improvement is that the driver will no longer
Matthew> cache the adapter file descriptor associated with a
Matthew> context. This results in a user API change that is documented
Matthew> alongside the modifications.

Applied patches 1-3 to 4.9/scsi-queue. The remainder await reviews.

Matthew> The series is based upon 4.8-rc1, intended for 4.9, and is
Matthew> bisectable.

Thanks for making that explicit. Makes my life easier!

-- 
Martin K. Petersen	Oracle Linux Engineering