RE: [PATCH v4] cxlflash: Base support for IBM CXL Flash Adapter
Hi I just wanted to add support for this patchset. There are others considering implementing block IO devices that use the CAPI (CXL) interface and having this patch-set upstream will be very useful in the future. Supported-by: Stephen Bates Cheers Stephen -Original Message- From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-ow...@vger.kernel.org] On Behalf Of Matthew R. Ochs Sent: Friday, June 5, 2015 3:46 PM To: linux-scsi@vger.kernel.org; james.bottom...@hansenpartnership.com; n...@linux-iscsi.org; brk...@linux.vnet.ibm.com; h...@infradead.org Cc: mi...@neuling.org; imun...@au1.ibm.com; Manoj N. Kumar Subject: [PATCH v4] cxlflash: Base support for IBM CXL Flash Adapter SCSI device driver to support filesystem access on the IBM CXL Flash adapter. Signed-off-by: Matthew R. Ochs Signed-off-by: Manoj N. Kumar --- drivers/scsi/Kconfig|1 + drivers/scsi/Makefile |1 + drivers/scsi/cxlflash/Kconfig | 11 + drivers/scsi/cxlflash/Makefile |2 + drivers/scsi/cxlflash/common.h | 180 drivers/scsi/cxlflash/main.c| 2263 +++ drivers/scsi/cxlflash/main.h| 104 ++ drivers/scsi/cxlflash/sislite.h | 465 8 files changed, 3027 insertions(+) create mode 100644 drivers/scsi/cxlflash/Kconfig create mode 100644 drivers/scsi/cxlflash/Makefile create mode 100644 drivers/scsi/cxlflash/common.h create mode 100644 drivers/scsi/cxlflash/main.c create mode 100644 drivers/scsi/cxlflash/main.h create mode 100755 drivers/scsi/cxlflash/sislite.h diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig index b021bcb..ebb12a7 100644 --- a/drivers/scsi/Kconfig +++ b/drivers/scsi/Kconfig @@ -345,6 +345,7 @@ source "drivers/scsi/cxgbi/Kconfig" source "drivers/scsi/bnx2i/Kconfig" source "drivers/scsi/bnx2fc/Kconfig" source "drivers/scsi/be2iscsi/Kconfig" +source "drivers/scsi/cxlflash/Kconfig" config SGIWD93_SCSI tristate "SGI WD93C93 SCSI Driver" diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile index dee160a..619f8fb 100644 --- a/drivers/scsi/Makefile +++ b/drivers/scsi/Makefile @@ -101,6 +101,7 @@ obj-$(CONFIG_SCSI_7000FASST)+= wd7000.o obj-$(CONFIG_SCSI_EATA)+= eata.o obj-$(CONFIG_SCSI_DC395x) += dc395x.o obj-$(CONFIG_SCSI_AM53C974)+= esp_scsi.o am53c974.o +obj-$(CONFIG_CXLFLASH) += cxlflash/ obj-$(CONFIG_MEGARAID_LEGACY) += megaraid.o obj-$(CONFIG_MEGARAID_NEWGEN) += megaraid/ obj-$(CONFIG_MEGARAID_SAS) += megaraid/ diff --git a/drivers/scsi/cxlflash/Kconfig b/drivers/scsi/cxlflash/Kconfig new file mode 100644 index 000..e98c3f6 --- /dev/null +++ b/drivers/scsi/cxlflash/Kconfig @@ -0,0 +1,11 @@ +# +# IBM CXL-attached Flash Accelerator SCSI Driver +# + +config CXLFLASH + tristate "Support for IBM CAPI Flash" + depends on CXL + default m + help + Allows CAPI Accelerated IO to Flash + If unsure, say N. diff --git a/drivers/scsi/cxlflash/Makefile b/drivers/scsi/cxlflash/Makefile new file mode 100644 index 000..dc95e20 --- /dev/null +++ b/drivers/scsi/cxlflash/Makefile @@ -0,0 +1,2 @@ +obj-$(CONFIG_CXLFLASH) += cxlflash.o +cxlflash-y += main.o diff --git a/drivers/scsi/cxlflash/common.h b/drivers/scsi/cxlflash/common.h new file mode 100644 index 000..2bd842b --- /dev/null +++ b/drivers/scsi/cxlflash/common.h @@ -0,0 +1,180 @@ +/* + * CXL Flash Device Driver + * + * Written by: Manoj N. Kumar , IBM Corporation + * Matthew R. Ochs , IBM Corporation + * + * Copyright (C) 2015 IBM Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version + * 2 of the License, or (at your option) any later version. + */ + +#ifndef _CXLFLASH_COMMON_H +#define _CXLFLASH_COMMON_H + +#include +#include +#include +#include + + +#define MAX_CONTEXT CXLFLASH_MAX_CONTEXT /* num contexts per afu */ + +#define CXLFLASH_BLOCK_SIZE4096/* 4K blocks */ +#define CXLFLASH_MAX_XFER_SIZE 16777216/* 16MB transfer */ +#define CXLFLASH_MAX_SECTORS (CXLFLASH_MAX_XFER_SIZE/512)/* SCSI wants + max_sectors + in units of + 512 byte + sectors + */ + +#define NUM_RRQ_ENTRY16 /* for master issued cmds */ +#define MAX_RHT_PER_CONTEXT (PAGE_SIZE / sizeof(struct sisl_rht_entry)) + +/* AFU command retry limit */ +#define MC_RETRY_CNT 5 /* sufficient for SCSI check and + certain AFU errors */ + +/* Command management definitions */ +#define CXLFLASH_NUM_CMDS (2 * CXLFLA
Re: [PATCH v4] cxlflash: Base support for IBM CXL Flash Adapter
On 06/08/2015 04:41 PM, Manoj Kumar wrote: > On 6/8/2015 12:54 PM, Brian King wrote: > >>> + >>> +rcr = send_tmf(afu, scp, TMF_LUN_RESET); >>> +if (unlikely(rcr)) >>> +rc = FAILED; >> >> Do you need to wait for all commands to the LUN to be returned before >> returning from >> here? You could put a simple loop here, polling until there are no ops >> outstanding >> to this LUN, if needed... > > Brian: > > Good suggestion. Would it be acceptable to add this capability in a future > patch? That's fine with me. Thanks, Brian -- Brian King Power Linux I/O IBM Linux Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] cxlflash: Base support for IBM CXL Flash Adapter
On 6/8/2015 12:54 PM, Brian King wrote: + + rcr = send_tmf(afu, scp, TMF_LUN_RESET); + if (unlikely(rcr)) + rc = FAILED; Do you need to wait for all commands to the LUN to be returned before returning from here? You could put a simple loop here, polling until there are no ops outstanding to this LUN, if needed... Brian: Good suggestion. Would it be acceptable to add this capability in a future patch? - Manoj -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] cxlflash: Base support for IBM CXL Flash Adapter
Brian:
Thank you for your review. Comments are inline.
- Manoj
On 6/8/2015 12:54 PM, Brian King wrote:
Looking pretty good. A few more comments.
Thanks,
Brian
+ spin_lock_irqsave(&cfg->tmf_waitq.lock, lock_flags);
+ if (cfg->tmf_active)
+ wait_event_interruptible_locked_irq(cfg->tmf_waitq,
+ !cfg->tmf_active);
+ spin_unlock_irqrestore(&cfg->tmf_waitq.lock, lock_flags);
This needs to return SCSI_MLQUEUE_HOST_BUSY instead of sleeping. You can't sleep
in queuecommand.
Okay, will revise in v5 to return an error instead of sleeping.
+ if (atomic64_dec_and_test(&afu->room)) {
If you have two threads executing this code concurrently you could have a
problem.
If afu->room = 1 and thread 1 decrements it and the return value is 0, we go
into this
leg. If a second thread then comes in right after afu->room goes to zero,
afu->room
will then get decremented to -1, and you'll send the command, regardless of
whether
the AFU has room or not. Either the AFU will have room and afu->room will then
end up being off by one, or it won't have room and you'll send a command when it
does not have room.
I think if you use atomic_dec_if_positive instead, you can get rid of this race
condition.
You'd then need to check the return value. If its positive, there is room, if
it zero,
you are out of room and you are the thread that will reset afu->room from the
AFU. If
it is negative, then you have to either return host busy, or wait for the other
thread to
reset afu->room and simply try the atomic_dec_if_positive again in the loop here
instead of reading from the adapter and trying to set it from two threads.
Good catch. Will switch to atomic_dec_if_positive() in v5 to avoid the race.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] cxlflash: Base support for IBM CXL Flash Adapter
On Jun 8, 2015, at 5:05 AM, Michael Neuling wrote:
>
> Reviewed-by: Michael Neuling
Thanks again for helping us to review the code.
>
> The core cxl changes needed for this are in mpe's powerpc next branch queued
> up
> for 4.2. They are in linux next-20150605. To compile this you'll need to
> pull
> in mpe's powerpc tree from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mpe/linux.git next
I went ahead and did a test build against this tree, everything worked.
>> +config CXLFLASH
>> +tristate "Support for IBM CAPI Flash"
>> +depends on CXL
>
> This should be depends on SCSI too?
Yep, and PCI. Will fix in v5.
>
>> +struct afu {
> < snip>
>> +u64 interface_version;
>
> This doesn't seem to be used anywhere except for in init_afu where it's
> only used local to that function.
True, the intention here is to save it off in case it is needed in a debug
scenario. We may
even make it retrievable via a sysfs attribute at some point.
>> +
>> +static inline u64 lun_to_lunid(u64 lun)
>> +{
>> +u64 lun_id;
>> +
>> +int_to_scsilun(lun, (struct scsi_lun *)&lun_id);
>> +return swab64(lun_id);
>> +}
>
> This is only used in main.c. Why is it not just in there?
We use it in other places within the driver that have not been submitted.
Rather than
migrating it around we decided to keep it defined in the common header.
>
>> +/**
>> + * cxlflash_probe() - PCI entry point to add host
>> + * @pdev: PCI device associated with the host.
>> + * @dev_id: PCI device id associated with device.
>> + *
>> + * Return: 0 on success / non-zero on failure
>> + */
>> +static int cxlflash_probe(struct pci_dev *pdev,
>> + const struct pci_device_id *dev_id)
>> +{
>
>> +phys_dev = cxl_get_phys_dev(pdev);
>> +if (!dev_is_pci(phys_dev)) {
>> +pr_err("%s: not a pci dev\n", __func__);
>> +rc = ENODEV;
>
> -ENODEV;
Good catch! Will fix in v5.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] cxlflash: Base support for IBM CXL Flash Adapter
Looking pretty good. A few more comments.
Thanks,
Brian
On 06/05/2015 04:46 PM, Matthew R. Ochs wrote:
> +/**
> + * cxlflash_queuecommand() - sends a mid-layer request
> + * @host:SCSI host associated with device.
> + * @scp: SCSI command to send.
> + *
> + * Return:
> + * 0 on success
> + * SCSI_MLQUEUE_HOST_BUSY when host is busy
> + */
> +static int cxlflash_queuecommand(struct Scsi_Host *host, struct scsi_cmnd
> *scp)
> +{
> + struct cxlflash_cfg *cfg = (struct cxlflash_cfg *)host->hostdata;
> + struct afu *afu = cfg->afu;
> + struct pci_dev *pdev = cfg->dev;
> + struct afu_cmd *cmd;
> + u32 port_sel = scp->device->channel + 1;
> + int nseg, i, ncount;
> + struct scatterlist *sg;
> + ulong lock_flags;
> + short lflag = 0;
> + int rc = 0;
> +
> + pr_debug("%s: (scp=%p) %d/%d/%d/%llu cdb=(%08X-%08X-%08X-%08X)\n",
> + __func__, scp, host->host_no, scp->device->channel,
> + scp->device->id, scp->device->lun,
> + get_unaligned_be32(&((u32 *)scp->cmnd)[0]),
> + get_unaligned_be32(&((u32 *)scp->cmnd)[1]),
> + get_unaligned_be32(&((u32 *)scp->cmnd)[2]),
> + get_unaligned_be32(&((u32 *)scp->cmnd)[3]));
> +
> + /* If a Task Management Function is active, wait for it to complete
> + * before continuing with regular commands.
> + */
> + spin_lock_irqsave(&cfg->tmf_waitq.lock, lock_flags);
> + if (cfg->tmf_active)
> + wait_event_interruptible_locked_irq(cfg->tmf_waitq,
> + !cfg->tmf_active);
> + spin_unlock_irqrestore(&cfg->tmf_waitq.lock, lock_flags);
This needs to return SCSI_MLQUEUE_HOST_BUSY instead of sleeping. You can't sleep
in queuecommand.
> +
> + cmd = cxlflash_cmd_checkout(afu);
> + if (unlikely(!cmd)) {
> + pr_err("%s: could not get a free command\n", __func__);
> + rc = SCSI_MLQUEUE_HOST_BUSY;
> + goto out;
> + }
> +
> + cmd->rcb.ctx_id = afu->ctx_hndl;
> + cmd->rcb.port_sel = port_sel;
> + cmd->rcb.lun_id = lun_to_lunid(scp->device->lun);
> +
> + if (scp->sc_data_direction == DMA_TO_DEVICE)
> + lflag = SISL_REQ_FLAGS_HOST_WRITE;
> + else
> + lflag = SISL_REQ_FLAGS_HOST_READ;
> +
> + cmd->rcb.req_flags = (SISL_REQ_FLAGS_PORT_LUN_ID |
> + SISL_REQ_FLAGS_SUP_UNDERRUN | lflag);
> +
> + /* Stash the scp in the reserved field, for reuse during interrupt */
> + cmd->rcb.scp = scp;
> +
> + nseg = scsi_dma_map(scp);
> + if (unlikely(nseg < 0)) {
> + dev_err(&pdev->dev, "%s: Fail DMA map! nseg=%d\n",
> + __func__, nseg);
> + rc = SCSI_MLQUEUE_HOST_BUSY;
> + goto out;
> + }
> +
> + ncount = scsi_sg_count(scp);
> + scsi_for_each_sg(scp, sg, ncount, i) {
> + cmd->rcb.data_len = sg_dma_len(sg);
> + cmd->rcb.data_ea = sg_dma_address(sg);
> + }
> +
> + /* Copy the CDB from the scsi_cmnd passed in */
> + memcpy(cmd->rcb.cdb, scp->cmnd, sizeof(cmd->rcb.cdb));
> +
> + /* Send the command */
> + rc = cxlflash_send_cmd(afu, cmd);
> + if (unlikely(rc)) {
> + cxlflash_cmd_checkin(cmd);
> + scsi_dma_unmap(scp);
> + }
> +
> +out:
> + return rc;
> +}
> +
> +/**
> + * cxlflash_eh_device_reset_handler() - reset a single LUN
> + * @scp: SCSI command to send.
> + *
> + * Return:
> + * SUCCESS as defined in scsi/scsi.h
> + * FAILED as defined in scsi/scsi.h
> + */
> +static int cxlflash_eh_device_reset_handler(struct scsi_cmnd *scp)
> +{
> + int rc = SUCCESS;
> + struct Scsi_Host *host = scp->device->host;
> + struct cxlflash_cfg *cfg = (struct cxlflash_cfg *)host->hostdata;
> + struct afu *afu = cfg->afu;
> + int rcr = 0;
> +
> + pr_debug("%s: (scp=%p) %d/%d/%d/%llu "
> + "cdb=(%08X-%08X-%08X-%08X)\n", __func__, scp,
> + host->host_no, scp->device->channel,
> + scp->device->id, scp->device->lun,
> + get_unaligned_be32(&((u32 *)scp->cmnd)[0]),
> + get_unaligned_be32(&((u32 *)scp->cmnd)[1]),
> + get_unaligned_be32(&((u32 *)scp->cmnd)[2]),
> + get_unaligned_be32(&((u32 *)scp->cmnd)[3]));
> +
> + rcr = send_tmf(afu, scp, TMF_LUN_RESET);
> + if (unlikely(rcr))
> + rc = FAILED;
Do you need to wait for all commands to the LUN to be returned before returning
from
here? You could put a simple loop here, polling until there are no ops
outstanding
to this LUN, if needed...
> +
> + pr_debug("%s: returning rc=%d\n", __func__, rc);
> + return rc;
> +}
> +
> +
> +/**
> + * cxlflash_send_cmd() - sends an AFU command
> + * @afu: AFU associated with the host.
> + * @cmd: AFU command to send.
> + *
> + * Return:
> + * 0 on success
> + * -1 on failure
> + */
> +int cxlflash_send_cm
Re: [PATCH v4] cxlflash: Base support for IBM CXL Flash Adapter
On Fri, 2015-06-05 at 16:46 -0500, Matthew R. Ochs wrote:
> SCSI device driver to support filesystem access on the IBM CXL Flash adapter.
Few minor nits below but other than that it looks good to me. The CXL parts
are good and the rest of the driver is looking decent. FWIW:
Reviewed-by: Michael Neuling
The core cxl changes needed for this are in mpe's powerpc next branch queued up
for 4.2. They are in linux next-20150605. To compile this you'll need to pull
in mpe's powerpc tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/mpe/linux.git next
> diff --git a/drivers/scsi/cxlflash/Kconfig b/drivers/scsi/cxlflash/Kconfig
> new file mode 100644
> index 000..e98c3f6
> --- /dev/null
> +++ b/drivers/scsi/cxlflash/Kconfig
> @@ -0,0 +1,11 @@
> +#
> +# IBM CXL-attached Flash Accelerator SCSI Driver
> +#
> +
> +config CXLFLASH
> + tristate "Support for IBM CAPI Flash"
> + depends on CXL
This should be depends on SCSI too?
> +struct afu {
< snip>
> + u64 interface_version;
This doesn't seem to be used anywhere except for in init_afu where it's
only used local to that function.
> + struct cxlflash_cfg *parent; /* Pointer back to parent cxlflash_cfg */
> +
> +};
> +
> +static inline u64 lun_to_lunid(u64 lun)
> +{
> + u64 lun_id;
> +
> + int_to_scsilun(lun, (struct scsi_lun *)&lun_id);
> + return swab64(lun_id);
> +}
This is only used in main.c. Why is it not just in there?
> +/**
> + * cxlflash_probe() - PCI entry point to add host
> + * @pdev:PCI device associated with the host.
> + * @dev_id: PCI device id associated with device.
> + *
> + * Return: 0 on success / non-zero on failure
> + */
> +static int cxlflash_probe(struct pci_dev *pdev,
> + const struct pci_device_id *dev_id)
> +{
> + phys_dev = cxl_get_phys_dev(pdev);
> + if (!dev_is_pci(phys_dev)) {
> + pr_err("%s: not a pci dev\n", __func__);
> + rc = ENODEV;
-ENODEV;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
