Re: [PATCH 0/2] Two refactoring patches for the qla2xxx driver
Bart, > The two patches in this series make the qla2xxx driver source code > easier to read without changing the driver functionality. Please > consider these patches for kernel v4.21. I applied patch #1. #2 had conflicts, please rebase. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2 0/9] qedi bug fixes
Nilesh, > Please consider below patch set for next 'scsi-fixes' submission. Some of these smelled more like features than bug fixes. So I applied the series to 4.21/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 01/23] zfcp: make DIX experimental, disabled, and independent of DIF
Steffen, As I said, I don't have a problem with having module parameters. > There's one more important thing that has performance impact: We need to > pack payload and protection data into the same queue of limited > length. So for the worst case with DIX, we have to use half the size for > sg_tablesize to get the other half for sg_prot_tablesize. Interesting. With the exception of NVMe over PCIe, it's kind of unusual for modern controllers to have real limits in this area. > This limits the maximum I/O request size and thus throughput. Using > read_verify and write_generate does not change the tablesizes, as zfcp > would still announce support for DIF and DIX. With the new zfcp.dif=1 > and zfcp.dix=0, we can use the full sg_tablesize for payload data and > sg_prot_tablesize=0. (The DIF "overhead" on the fibre still exists of > course.) > > Are there other ways for accomplishing this which I'm not aware of? You can set your shost->sg_prot_tablesize. There are pathological corner cases like dd to the raw block device where you'll suffer if there is not a 1:1 mapping between data and protection segments. But for regular I/O where the protection is generated over a whole bio at a time, you can get away with something smaller. Anyway. I don't have any problems with you making DIX experimental for zfcp. Just want to make sure it's done for the right reasons (i.e. not problems in SCSI or the block layer). -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v4 7/7] target: use printf width specifier for t10_wwn field dumps
On 11/28/18 5:01 PM, David Disseldorp wrote:
> From: Bart Van Assche
>
> The existing for loops step over null-terminators for right-padding.
> Padding can be achieved in a much simpler way using printf width
> specifiers.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_device.c | 35 ---
> drivers/target/target_core_stat.c | 32 +++-
> 2 files changed, 15 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/target/target_core_device.c
> b/drivers/target/target_core_device.c
> index b3d0bd1ab09f..7f2d307e411b 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -720,36 +720,17 @@ void core_dev_free_initiator_node_lun_acl(
> static void scsi_dump_inquiry(struct se_device *dev)
> {
> struct t10_wwn *wwn = >t10_wwn;
> - char buf[INQUIRY_MODEL_LEN + 1];
> - int i, device_type;
> + int device_type = dev->transport->get_device_type(dev);
> +
> /*
>* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
>*/
> - for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
> - if (wwn->vendor[i] >= 0x20)
> - buf[i] = wwn->vendor[i];
> - else
> - buf[i] = ' ';
> - buf[i] = '\0';
> - pr_debug(" Vendor: %s\n", buf);
> -
> - for (i = 0; i < INQUIRY_MODEL_LEN; i++)
> - if (wwn->model[i] >= 0x20)
> - buf[i] = wwn->model[i];
> - else
> - buf[i] = ' ';
> - buf[i] = '\0';
> - pr_debug(" Model: %s\n", buf);
> -
> - for (i = 0; i < INQUIRY_REVISION_LEN; i++)
> - if (wwn->revision[i] >= 0x20)
> - buf[i] = wwn->revision[i];
> - else
> - buf[i] = ' ';
> - buf[i] = '\0';
> - pr_debug(" Revision: %s\n", buf);
> -
> - device_type = dev->transport->get_device_type(dev);
> + pr_debug(" Vendor: %-" __stringify(INQUIRY_VENDOR_LEN) "s\n",
> + wwn->vendor);
> + pr_debug(" Model: %-" __stringify(INQUIRY_MODEL_LEN) "s\n",
> + wwn->model);
> + pr_debug(" Revision: %-" __stringify(INQUIRY_REVISION_LEN) "s\n",
> + wwn->revision);
> pr_debug(" Type: %s ", scsi_device_type(device_type));
> }
>
> diff --git a/drivers/target/target_core_stat.c
> b/drivers/target/target_core_stat.c
> index e437ba494865..87fd2b11fe3b 100644
> --- a/drivers/target/target_core_stat.c
> +++ b/drivers/target/target_core_stat.c
> @@ -246,43 +246,25 @@ static ssize_t target_stat_lu_lu_name_show(struct
> config_item *item, char *page)
> static ssize_t target_stat_lu_vend_show(struct config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> - int i;
> - char str[INQUIRY_VENDOR_LEN+1];
>
> - /* scsiLuVendorId */
> - for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
> - str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
> - dev->t10_wwn.vendor[i] : ' ';
> - str[i] = '\0';
> - return snprintf(page, PAGE_SIZE, "%s\n", str);
> + return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_VENDOR_LEN)
> + "s\n", dev->t10_wwn.vendor);
> }
>
> static ssize_t target_stat_lu_prod_show(struct config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> - int i;
> - char str[INQUIRY_MODEL_LEN+1];
>
> - /* scsiLuProductId */
> - for (i = 0; i < INQUIRY_MODEL_LEN; i++)
> - str[i] = ISPRINT(dev->t10_wwn.model[i]) ?
> - dev->t10_wwn.model[i] : ' ';
> - str[i] = '\0';
> - return snprintf(page, PAGE_SIZE, "%s\n", str);
> + return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_MODEL_LEN)
> + "s\n", dev->t10_wwn.model);
> }
>
> static ssize_t target_stat_lu_rev_show(struct config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> - int i;
> - char str[INQUIRY_REVISION_LEN+1];
> -
> - /* scsiLuRevisionId */
> - for (i = 0; i < INQUIRY_REVISION_LEN; i++)
> - str[i] = ISPRINT(dev->t10_wwn.revision[i]) ?
> - dev->t10_wwn.revision[i] : ' ';
> - str[i] = '\0';
> - return snprintf(page, PAGE_SIZE, "%s\n", str);
> +
> + return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_REVISION_LEN)
> + "s\n", dev->t10_wwn.revision);
> }
>
> static ssize_t target_stat_lu_dev_type_show(struct config_item *item, char
> *page)
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
On 11/28/18 5:01 PM, David Disseldorp wrote:
> Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but
> can be reconfigured via the vendor_id ConfigFS attribute.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_spc.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/target/target_core_spc.c
> b/drivers/target/target_core_spc.c
> index 8ffe712cb44d..4503f3336bc2 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -115,7 +115,8 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char
> *buf)
>*/
> memset([8], 0x20,
> INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
> - memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> + memcpy([8], dev->t10_wwn.vendor,
> +strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
> memcpy([16], dev->t10_wwn.model,
> strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
> memcpy([32], dev->t10_wwn.revision,
> @@ -258,8 +259,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char
> *buf)
> buf[off+1] = 0x1; /* T10 Vendor ID */
> buf[off+2] = 0x0;
> /* left align Vendor ID and pad with spaces */
> - memset([off+4], 0x20, 8);
> - memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
> + memset([off+4], 0x20, INQUIRY_VENDOR_LEN);
> + memcpy([off+4], dev->t10_wwn.vendor,
> +strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
> /* Extra Byte for NULL Terminator */
> id_len++;
> /* Identifier Length */
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 5/7] target: add device vendor_id configfs attribute
On 11/28/18 5:01 PM, David Disseldorp wrote:
> The vendor_id attribute will allow for the modification of the T10
> Vendor Identification string returned in inquiry responses. Its value
> can be viewed and modified via the ConfigFS path at:
> target/core/$backstore/$name/wwn/vendor_id
>
> "LIO-ORG" remains the default value, which is set when the backstore
> device is enabled.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_configfs.c | 48
> +++
> 1 file changed, 48 insertions(+)
>
> diff --git a/drivers/target/target_core_configfs.c
> b/drivers/target/target_core_configfs.c
> index 9f49b1afd685..fc60c4db718d 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -1213,6 +1213,52 @@ static struct t10_wwn *to_t10_wwn(struct config_item
> *item)
> }
>
> /*
> + * STANDARD and VPD page 0x80 T10 Vendor Identification
> + */
> +static ssize_t target_wwn_vendor_id_show(struct config_item *item,
> + char *page)
> +{
> + return sprintf(page, "%s\n", _t10_wwn(item)->vendor[0]);
> +}
> +
> +static ssize_t target_wwn_vendor_id_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct t10_wwn *t10_wwn = to_t10_wwn(item);
> + struct se_device *dev = t10_wwn->t10_dev;
> + unsigned char buf[INQUIRY_VENDOR_LEN + 1];
> +
> + if (strlen(page) > INQUIRY_VENDOR_LEN) {
> + pr_err("Emulated T10 Vendor Identification exceeds"
> + " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
> + "\n");
> + return -EOVERFLOW;
> + }
> + strlcpy(buf, page, sizeof(buf));
> + /*
> + * Check to see if any active exports exist. If they do exist, fail
> + * here as changing this information on the fly (underneath the
> + * initiator side OS dependent multipath code) could cause negative
> + * effects.
> + */
> + if (dev->export_count) {
> + pr_err("Unable to set T10 Vendor Identification while"
> + " active %d exports exist\n", dev->export_count);
> + return -EINVAL;
> + }
> +
> + /* Assume ASCII encoding. Strip any newline added from userspace. */
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> + strlcpy(dev->t10_wwn.vendor, strstrip(buf),
> + sizeof(dev->t10_wwn.vendor));
> +
> + pr_debug("Target_Core_ConfigFS: Set emulated T10 Vendor Identification:"
> + " %s\n", dev->t10_wwn.vendor);
> +
> + return count;
> +}
> +
> +/*
> * VPD page 0x80 Unit serial
> */
> static ssize_t target_wwn_vpd_unit_serial_show(struct config_item *item,
> @@ -1358,6 +1404,7 @@ DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_target_port, 0x10);
> /* VPD page 0x83 Association: SCSI Target Device */
> DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_scsi_target_device, 0x20);
>
> +CONFIGFS_ATTR(target_wwn_, vendor_id);
> CONFIGFS_ATTR(target_wwn_, vpd_unit_serial);
> CONFIGFS_ATTR_RO(target_wwn_, vpd_protocol_identifier);
> CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
> @@ -1365,6 +1412,7 @@ CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
> CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
>
> static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
> + _wwn_attr_vendor_id,
> _wwn_attr_vpd_unit_serial,
> _wwn_attr_vpd_protocol_identifier,
> _wwn_attr_vpd_assoc_logical_unit,
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 4/7] target: consistently null-terminate t10_wwn.revision
On 11/28/18 5:01 PM, David Disseldorp wrote:
> The pscsi_set_inquiry_info() codepath doesn't currently explicitly
> null-terminate t10_wwn.revision.
> Add an extra byte to the t10_wwn.model buffer and perform null string
> termination in all cases.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_device.c | 6 --
> drivers/target/target_core_pscsi.c | 4 +++-
> drivers/target/target_core_spc.c| 5 +++--
> drivers/target/target_core_stat.c | 4 ++--
> include/target/target_core_base.h | 3 ++-
> 5 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/target/target_core_device.c
> b/drivers/target/target_core_device.c
> index 0d7382efb2d4..b3d0bd1ab09f 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -741,7 +741,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
> buf[i] = '\0';
> pr_debug(" Model: %s\n", buf);
>
> - for (i = 0; i < 4; i++)
> + for (i = 0; i < INQUIRY_REVISION_LEN; i++)
> if (wwn->revision[i] >= 0x20)
> buf[i] = wwn->revision[i];
> else
> @@ -1010,6 +1010,7 @@ int target_configure_device(struct se_device *dev)
>*/
> BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.revision) != INQUIRY_REVISION_LEN + 1);
> if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
> strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
> dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
> @@ -1017,7 +1018,8 @@ int target_configure_device(struct se_device *dev)
> dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
> dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
> strncpy(>t10_wwn.revision[0],
> - dev->transport->inquiry_rev, 4);
> + dev->transport->inquiry_rev, INQUIRY_REVISION_LEN);
> + dev->t10_wwn.revision[INQUIRY_REVISION_LEN] = '\0';
> }
>
> scsi_dump_inquiry(dev);
> diff --git a/drivers/target/target_core_pscsi.c
> b/drivers/target/target_core_pscsi.c
> index 1633babc2d4e..5493f620b7f4 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -196,7 +196,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct
> t10_wwn *wwn)
> BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
> memcpy(>model[0], [16], INQUIRY_MODEL_LEN);
> wwn->model[INQUIRY_MODEL_LEN] = '\0';
> - memcpy(>revision[0], [32], sizeof(wwn->revision));
> + BUILD_BUG_ON(sizeof(wwn->revision) != INQUIRY_REVISION_LEN + 1);
> + memcpy(>revision[0], [32], INQUIRY_REVISION_LEN);
> + wwn->revision[INQUIRY_REVISION_LEN] = '\0';
> }
>
> static int
> diff --git a/drivers/target/target_core_spc.c
> b/drivers/target/target_core_spc.c
> index 78eddee4b6e6..8ffe712cb44d 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -113,12 +113,13 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned
> char *buf)
>* unused bytes at the end of the field (i.e., highest offset) and the
>* unused bytes shall be filled with ASCII space characters (20h).
>*/
> - memset([8], 0x20, 8 + 16 + 4);
> + memset([8], 0x20,
> +INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
> memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> memcpy([16], dev->t10_wwn.model,
> strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
> memcpy([32], dev->t10_wwn.revision,
> -strnlen(dev->t10_wwn.revision, 4));
> +strnlen(dev->t10_wwn.revision, INQUIRY_REVISION_LEN));
> buf[4] = 31; /* Set additional length to 31 */
>
> return 0;
> diff --git a/drivers/target/target_core_stat.c
> b/drivers/target/target_core_stat.c
> index 9123c5137da5..e437ba494865 100644
> --- a/drivers/target/target_core_stat.c
> +++ b/drivers/target/target_core_stat.c
> @@ -275,10 +275,10 @@ static ssize_t target_stat_lu_rev_show(struct
> config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> int i;
> - char str[sizeof(dev->t10_wwn.revision)+1];
> + char str[INQUIRY_REVISION_LEN+1];
>
> /* scsiLuRevisionId */
> - for (i = 0; i < sizeof(dev->t10_wwn.revision); i++)
> + for (i = 0; i < INQUIRY_REVISION_LEN; i++)
> str[i] = ISPRINT(dev->t10_wwn.revision[i]) ?
> dev->t10_wwn.revision[i] : ' ';
> str[i] = '\0';
> diff --git a/include/target/target_core_base.h
> b/include/target/target_core_base.h
> index cfc279686cf4..497853a09fee 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -48,6 +48,7 @@
>
> #define INQUIRY_VENDOR_LEN
Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
On 11/28/18 5:01 PM, David Disseldorp wrote:
> The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths
> don't currently explicitly null-terminate t10_wwn.model.
> Add an extra byte to the t10_wwn.model buffer and perform null string
> termination in all cases.
>
> dev_set_t10_wwn_model_alias() continues to truncate at the same length
> to avoid changing the model string for existing deployments.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_configfs.c | 8 +---
> drivers/target/target_core_device.c | 8 +---
> drivers/target/target_core_pscsi.c| 6 --
> drivers/target/target_core_spc.c | 2 +-
> drivers/target/target_core_stat.c | 4 ++--
> include/target/target_core_base.h | 3 ++-
> 6 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/target/target_core_configfs.c
> b/drivers/target/target_core_configfs.c
> index f6b1549f4142..9f49b1afd685 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -613,12 +613,12 @@ static void dev_set_t10_wwn_model_alias(struct
> se_device *dev)
> const char *configname;
>
> configname = config_item_name(>dev_group.cg_item);
> - if (strlen(configname) >= 16) {
> + if (strlen(configname) >= INQUIRY_MODEL_LEN) {
> pr_warn("dev[%p]: Backstore name '%s' is too long for "
> "INQUIRY_MODEL, truncating to 16 bytes\n", dev,
> configname);
> }
> - snprintf(>t10_wwn.model[0], 16, "%s", configname);
> + snprintf(>t10_wwn.model[0], INQUIRY_MODEL_LEN, "%s", configname);
> }
>
> static ssize_t emulate_model_alias_store(struct config_item *item,
> @@ -640,11 +640,13 @@ static ssize_t emulate_model_alias_store(struct
> config_item *item,
> if (ret < 0)
> return ret;
>
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
> if (flag) {
> dev_set_t10_wwn_model_alias(dev);
> } else {
> strncpy(>t10_wwn.model[0],
> - dev->transport->inquiry_prod, 16);
> + dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
> + dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
> }
> da->emulate_model_alias = flag;
> return count;
> diff --git a/drivers/target/target_core_device.c
> b/drivers/target/target_core_device.c
> index fe4c4db51137..0d7382efb2d4 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -720,7 +720,7 @@ void core_dev_free_initiator_node_lun_acl(
> static void scsi_dump_inquiry(struct se_device *dev)
> {
> struct t10_wwn *wwn = >t10_wwn;
> - char buf[17];
> + char buf[INQUIRY_MODEL_LEN + 1];
> int i, device_type;
> /*
>* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
> @@ -733,7 +733,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
> buf[i] = '\0';
> pr_debug(" Vendor: %s\n", buf);
>
> - for (i = 0; i < 16; i++)
> + for (i = 0; i < INQUIRY_MODEL_LEN; i++)
> if (wwn->model[i] >= 0x20)
> buf[i] = wwn->model[i];
> else
> @@ -1009,11 +1009,13 @@ int target_configure_device(struct se_device *dev)
>* passthrough because this is being provided by the backend LLD.
>*/
> BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
> if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
> strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
> dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
> strncpy(>t10_wwn.model[0],
> - dev->transport->inquiry_prod, 16);
> + dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
> + dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
> strncpy(>t10_wwn.revision[0],
> dev->transport->inquiry_rev, 4);
> }
> diff --git a/drivers/target/target_core_pscsi.c
> b/drivers/target/target_core_pscsi.c
> index ee65b5bb674c..1633babc2d4e 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -193,7 +193,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct
> t10_wwn *wwn)
> BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
> memcpy(>vendor[0], [8], INQUIRY_VENDOR_LEN);
> wwn->vendor[INQUIRY_VENDOR_LEN] = '\0';
> - memcpy(>model[0], [16], sizeof(wwn->model));
> + BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
> + memcpy(>model[0], [16], INQUIRY_MODEL_LEN);
> + wwn->model[INQUIRY_MODEL_LEN] = '\0';
> memcpy(>revision[0], [32], sizeof(wwn->revision));
> }
>
> @@ -835,7 +837,7 @@ static ssize_t pscsi_show_configfs_dev_params(struct
>
Re: [PATCH v4 2/7] target: consistently null-terminate t10_wwn.vendor
On 11/28/18 5:01 PM, David Disseldorp wrote:
> In preparation for supporting user provided vendor strings, add an extra
> byte to t10_wwn.vendor which will ensure null string termination when an
> eight byte vendor string is provided by the user.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_device.c | 6 --
> drivers/target/target_core_pscsi.c | 6 --
> drivers/target/target_core_stat.c | 4 ++--
> include/target/target_core_base.h | 8 +++-
> 4 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/target/target_core_device.c
> b/drivers/target/target_core_device.c
> index 47b5ef153135..fe4c4db51137 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -725,7 +725,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
> /*
>* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
>*/
> - for (i = 0; i < 8; i++)
> + for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
> if (wwn->vendor[i] >= 0x20)
> buf[i] = wwn->vendor[i];
> else
> @@ -1008,8 +1008,10 @@ int target_configure_device(struct se_device *dev)
>* anything virtual (IBLOCK, FILEIO, RAMDISK), but not for TCM/pSCSI
>* passthrough because this is being provided by the backend LLD.
>*/
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
> - strncpy(>t10_wwn.vendor[0], "LIO-ORG", 8);
> + strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
> + dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
> strncpy(>t10_wwn.model[0],
> dev->transport->inquiry_prod, 16);
> strncpy(>t10_wwn.revision[0],
> diff --git a/drivers/target/target_core_pscsi.c
> b/drivers/target/target_core_pscsi.c
> index 47d76c862014..ee65b5bb674c 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -190,7 +190,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct
> t10_wwn *wwn)
> /*
>* Use sdev->inquiry from drivers/scsi/scsi_scan.c:scsi_alloc_sdev()
>*/
> - memcpy(>vendor[0], [8], sizeof(wwn->vendor));
> + BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
> + memcpy(>vendor[0], [8], INQUIRY_VENDOR_LEN);
> + wwn->vendor[INQUIRY_VENDOR_LEN] = '\0';
> memcpy(>model[0], [16], sizeof(wwn->model));
> memcpy(>revision[0], [32], sizeof(wwn->revision));
> }
> @@ -826,7 +828,7 @@ static ssize_t pscsi_show_configfs_dev_params(struct
> se_device *dev, char *b)
> if (sd) {
> bl += sprintf(b + bl, "");
> bl += sprintf(b + bl, "Vendor: ");
> - for (i = 0; i < 8; i++) {
> + for (i = 0; i < INQUIRY_VENDOR_LEN; i++) {
> if (ISPRINT(sd->vendor[i])) /* printable character? */
> bl += sprintf(b + bl, "%c", sd->vendor[i]);
> else
> diff --git a/drivers/target/target_core_stat.c
> b/drivers/target/target_core_stat.c
> index f0db91ebd735..4210cf625d84 100644
> --- a/drivers/target/target_core_stat.c
> +++ b/drivers/target/target_core_stat.c
> @@ -247,10 +247,10 @@ static ssize_t target_stat_lu_vend_show(struct
> config_item *item, char *page)
> {
> struct se_device *dev = to_stat_lu_dev(item);
> int i;
> - char str[sizeof(dev->t10_wwn.vendor)+1];
> + char str[INQUIRY_VENDOR_LEN+1];
>
> /* scsiLuVendorId */
> - for (i = 0; i < sizeof(dev->t10_wwn.vendor); i++)
> + for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
> str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
> dev->t10_wwn.vendor[i] : ' ';
> str[i] = '\0';
> diff --git a/include/target/target_core_base.h
> b/include/target/target_core_base.h
> index e3bdb0550a59..cb1f3f574e2a 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -46,6 +46,8 @@
> /* Used by transport_get_inquiry_vpd_device_ident() */
> #define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN254
>
> +#define INQUIRY_VENDOR_LEN 8
> +
> /* Attempts before moving from SHORT to LONG */
> #define PYX_TRANSPORT_WINDOW_CLOSED_THRESHOLD3
> #define PYX_TRANSPORT_WINDOW_CLOSED_WAIT_SHORT 3 /* In milliseconds */
> @@ -314,7 +316,11 @@ struct t10_vpd {
> };
>
> struct t10_wwn {
> - char vendor[8];
> + /*
> + * SCSI left aligned strings may not be null terminated. +1 to ensure a
> + * null terminator is always present.
> + */
> + char vendor[INQUIRY_VENDOR_LEN + 1];
> char model[16];
> char revision[4];
> char unit_serial[INQUIRY_VPD_SERIAL_LEN];
>
Reviewed-by: Lee Duncan
Re: [PATCH v4 2/7] target: consistently null-terminate t10_wwn.vendor
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > In preparation for supporting user provided vendor strings, add an extra > byte to t10_wwn.vendor which will ensure null string termination when an > eight byte vendor string is provided by the user. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_device.c | 6 -- > drivers/target/target_core_pscsi.c | 6 -- > drivers/target/target_core_stat.c | 4 ++-- > include/target/target_core_base.h | 8 +++- > 4 files changed, 17 insertions(+), 7 deletions(-) > > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 1/7] target: use consistent left-aligned ASCII INQUIRY data
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > spc5r17.pdf specifies: > 4.3.1 ASCII data field requirements > ASCII data fields shall contain only ASCII printable characters (i.e., > code values 20h to 7Eh) and may be terminated with one or more ASCII > null (00h) characters. > ASCII data fields described as being left-aligned shall have any > unused bytes at the end of the field (i.e., highest offset) and the > unused bytes shall be filled with ASCII space characters (20h). > > LIO currently space-pads the T10 VENDOR IDENTIFICATION and PRODUCT > IDENTIFICATION fields in the standard INQUIRY data. However, the > PRODUCT REVISION LEVEL field in the standard INQUIRY data as well as the > T10 VENDOR IDENTIFICATION field in the INQUIRY Device Identification VPD > Page are zero-terminated/zero-padded. > > Fix this inconsistency by using space-padding for all of the above > fields. > > Signed-off-by: David Disseldorp > Reviewed-by: Christoph Hellwig > --- > drivers/target/target_core_spc.c | 17 - > 1 file changed, 12 insertions(+), 5 deletions(-) > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 1/7] target: use consistent left-aligned ASCII INQUIRY data
On 11/28/18 5:01 PM, David Disseldorp wrote:
> spc5r17.pdf specifies:
> 4.3.1 ASCII data field requirements
> ASCII data fields shall contain only ASCII printable characters (i.e.,
> code values 20h to 7Eh) and may be terminated with one or more ASCII
> null (00h) characters.
> ASCII data fields described as being left-aligned shall have any
> unused bytes at the end of the field (i.e., highest offset) and the
> unused bytes shall be filled with ASCII space characters (20h).
>
> LIO currently space-pads the T10 VENDOR IDENTIFICATION and PRODUCT
> IDENTIFICATION fields in the standard INQUIRY data. However, the
> PRODUCT REVISION LEVEL field in the standard INQUIRY data as well as the
> T10 VENDOR IDENTIFICATION field in the INQUIRY Device Identification VPD
> Page are zero-terminated/zero-padded.
>
> Fix this inconsistency by using space-padding for all of the above
> fields.
>
> Signed-off-by: David Disseldorp
> Reviewed-by: Christoph Hellwig
> ---
> drivers/target/target_core_spc.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/target/target_core_spc.c
> b/drivers/target/target_core_spc.c
> index f459118bc11b..c37dd36ec77d 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -108,12 +108,17 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned
> char *buf)
>
> buf[7] = 0x2; /* CmdQue=1 */
>
> - memcpy([8], "LIO-ORG ", 8);
> - memset([16], 0x20, 16);
> + /*
> + * ASCII data fields described as being left-aligned shall have any
> + * unused bytes at the end of the field (i.e., highest offset) and the
> + * unused bytes shall be filled with ASCII space characters (20h).
> + */
> + memset([8], 0x20, 8 + 16 + 4);
I dislike that you are using 0x20 here (and below) instead of ' '.
> + memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> memcpy([16], dev->t10_wwn.model,
> -min_t(size_t, strlen(dev->t10_wwn.model), 16));
> +strnlen(dev->t10_wwn.model, 16));
> memcpy([32], dev->t10_wwn.revision,
> -min_t(size_t, strlen(dev->t10_wwn.revision), 4));
> +strnlen(dev->t10_wwn.revision, 4));
> buf[4] = 31; /* Set additional length to 31 */
>
> return 0;
> @@ -251,7 +256,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char
> *buf)
> buf[off] = 0x2; /* ASCII */
> buf[off+1] = 0x1; /* T10 Vendor ID */
> buf[off+2] = 0x0;
> - memcpy([off+4], "LIO-ORG", 8);
> + /* left align Vendor ID and pad with spaces */
> + memset([off+4], 0x20, 8);
> + memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
> /* Extra Byte for NULL Terminator */
> id_len++;
> /* Identifier Length */
>
Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but > can be reconfigured via the vendor_id ConfigFS attribute. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_spc.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 5/7] target: add device vendor_id configfs attribute
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > The vendor_id attribute will allow for the modification of the T10 > Vendor Identification string returned in inquiry responses. Its value > can be viewed and modified via the ConfigFS path at: > target/core/$backstore/$name/wwn/vendor_id > > "LIO-ORG" remains the default value, which is set when the backstore > device is enabled. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_configfs.c | 48 +++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/target/target_core_configfs.c > b/drivers/target/target_core_configfs.c > index 9f49b1afd685..fc60c4db718d 100644 > --- a/drivers/target/target_core_configfs.c > +++ b/drivers/target/target_core_configfs.c > @@ -1213,6 +1213,52 @@ static struct t10_wwn *to_t10_wwn(struct config_item > *item) > } > Thanks for making it configurable. I know back when I was at IBM we made changes internally to support LIO-ORG. Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths > don't currently explicitly null-terminate t10_wwn.model. > Add an extra byte to the t10_wwn.model buffer and perform null string > termination in all cases. > > dev_set_t10_wwn_model_alias() continues to truncate at the same length > to avoid changing the model string for existing deployments. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_configfs.c | 8 +--- > drivers/target/target_core_device.c | 8 +--- > drivers/target/target_core_pscsi.c| 6 -- > drivers/target/target_core_spc.c | 2 +- > drivers/target/target_core_stat.c | 4 ++-- > include/target/target_core_base.h | 3 ++- > 6 files changed, 19 insertions(+), 12 deletions(-) > > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 4/7] target: consistently null-terminate t10_wwn.revision
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > The pscsi_set_inquiry_info() codepath doesn't currently explicitly > null-terminate t10_wwn.revision. > Add an extra byte to the t10_wwn.model buffer and perform null string > termination in all cases. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_device.c | 6 -- > drivers/target/target_core_pscsi.c | 4 +++- > drivers/target/target_core_spc.c| 5 +++-- > drivers/target/target_core_stat.c | 4 ++-- > include/target/target_core_base.h | 3 ++- > 5 files changed, 14 insertions(+), 8 deletions(-) > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v2 8/9] qedi: Move LL2 producer index processing in BH.
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> From: Manish Rangankar
>
> 1. Removed logic to update HW producer index in interrupt context.
> 2. Update HW producer index after UIO ring and buffer gets initialized.
>
> Signed-off-by: Manish Rangankar
> ---
> drivers/scsi/qedi/qedi_main.c | 31 +++
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 8942f62..053a947 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -665,7 +665,6 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff *skb,
> u32 arg1, u32 arg2)
> struct qedi_uio_ctrl *uctrl;
> struct skb_work_list *work;
> struct ethhdr *eh;
> - u32 prod;
>
> if (!qedi) {
> QEDI_ERR(NULL, "qedi is NULL\n");
> @@ -724,17 +723,10 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff
> *skb, u32 arg1, u32 arg2)
>
> spin_lock_bh(>ll2_lock);
> list_add_tail(>list, >ll2_skb_list);
> + spin_unlock_bh(>ll2_lock);
>
> - ++uctrl->hw_rx_prod_cnt;
> - prod = (uctrl->hw_rx_prod + 1) % RX_RING;
> - if (prod != uctrl->host_rx_cons) {
> - uctrl->hw_rx_prod = prod;
> - spin_unlock_bh(>ll2_lock);
> - wake_up_process(qedi->ll2_recv_thread);
> - return 0;
> - }
> + wake_up_process(qedi->ll2_recv_thread);
>
> - spin_unlock_bh(>ll2_lock);
> return 0;
> }
>
> @@ -749,6 +741,7 @@ static int qedi_ll2_process_skb(struct qedi_ctx *qedi,
> struct sk_buff *skb,
> u32 rx_bd_prod;
> void *pkt;
> int len = 0;
> + u32 prod;
>
> if (!qedi) {
> QEDI_ERR(NULL, "qedi is NULL\n");
> @@ -757,12 +750,16 @@ static int qedi_ll2_process_skb(struct qedi_ctx *qedi,
> struct sk_buff *skb,
>
> udev = qedi->udev;
> uctrl = udev->uctrl;
> - pkt = udev->rx_pkt + (uctrl->hw_rx_prod * qedi_ll2_buf_size);
> +
> + ++uctrl->hw_rx_prod_cnt;
> + prod = (uctrl->hw_rx_prod + 1) % RX_RING;
> +
> + pkt = udev->rx_pkt + (prod * qedi_ll2_buf_size);
> len = min_t(u32, skb->len, (u32)qedi_ll2_buf_size);
> memcpy(pkt, skb->data, len);
>
> memset(, 0, sizeof(rxbd));
> - rxbd.rx_pkt_index = uctrl->hw_rx_prod;
> + rxbd.rx_pkt_index = prod;
> rxbd.rx_pkt_len = len;
> rxbd.vlan_id = vlan_id;
>
> @@ -773,6 +770,16 @@ static int qedi_ll2_process_skb(struct qedi_ctx *qedi,
> struct sk_buff *skb,
>
> memcpy(p_rxbd, , sizeof(rxbd));
>
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "hw_rx_prod [%d] prod [%d] hw_rx_bd_prod [%d] rx_pkt_idx [%d]
> rx_len [%d].\n",
> + uctrl->hw_rx_prod, prod, uctrl->hw_rx_bd_prod,
> + rxbd.rx_pkt_index, rxbd.rx_pkt_len);
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "host_rx_cons [%d] hw_rx_bd_cons [%d].\n",
> + uctrl->host_rx_cons, uctrl->host_rx_bd_cons);
> +
> + uctrl->hw_rx_prod = prod;
> +
> /* notify the iscsiuio about new packet */
> uio_event_notify(>qedi_uinfo);
>
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 7/9] qedi: add module param to set ping packet size
On 11/21/18 1:25 AM, Nilesh Javali wrote: > Default packet size is 0x400. > For jumbo packets set to 0x2400. > > Signed-off-by: Nilesh Javali > --- > drivers/scsi/qedi/qedi.h | 1 - > drivers/scsi/qedi/qedi_main.c | 13 + > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h > index 6fa02c5..a26bb506 100644 > --- a/drivers/scsi/qedi/qedi.h > +++ b/drivers/scsi/qedi/qedi.h > @@ -63,7 +63,6 @@ > #define QEDI_LOCAL_PORT_INVALID 0x > #define TX_RX_RING 16 > #define RX_RING (TX_RX_RING - 1) > -#define LL2_SINGLE_BUF_SIZE 0x400 > #define QEDI_PAGE_ALIGN(addr)ALIGN(addr, QEDI_PAGE_SIZE) > #define QEDI_PAGE_MASK (~((QEDI_PAGE_SIZE) - 1)) > > diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c > index 2621dee..8942f62 100644 > --- a/drivers/scsi/qedi/qedi_main.c > +++ b/drivers/scsi/qedi/qedi_main.c > @@ -44,6 +44,11 @@ > MODULE_PARM_DESC(qedi_io_tracing, >" Enable logging of SCSI requests/completions into trace > buffer. (default off)."); > > +uint qedi_ll2_buf_size = 0x400; > +module_param(qedi_ll2_buf_size, uint, 0644); > +MODULE_PARM_DESC(qedi_ll2_buf_size, > + "parameter to set ping packet size, default - 0x400, Jumbo > packets - 0x2400."); > + > const struct qed_iscsi_ops *qedi_ops; > static struct scsi_transport_template *qedi_scsi_transport; > static struct pci_driver qedi_pci_driver; > @@ -228,7 +233,7 @@ static int __qedi_alloc_uio_rings(struct qedi_uio_dev > *udev) > } > > /* Allocating memory for Tx/Rx pkt buffer */ > - udev->ll2_buf_size = TX_RX_RING * LL2_SINGLE_BUF_SIZE; > + udev->ll2_buf_size = TX_RX_RING * qedi_ll2_buf_size; > udev->ll2_buf_size = QEDI_PAGE_ALIGN(udev->ll2_buf_size); > udev->ll2_buf = (void *)__get_free_pages(GFP_KERNEL | __GFP_COMP | >__GFP_ZERO, 2); > @@ -283,7 +288,7 @@ static int qedi_alloc_uio_rings(struct qedi_ctx *qedi) > qedi->udev = udev; > > udev->tx_pkt = udev->ll2_buf; > - udev->rx_pkt = udev->ll2_buf + LL2_SINGLE_BUF_SIZE; > + udev->rx_pkt = udev->ll2_buf + qedi_ll2_buf_size; > return 0; > > err_uctrl: > @@ -752,8 +757,8 @@ static int qedi_ll2_process_skb(struct qedi_ctx *qedi, > struct sk_buff *skb, > > udev = qedi->udev; > uctrl = udev->uctrl; > - pkt = udev->rx_pkt + (uctrl->hw_rx_prod * LL2_SINGLE_BUF_SIZE); > - len = min_t(u32, skb->len, (u32)LL2_SINGLE_BUF_SIZE); > + pkt = udev->rx_pkt + (uctrl->hw_rx_prod * qedi_ll2_buf_size); > + len = min_t(u32, skb->len, (u32)qedi_ll2_buf_size); > memcpy(pkt, skb->data, len); > > memset(, 0, sizeof(rxbd)); > Reviewed-by: Lee Duncan
Re: [PATCH v2 6/9] qedi: Add packet filter in light L2 Rx path.
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> From: Manish Rangankar
>
> Add packet filter to avoid unnecessary packet processing in iscsiuio.
>
> Signed-off-by: Manish Rangankar
> ---
> drivers/scsi/qedi/qedi_main.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 713db9c..2621dee 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -659,6 +659,7 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff *skb,
> u32 arg1, u32 arg2)
> struct qedi_uio_dev *udev;
> struct qedi_uio_ctrl *uctrl;
> struct skb_work_list *work;
> + struct ethhdr *eh;
> u32 prod;
>
> if (!qedi) {
> @@ -673,6 +674,29 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff
> *skb, u32 arg1, u32 arg2)
> return 0;
> }
>
> + eh = (struct ethhdr *)skb->data;
> + /* Undo VLAN encapsulation */
> + if (eh->h_proto == htons(ETH_P_8021Q)) {
> + memmove((u8 *)eh + VLAN_HLEN, eh, ETH_ALEN * 2);
> + eh = (struct ethhdr *)skb_pull(skb, VLAN_HLEN);
> + skb_reset_mac_header(skb);
> + }
> +
> + /* Filter out non FIP/FCoE frames here to free them faster */
> + if (eh->h_proto != htons(ETH_P_ARP) &&
> + eh->h_proto != htons(ETH_P_IP) &&
> + eh->h_proto != htons(ETH_P_IPV6)) {
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "Dropping frame ethertype [0x%x] len [0x%x].\n",
> + eh->h_proto, skb->len);
> + kfree_skb(skb);
> + return 0;
> + }
> +
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_LL2,
> + "Allowed frame ethertype [0x%x] len [0x%x].\n",
> + eh->h_proto, skb->len);
> +
> udev = qedi->udev;
> uctrl = udev->uctrl;
>
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 5/9] qedi: Check for session online before getting iSCSI TLV data.
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> From: Manish Rangankar
>
> The kernel panic was observed after switch side perturbation,
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [] strcmp+0x20/0x40
> PGD 0 Oops: [#1] SMP
> CPU: 8 PID: 647 Comm: kworker/8:1 Tainted: GW OE
> 3.10.0-693.el7.x86_64 #1
> Hardware name: HPE ProLiant DL380 Gen10/ProLiant DL380 Gen10, BIOS U30
> 06/20/2018
> Workqueue: slowpath-13:00. qed_slowpath_task [qed]
> task: 880429eb8fd0 ti: 88042919 task.ti: 88042919
> RIP: 0010:[] [] strcmp+0x20/0x40
> RSP: 0018:880429193c68 EFLAGS: 00010202
> RAX: 000a RBX: 0002 RCX:
> RDX: 0001 RSI: 0001 RDI: 88042bda7a41
> RBP: 880429193c68 R08: R09:
> R10: 0007 R11: 88042b3af338 R12: 880420b007a0
> R13: 88081aa56af8 R14: 0001 R15: 88081aa50410
> FS: () GS:88042fe0() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: CR3: 019f2000 CR4: 003407e0
> DR0: DR1: DR2:
> DR3: DR6: fffe0ff0 DR7: 0400
> Stack:
> 880429193d20 c02a0c90 c90004b32000 8803fd3ec600
> 88042bda7800 88042bda7a00 88042bda7840 88042bda7a40
> 000129193d10 2e3836312e323931 ff000a342e363232 c01ad99d
> Call Trace:
> [] qedi_get_protocol_tlv_data+0x270/0x470 [qedi]
> [] ? qed_mfw_process_tlv_req+0x24d/0xbf0 [qed]
> [] qed_mfw_fill_tlv_data+0x5e/0xd0 [qed]
> [] qed_mfw_process_tlv_req+0x269/0xbf0 [qed]
>
> Fix kernel NULL pointer deref by checking for session is online
> before getting iSCSI TLV data.
>
> Signed-off-by: Manish Rangankar
> ---
> drivers/scsi/qedi/qedi_main.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 5308e6b..713db9c 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -952,6 +952,9 @@ static int qedi_find_boot_info(struct qedi_ctx *qedi,
> cls_sess = iscsi_conn_to_session(cls_conn);
> sess = cls_sess->dd_data;
>
> + if (!iscsi_is_session_online(cls_sess))
> + continue;
> +
> if (pri_ctrl_flags) {
> if (!strcmp(pri_tgt->iscsi_name, sess->targetname) &&
> !strcmp(pri_tgt->ip_addr, ep_ip_addr)) {
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 3/9] qedi: Replace PAGE_SIZE with QEDI_PAGE_SIZE
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> Use QEDI_PAGE_SIZE for enablement of module on systems with 64K page size.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi_main.c | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 0f8eb5f..a1225ae 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -796,7 +796,7 @@ static int qedi_set_iscsi_pf_param(struct qedi_ctx *qedi)
> int rval = 0;
>
>
> - num_sq_pages = (MAX_OUTSTANDING_TASKS_PER_CON * 8) / PAGE_SIZE;
> + num_sq_pages = (MAX_OUTSTANDING_TASKS_PER_CON * 8) / QEDI_PAGE_SIZE;
>
> qedi->num_queues = MIN_NUM_CPUS_MSIX(qedi);
>
> @@ -834,7 +834,7 @@ static int qedi_set_iscsi_pf_param(struct qedi_ctx *qedi)
> qedi->pf_params.iscsi_pf_params.max_fin_rt = 2;
>
> for (log_page_size = 0 ; log_page_size < 32 ; log_page_size++) {
> - if ((1 << log_page_size) == PAGE_SIZE)
> + if ((1 << log_page_size) == QEDI_PAGE_SIZE)
> break;
> }
> qedi->pf_params.iscsi_pf_params.log_page_size = log_page_size;
> @@ -1376,7 +1376,7 @@ static void qedi_free_bdq(struct qedi_ctx *qedi)
> int i;
>
> if (qedi->bdq_pbl_list)
> - dma_free_coherent(>pdev->dev, PAGE_SIZE,
> + dma_free_coherent(>pdev->dev, QEDI_PAGE_SIZE,
> qedi->bdq_pbl_list, qedi->bdq_pbl_list_dma);
>
> if (qedi->bdq_pbl)
> @@ -1437,7 +1437,7 @@ static int qedi_alloc_bdq(struct qedi_ctx *qedi)
>
> /* Alloc dma memory for BDQ page buffer list */
> qedi->bdq_pbl_mem_size = QEDI_BDQ_NUM * sizeof(struct scsi_bd);
> - qedi->bdq_pbl_mem_size = ALIGN(qedi->bdq_pbl_mem_size, PAGE_SIZE);
> + qedi->bdq_pbl_mem_size = ALIGN(qedi->bdq_pbl_mem_size, QEDI_PAGE_SIZE);
> qedi->rq_num_entries = qedi->bdq_pbl_mem_size / sizeof(struct scsi_bd);
>
> QEDI_INFO(>dbg_ctx, QEDI_LOG_CONN, "rq_num_entries = %d.\n",
> @@ -1472,7 +1472,8 @@ static int qedi_alloc_bdq(struct qedi_ctx *qedi)
> }
>
> /* Allocate list of PBL pages */
> - qedi->bdq_pbl_list = dma_zalloc_coherent(>pdev->dev, PAGE_SIZE,
> + qedi->bdq_pbl_list = dma_zalloc_coherent(>pdev->dev,
> + QEDI_PAGE_SIZE,
>>bdq_pbl_list_dma,
>GFP_KERNEL);
> if (!qedi->bdq_pbl_list) {
> @@ -1485,13 +1486,14 @@ static int qedi_alloc_bdq(struct qedi_ctx *qedi)
>* Now populate PBL list with pages that contain pointers to the
>* individual buffers.
>*/
> - qedi->bdq_pbl_list_num_entries = qedi->bdq_pbl_mem_size / PAGE_SIZE;
> + qedi->bdq_pbl_list_num_entries = qedi->bdq_pbl_mem_size /
> + QEDI_PAGE_SIZE;
> list = (u64 *)qedi->bdq_pbl_list;
> page = qedi->bdq_pbl_list_dma;
> for (i = 0; i < qedi->bdq_pbl_list_num_entries; i++) {
> *list = qedi->bdq_pbl_dma;
> list++;
> - page += PAGE_SIZE;
> + page += QEDI_PAGE_SIZE;
> }
>
> return 0;
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 2/9] qedi: Fix spelling mistake "OUSTANDING" -> "OUTSTANDING"
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> Fix trivial spelling mistake within macro definition.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi.h | 4 ++--
> drivers/scsi/qedi/qedi_main.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h
> index e966855..6fa02c5 100644
> --- a/drivers/scsi/qedi/qedi.h
> +++ b/drivers/scsi/qedi/qedi.h
> @@ -45,7 +45,7 @@
> #define QEDI_MAX_TASK_NUM0x0FFF
> #define QEDI_MAX_ISCSI_CONNS_PER_HBA 1024
> #define QEDI_ISCSI_MAX_BDS_PER_CMD 255 /* Firmware max BDs is 255 */
> -#define MAX_OUSTANDING_TASKS_PER_CON 1024
> +#define MAX_OUTSTANDING_TASKS_PER_CON1024
>
> #define QEDI_MAX_BD_LEN 0x
> #define QEDI_BD_SPLIT_SZ 0x1000
> @@ -144,7 +144,7 @@ struct skb_work_list {
> };
>
> /* Queue sizes in number of elements */
> -#define QEDI_SQ_SIZE MAX_OUSTANDING_TASKS_PER_CON
> +#define QEDI_SQ_SIZE MAX_OUTSTANDING_TASKS_PER_CON
> #define QEDI_CQ_SIZE 2048
> #define QEDI_CMDQ_SIZE QEDI_MAX_ISCSI_TASK
> #define QEDI_PROTO_CQ_PROD_IDX 0
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 105b0e4..0f8eb5f 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -796,7 +796,7 @@ static int qedi_set_iscsi_pf_param(struct qedi_ctx *qedi)
> int rval = 0;
>
>
> - num_sq_pages = (MAX_OUSTANDING_TASKS_PER_CON * 8) / PAGE_SIZE;
> + num_sq_pages = (MAX_OUTSTANDING_TASKS_PER_CON * 8) / PAGE_SIZE;
>
> qedi->num_queues = MIN_NUM_CPUS_MSIX(qedi);
>
>
Reviewed-by: Lee Duncan
Re: [PATCH v2 1/9] qedi: Cleanup redundant QEDI_PAGE_SIZE macro definition
On 11/21/18 1:25 AM, Nilesh Javali wrote: > Remove redundant macro definition. > > Signed-off-by: Nilesh Javali > --- > drivers/scsi/qedi/qedi.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h > index a6f96b3..e966855 100644 > --- a/drivers/scsi/qedi/qedi.h > +++ b/drivers/scsi/qedi/qedi.h > @@ -64,11 +64,9 @@ > #define TX_RX_RING 16 > #define RX_RING (TX_RX_RING - 1) > #define LL2_SINGLE_BUF_SIZE 0x400 > -#define QEDI_PAGE_SIZE 4096 > #define QEDI_PAGE_ALIGN(addr)ALIGN(addr, QEDI_PAGE_SIZE) > #define QEDI_PAGE_MASK (~((QEDI_PAGE_SIZE) - 1)) > > -#define QEDI_PAGE_SIZE 4096 > #define QEDI_HW_DMA_BOUNDARY 0xfff > #define QEDI_PATH_HANDLE 0xFE000UL > > Reviewed-by: Lee Duncan
Re: [PATCH v2 4/9] qedi: Allocate IRQs based on msix_cnt
On 11/21/18 1:25 AM, Nilesh Javali wrote:
> The driver load on some systems failed with error,
> [0004:01:00.5]:[qedi_request_msix_irq:2524]:8: request_irq failed.
>
> Allocate the IRQs based on MSIX count obtained from qed module
> instead of number of queues.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index a1225ae..5308e6b 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -1298,7 +1298,7 @@ static int qedi_request_msix_irq(struct qedi_ctx *qedi)
> int i, rc, cpu;
>
> cpu = cpumask_first(cpu_online_mask);
> - for (i = 0; i < MIN_NUM_CPUS_MSIX(qedi); i++) {
> + for (i = 0; i < qedi->int_info.msix_cnt; i++) {
> rc = request_irq(qedi->int_info.msix[i].vector,
>qedi_msix_handler, 0, "qedi",
>>fp_array[i]);
>
Reviewed-by: Lee Duncan
[PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths
don't currently explicitly null-terminate t10_wwn.model.
Add an extra byte to the t10_wwn.model buffer and perform null string
termination in all cases.
dev_set_t10_wwn_model_alias() continues to truncate at the same length
to avoid changing the model string for existing deployments.
Signed-off-by: David Disseldorp
---
drivers/target/target_core_configfs.c | 8 +---
drivers/target/target_core_device.c | 8 +---
drivers/target/target_core_pscsi.c| 6 --
drivers/target/target_core_spc.c | 2 +-
drivers/target/target_core_stat.c | 4 ++--
include/target/target_core_base.h | 3 ++-
6 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/target/target_core_configfs.c
b/drivers/target/target_core_configfs.c
index f6b1549f4142..9f49b1afd685 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -613,12 +613,12 @@ static void dev_set_t10_wwn_model_alias(struct se_device
*dev)
const char *configname;
configname = config_item_name(>dev_group.cg_item);
- if (strlen(configname) >= 16) {
+ if (strlen(configname) >= INQUIRY_MODEL_LEN) {
pr_warn("dev[%p]: Backstore name '%s' is too long for "
"INQUIRY_MODEL, truncating to 16 bytes\n", dev,
configname);
}
- snprintf(>t10_wwn.model[0], 16, "%s", configname);
+ snprintf(>t10_wwn.model[0], INQUIRY_MODEL_LEN, "%s", configname);
}
static ssize_t emulate_model_alias_store(struct config_item *item,
@@ -640,11 +640,13 @@ static ssize_t emulate_model_alias_store(struct
config_item *item,
if (ret < 0)
return ret;
+ BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
if (flag) {
dev_set_t10_wwn_model_alias(dev);
} else {
strncpy(>t10_wwn.model[0],
- dev->transport->inquiry_prod, 16);
+ dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
+ dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
}
da->emulate_model_alias = flag;
return count;
diff --git a/drivers/target/target_core_device.c
b/drivers/target/target_core_device.c
index fe4c4db51137..0d7382efb2d4 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -720,7 +720,7 @@ void core_dev_free_initiator_node_lun_acl(
static void scsi_dump_inquiry(struct se_device *dev)
{
struct t10_wwn *wwn = >t10_wwn;
- char buf[17];
+ char buf[INQUIRY_MODEL_LEN + 1];
int i, device_type;
/*
* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
@@ -733,7 +733,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
buf[i] = '\0';
pr_debug(" Vendor: %s\n", buf);
- for (i = 0; i < 16; i++)
+ for (i = 0; i < INQUIRY_MODEL_LEN; i++)
if (wwn->model[i] >= 0x20)
buf[i] = wwn->model[i];
else
@@ -1009,11 +1009,13 @@ int target_configure_device(struct se_device *dev)
* passthrough because this is being provided by the backend LLD.
*/
BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
+ BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
strncpy(>t10_wwn.model[0],
- dev->transport->inquiry_prod, 16);
+ dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
+ dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
strncpy(>t10_wwn.revision[0],
dev->transport->inquiry_rev, 4);
}
diff --git a/drivers/target/target_core_pscsi.c
b/drivers/target/target_core_pscsi.c
index ee65b5bb674c..1633babc2d4e 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -193,7 +193,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct
t10_wwn *wwn)
BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
memcpy(>vendor[0], [8], INQUIRY_VENDOR_LEN);
wwn->vendor[INQUIRY_VENDOR_LEN] = '\0';
- memcpy(>model[0], [16], sizeof(wwn->model));
+ BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
+ memcpy(>model[0], [16], INQUIRY_MODEL_LEN);
+ wwn->model[INQUIRY_MODEL_LEN] = '\0';
memcpy(>revision[0], [32], sizeof(wwn->revision));
}
@@ -835,7 +837,7 @@ static ssize_t pscsi_show_configfs_dev_params(struct
se_device *dev, char *b)
bl += sprintf(b + bl, " ");
}
bl += sprintf(b + bl, " Model: ");
-
[PATCH v4 7/7] target: use printf width specifier for t10_wwn field dumps
From: Bart Van Assche
The existing for loops step over null-terminators for right-padding.
Padding can be achieved in a much simpler way using printf width
specifiers.
Signed-off-by: David Disseldorp
---
drivers/target/target_core_device.c | 35 ---
drivers/target/target_core_stat.c | 32 +++-
2 files changed, 15 insertions(+), 52 deletions(-)
diff --git a/drivers/target/target_core_device.c
b/drivers/target/target_core_device.c
index b3d0bd1ab09f..7f2d307e411b 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -720,36 +720,17 @@ void core_dev_free_initiator_node_lun_acl(
static void scsi_dump_inquiry(struct se_device *dev)
{
struct t10_wwn *wwn = >t10_wwn;
- char buf[INQUIRY_MODEL_LEN + 1];
- int i, device_type;
+ int device_type = dev->transport->get_device_type(dev);
+
/*
* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
*/
- for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
- if (wwn->vendor[i] >= 0x20)
- buf[i] = wwn->vendor[i];
- else
- buf[i] = ' ';
- buf[i] = '\0';
- pr_debug(" Vendor: %s\n", buf);
-
- for (i = 0; i < INQUIRY_MODEL_LEN; i++)
- if (wwn->model[i] >= 0x20)
- buf[i] = wwn->model[i];
- else
- buf[i] = ' ';
- buf[i] = '\0';
- pr_debug(" Model: %s\n", buf);
-
- for (i = 0; i < INQUIRY_REVISION_LEN; i++)
- if (wwn->revision[i] >= 0x20)
- buf[i] = wwn->revision[i];
- else
- buf[i] = ' ';
- buf[i] = '\0';
- pr_debug(" Revision: %s\n", buf);
-
- device_type = dev->transport->get_device_type(dev);
+ pr_debug(" Vendor: %-" __stringify(INQUIRY_VENDOR_LEN) "s\n",
+ wwn->vendor);
+ pr_debug(" Model: %-" __stringify(INQUIRY_MODEL_LEN) "s\n",
+ wwn->model);
+ pr_debug(" Revision: %-" __stringify(INQUIRY_REVISION_LEN) "s\n",
+ wwn->revision);
pr_debug(" Type: %s ", scsi_device_type(device_type));
}
diff --git a/drivers/target/target_core_stat.c
b/drivers/target/target_core_stat.c
index e437ba494865..87fd2b11fe3b 100644
--- a/drivers/target/target_core_stat.c
+++ b/drivers/target/target_core_stat.c
@@ -246,43 +246,25 @@ static ssize_t target_stat_lu_lu_name_show(struct
config_item *item, char *page)
static ssize_t target_stat_lu_vend_show(struct config_item *item, char *page)
{
struct se_device *dev = to_stat_lu_dev(item);
- int i;
- char str[INQUIRY_VENDOR_LEN+1];
- /* scsiLuVendorId */
- for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
- str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
- dev->t10_wwn.vendor[i] : ' ';
- str[i] = '\0';
- return snprintf(page, PAGE_SIZE, "%s\n", str);
+ return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_VENDOR_LEN)
+ "s\n", dev->t10_wwn.vendor);
}
static ssize_t target_stat_lu_prod_show(struct config_item *item, char *page)
{
struct se_device *dev = to_stat_lu_dev(item);
- int i;
- char str[INQUIRY_MODEL_LEN+1];
- /* scsiLuProductId */
- for (i = 0; i < INQUIRY_MODEL_LEN; i++)
- str[i] = ISPRINT(dev->t10_wwn.model[i]) ?
- dev->t10_wwn.model[i] : ' ';
- str[i] = '\0';
- return snprintf(page, PAGE_SIZE, "%s\n", str);
+ return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_MODEL_LEN)
+ "s\n", dev->t10_wwn.model);
}
static ssize_t target_stat_lu_rev_show(struct config_item *item, char *page)
{
struct se_device *dev = to_stat_lu_dev(item);
- int i;
- char str[INQUIRY_REVISION_LEN+1];
-
- /* scsiLuRevisionId */
- for (i = 0; i < INQUIRY_REVISION_LEN; i++)
- str[i] = ISPRINT(dev->t10_wwn.revision[i]) ?
- dev->t10_wwn.revision[i] : ' ';
- str[i] = '\0';
- return snprintf(page, PAGE_SIZE, "%s\n", str);
+
+ return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_REVISION_LEN)
+ "s\n", dev->t10_wwn.revision);
}
static ssize_t target_stat_lu_dev_type_show(struct config_item *item, char
*page)
--
2.13.7
[PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but
can be reconfigured via the vendor_id ConfigFS attribute.
Signed-off-by: David Disseldorp
---
drivers/target/target_core_spc.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 8ffe712cb44d..4503f3336bc2 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -115,7 +115,8 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char
*buf)
*/
memset([8], 0x20,
INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
- memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
+ memcpy([8], dev->t10_wwn.vendor,
+ strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
memcpy([16], dev->t10_wwn.model,
strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
memcpy([32], dev->t10_wwn.revision,
@@ -258,8 +259,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
buf[off+1] = 0x1; /* T10 Vendor ID */
buf[off+2] = 0x0;
/* left align Vendor ID and pad with spaces */
- memset([off+4], 0x20, 8);
- memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
+ memset([off+4], 0x20, INQUIRY_VENDOR_LEN);
+ memcpy([off+4], dev->t10_wwn.vendor,
+ strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
/* Extra Byte for NULL Terminator */
id_len++;
/* Identifier Length */
--
2.13.7
[PATCH v4 4/7] target: consistently null-terminate t10_wwn.revision
The pscsi_set_inquiry_info() codepath doesn't currently explicitly
null-terminate t10_wwn.revision.
Add an extra byte to the t10_wwn.model buffer and perform null string
termination in all cases.
Signed-off-by: David Disseldorp
---
drivers/target/target_core_device.c | 6 --
drivers/target/target_core_pscsi.c | 4 +++-
drivers/target/target_core_spc.c| 5 +++--
drivers/target/target_core_stat.c | 4 ++--
include/target/target_core_base.h | 3 ++-
5 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/target/target_core_device.c
b/drivers/target/target_core_device.c
index 0d7382efb2d4..b3d0bd1ab09f 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -741,7 +741,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
buf[i] = '\0';
pr_debug(" Model: %s\n", buf);
- for (i = 0; i < 4; i++)
+ for (i = 0; i < INQUIRY_REVISION_LEN; i++)
if (wwn->revision[i] >= 0x20)
buf[i] = wwn->revision[i];
else
@@ -1010,6 +1010,7 @@ int target_configure_device(struct se_device *dev)
*/
BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
+ BUILD_BUG_ON(sizeof(dev->t10_wwn.revision) != INQUIRY_REVISION_LEN + 1);
if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
@@ -1017,7 +1018,8 @@ int target_configure_device(struct se_device *dev)
dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
strncpy(>t10_wwn.revision[0],
- dev->transport->inquiry_rev, 4);
+ dev->transport->inquiry_rev, INQUIRY_REVISION_LEN);
+ dev->t10_wwn.revision[INQUIRY_REVISION_LEN] = '\0';
}
scsi_dump_inquiry(dev);
diff --git a/drivers/target/target_core_pscsi.c
b/drivers/target/target_core_pscsi.c
index 1633babc2d4e..5493f620b7f4 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -196,7 +196,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct
t10_wwn *wwn)
BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
memcpy(>model[0], [16], INQUIRY_MODEL_LEN);
wwn->model[INQUIRY_MODEL_LEN] = '\0';
- memcpy(>revision[0], [32], sizeof(wwn->revision));
+ BUILD_BUG_ON(sizeof(wwn->revision) != INQUIRY_REVISION_LEN + 1);
+ memcpy(>revision[0], [32], INQUIRY_REVISION_LEN);
+ wwn->revision[INQUIRY_REVISION_LEN] = '\0';
}
static int
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 78eddee4b6e6..8ffe712cb44d 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -113,12 +113,13 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char
*buf)
* unused bytes at the end of the field (i.e., highest offset) and the
* unused bytes shall be filled with ASCII space characters (20h).
*/
- memset([8], 0x20, 8 + 16 + 4);
+ memset([8], 0x20,
+ INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
memcpy([16], dev->t10_wwn.model,
strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
memcpy([32], dev->t10_wwn.revision,
- strnlen(dev->t10_wwn.revision, 4));
+ strnlen(dev->t10_wwn.revision, INQUIRY_REVISION_LEN));
buf[4] = 31; /* Set additional length to 31 */
return 0;
diff --git a/drivers/target/target_core_stat.c
b/drivers/target/target_core_stat.c
index 9123c5137da5..e437ba494865 100644
--- a/drivers/target/target_core_stat.c
+++ b/drivers/target/target_core_stat.c
@@ -275,10 +275,10 @@ static ssize_t target_stat_lu_rev_show(struct config_item
*item, char *page)
{
struct se_device *dev = to_stat_lu_dev(item);
int i;
- char str[sizeof(dev->t10_wwn.revision)+1];
+ char str[INQUIRY_REVISION_LEN+1];
/* scsiLuRevisionId */
- for (i = 0; i < sizeof(dev->t10_wwn.revision); i++)
+ for (i = 0; i < INQUIRY_REVISION_LEN; i++)
str[i] = ISPRINT(dev->t10_wwn.revision[i]) ?
dev->t10_wwn.revision[i] : ' ';
str[i] = '\0';
diff --git a/include/target/target_core_base.h
b/include/target/target_core_base.h
index cfc279686cf4..497853a09fee 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -48,6 +48,7 @@
#define INQUIRY_VENDOR_LEN 8
#define INQUIRY_MODEL_LEN 16
+#define INQUIRY_REVISION_LEN 4
/* Attempts before moving from SHORT to
[PATCH v4 5/7] target: add device vendor_id configfs attribute
The vendor_id attribute will allow for the modification of the T10
Vendor Identification string returned in inquiry responses. Its value
can be viewed and modified via the ConfigFS path at:
target/core/$backstore/$name/wwn/vendor_id
"LIO-ORG" remains the default value, which is set when the backstore
device is enabled.
Signed-off-by: David Disseldorp
---
drivers/target/target_core_configfs.c | 48 +++
1 file changed, 48 insertions(+)
diff --git a/drivers/target/target_core_configfs.c
b/drivers/target/target_core_configfs.c
index 9f49b1afd685..fc60c4db718d 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1213,6 +1213,52 @@ static struct t10_wwn *to_t10_wwn(struct config_item
*item)
}
/*
+ * STANDARD and VPD page 0x80 T10 Vendor Identification
+ */
+static ssize_t target_wwn_vendor_id_show(struct config_item *item,
+ char *page)
+{
+ return sprintf(page, "%s\n", _t10_wwn(item)->vendor[0]);
+}
+
+static ssize_t target_wwn_vendor_id_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ struct t10_wwn *t10_wwn = to_t10_wwn(item);
+ struct se_device *dev = t10_wwn->t10_dev;
+ unsigned char buf[INQUIRY_VENDOR_LEN + 1];
+
+ if (strlen(page) > INQUIRY_VENDOR_LEN) {
+ pr_err("Emulated T10 Vendor Identification exceeds"
+ " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
+ "\n");
+ return -EOVERFLOW;
+ }
+ strlcpy(buf, page, sizeof(buf));
+ /*
+* Check to see if any active exports exist. If they do exist, fail
+* here as changing this information on the fly (underneath the
+* initiator side OS dependent multipath code) could cause negative
+* effects.
+*/
+ if (dev->export_count) {
+ pr_err("Unable to set T10 Vendor Identification while"
+ " active %d exports exist\n", dev->export_count);
+ return -EINVAL;
+ }
+
+ /* Assume ASCII encoding. Strip any newline added from userspace. */
+ BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
+ strlcpy(dev->t10_wwn.vendor, strstrip(buf),
+ sizeof(dev->t10_wwn.vendor));
+
+ pr_debug("Target_Core_ConfigFS: Set emulated T10 Vendor Identification:"
+" %s\n", dev->t10_wwn.vendor);
+
+ return count;
+}
+
+/*
* VPD page 0x80 Unit serial
*/
static ssize_t target_wwn_vpd_unit_serial_show(struct config_item *item,
@@ -1358,6 +1404,7 @@ DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_target_port, 0x10);
/* VPD page 0x83 Association: SCSI Target Device */
DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_scsi_target_device, 0x20);
+CONFIGFS_ATTR(target_wwn_, vendor_id);
CONFIGFS_ATTR(target_wwn_, vpd_unit_serial);
CONFIGFS_ATTR_RO(target_wwn_, vpd_protocol_identifier);
CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
@@ -1365,6 +1412,7 @@ CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
+ _wwn_attr_vendor_id,
_wwn_attr_vpd_unit_serial,
_wwn_attr_vpd_protocol_identifier,
_wwn_attr_vpd_assoc_logical_unit,
--
2.13.7
[PATCH v4 2/7] target: consistently null-terminate t10_wwn.vendor
In preparation for supporting user provided vendor strings, add an extra
byte to t10_wwn.vendor which will ensure null string termination when an
eight byte vendor string is provided by the user.
Signed-off-by: David Disseldorp
---
drivers/target/target_core_device.c | 6 --
drivers/target/target_core_pscsi.c | 6 --
drivers/target/target_core_stat.c | 4 ++--
include/target/target_core_base.h | 8 +++-
4 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/target/target_core_device.c
b/drivers/target/target_core_device.c
index 47b5ef153135..fe4c4db51137 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -725,7 +725,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
/*
* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
*/
- for (i = 0; i < 8; i++)
+ for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
if (wwn->vendor[i] >= 0x20)
buf[i] = wwn->vendor[i];
else
@@ -1008,8 +1008,10 @@ int target_configure_device(struct se_device *dev)
* anything virtual (IBLOCK, FILEIO, RAMDISK), but not for TCM/pSCSI
* passthrough because this is being provided by the backend LLD.
*/
+ BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
- strncpy(>t10_wwn.vendor[0], "LIO-ORG", 8);
+ strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
+ dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
strncpy(>t10_wwn.model[0],
dev->transport->inquiry_prod, 16);
strncpy(>t10_wwn.revision[0],
diff --git a/drivers/target/target_core_pscsi.c
b/drivers/target/target_core_pscsi.c
index 47d76c862014..ee65b5bb674c 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -190,7 +190,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct
t10_wwn *wwn)
/*
* Use sdev->inquiry from drivers/scsi/scsi_scan.c:scsi_alloc_sdev()
*/
- memcpy(>vendor[0], [8], sizeof(wwn->vendor));
+ BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
+ memcpy(>vendor[0], [8], INQUIRY_VENDOR_LEN);
+ wwn->vendor[INQUIRY_VENDOR_LEN] = '\0';
memcpy(>model[0], [16], sizeof(wwn->model));
memcpy(>revision[0], [32], sizeof(wwn->revision));
}
@@ -826,7 +828,7 @@ static ssize_t pscsi_show_configfs_dev_params(struct
se_device *dev, char *b)
if (sd) {
bl += sprintf(b + bl, "");
bl += sprintf(b + bl, "Vendor: ");
- for (i = 0; i < 8; i++) {
+ for (i = 0; i < INQUIRY_VENDOR_LEN; i++) {
if (ISPRINT(sd->vendor[i])) /* printable character? */
bl += sprintf(b + bl, "%c", sd->vendor[i]);
else
diff --git a/drivers/target/target_core_stat.c
b/drivers/target/target_core_stat.c
index f0db91ebd735..4210cf625d84 100644
--- a/drivers/target/target_core_stat.c
+++ b/drivers/target/target_core_stat.c
@@ -247,10 +247,10 @@ static ssize_t target_stat_lu_vend_show(struct
config_item *item, char *page)
{
struct se_device *dev = to_stat_lu_dev(item);
int i;
- char str[sizeof(dev->t10_wwn.vendor)+1];
+ char str[INQUIRY_VENDOR_LEN+1];
/* scsiLuVendorId */
- for (i = 0; i < sizeof(dev->t10_wwn.vendor); i++)
+ for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
dev->t10_wwn.vendor[i] : ' ';
str[i] = '\0';
diff --git a/include/target/target_core_base.h
b/include/target/target_core_base.h
index e3bdb0550a59..cb1f3f574e2a 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -46,6 +46,8 @@
/* Used by transport_get_inquiry_vpd_device_ident() */
#define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN 254
+#define INQUIRY_VENDOR_LEN 8
+
/* Attempts before moving from SHORT to LONG */
#define PYX_TRANSPORT_WINDOW_CLOSED_THRESHOLD 3
#define PYX_TRANSPORT_WINDOW_CLOSED_WAIT_SHORT 3 /* In milliseconds */
@@ -314,7 +316,11 @@ struct t10_vpd {
};
struct t10_wwn {
- char vendor[8];
+ /*
+* SCSI left aligned strings may not be null terminated. +1 to ensure a
+* null terminator is always present.
+*/
+ char vendor[INQUIRY_VENDOR_LEN + 1];
char model[16];
char revision[4];
char unit_serial[INQUIRY_VPD_SERIAL_LEN];
--
2.13.7
[PATCH v4 0/7] target: user configurable T10 Vendor ID
This patchset allows for the modification of the T10 Vendor Identification string returned in the SCSI INQUIRY response, via the target/core/$backstore/$name/wwn/vendor_id ConfigFS path. Changes since v3: - perform explicit null termination of t10_wwn vendor, model and revision fields. - replace field dump for-loops Changes since v2: - https://www.spinics.net/lists/target-devel/msg10720.html - Support eight byte vendor ID strings - Split out consistent INQUIRY data padding as a separate patch - Drop t10_wwn.model buffer print fix, already upstream Changes since v1: - https://www.spinics.net/lists/target-devel/msg10545.html - Rebase against nab's for-next branch, which includes Christoph's configfs API changes. Bart Van Assche (1): target: use printf width specifier for t10_wwn field dumps David Disseldorp (6): target: use consistent left-aligned ASCII INQUIRY data target: consistently null-terminate t10_wwn.vendor target: consistently null-terminate t10_wwn.model target: consistently null-terminate t10_wwn.revision target: add device vendor_id configfs attribute target: remove hardcoded T10 Vendor ID in INQUIRY response drivers/target/target_core_configfs.c | 56 +-- drivers/target/target_core_device.c | 47 -- drivers/target/target_core_pscsi.c| 16 +--- drivers/target/target_core_spc.c | 20 +++--- drivers/target/target_core_stat.c | 32 --- include/target/target_core_base.h | 14 +-- 6 files changed, 114 insertions(+), 71 deletions(-)
[PATCH v4 1/7] target: use consistent left-aligned ASCII INQUIRY data
spc5r17.pdf specifies:
4.3.1 ASCII data field requirements
ASCII data fields shall contain only ASCII printable characters (i.e.,
code values 20h to 7Eh) and may be terminated with one or more ASCII
null (00h) characters.
ASCII data fields described as being left-aligned shall have any
unused bytes at the end of the field (i.e., highest offset) and the
unused bytes shall be filled with ASCII space characters (20h).
LIO currently space-pads the T10 VENDOR IDENTIFICATION and PRODUCT
IDENTIFICATION fields in the standard INQUIRY data. However, the
PRODUCT REVISION LEVEL field in the standard INQUIRY data as well as the
T10 VENDOR IDENTIFICATION field in the INQUIRY Device Identification VPD
Page are zero-terminated/zero-padded.
Fix this inconsistency by using space-padding for all of the above
fields.
Signed-off-by: David Disseldorp
Reviewed-by: Christoph Hellwig
---
drivers/target/target_core_spc.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index f459118bc11b..c37dd36ec77d 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -108,12 +108,17 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char
*buf)
buf[7] = 0x2; /* CmdQue=1 */
- memcpy([8], "LIO-ORG ", 8);
- memset([16], 0x20, 16);
+ /*
+* ASCII data fields described as being left-aligned shall have any
+* unused bytes at the end of the field (i.e., highest offset) and the
+* unused bytes shall be filled with ASCII space characters (20h).
+*/
+ memset([8], 0x20, 8 + 16 + 4);
+ memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
memcpy([16], dev->t10_wwn.model,
- min_t(size_t, strlen(dev->t10_wwn.model), 16));
+ strnlen(dev->t10_wwn.model, 16));
memcpy([32], dev->t10_wwn.revision,
- min_t(size_t, strlen(dev->t10_wwn.revision), 4));
+ strnlen(dev->t10_wwn.revision, 4));
buf[4] = 31; /* Set additional length to 31 */
return 0;
@@ -251,7 +256,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
buf[off] = 0x2; /* ASCII */
buf[off+1] = 0x1; /* T10 Vendor ID */
buf[off+2] = 0x0;
- memcpy([off+4], "LIO-ORG", 8);
+ /* left align Vendor ID and pad with spaces */
+ memset([off+4], 0x20, 8);
+ memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
/* Extra Byte for NULL Terminator */
id_len++;
/* Identifier Length */
--
2.13.7
Re: [PATCH 0/3] target: drop unneeded pi_prot_format and get_fabric_name()
David, > This patchset removes unneeded se_dev_attrib.pi_prot_format and > fabric_ops.get_fabric_name() struct members. Removal of the latter > allowed for further cleanup due to the fact that all fabric modules > except iscsi_target_mod provide matching strings for fabric_ops.name > and fabric_ops.get_fabric_name() - use a new fabric_ops.fabric_alias > member to handle the iscsi_target_mod special case. Applied to 4.21/scsi-queue. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 2/2] qla2xxx: Split the __qla2x00_abort_all_cmds() function
> On Nov 27, 2018, at 3:04 PM, Bart Van Assche wrote:
>
> External Email
>
> Nesting in __qla2x00_abort_all_cmds() is way too deep. Reduce the nesting
> level by introducing a helper function. This patch does not change any
> functionality.
>
> Cc: Himanshu Madhani
> Signed-off-by: Bart Van Assche
> ---
> drivers/scsi/qla2xxx/qla_os.c | 89 +--
> 1 file changed, 44 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index b658b9a5eb1e..247f16969f0f 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -1746,10 +1746,52 @@ qla2x00_loop_reset(scsi_qla_host_t *vha)
>return QLA_SUCCESS;
> }
>
> +static void qla2x00_abort_srb(struct qla_qpair *qp, srb_t *sp, const int res,
> + unsigned long *flags)
> + __releases(qp->qp_lock_ptr)
> + __acquires(qp->qp_lock_ptr)
> +{
> + scsi_qla_host_t *vha = qp->vha;
> + struct qla_hw_data *ha = vha->hw;
> + int status;
> +
> + if (sp->type == SRB_NVME_CMD || sp->type == SRB_NVME_LS) {
> + if (!sp_get(sp)) {
> + /* got sp */
> + spin_unlock_irqrestore(qp->qp_lock_ptr, *flags);
> + qla_nvme_abort(ha, sp, res);
> + spin_lock_irqsave(qp->qp_lock_ptr, *flags);
> + }
> + } else if (GET_CMD_SP(sp) && !ha->flags.eeh_busy &&
> + !test_bit(ABORT_ISP_ACTIVE, >dpc_flags) &&
> + !qla2x00_isp_reg_stat(ha) && sp->type == SRB_SCSI_CMD) {
> + /*
> +* Don't abort commands in adapter during EEH recovery as it's
> +* not accessible/responding.
> +*
> +* Get a reference to the sp and drop the lock. The reference
> +* ensures this sp->done() call and not the call in
> +* qla2xxx_eh_abort() ends the SCSI cmd (with result 'res').
> +*/
> + if (!sp_get(sp)) {
> + spin_unlock_irqrestore(qp->qp_lock_ptr, *flags);
> + status = qla2xxx_eh_abort(GET_CMD_SP(sp));
> + spin_lock_irqsave(qp->qp_lock_ptr, *flags);
> + /*
> +* Get rid of extra reference caused by early exit
> +* from qla2xxx_eh_abort().
> +*/
> + if (status == FAST_IO_FAIL)
> + atomic_dec(>ref_count);
> + }
> + }
> + sp->done(sp, res);
> +}
> +
> static void
> __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
> {
> - int cnt, status;
> + int cnt;
>unsigned long flags;
>srb_t *sp;
>scsi_qla_host_t *vha = qp->vha;
> @@ -1768,50 +1810,7 @@ __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
>req->outstanding_cmds[cnt] = NULL;
>switch (sp->cmd_type) {
>case TYPE_SRB:
> - if (sp->type == SRB_NVME_CMD ||
> - sp->type == SRB_NVME_LS) {
> - if (!sp_get(sp)) {
> - /* got sp */
> - spin_unlock_irqrestore
> - (qp->qp_lock_ptr,
> -flags);
> - qla_nvme_abort(ha, sp, res);
> - spin_lock_irqsave
> - (qp->qp_lock_ptr,
> flags);
> - }
> - } else if (GET_CMD_SP(sp) &&
> - !ha->flags.eeh_busy &&
> - (!test_bit(ABORT_ISP_ACTIVE,
> - >dpc_flags)) &&
> - !qla2x00_isp_reg_stat(ha) &&
> - (sp->type == SRB_SCSI_CMD)) {
> - /*
> -* Don't abort commands in adapter
> -* during EEH recovery as it's not
> -* accessible/responding.
> -*
> -* Get a reference to the sp and drop
> -* the lock. The reference ensures
> this
> -* sp->done() call and not the call in
> -* qla2xxx_eh_abort() ends the SCSI
> cmd
> -* (with result 'res').
> -
Re: [PATCH 1/2] qla2xxx: Introduce a switch/case statement in qlt_xmit_tm_rsp()
> On Nov 27, 2018, at 3:04 PM, Bart Van Assche wrote:
>
> External Email
>
> This patch improves code readability but does not change any
> functionality.
>
> Cc: Himanshu Madhani
> Signed-off-by: Bart Van Assche
> ---
> drivers/scsi/qla2xxx/qla_target.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_target.c
> b/drivers/scsi/qla2xxx/qla_target.c
> index c4504740f0e2..802007a7b21c 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -2379,20 +2379,20 @@ void qlt_xmit_tm_rsp(struct qla_tgt_mgmt_cmd *mcmd)
>}
>
>if (mcmd->flags == QLA24XX_MGMT_SEND_NACK) {
> - if (mcmd->orig_iocb.imm_ntfy.u.isp24.status_subcode ==
> - ELS_LOGO ||
> - mcmd->orig_iocb.imm_ntfy.u.isp24.status_subcode ==
> - ELS_PRLO ||
> - mcmd->orig_iocb.imm_ntfy.u.isp24.status_subcode ==
> - ELS_TPRLO) {
> + switch (mcmd->orig_iocb.imm_ntfy.u.isp24.status_subcode) {
> + case ELS_LOGO:
> + case ELS_PRLO:
> + case ELS_TPRLO:
>ql_dbg(ql_dbg_disc, vha, 0x2106,
>"TM response logo %phC status %#x state %#x",
>mcmd->sess->port_name, mcmd->fc_tm_rsp,
>mcmd->flags);
>qlt_schedule_sess_for_deletion(mcmd->sess);
> - } else {
> + break;
> + default:
>qlt_send_notify_ack(vha->hw->base_qpair,
>>orig_iocb.imm_ntfy, 0, 0, 0, 0, 0, 0);
> + break;
>}
>} else {
>if (mcmd->orig_iocb.atio.u.raw.entry_type == ABTS_RECV_24XX) {
> --
> 2.20.0.rc0.387.gc7a69e6b6c-goog
>
Looks Good.
Acked-by: Himanshu Madhani
Thanks,
- Himanshu
Re: [PATCH] scsi: lpfc: fix block guard enablement on SLI3 adapters
Martin, > Since f44ac12f1dcc, BG enablement is tracked with the > LPFC_SLI3_BG_ENABLED bit, which is set in lpfc_get_cfgparam before > lpfc_sli_config_sli_port() is called. The bit shouldn't be cleared > before checking the feature. Based on problem analysis by David Bond. Applied to 4.20/scsi-fixes, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute
On Wed, 2018-11-28 at 17:44 +0100, David Disseldorp wrote: > Hi Bart, > > On Wed, 28 Nov 2018 08:36:19 -0800, Bart Van Assche wrote: > > > Maybe I'm missing something, but why is zeroing of unused bytes in these > > functions > > necessary? Would the following be correct if all strings in struct t10_wwn > > would be > > '\0'-terminated? > > Your patch looks good to me. Mind if I tack it on to the end of my > t10_wwn.vendor/model/revision[size+1] patchset, with your authorship? Sure, that sounds fine to me. Bart.
Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute
Hi Bart, On Wed, 28 Nov 2018 08:36:19 -0800, Bart Van Assche wrote: > Maybe I'm missing something, but why is zeroing of unused bytes in these > functions > necessary? Would the following be correct if all strings in struct t10_wwn > would be > '\0'-terminated? Your patch looks good to me. Mind if I tack it on to the end of my t10_wwn.vendor/model/revision[size+1] patchset, with your authorship? Cheers, David
Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute
On Wed, 2018-11-28 at 17:28 +0100, David Disseldorp wrote:
> On Wed, 28 Nov 2018 08:08:30 -0800, Bart Van Assche wrote:
>
> > > Just a follow up here. I think it's safer to retain strncpy() in this
> > > function for the purpose of zero filling. scsi_dump_inquiry() and
> > > target_stat_lu_vend_show() walk the entire length of the t10_wwn.vendor
> > > buffer.
> >
> > How about adding a comment about above struct t10_wwn that vendor[], model[]
> > and revision[] in that structure may but do not have to be terminated with a
> > trailing '\0' and also that unit_serial[] is '\0'-terminated?
> >
> > It would make me happy if the size of the character arrays in that structure
> > would be increased by one and if the target code would be modified such that
> > these strings are always '\0'-terminated.
>
> I'm working on the +1 length increase for those t10_wwn members ATM, but
> just thought I'd mention that the zeroing of unused bytes is still
> required due to the scsi_dump_inquiry() + target_stat_lu_vend_show()
> behaviour.
Hi David,
Maybe I'm missing something, but why is zeroing of unused bytes in these
functions
necessary? Would the following be correct if all strings in struct t10_wwn
would be
'\0'-terminated?
static ssize_t target_stat_lu_vend_show(struct config_item *item, char *page)
{
struct se_device *dev = to_stat_lu_dev(item);
- int i;
- char str[sizeof(dev->t10_wwn.vendor)+1];
- /* scsiLuVendorId */
- for (i = 0; i < sizeof(dev->t10_wwn.vendor); i++)
- str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
- dev->t10_wwn.vendor[i] : ' ';
- str[i] = '\0';
- return snprintf(page, PAGE_SIZE, "%s\n", str);
+ return snprintf(page, PAGE_SIZE, "%-16s\n", dev->t10_wwn.vendor);
}
[ ... ]
static void scsi_dump_inquiry(struct se_device *dev)
{
struct t10_wwn *wwn = >t10_wwn;
- char buf[17];
- int i, device_type;
+ int device_type = dev->transport->get_device_type(dev);
+
/*
* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
*/
- for (i = 0; i < 8; i++)
- if (wwn->vendor[i] >= 0x20)
- buf[i] = wwn->vendor[i];
- else
- buf[i] = ' ';
- buf[i] = '\0';
- pr_debug(" Vendor: %s\n", buf);
-
- for (i = 0; i < 16; i++)
- if (wwn->model[i] >= 0x20)
- buf[i] = wwn->model[i];
- else
- buf[i] = ' ';
- buf[i] = '\0';
- pr_debug(" Model: %s\n", buf);
-
- for (i = 0; i < 4; i++)
- if (wwn->revision[i] >= 0x20)
- buf[i] = wwn->revision[i];
- else
- buf[i] = ' ';
- buf[i] = '\0';
- pr_debug(" Revision: %s\n", buf);
-
- device_type = dev->transport->get_device_type(dev);
+ pr_debug(" Vendor: %-8s\n", wwn->vendor);
+ pr_debug(" Model: %-16s\n", wwn->model);
+ pr_debug(" Revision: %-4s\n", wwn->revision);
pr_debug(" Type: %s ", scsi_device_type(device_type));
}
Thanks,
Bart.
Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute
On Wed, 28 Nov 2018 08:08:30 -0800, Bart Van Assche wrote: > > Just a follow up here. I think it's safer to retain strncpy() in this > > function for the purpose of zero filling. scsi_dump_inquiry() and > > target_stat_lu_vend_show() walk the entire length of the t10_wwn.vendor > > buffer. > > How about adding a comment about above struct t10_wwn that vendor[], model[] > and revision[] in that structure may but do not have to be terminated with a > trailing '\0' and also that unit_serial[] is '\0'-terminated? > > It would make me happy if the size of the character arrays in that structure > would be increased by one and if the target code would be modified such that > these strings are always '\0'-terminated. I'm working on the +1 length increase for those t10_wwn members ATM, but just thought I'd mention that the zeroing of unused bytes is still required due to the scsi_dump_inquiry() + target_stat_lu_vend_show() behaviour. Cheers, David
Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute
On Wed, 2018-11-28 at 16:37 +0100, David Disseldorp wrote: > On Tue, 20 Nov 2018 19:00:57 +0100, David Disseldorp wrote: > > > > > + strncpy(buf, page, sizeof(buf)); > > > > > > Isn't strncpy() deprecated? How about using strlcpy() instead? > > > > Will change to use strlcpy in the next round. > > Just a follow up here. I think it's safer to retain strncpy() in this > function for the purpose of zero filling. scsi_dump_inquiry() and > target_stat_lu_vend_show() walk the entire length of the t10_wwn.vendor > buffer. How about adding a comment about above struct t10_wwn that vendor[], model[] and revision[] in that structure may but do not have to be terminated with a trailing '\0' and also that unit_serial[] is '\0'-terminated? It would make me happy if the size of the character arrays in that structure would be increased by one and if the target code would be modified such that these strings are always '\0'-terminated. Thanks, Bart.
Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute
On Tue, 20 Nov 2018 19:00:57 +0100, David Disseldorp wrote: > > > + strncpy(buf, page, sizeof(buf)); > > > > Isn't strncpy() deprecated? How about using strlcpy() instead? > > Will change to use strlcpy in the next round. Just a follow up here. I think it's safer to retain strncpy() in this function for the purpose of zero filling. scsi_dump_inquiry() and target_stat_lu_vend_show() walk the entire length of the t10_wwn.vendor buffer. Cheers, David
Hello My Beloved One
Assalamu alaikum, I came across your e-mail contact prior a private search while in need of a trusted person. My name is Mrs. Aisha Gaddafi, a single Mother and a Widow with three Children. I am the only biological Daughter of late Libyan President (Late Colonel Muammar Gaddafi)I have a business Proposal for you worth $5.8Million dollars and I need mutual respect, trust, honesty, transparency, adequate support and assistance, Hope to hear from you for more details. Warmest regards Mrs Aisha Gaddafi
