* [PATCH 2/7] cxlflash: Unmap problem state area before detaching master context
2016-03-04 21:55 ` [PATCH 1/7] cxlflash: Simplify PCI registration Uma Krishnan
@ 2016-03-04 21:55 ` Uma Krishnan
2016-03-07 18:33 ` Matthew R. Ochs
2016-03-04 21:55 ` [PATCH 3/7] cxlflash: Split out context initialization Uma Krishnan
` (6 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Uma Krishnan @ 2016-03-04 21:55 UTC (permalink / raw)
To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
Manoj N. Kumar
Cc: linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
When operating in the PowerVM environment, the cxlflash module can
receive an error from the hypervisor indicating that there are
existing mappings in the page table for the process MMIO space.
This issue exists because term_afu() currently invokes term_mc()
before stop_afu(), allowing for the master context to be detached
first and the problem state area to be unmapped second.
To resolve this issue, stop_afu() should be called before term_mc().
Signed-off-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
---
drivers/scsi/cxlflash/main.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index 3dbb9fa..ca702d8 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -726,11 +726,11 @@ static void term_mc(struct cxlflash_cfg *cfg, enum undo_level level)
*/
static void term_afu(struct cxlflash_cfg *cfg)
{
- term_mc(cfg, UNDO_START);
-
if (cfg->afu)
stop_afu(cfg);
+ term_mc(cfg, UNDO_START);
+
pr_debug("%s: returning\n", __func__);
}
@@ -2492,8 +2492,8 @@ static pci_ers_result_t cxlflash_pci_error_detected(struct pci_dev *pdev,
if (unlikely(rc))
dev_err(dev, "%s: Failed to mark user contexts!(%d)\n",
__func__, rc);
- term_mc(cfg, UNDO_START);
stop_afu(cfg);
+ term_mc(cfg, UNDO_START);
return PCI_ERS_RESULT_NEED_RESET;
case pci_channel_io_perm_failure:
cfg->state = STATE_FAILTERM;
--
2.1.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/7] cxlflash: Unmap problem state area before detaching master context
2016-03-04 21:55 ` [PATCH 2/7] cxlflash: Unmap problem state area before detaching master context Uma Krishnan
@ 2016-03-07 18:33 ` Matthew R. Ochs
0 siblings, 0 replies; 19+ messages in thread
From: Matthew R. Ochs @ 2016-03-07 18:33 UTC (permalink / raw)
To: Uma Krishnan
Cc: linux-scsi, James Bottomley, Martin K. Petersen, Manoj N. Kumar,
linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
> On Mar 4, 2016, at 3:55 PM, Uma Krishnan <ukrishn@linux.vnet.ibm.com> =
wrote:
>=20
> When operating in the PowerVM environment, the cxlflash module can
> receive an error from the hypervisor indicating that there are
> existing mappings in the page table for the process MMIO space.
>=20
> This issue exists because term_afu() currently invokes term_mc()
> before stop_afu(), allowing for the master context to be detached
> first and the problem state area to be unmapped second.
>=20
> To resolve this issue, stop_afu() should be called before term_mc().
>=20
> Signed-off-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
Acked-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/7] cxlflash: Split out context initialization
2016-03-04 21:55 ` [PATCH 1/7] cxlflash: Simplify PCI registration Uma Krishnan
2016-03-04 21:55 ` [PATCH 2/7] cxlflash: Unmap problem state area before detaching master context Uma Krishnan
@ 2016-03-04 21:55 ` Uma Krishnan
2016-03-08 17:55 ` Uma Krishnan
2016-03-04 21:55 ` [PATCH 4/7] cxlflash: Simplify attach path error cleanup Uma Krishnan
` (5 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Uma Krishnan @ 2016-03-04 21:55 UTC (permalink / raw)
To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
Manoj N. Kumar
Cc: linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
From: "Matthew R. Ochs" <mrochs@linux.vnet.ibm.com>
Presently, context information structures are allocated and
initialized in the same routine, create_context(). This imposes
an ordering restriction such that all pieces of information needed
to initialize a context must be known before the context is even
allocated.
This design point is not flexible when the order of context
creation needs to be modified. Specifically, this can lead to
problems when members of the context information structure are
a part of an ordering dependency (i.e. - the 'work' structure
embedded within the context).
To remedy, the allocation is left as-is, inside of the existing
create_context() routine and the initialization is transitioned
to a new void routine, init_context(). At the same time, in
anticipation of these routines not being called in sequence, a
state boolean is added to the context information structure to
track when the context has been initilized. The context teardown
routine, destroy_context(), is modified to support being called
with a non-initialized context.
Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
drivers/scsi/cxlflash/superpipe.c | 92 +++++++++++++++++++++++----------------
drivers/scsi/cxlflash/superpipe.h | 1 +
2 files changed, 56 insertions(+), 37 deletions(-)
diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index f4020db..b30b362 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -709,27 +709,32 @@ int cxlflash_disk_release(struct scsi_device *sdev,
* @cfg: Internal structure associated with the host.
* @ctxi: Context to release.
*
- * Note that the rht_lun member of the context was cut from a single
- * allocation when the context was created and therefore does not need
- * to be explicitly freed. Also note that we conditionally check 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 we don't have 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
+ * 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).
*/
static void destroy_context(struct cxlflash_cfg *cfg,
struct ctx_info *ctxi)
{
struct afu *afu = cfg->afu;
- WARN_ON(!list_empty(&ctxi->luns));
+ if (ctxi->initialized) {
+ WARN_ON(!list_empty(&ctxi->luns));
- /* Clear RHT registers and drop all capabilities for this context */
- if (afu->afu_map && ctxi->ctrl_map) {
- writeq_be(0, &ctxi->ctrl_map->rht_start);
- writeq_be(0, &ctxi->ctrl_map->rht_cnt_id);
- writeq_be(0, &ctxi->ctrl_map->ctx_cap);
+ /* Clear RHT registers and drop all capabilities for context */
+ if (afu->afu_map && ctxi->ctrl_map) {
+ writeq_be(0, &ctxi->ctrl_map->rht_start);
+ 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 */
@@ -742,23 +747,12 @@ static void destroy_context(struct cxlflash_cfg *cfg,
/**
* create_context() - allocates and initializes a context
* @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.
- *
- * The context's mutex is locked when an allocated context is returned.
*
* Return: Allocated context on success, NULL on failure
*/
-static struct ctx_info *create_context(struct cxlflash_cfg *cfg,
- struct cxl_context *ctx, int ctxid,
- int adap_fd, struct file *file,
- u32 perms)
+static struct ctx_info *create_context(struct cxlflash_cfg *cfg)
{
struct device *dev = &cfg->dev->dev;
- struct afu *afu = cfg->afu;
struct ctx_info *ctxi = NULL;
struct llun_info **lli = NULL;
u8 *ws = NULL;
@@ -781,28 +775,49 @@ static struct ctx_info *create_context(struct cxlflash_cfg *cfg,
ctxi->rht_lun = lli;
ctxi->rht_needs_ws = ws;
ctxi->rht_start = rhte;
- ctxi->rht_perms = perms;
+out:
+ return ctxi;
+
+err:
+ kfree(ws);
+ kfree(lli);
+ kfree(ctxi);
+ ctxi = NULL;
+ goto out;
+}
+
+/**
+ * init_context() - initializes a previously allocated context
+ * @ctxi: Previously allocated context
+ * @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.
+ *
+ * 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,
+ 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->file = file;
+ ctxi->initialized = true;
mutex_init(&ctxi->mutex);
INIT_LIST_HEAD(&ctxi->luns);
INIT_LIST_HEAD(&ctxi->list); /* initialize for list_empty() */
mutex_lock(&ctxi->mutex);
-out:
- return ctxi;
-
-err:
- kfree(ws);
- kfree(lli);
- kfree(ctxi);
- ctxi = NULL;
- goto out;
}
/**
@@ -1396,13 +1411,16 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
/* Translate read/write O_* flags from fcntl.h to AFU permission bits */
perms = SISL_RHT_PERM(attach->hdr.flags + 1);
- ctxi = create_context(cfg, ctx, ctxid, fd, file, perms);
+ ctxi = create_context(cfg);
if (unlikely(!ctxi)) {
dev_err(dev, "%s: Failed to create context! (%d)\n",
__func__, ctxid);
goto err3;
}
+ /* Context mutex is locked upon return */
+ init_context(ctxi, cfg, ctx, ctxid, fd, file, perms);
+
work = &ctxi->work;
work->num_interrupts = attach->num_interrupts;
work->flags = CXL_START_WORK_NUM_IRQS;
diff --git a/drivers/scsi/cxlflash/superpipe.h b/drivers/scsi/cxlflash/superpipe.h
index bede574..5f9a091 100644
--- a/drivers/scsi/cxlflash/superpipe.h
+++ b/drivers/scsi/cxlflash/superpipe.h
@@ -102,6 +102,7 @@ struct ctx_info {
u64 ctxid;
int lfd;
pid_t pid;
+ bool initialized;
bool unavail;
bool err_recovery_active;
struct mutex mutex; /* Context protection */
--
2.1.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/7] cxlflash: Split out context initialization
2016-03-04 21:55 ` [PATCH 3/7] cxlflash: Split out context initialization Uma Krishnan
@ 2016-03-08 17:55 ` Uma Krishnan
0 siblings, 0 replies; 19+ messages in thread
From: Uma Krishnan @ 2016-03-08 17:55 UTC (permalink / raw)
To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
Manoj N. Kumar
Cc: Christophe Lombard, Frederic Barrat, Ian Munsie, Andrew Donnellan,
Brian King, linuxppc-dev
On 3/4/2016 3:55 PM, Uma Krishnan wrote:
> From: "Matthew R. Ochs" <mrochs@linux.vnet.ibm.com>
>
> Presently, context information structures are allocated and
> initialized in the same routine, create_context(). This imposes
> an ordering restriction such that all pieces of information needed
> to initialize a context must be known before the context is even
> allocated.
>
> This design point is not flexible when the order of context
> creation needs to be modified. Specifically, this can lead to
> problems when members of the context information structure are
> a part of an ordering dependency (i.e. - the 'work' structure
> embedded within the context).
>
> To remedy, the allocation is left as-is, inside of the existing
> create_context() routine and the initialization is transitioned
> to a new void routine, init_context(). At the same time, in
> anticipation of these routines not being called in sequence, a
> state boolean is added to the context information structure to
> track when the context has been initilized. The context teardown
> routine, destroy_context(), is modified to support being called
> with a non-initialized context.
>
> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
Reviewed-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/7] cxlflash: Simplify attach path error cleanup
2016-03-04 21:55 ` [PATCH 1/7] cxlflash: Simplify PCI registration Uma Krishnan
2016-03-04 21:55 ` [PATCH 2/7] cxlflash: Unmap problem state area before detaching master context Uma Krishnan
2016-03-04 21:55 ` [PATCH 3/7] cxlflash: Split out context initialization Uma Krishnan
@ 2016-03-04 21:55 ` Uma Krishnan
2016-03-08 17:55 ` Uma Krishnan
2016-03-04 21:55 ` [PATCH 5/7] cxlflash: Reorder user context initialization Uma Krishnan
` (4 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Uma Krishnan @ 2016-03-04 21:55 UTC (permalink / raw)
To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
Manoj N. Kumar
Cc: linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
From: "Matthew R. Ochs" <mrochs@linux.vnet.ibm.com>
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>
---
drivers/scsi/cxlflash/superpipe.c | 55 ++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 24 deletions(-)
diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index b30b362..7ec0b7a 100644
--- 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;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/7] cxlflash: Simplify attach path error cleanup
2016-03-04 21:55 ` [PATCH 4/7] cxlflash: Simplify attach path error cleanup Uma Krishnan
@ 2016-03-08 17:55 ` Uma Krishnan
0 siblings, 0 replies; 19+ messages in thread
From: Uma Krishnan @ 2016-03-08 17:55 UTC (permalink / raw)
To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
Manoj N. Kumar
Cc: linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
On 3/4/2016 3:55 PM, Uma Krishnan wrote:
> From: "Matthew R. Ochs" <mrochs@linux.vnet.ibm.com>
>
> 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>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/7] cxlflash: Reorder user context initialization
2016-03-04 21:55 ` [PATCH 1/7] cxlflash: Simplify PCI registration Uma Krishnan
` (2 preceding siblings ...)
2016-03-04 21:55 ` [PATCH 4/7] cxlflash: Simplify attach path error cleanup Uma Krishnan
@ 2016-03-04 21:55 ` Uma Krishnan
2016-03-07 18:37 ` Matthew R. Ochs
2016-03-04 21:55 ` [PATCH 6/7] cxlflash: Fix to avoid unnecessary scan with internal LUNs Uma Krishnan
` (3 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Uma Krishnan @ 2016-03-04 21:55 UTC (permalink / raw)
To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
Manoj N. Kumar
Cc: linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
In order to support cxlflash in the PowerVM environment, underlying
hypervisor APIs have imposed a kernel API ordering change.
For the superpipe access to LUN, user applications need a context.
The cxlflash module creates this context by making a sequence of
cxl calls. In the current code, a context is initialized via
cxl_dev_context_init() followed by cxl_process_element(), a function
that obtains the process element id. Finally, cxl_start_work()
is called to attach the process element.
In the PowerVM environment, a process element id cannot be obtained
from the hypervisor until the process element is attached. The
cxlflash module is unable to create contexts without a valid
process element id.
To fix this problem, cxl_start_work() is called before obtaining
the process element id.
Signed-off-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
---
drivers/scsi/cxlflash/superpipe.c | 56 +++++++++++++++++++--------------------
1 file changed, 28 insertions(+), 28 deletions(-)
diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index 7ec0b7a..d8a5cb3 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -1386,6 +1386,13 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
goto out_attach;
}
+ ctxi = create_context(cfg);
+ if (unlikely(!ctxi)) {
+ dev_err(dev, "%s: Failed to create context! (%d)\n",
+ __func__, ctxid);
+ goto err;
+ }
+
ctx = cxl_dev_context_init(cfg->dev);
if (IS_ERR_OR_NULL(ctx)) {
dev_err(dev, "%s: Could not initialize context %p\n",
@@ -1394,6 +1401,17 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
goto err;
}
+ work = &ctxi->work;
+ work->num_interrupts = attach->num_interrupts;
+ work->flags = CXL_START_WORK_NUM_IRQS;
+
+ rc = cxl_start_work(ctx, work);
+ if (unlikely(rc)) {
+ dev_dbg(dev, "%s: Could not start context rc=%d\n",
+ __func__, rc);
+ goto err;
+ }
+
ctxid = cxl_process_element(ctx);
if (unlikely((ctxid >= MAX_CONTEXT) || (ctxid < 0))) {
dev_err(dev, "%s: ctxid (%d) invalid!\n", __func__, ctxid);
@@ -1411,27 +1429,9 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
/* Translate read/write O_* flags from fcntl.h to AFU permission bits */
perms = SISL_RHT_PERM(attach->hdr.flags + 1);
- ctxi = create_context(cfg);
- if (unlikely(!ctxi)) {
- dev_err(dev, "%s: Failed to create context! (%d)\n",
- __func__, ctxid);
- goto err;
- }
-
/* Context mutex is locked upon return */
init_context(ctxi, cfg, ctx, ctxid, fd, file, perms);
- work = &ctxi->work;
- work->num_interrupts = attach->num_interrupts;
- work->flags = CXL_START_WORK_NUM_IRQS;
-
- rc = cxl_start_work(ctx, work);
- if (unlikely(rc)) {
- dev_dbg(dev, "%s: Could not start context rc=%d\n",
- __func__, rc);
- goto err;
- }
-
rc = afu_attach(cfg, ctxi);
if (unlikely(rc)) {
dev_err(dev, "%s: Could not attach AFU rc %d\n", __func__, rc);
@@ -1532,24 +1532,24 @@ static int recover_context(struct cxlflash_cfg *cfg, struct ctx_info *ctxi)
goto out;
}
+ rc = cxl_start_work(ctx, &ctxi->work);
+ if (unlikely(rc)) {
+ dev_dbg(dev, "%s: Could not start context rc=%d\n",
+ __func__, rc);
+ goto err1;
+ }
+
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 err1;
+ goto err2;
}
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 err1;
- }
-
- rc = cxl_start_work(ctx, &ctxi->work);
- if (unlikely(rc)) {
- dev_dbg(dev, "%s: Could not start context rc=%d\n",
- __func__, rc);
goto err2;
}
@@ -1594,10 +1594,10 @@ out:
return rc;
err3:
- cxl_stop_context(ctx);
-err2:
fput(file);
put_unused_fd(fd);
+err2:
+ cxl_stop_context(ctx);
err1:
cxl_release_context(ctx);
goto out;
--
2.1.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 5/7] cxlflash: Reorder user context initialization
2016-03-04 21:55 ` [PATCH 5/7] cxlflash: Reorder user context initialization Uma Krishnan
@ 2016-03-07 18:37 ` Matthew R. Ochs
0 siblings, 0 replies; 19+ messages in thread
From: Matthew R. Ochs @ 2016-03-07 18:37 UTC (permalink / raw)
To: Uma Krishnan
Cc: linux-scsi, James Bottomley, Martin K. Petersen, Manoj N. Kumar,
Christophe Lombard, Frederic Barrat, Ian Munsie, Andrew Donnellan,
Brian King, linuxppc-dev
> On Mar 4, 2016, at 3:55 PM, Uma Krishnan <ukrishn@linux.vnet.ibm.com> =
wrote:
>=20
> In order to support cxlflash in the PowerVM environment, underlying
> hypervisor APIs have imposed a kernel API ordering change.
>=20
> For the superpipe access to LUN, user applications need a context.
> The cxlflash module creates this context by making a sequence of
> cxl calls. In the current code, a context is initialized via
> cxl_dev_context_init() followed by cxl_process_element(), a function
> that obtains the process element id. Finally, cxl_start_work()
> is called to attach the process element.
>=20
> In the PowerVM environment, a process element id cannot be obtained
> from the hypervisor until the process element is attached. The
> cxlflash module is unable to create contexts without a valid
> process element id.
>=20
> To fix this problem, cxl_start_work() is called before obtaining
> the process element id.
>=20
> Signed-off-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
Acked-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 6/7] cxlflash: Fix to avoid unnecessary scan with internal LUNs
2016-03-04 21:55 ` [PATCH 1/7] cxlflash: Simplify PCI registration Uma Krishnan
` (3 preceding siblings ...)
2016-03-04 21:55 ` [PATCH 5/7] cxlflash: Reorder user context initialization Uma Krishnan
@ 2016-03-04 21:55 ` Uma Krishnan
2016-03-07 18:45 ` Matthew R. Ochs
2016-03-08 17:56 ` Uma Krishnan
2016-03-04 21:55 ` [PATCH 7/7] cxlflash: Increase cmd_per_lun for better throughput Uma Krishnan
` (2 subsequent siblings)
7 siblings, 2 replies; 19+ messages in thread
From: Uma Krishnan @ 2016-03-04 21:55 UTC (permalink / raw)
To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
Manoj N. Kumar
Cc: linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
From: "Manoj N. Kumar" <manoj@linux.vnet.ibm.com>
When switching to the internal LUN defined on the
IBM CXL flash adapter, there is an unnecessary
scan occurring on the second port. This scan leads
to the following extra lines in the log:
Dec 17 10:09:00 tul83p1 kernel: [ 3708.561134] cxlflash 0008:00:00.0: cxlflash_queuecommand: (scp=c0000000fc1f0f00) 11/1/0/0 cdb=(A0000000-00000000-10000000-00000000)
Dec 17 10:09:00 tul83p1 kernel: [ 3708.561147] process_cmd_err: cmd failed afu_rc=32 scsi_rc=0 fc_rc=0 afu_extra=0xE, scsi_extra=0x0, fc_extra=0x0
By definition, both of the internal LUNs are on the first port/channel.
When the lun_mode is switched to internal LUN the
same value for host->max_channel is retained. This
causes an unnecessary scan over the second port/channel.
This fix alters the host->max_channel to 0 (1 port), if internal
LUNs are configured and switches it back to 1 (2 ports) while
going back to external LUNs.
Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
---
drivers/scsi/cxlflash/main.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index ca702d8..0f062a4 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -2097,6 +2097,16 @@ static ssize_t lun_mode_store(struct device *dev,
rc = kstrtouint(buf, 10, &lun_mode);
if (!rc && (lun_mode < 5) && (lun_mode != afu->internal_lun)) {
afu->internal_lun = lun_mode;
+
+ /*
+ * When configured for internal LUN, there is only one channel,
+ * channel number 0, else there will be 2 (default).
+ */
+ if (afu->internal_lun)
+ shost->max_channel = 0;
+ else
+ shost->max_channel = NUM_FC_PORTS - 1;
+
afu_reset(cfg);
scsi_scan_host(cfg->host);
}
--
2.1.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 6/7] cxlflash: Fix to avoid unnecessary scan with internal LUNs
2016-03-04 21:55 ` [PATCH 6/7] cxlflash: Fix to avoid unnecessary scan with internal LUNs Uma Krishnan
@ 2016-03-07 18:45 ` Matthew R. Ochs
2016-03-08 17:56 ` Uma Krishnan
1 sibling, 0 replies; 19+ messages in thread
From: Matthew R. Ochs @ 2016-03-07 18:45 UTC (permalink / raw)
To: Uma Krishnan
Cc: linux-scsi, James Bottomley, Martin K. Petersen, Manoj N. Kumar,
linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
> On Mar 4, 2016, at 3:55 PM, Uma Krishnan <ukrishn@linux.vnet.ibm.com> =
wrote:
>=20
> From: "Manoj N. Kumar" <manoj@linux.vnet.ibm.com>
>=20
> When switching to the internal LUN defined on the
> IBM CXL flash adapter, there is an unnecessary
> scan occurring on the second port. This scan leads
> to the following extra lines in the log:
>=20
> Dec 17 10:09:00 tul83p1 kernel: [ 3708.561134] cxlflash 0008:00:00.0: =
cxlflash_queuecommand: (scp=3Dc0000000fc1f0f00) 11/1/0/0 =
cdb=3D(A0000000-00000000-10000000-00000000)
> Dec 17 10:09:00 tul83p1 kernel: [ 3708.561147] process_cmd_err: cmd =
failed afu_rc=3D32 scsi_rc=3D0 fc_rc=3D0 afu_extra=3D0xE, =
scsi_extra=3D0x0, fc_extra=3D0x0
>=20
> By definition, both of the internal LUNs are on the first =
port/channel.
>=20
> When the lun_mode is switched to internal LUN the
> same value for host->max_channel is retained. This
> causes an unnecessary scan over the second port/channel.
>=20
> This fix alters the host->max_channel to 0 (1 port), if internal
> LUNs are configured and switches it back to 1 (2 ports) while
> going back to external LUNs.
>=20
> Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
Acked-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/7] cxlflash: Fix to avoid unnecessary scan with internal LUNs
2016-03-04 21:55 ` [PATCH 6/7] cxlflash: Fix to avoid unnecessary scan with internal LUNs Uma Krishnan
2016-03-07 18:45 ` Matthew R. Ochs
@ 2016-03-08 17:56 ` Uma Krishnan
1 sibling, 0 replies; 19+ messages in thread
From: Uma Krishnan @ 2016-03-08 17:56 UTC (permalink / raw)
To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
Manoj N. Kumar
Cc: linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
On 3/4/2016 3:55 PM, Uma Krishnan wrote:
> From: "Manoj N. Kumar" <manoj@linux.vnet.ibm.com>
>
> When switching to the internal LUN defined on the
> IBM CXL flash adapter, there is an unnecessary
> scan occurring on the second port. This scan leads
> to the following extra lines in the log:
>
> Dec 17 10:09:00 tul83p1 kernel: [ 3708.561134] cxlflash 0008:00:00.0: cxlflash_queuecommand: (scp=c0000000fc1f0f00) 11/1/0/0 cdb=(A0000000-00000000-10000000-00000000)
> Dec 17 10:09:00 tul83p1 kernel: [ 3708.561147] process_cmd_err: cmd failed afu_rc=32 scsi_rc=0 fc_rc=0 afu_extra=0xE, scsi_extra=0x0, fc_extra=0x0
>
> By definition, both of the internal LUNs are on the first port/channel.
>
> When the lun_mode is switched to internal LUN the
> same value for host->max_channel is retained. This
> causes an unnecessary scan over the second port/channel.
>
> This fix alters the host->max_channel to 0 (1 port), if internal
> LUNs are configured and switches it back to 1 (2 ports) while
> going back to external LUNs.
>
> Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
Reviewed-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 7/7] cxlflash: Increase cmd_per_lun for better throughput
2016-03-04 21:55 ` [PATCH 1/7] cxlflash: Simplify PCI registration Uma Krishnan
` (4 preceding siblings ...)
2016-03-04 21:55 ` [PATCH 6/7] cxlflash: Fix to avoid unnecessary scan with internal LUNs Uma Krishnan
@ 2016-03-04 21:55 ` Uma Krishnan
2016-03-07 18:45 ` Matthew R. Ochs
2016-03-08 17:56 ` Uma Krishnan
2016-03-07 18:30 ` [PATCH 1/7] cxlflash: Simplify PCI registration Matthew R. Ochs
2016-03-08 17:54 ` Uma Krishnan
7 siblings, 2 replies; 19+ messages in thread
From: Uma Krishnan @ 2016-03-04 21:55 UTC (permalink / raw)
To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
Manoj N. Kumar
Cc: linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
From: "Manoj N. Kumar" <manoj@linux.vnet.ibm.com>
With the current value of cmd_per_lun at 16, the throughput
over a single adapter is limited to around 150kIOPS.
Increase the value of cmd_per_lun to 256 to improve
throughput. With this change a single adapter is able to
attain close to the maximum throughput (380kIOPS).
Also change the number of RRQ entries that can be queued.
Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
---
drivers/scsi/cxlflash/common.h | 8 +++++---
drivers/scsi/cxlflash/main.c | 2 +-
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/cxlflash/common.h b/drivers/scsi/cxlflash/common.h
index 5ada926..a8ac4c0 100644
--- a/drivers/scsi/cxlflash/common.h
+++ b/drivers/scsi/cxlflash/common.h
@@ -34,7 +34,6 @@ extern const struct file_operations cxlflash_cxl_fops;
sectors
*/
-#define NUM_RRQ_ENTRY 16 /* for master issued cmds */
#define MAX_RHT_PER_CONTEXT (PAGE_SIZE / sizeof(struct sisl_rht_entry))
/* AFU command retry limit */
@@ -48,9 +47,12 @@ extern const struct file_operations cxlflash_cxl_fops;
index derivation
*/
-#define CXLFLASH_MAX_CMDS 16
+#define CXLFLASH_MAX_CMDS 256
#define CXLFLASH_MAX_CMDS_PER_LUN CXLFLASH_MAX_CMDS
+/* RRQ for master issued cmds */
+#define NUM_RRQ_ENTRY CXLFLASH_MAX_CMDS
+
static inline void check_sizes(void)
{
@@ -149,7 +151,7 @@ struct afu_cmd {
struct afu {
/* Stuff requiring alignment go first. */
- u64 rrq_entry[NUM_RRQ_ENTRY]; /* 128B RRQ */
+ u64 rrq_entry[NUM_RRQ_ENTRY]; /* 2K RRQ */
/*
* Command & data for AFU commands.
*/
diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index 0f062a4..3879b46 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -2253,7 +2253,7 @@ static struct scsi_host_template driver_template = {
.eh_device_reset_handler = cxlflash_eh_device_reset_handler,
.eh_host_reset_handler = cxlflash_eh_host_reset_handler,
.change_queue_depth = cxlflash_change_queue_depth,
- .cmd_per_lun = 16,
+ .cmd_per_lun = CXLFLASH_MAX_CMDS_PER_LUN,
.can_queue = CXLFLASH_MAX_CMDS,
.this_id = -1,
.sg_tablesize = SG_NONE, /* No scatter gather support */
--
2.1.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 7/7] cxlflash: Increase cmd_per_lun for better throughput
2016-03-04 21:55 ` [PATCH 7/7] cxlflash: Increase cmd_per_lun for better throughput Uma Krishnan
@ 2016-03-07 18:45 ` Matthew R. Ochs
2016-03-08 17:56 ` Uma Krishnan
1 sibling, 0 replies; 19+ messages in thread
From: Matthew R. Ochs @ 2016-03-07 18:45 UTC (permalink / raw)
To: Uma Krishnan
Cc: linux-scsi, James Bottomley, Martin K. Petersen, Manoj N. Kumar,
linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
> On Mar 4, 2016, at 3:55 PM, Uma Krishnan <ukrishn@linux.vnet.ibm.com> =
wrote:
>=20
> From: "Manoj N. Kumar" <manoj@linux.vnet.ibm.com>
>=20
> With the current value of cmd_per_lun at 16, the throughput
> over a single adapter is limited to around 150kIOPS.
>=20
> Increase the value of cmd_per_lun to 256 to improve
> throughput. With this change a single adapter is able to
> attain close to the maximum throughput (380kIOPS).
> Also change the number of RRQ entries that can be queued.
>=20
> Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
Acked-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 7/7] cxlflash: Increase cmd_per_lun for better throughput
2016-03-04 21:55 ` [PATCH 7/7] cxlflash: Increase cmd_per_lun for better throughput Uma Krishnan
2016-03-07 18:45 ` Matthew R. Ochs
@ 2016-03-08 17:56 ` Uma Krishnan
1 sibling, 0 replies; 19+ messages in thread
From: Uma Krishnan @ 2016-03-08 17:56 UTC (permalink / raw)
To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
Manoj N. Kumar
Cc: linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
On 3/4/2016 3:55 PM, Uma Krishnan wrote:
> From: "Manoj N. Kumar" <manoj@linux.vnet.ibm.com>
>
> With the current value of cmd_per_lun at 16, the throughput
> over a single adapter is limited to around 150kIOPS.
>
> Increase the value of cmd_per_lun to 256 to improve
> throughput. With this change a single adapter is able to
> attain close to the maximum throughput (380kIOPS).
> Also change the number of RRQ entries that can be queued.
>
> Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
Reviewed-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] cxlflash: Simplify PCI registration
2016-03-04 21:55 ` [PATCH 1/7] cxlflash: Simplify PCI registration Uma Krishnan
` (5 preceding siblings ...)
2016-03-04 21:55 ` [PATCH 7/7] cxlflash: Increase cmd_per_lun for better throughput Uma Krishnan
@ 2016-03-07 18:30 ` Matthew R. Ochs
2016-03-08 17:54 ` Uma Krishnan
7 siblings, 0 replies; 19+ messages in thread
From: Matthew R. Ochs @ 2016-03-07 18:30 UTC (permalink / raw)
To: Uma Krishnan
Cc: linux-scsi, James Bottomley, Martin K. Petersen, Manoj N. Kumar,
linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
> On Mar 4, 2016, at 3:55 PM, Uma Krishnan <ukrishn@linux.vnet.ibm.com> =
wrote:
>=20
> From: "Manoj N. Kumar" <manoj@linux.vnet.ibm.com>
>=20
> The calls to pci_request_regions(), pci_resource_start(),
> pci_set_dma_mask(), pci_set_master() and pci_save_state() are all
> unnecessary for the IBM CXL flash adapter since data buffers
> are not required to be mapped to the device's memory.
>=20
> The use of services such as pci_set_dma_mask() are problematic on
> hypervisor managed systems as the IBM CXL flash adapter is operating
> under a virtual PCI Host Bridge (virtual PHB) which does not support
> these services.
>=20
> cxlflash 0001:00:00.0: init_pci: Failed to set PCI DMA mask rc=3D-5
>=20
> The resolution is to simplify init_pci(), to a point where it does the
> bare minimum (pci_enable_device). Similarly, remove the call the
> pci_release_regions() from cxlflash_remove().
>=20
> Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
Acked-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/7] cxlflash: Simplify PCI registration
2016-03-04 21:55 ` [PATCH 1/7] cxlflash: Simplify PCI registration Uma Krishnan
` (6 preceding siblings ...)
2016-03-07 18:30 ` [PATCH 1/7] cxlflash: Simplify PCI registration Matthew R. Ochs
@ 2016-03-08 17:54 ` Uma Krishnan
7 siblings, 0 replies; 19+ messages in thread
From: Uma Krishnan @ 2016-03-08 17:54 UTC (permalink / raw)
To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
Manoj N. Kumar
Cc: linuxppc-dev, Brian King, Ian Munsie, Andrew Donnellan,
Frederic Barrat, Christophe Lombard
On 3/4/2016 3:55 PM, Uma Krishnan wrote:
> From: "Manoj N. Kumar" <manoj@linux.vnet.ibm.com>
>
> The calls to pci_request_regions(), pci_resource_start(),
> pci_set_dma_mask(), pci_set_master() and pci_save_state() are all
> unnecessary for the IBM CXL flash adapter since data buffers
> are not required to be mapped to the device's memory.
>
> The use of services such as pci_set_dma_mask() are problematic on
> hypervisor managed systems as the IBM CXL flash adapter is operating
> under a virtual PCI Host Bridge (virtual PHB) which does not support
> these services.
>
> cxlflash 0001:00:00.0: init_pci: Failed to set PCI DMA mask rc=-5
>
> The resolution is to simplify init_pci(), to a point where it does the
> bare minimum (pci_enable_device). Similarly, remove the call the
> pci_release_regions() from cxlflash_remove().
>
> Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
Reviewed-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread