Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
On 12/5/18 6:04 AM, Martin K. Petersen wrote: Since the return value of this function is 'u32', can the ' & 0x' be left out? Absolutely, and I almost zapped it. However, I decided to leave it to emphasize the point that the reference tag is truncated to a 32-bit value. To me, this is more obvious than having to backtrack and spot the u32 in the function definition. I generally appreciate some sort of commentary around a return statement if the value deviates from the ordinary. The parentheses around the shift value irk me but had to leave those in place to silence gcc. Hi Martin, Had you considered to use lower_32_bits() instead of "0x"? That would to avoid that reviewers have to count the 'f'-s to verify correctness of t10_pi_ref_tag(). Thanks, Bart.
Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
On 12/4/18 6:31 PM, Martin K. Petersen wrote:
Commit ddd0bc756983 ("block: move ref_tag calculation func to the block
layer") moved ref tag calculation from SCSI to a library function. However,
this change broke returning the correct ref tag for devices operating in
DIF mode since these do not have an associated block integrity profile.
This in turn caused read/write failures on PI-formatted disks attached to
an mpt3sas controller.
Fixes: ddd0bc756983 ("block: move ref_tag calculation func to the block layer")
Cc: sta...@vger.kernel.org # 4.19+
Reported-by: John Garry
Signed-off-by: Martin K. Petersen
---
include/linux/t10-pi.h | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/include/linux/t10-pi.h b/include/linux/t10-pi.h
index b9626aa7e90c..3e2a80cc7b56 100644
--- a/include/linux/t10-pi.h
+++ b/include/linux/t10-pi.h
@@ -39,12 +39,13 @@ struct t10_pi_tuple {
static inline u32 t10_pi_ref_tag(struct request *rq)
{
+ unsigned int shift = ilog2(queue_logical_block_size(rq->q));
+
#ifdef CONFIG_BLK_DEV_INTEGRITY
- return blk_rq_pos(rq) >>
- (rq->q->integrity.interval_exp - 9) & 0x;
-#else
- return -1U;
+ if (rq->q->integrity.interval_exp)
+ shift = rq->q->integrity.interval_exp;
#endif
+ return blk_rq_pos(rq) >> (shift - SECTOR_SHIFT) & 0x;
}
Since the return value of this function is 'u32', can the ' &
0x' be left out?
Thanks,
Bart.
Re: [PATCH v6 3/5] target: add device vendor_id configfs attribute
On Tue, 2018-12-04 at 16:26 +0300, Roman Bolshakov wrote: > wrt PATCH 5 in the series. Should we allow to set vendor_id for for > pscsi? I think we should allow that. Bart.
Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
On Sat, 2018-12-01 at 15:59 +0100, Hannes Reinecke wrote:
> On 12/1/18 12:34 AM, David Disseldorp wrote:
> > Initialise the t10_wwn vendor, model and revision defaults when a
> > device is allocated instead of when it's enabled. This ensures that
> > custom vendor or model strings set prior to enablement are not later
> > overwritten with default values.
> >
> > Signed-off-by: David Disseldorp
> > ---
> > drivers/target/target_core_device.c | 34
> > +-
> > 1 file changed, 17 insertions(+), 17 deletions(-)
> > > diff --git a/drivers/target/target_core_device.c
>
> b/drivers/target/target_core_device.c
> > index 5512871f50e4..6318d59a1564 100644
> > --- a/drivers/target/target_core_device.c
> > +++ b/drivers/target/target_core_device.c
> > @@ -810,6 +810,23 @@ struct se_device *target_alloc_device(struct se_hba
> > *hba, const char *name)
> > mutex_init(_lun->lun_tg_pt_md_mutex);
> > xcopy_lun->lun_tpg = _pt_tpg;
> >
> > + /*
> > +* Preload the initial INQUIRY const values if we are doing
> > +* 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);
> > + 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)) {
> > + strlcpy(dev->t10_wwn.vendor, "LIO-ORG",
> > + sizeof(dev->t10_wwn.vendor));
> > + strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
> > + sizeof(dev->t10_wwn.model));
> > + strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
> > + sizeof(dev->t10_wwn.revision));
> > + }
> > +
> > return dev;
> > }
> >
>
> This is odd. I'd rather have it consistent across backends, ie either
> move the initialisation into the backends, or provide a means to check
> if the inquiry data has already been pre-filled.
> But this check really is awkward.
Agreed. I also would like to see that that if-condition is removed ...
Thanks,
Bart.
Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
On Fri, 2018-11-30 at 15:41 +0100, David Disseldorp wrote: > On Fri, 30 Nov 2018 14:17:49 +0100, David Disseldorp wrote: > > > > Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I > > > perhaps overlook something? > > > > This is done in target_configure_device() . > > Hmm, continuing to do it there may cause a bit of confusion if vendor_id > is set before the backstore is enabled, which would cause it to be > overwritten. That said, we already have similarly strange behaviour for > emulate_model_alias / product. E.g.: > > rapido1:/# cd /sys/kernel/config/target/ > rapido1:target# mkdir -p core/fileio_1/testing > rapido1:target# echo -n "fd_dev_name=/asdf,fd_dev_size=4096" > > core/fileio_1/testing/control > rapido1:target# echo -n "testing1" > core/fileio_1/testing/wwn/vendor_id > rapido1:target# cat core/fileio_1/testing/wwn/vendor_id > testing1 > rapido1:target# echo 1 > ./core/fileio_1/testing/attrib/emulate_model_alias > rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod > testing > rapido1:target# echo 1 > core/fileio_1/testing/enable > rapido1:target# cat core/fileio_1/testing/wwn/vendor_id > LIO-ORG > rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod > FILEIO > rapido1:target# cat ./core/fileio_1/testing/attrib/emulate_model_alias > 1 > > Not sure how best to handle this... > 1. move vendor/model/rev initialization into target_alloc_device() > 2. move vendor (only) initialization into target_alloc_device() > 3. fail attempts to set emulate_model_alias or vendor_id before the >backstore has been enabled > 4. leave as-is > > (1) would IMO be the most straightforward, but it's a slight change to > the existing (IMO broken) emulate_model_alias user interface. I'm in favor of moving some of the target_configure_device() code into the target_alloc_device() function. Today target_configure_device() overwrites the vendor, model and revision string and is called when "1" is written into the "enable" attribute. Overwriting these attributes when enabling a backstore seems wrong to me. Thanks, Bart.
Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
On Thu, 2018-11-29 at 21:31 +0100, David Disseldorp wrote: > On Thu, 29 Nov 2018 08:24:38 -0800, Bart Van Assche wrote: > > On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote: > > > [ ... ] > > Additionally, have you considered to use strlcpy() > > instead of snprintf()? > > Happy to change the logic below over if you find it easier to follow. It would make the code shorter without hurting readability, so I think it would be better to use strlcpy(). But it's not that important to me. Bart.
[PATCH v2] qla2xxx: Split the __qla2x00_abort_all_cmds() function
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.
Reviewed-by: Laurence Oberman
Acked-by: Himanshu Madhani
Signed-off-by: Bart Van Assche
---
Changes in v2 compared to v1: rebased on top of Martin's 4.21/scsi-queue
branch.
drivers/scsi/qla2xxx/qla_os.c | 74 +--
1 file changed, 36 insertions(+), 38 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 518f15141170..4a75e0572121 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -1746,6 +1746,41 @@ 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;
+
+ 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);
+ qla2xxx_eh_abort(GET_CMD_SP(sp));
+ spin_lock_irqsave(qp->qp_lock_ptr, *flags);
+ }
+ }
+ sp->done(sp, res);
+}
+
static void
__qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
{
@@ -1768,44 +1803,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').
-*/
- if (!sp_get(sp)) {
- spin_unlock_irqrestore
- (qp->qp_lock_ptr,
flags);
- qla2xxx_eh_abort(
- GET_CMD_SP(sp));
-
Re: [PATCH v4 7/7] target: use printf width specifier for t10_wwn field dumps
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote: > The existing for loops step over null-terminators for right-padding. > Padding can be achieved in a much simpler way using printf width > specifiers. How about squashing patches 2, 3, 4 and 7 into a single patch? I think that would make the entire patch series easier to review. Thanks, Bart.
Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
On Thu, 2018-11-29 at 02:01 +0100, 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 */
Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I
perhaps overlook something? Additionally, why are you using strnlen() for
a string of which it is guaranteed that its length is less than or equal to
the second strnlen() argument?
Thanks,
Bart.
Re: [PATCH v4 5/7] target: add device vendor_id configfs attribute
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote:
> +static ssize_t target_wwn_vendor_id_show(struct config_item *item,
> + char *page)
> +{
> + return sprintf(page, "%s\n", _t10_wwn(item)->vendor[0]);
> +}
The "&" and "[0]" are superfluous in the above sprintf() statement.
> +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;
> + }
Does this force users to use "echo -n" to configure vendor IDs that are
INQUIRY_VENDOR_LEN bytes long?
Bart.
Re: [PATCH v4 4/7] target: consistently null-terminate t10_wwn.revision
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote: > 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'; Can the above two statements be changed into a single strlcpy() call? > - memcpy(>revision[0], [32], sizeof(wwn->revision)); > + memcpy(>revision[0], [32], INQUIRY_REVISION_LEN); > + wwn->revision[INQUIRY_REVISION_LEN] = '\0'; Have you considered to use snprintf(..., "%.*s", ...) instead? Thanks, Bart.
Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote:
> 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);
Both the old and the new statement truncate inquiry strings that are 16 bytes
long, which is a bug. Additionally, have you considered to use strlcpy()
instead of snprintf()?
> 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';
Have you considered to use strlcpy() instead of strncpy() followed by explicit
'\0'-termination?
> 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';
Same question here: have you considered to use strlcpy() instead of strncpy()
followed by explicit '\0'-termination?
> - 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';
Can the memcpy() and '\0'-termination be changed into an snprintf(..., "%.*s",
...)
call?
Thanks,
Bart.
Re: [PATCH v4 2/7] target: consistently null-terminate t10_wwn.vendor
On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote: > - 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'; This looks weird to me. Have you considered to use strlcpy() instead of strncpy() and explicit '\0'-termination? > 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'; Have you considered to use snprintf(..., "%.*s", ...) instead of memcpy() followed by explicit '\0'-termination? I think that would result in code that is easier to read. strlcpy() can't be used here because it is not guaranteed that the input buffer is '\0'-terminated. Thanks, Bart.
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
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, 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.
[PATCH 0/2] Two refactoring patches for the qla2xxx driver
Hi Martin, 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. Thanks, Bart. Bart Van Assche (2): qla2xxx: Introduce a switch/case statement in qlt_xmit_tm_rsp() qla2xxx: Split the __qla2x00_abort_all_cmds() function drivers/scsi/qla2xxx/qla_os.c | 89 +++ drivers/scsi/qla2xxx/qla_target.c | 14 ++--- 2 files changed, 51 insertions(+), 52 deletions(-) -- 2.20.0.rc0.387.gc7a69e6b6c-goog
[PATCH 1/2] qla2xxx: Introduce a switch/case statement in qlt_xmit_tm_rsp()
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
[PATCH 2/2] qla2xxx: Split the __qla2x00_abort_all_cmds() function
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').
-*/
- if (!sp_get(sp)) {
-
Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute
On Tue, 2018-11-20 at 19:00 +0100, David Disseldorp wrote: > I tend to agree that it's dangerous, but chose to stay somewhat > consistent with the other t10_wwn strings that are treated as though > they may not be NULL terminated. > > If you're in favour adding an extra terminator byte here, then I think > it'd make sense to do the same for model[], revision[] and unit_serial[] > too. Are you okay with that approach? Sure, I would welcome that change. Thanks, Bart.
Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute
On Mon, 2018-11-19 at 22:06 +0100, David Disseldorp wrote:
> /*
> + * 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, "T10 Vendor Identification: %."
> +__stringify(INQUIRY_VENDOR_IDENTIFIER_LEN) "s\n",
> +_t10_wwn(item)->vendor[0]);
> +}
This doesn't follow the convention used by other configfs attributes,
namely that only the value should be reported and no prefix. Please leave
out the "T10 Vendor Identification: " prefix.
> +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;
> + /* +1 to ensure buf is zero terminated for stripping */
> + unsigned char buf[INQUIRY_VENDOR_IDENTIFIER_LEN + 1];
> +
> + if (strlen(page) > INQUIRY_VENDOR_IDENTIFIER_LEN) {
> + pr_err("Emulated T10 Vendor Identification exceeds"
> + " INQUIRY_VENDOR_IDENTIFIER_LEN: %d\n",
> + INQUIRY_VENDOR_IDENTIFIER_LEN);
> + return -EOVERFLOW;
> + }
Trailing newline(s) should be stripped before the length check is performed. I
don't think that you want to force users to use "echo -n" instead of "echo" when
setting this attribute.
> + strncpy(buf, page, sizeof(buf));
Isn't strncpy() deprecated? How about using strlcpy() instead?
> + /*
> + * Check to see if any active $FABRIC_MOD 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 $FABRIC_MOD exports exist\n",
> + dev->export_count);
> + return -EINVAL;
> + }
Are there any users who understand what "$FABRIC_MOD" means? Please leave out
that
string or change it into the name of the fabric driver followed by the name of
the
target port associated with 'item'.
> +
> + /*
> + * Assume ASCII encoding. Strip any newline added from userspace.
> + * The result may *not* be null terminated.
> + */
> + strncpy(dev->t10_wwn.vendor, strstrip(buf),
> + INQUIRY_VENDOR_IDENTIFIER_LEN);
Keeping strings around that are not '\0'-terminated is a booby trap. It is very
easy for anyone who modifies or reviews code that uses such strings to overlook
that the string is not '\0'-terminated. Please increase the size of the vendor[]
array by one and make sure that that string is '\0'-terminated.
Thanks,
Bart.
Re: [PATCH v3 1/4] target: use consistent left-aligned ASCII INQUIRY data
On Mon, 2018-11-19 at 22:06 +0100, David Disseldorp wrote:
> 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 */
A helper function that accepts a '\0'-terminated string and a field length as
input and that copies the input string and pads it with spaces would be welcome.
>From the SCST source code:
/*
* 8 byte ASCII Vendor Identification of the target
* - left aligned.
*/
scst_copy_and_fill([8], virt_dev->t10_vend_id, 8);
/*
* 16 byte ASCII Product Identification of the target - left
* aligned.
*/
scst_copy_and_fill([16], virt_dev->prod_id, 16);
/*
* 4 byte ASCII Product Revision Level of the target - left
* aligned.
*/
scst_copy_and_fill([32], virt_dev->prod_rev_lvl, 4);
/*
* Copy a zero-terminated string into a fixed-size byte array and fill the
* trailing bytes with @fill_byte.
*/
static void scst_copy_and_fill_b(char *dst, const char *src, int len,
uint8_t fill_byte)
{
int cpy_len = min_t(int, strlen(src), len);
memcpy(dst, src, cpy_len);
memset(dst + cpy_len, fill_byte, len - cpy_len);
}
/*
* Copy a zero-terminated string into a fixed-size char array and fill the
* trailing characters with spaces.
*/
static void scst_copy_and_fill(char *dst, const char *src, int len)
{
scst_copy_and_fill_b(dst, src, len, ' ');
}
Bart.
Re: [PATCH] qla2xxx: Add SysFS hook for FC-NVMe autoconnect
On Tue, 2018-11-13 at 17:38 +, Madhani, Himanshu wrote: > On Nov 13, 2018, at 6:23 AM, Bart Van Assche wrote: > > On Tue, 2018-11-13 at 01:02 +, Madhani, Himanshu wrote: > > > I see other drivers also use similar information populated for NVMe > > > Connection at boot time. > > > > Hi Himanshu, > > > > Which other drivers are you referring to? > > > > Thanks, > > > > Bart. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/dri > vers/scsi/lpfc/lpfc_attr.c Hello James, Please either move the information exported by lpfc_nvme_info_show() into debugfs or split the information exported by that function into multiple sysfs attributes. Otherwise you will get flamed by Greg KH as soon as he encounters that code because of not following the rules explained in Documentation/filesystems/sysfs.txt. Bart.
Re: [PATCH] qla2xxx: Add SysFS hook for FC-NVMe autoconnect
On Tue, 2018-11-13 at 01:02 +, Madhani, Himanshu wrote: > I see other drivers also use similar information populated for NVMe > Connection at boot time. Hi Himanshu, Which other drivers are you referring to? Thanks, Bart.
Re: [PATCH] qla2xxx: Add SysFS hook for FC-NVMe autoconnect
On Mon, 2018-11-12 at 13:40 -0800, Himanshu Madhani wrote:
> diff --git a/drivers/scsi/qla2xxx/qla_attr.c
> b/drivers/scsi/qla2xxx/qla_attr.c
> index 678aff5ca947..323a4aa35f16 100644
> --- a/drivers/scsi/qla2xxx/qla_attr.c
> +++ b/drivers/scsi/qla2xxx/qla_attr.c
> @@ -1665,6 +1665,125 @@ qla2x00_max_speed_sup_show(struct device *dev,
> struct device_attribute *attr,
> ha->max_speed_sup ? "32Gps" : "16Gps");
> }
>
> +static ssize_t
> +qla27xx_nvme_connect_str_show(struct device *dev, struct device_attribute
> *attr,
> +char *buf)
> +{
> + scsi_qla_host_t *vha = shost_priv(class_to_shost(dev));
> + struct nvme_fc_remote_port *rport;
> + struct nvme_fc_local_port *lport;
> + struct qla_hw_data *ha = vha->hw;
> + struct qla_nvme_rport *qla_rport, *trport;
> + fc_port_t *fcport;
> + char temp[150] = {0};
> + char *rportstate = "";
> +
> + if (!IS_QLA27XX(ha) && !IS_QLA28XX(ha))
> + return scnprintf(buf, PAGE_SIZE, "\n");
> +
> + if (!vha->flags.nvme_enabled)
> + return scnprintf(buf, PAGE_SIZE, "%s\n",
> + "FC-NVMe is not enabled");
> +
> + list_for_each_entry(fcport, >vp_fcports, list) {
> + if (!fcport) {
> + scnprintf(buf, PAGE_SIZE, "No FC host\n");
> + return strlen(buf);
> + }
> +
> + if (!vha->nvme_local_port) {
> + scnprintf(buf, PAGE_SIZE,
> + "FC-NVMe Initiator on 0x%16llx not
> registered.\n",
> + wwn_to_u64(fcport->port_name));
> + return strlen(buf);
> + }
> +
> + list_for_each_entry_safe(qla_rport, trport,
> + >nvme_rport_list, list) {
> + if (qla_rport->fcport == fcport) {
> + rport = fcport->nvme_remote_port;
> +
> + lport = vha->nvme_local_port;
> +
> + scnprintf(temp, sizeof(temp),
> + "FC-NVMe LPORT: host%ld nn-
> 0x%16llx:pn-0x%16llx port_id %06x %s\n",
> + vha->host_no, lport->node_name,
> + lport->port_name, lport->port_id,
> "ONLINE");
> +
> + if (strlcat(buf, temp, PAGE_SIZE) >=
> PAGE_SIZE)
> + goto done;
> +
> + scnprintf(temp, sizeof(temp),
> + "FC-NVMe RPORT: host%ld nn-0x%llx:pn-
> 0x%llx port_id %06x ",
> + vha->host_no, rport->node_name,
> + rport->port_name, rport->port_id);
> +
> + /* Find out Rport State */
> + if (rport->port_state &
> FC_OBJSTATE_ONLINE)
> + rportstate = "ONLINE";
> +
> + if (rport->port_state &
> FC_OBJSTATE_UNKNOWN)
> + rportstate = "UNKNOWN";
> +
> + if (rport->port_state &
> ~(FC_OBJSTATE_ONLINE |
> + FC_OBJSTATE_UNKNOWN))
> + rportstate = "UNSUPPORTED";
> +
> + if (strlcat(buf, temp, PAGE_SIZE) >=
> + PAGE_SIZE)
> + goto done;
> +
> + if (rport->port_role &
> + (FC_PORT_ROLE_NVME_INITIATOR |
> + FC_PORT_ROLE_NVME_TARGET |
> + FC_PORT_ROLE_NVME_DISCOVERY)) {
> + if (rport->port_role &
> + FC_PORT_ROLE_NVME_INITIATOR)
> + if (strlcat(buf,
> "INITIATOR ",
> + PAGE_SIZE) >=
> PAGE_SIZE)
> + goto done;
> +
> + if (rport->port_role &
> + FC_PORT_ROLE_NVME_TARGET)
> + if (strlcat(buf, "TARGET
> ",
> + PAGE_SIZE) >=
> PAGE_SIZE)
> + goto done;
> +
> + if (rport->port_role &
> + FC_PORT_ROLE_NVME_DISCOVERY)
> + if (strlcat(buf,
> "DISCOVERY ",
> + PAGE_SIZE) >=
> PAGE_SIZE)
> + goto done;
> + } else {
> + if (strlcat(buf, "UNKNOWN_ROLE ",
> + PAGE_SIZE) >=
Re: [PATCH 0/3] sd: Rely on the driver core for asynchronous probing
On 10/28/18 7:04 AM, Laurence Oberman wrote:
I ran multiple tests for two days using the series from Bart's tree on
the SRP test bed here.
Multiple disconnects/reconnects during heavy IO activity on 32 SRP 64
LUNS/ 32 mpaths with no issues seen with probes and reconnects.
For the series.
Tested-by: Laurence Oberman
Thanks Laurence for the testing!
In case this would not be clear, Laurence tested commit 55d2bab790e2
("sd: Inline sd_probe_part2()") from my github repository.
Bart.
Re: [PATCH 0/3] sd: Rely on the driver core for asynchronous probing
On Fri, 2018-10-26 at 15:58 +0200, Hannes Reinecke wrote: > On 10/2/18 11:52 PM, Bart Van Assche wrote: > > Hello Martin, > > > > During the 2018 edition of LSF/MM there was a session about increasing SCSI > > disk probing concurrency. This patch series implements what has been > > proposed > > during that session, namely: > > - Make sure that the driver core is aware of asynchronous SCSI LUN probing. > > - Avoid unnecessary serialization between sd_probe() and sd_remove() because > >this could lead to a deadlock. > > > > Please consider this patch series for kernel v4.20. > > > > Thanks, > > > > Bart. > > > > Bart Van Assche (3): > >sd: Rely on the driver core for asynchronous probing > >sd: Inline sd_probe_part2() > >__device_release_driver(): Do not wait for asynchronous probing > > > > drivers/base/dd.c| 3 -- > > drivers/scsi/scsi.c | 14 - > > drivers/scsi/scsi_pm.c | 22 +--- > > drivers/scsi/scsi_priv.h | 3 -- > > drivers/scsi/sd.c| 110 --- > > 5 files changed, 46 insertions(+), 106 deletions(-) > > > > Initially it looks ok-ish. > But I've has so many issues with asynchronous probing that I'd be really > careful here. > However, inlining sd_probe_async() should amount for some contention to > be removed; but I'd rather give it some more testing here. > I've found that testing on large configurations on iSER or SRP tend to > excite the issues. > Can you check against those, too? > I'll see to give it a spin on my test bed. Hi Hannes, As you may have noticed the build bot was not very happy with the patches in v2 of this series for the device driver core. Additional testing is welcome but please start from the patches in the following branch for any tests: https://github.com/bvanassche/linux/tree/for-next. On that branch my patches for the device driver core have been replaced with patches from Alexander Duyck. Thanks, Bart.
Re: [PATCH 5/8] sg: add free list, rework locking
On Fri, 2018-10-19 at 15:55 -0400, Douglas Gilbert wrote:
> On 2018-10-19 11:22 a.m., Bart Van Assche wrote:
> > On Fri, 2018-10-19 at 02:24 -0400, Douglas Gilbert wrote:
> > > static void
> > > -sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo)
> > > +sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo,
> > > + int max_num)
> > > {
> > > struct sg_request *srp;
> > > int val;
> > > - unsigned int ms;
> > >
> > > val = 0;
> > > - list_for_each_entry(srp, >rq_list, entry) {
> > > - if (val >= SG_MAX_QUEUE)
> > > - break;
> > > - rinfo[val].req_state = srp->done + 1;
> > > + list_for_each_entry(srp, >rq_list, rq_entry) {
> > > + if (val >= max_num)
> > > + return;
> >
> > What protects the sfp->rq_list against concurrent changes? It seems to me
> > like all other code that iterates over or modifies that list protects that
> > list with rq_list_lock?
>
> Bart,
> The function is called from sg_ioctl() case SG_GET_REQUEST_TABLE and at the
> call point the read_lock is held on rq_list_lock. Maybe I can add a comment
> above the function about the lock being held. [At least it is as the end
> of the patch series, and that is all I care about :-)]
Hi Doug,
Thanks for the clarification. How about adding a __must_hold() annotation?
Bart.
Re: [PATCH 1/5] ips: use lower_32_bits and upper_32_bits instead of reinventing them
On Thu, 2018-10-18 at 15:03 +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig
> ---
> drivers/scsi/ips.c | 6 +++---
> drivers/scsi/ips.h | 3 ---
> 2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
> index ee8a1ecd58fd..679321e96a86 100644
> --- a/drivers/scsi/ips.c
> +++ b/drivers/scsi/ips.c
> @@ -1801,13 +1801,13 @@ ips_fill_scb_sg_single(ips_ha_t * ha, dma_addr_t
> busaddr,
> }
> if (IPS_USE_ENH_SGLIST(ha)) {
> scb->sg_list.enh_list[indx].address_lo =
> - cpu_to_le32(pci_dma_lo32(busaddr));
> + cpu_to_le32(lower_32_bits(busaddr));
> scb->sg_list.enh_list[indx].address_hi =
> - cpu_to_le32(pci_dma_hi32(busaddr));
> + cpu_to_le32(upper_32_bits(busaddr));
> scb->sg_list.enh_list[indx].length = cpu_to_le32(e_len);
> } else {
> scb->sg_list.std_list[indx].address =
> - cpu_to_le32(pci_dma_lo32(busaddr));
> + cpu_to_le32(lower_32_bits(busaddr));
> scb->sg_list.std_list[indx].length = cpu_to_le32(e_len);
> }
>
> diff --git a/drivers/scsi/ips.h b/drivers/scsi/ips.h
> index db546171e97f..42c180e3938b 100644
> --- a/drivers/scsi/ips.h
> +++ b/drivers/scsi/ips.h
> @@ -96,9 +96,6 @@
>#define __iomem
> #endif
>
> - #define pci_dma_hi32(a) ((a >> 16) >> 16)
> - #define pci_dma_lo32(a) (a & 0xffff)
> -
> #if (BITS_PER_LONG > 32) || defined(CONFIG_HIGHMEM64G)
>#define IPS_ENABLE_DMA64(1)
> #else
Reviewed-by: Bart Van Assche
Re: [PATCH 5/8] sg: add free list, rework locking
On Fri, 2018-10-19 at 10:38 -0400, Tony Battersby wrote: > Incidentally, I have been using my own home-grown target-mode SCSI > system for the past 16 years, but now I am starting to look into > switching to SCST. I was just reading about their "SGV cache": > > http://scst.sourceforge.net/scst_pg.html > > It looks like it serves a similar purpose to what you are trying to > accomplish with recycling the indirect I/O buffers between different > requests. Perhaps you can borrow some inspiration from them (or even > some code). Although the current SCST SGV cache implementation is more complicated than necessary, I agree that it would be useful to have such a cache implementation in the Linux kernel. The NVMe target driver and the SCSI target core would also benefit from caching SGV allocations. Bart.
Re: [PATCH 5/8] sg: add free list, rework locking
On Fri, 2018-10-19 at 02:24 -0400, Douglas Gilbert wrote:
> static void
> -sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo)
> +sg_fill_request_table(struct sg_fd *sfp, struct sg_req_info *rinfo,
> + int max_num)
> {
> struct sg_request *srp;
> int val;
> - unsigned int ms;
>
> val = 0;
> - list_for_each_entry(srp, >rq_list, entry) {
> - if (val >= SG_MAX_QUEUE)
> - break;
> - rinfo[val].req_state = srp->done + 1;
> + list_for_each_entry(srp, >rq_list, rq_entry) {
> + if (val >= max_num)
> + return;
What protects the sfp->rq_list against concurrent changes? It seems to me
like all other code that iterates over or modifies that list protects that
list with rq_list_lock?
Thanks,
Bart.
Re: [PATCH 4/8] sg: expand request states
On Fri, 2018-10-19 at 02:24 -0400, Douglas Gilbert wrote: > +/* Following defines are states of sg_request::rq_state */ > +#define SG_RQ_INACTIVE 0/* request not in use (e.g. on fl) */ > +#define SG_RQ_INFLIGHT 1/* SCSI request issued, no response yet */ > +#define SG_RQ_AWAIT_READ 2 /* response received, awaiting read */ > +#define SG_RQ_DONE_READ 3 /* read is ongoing or done */ > +#define SG_RQ_BUSY 4/* example: reserve request changing size */ Please introduce an enumeration type instead of #defining these constants to allow the compiler to verify assignments to sg_request::rq_state. Thanks, Bart.
Re: [PATCH 3/5] qla1280: use lower_32_bits and upper_32_bits instead of reinventing them
On 10/18/18 9:47 PM, Bart Van Assche wrote: On 10/18/18 6:03 AM, Christoph Hellwig wrote: @@ -1790,8 +1783,8 @@ qla1280_load_firmware_dma(struct scsi_qla_host *ha) mb[4] = cnt; mb[3] = ha->request_dma & 0x; mb[2] = (ha->request_dma >> 16) & 0x; - mb[7] = pci_dma_hi32(ha->request_dma) & 0x; - mb[6] = pci_dma_hi32(ha->request_dma) >> 16; + mb[7] = upper_32_bits(ha->request_dma) & 0x; + mb[6] = upper_32_bits(ha->request_dma) >> 16; Have you considered to use put_unaligned_be32()? Answering my own question: put_unaligned_be32() is inappropriate here because it stores each pair of bytes in the opposite order of the current code on little endian systems. Bart.
Re: [PATCH 3/5] qla1280: use lower_32_bits and upper_32_bits instead of reinventing them
On 10/18/18 6:03 AM, Christoph Hellwig wrote: @@ -1790,8 +1783,8 @@ qla1280_load_firmware_dma(struct scsi_qla_host *ha) mb[4] = cnt; mb[3] = ha->request_dma & 0x; mb[2] = (ha->request_dma >> 16) & 0x; - mb[7] = pci_dma_hi32(ha->request_dma) & 0x; - mb[6] = pci_dma_hi32(ha->request_dma) >> 16; + mb[7] = upper_32_bits(ha->request_dma) & 0x; + mb[6] = upper_32_bits(ha->request_dma) >> 16; Hi Christoph, Have you considered to use put_unaligned_be32()? Thanks, Bart.
Re: [PATCH 5/5] qla2xxx: use lower_32_bits and upper_32_bits instead of reinventing them
On 10/18/18 6:03 AM, Christoph Hellwig wrote:
This also moves the optimization for builds with 32-bit dma_addr_t to
the compiler (where it belongs) instead of opencoding it based on
incorrect assumptions.
Signed-off-by: Christoph Hellwig
---
drivers/scsi/qla2xxx/qla_target.c | 8
drivers/scsi/qla2xxx/qla_target.h | 8
2 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_target.c
b/drivers/scsi/qla2xxx/qla_target.c
index 39828207bc1d..443711238c0e 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -2660,9 +2660,9 @@ static void qlt_load_cont_data_segments(struct
qla_tgt_prm *prm)
cnt < QLA_TGT_DATASEGS_PER_CONT_24XX && prm->seg_cnt;
cnt++, prm->seg_cnt--) {
*dword_ptr++ =
- cpu_to_le32(pci_dma_lo32
+ cpu_to_le32(lower_32_bits
(sg_dma_address(prm->sg)));
- *dword_ptr++ = cpu_to_le32(pci_dma_hi32
+ *dword_ptr++ = cpu_to_le32(upper_32_bits
(sg_dma_address(prm->sg)));
*dword_ptr++ = cpu_to_le32(sg_dma_len(prm->sg));
@@ -2704,9 +2704,9 @@ static void qlt_load_data_segments(struct qla_tgt_prm *prm)
(cnt < QLA_TGT_DATASEGS_PER_CMD_24XX) && prm->seg_cnt;
cnt++, prm->seg_cnt--) {
*dword_ptr++ =
- cpu_to_le32(pci_dma_lo32(sg_dma_address(prm->sg)));
+ cpu_to_le32(lower_32_bits(sg_dma_address(prm->sg)));
- *dword_ptr++ = cpu_to_le32(pci_dma_hi32(
+ *dword_ptr++ = cpu_to_le32(upper_32_bits(
sg_dma_address(prm->sg)));
Hi Christoph,
Have you considered to use put_unaligned_le64() instead of storing the
lower and upper 32 bits separately?
Thanks,
Bart.
[PATCH 5/7] qla2xxx: Remove a set-but-not-used variable
This patch does not change any functionality.
Cc: Himanshu Madhani
Signed-off-by: Bart Van Assche
---
drivers/scsi/qla2xxx/qla_os.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index dba672f87cb2..01607d2f2c34 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -1749,7 +1749,7 @@ qla2x00_loop_reset(scsi_qla_host_t *vha)
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;
@@ -1799,8 +1799,8 @@ __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
if (!sp_get(sp)) {
spin_unlock_irqrestore
(qp->qp_lock_ptr,
flags);
- status = qla2xxx_eh_abort(
- GET_CMD_SP(sp));
+ qla2xxx_eh_abort(
+ GET_CMD_SP(sp));
spin_lock_irqsave
(qp->qp_lock_ptr,
flags);
}
--
2.19.1.568.g152ad8e336-goog
[PATCH 6/7] qla2xxx: Make sure that qlafx00_ioctl_iosb_entry() initializes 'res'
Only one of the two code paths in qlafx00_ioctl_iosb_entry() initializes the variable 'res'. Make sure that 'res' is initialized before sp->done(sp, res) is called. Cc: Himanshu Madhani Signed-off-by: Bart Van Assche --- drivers/scsi/qla2xxx/qla_mr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c index 2d96f3b7e3e3..b8f967e61891 100644 --- a/drivers/scsi/qla2xxx/qla_mr.c +++ b/drivers/scsi/qla2xxx/qla_mr.c @@ -2212,7 +2212,7 @@ qlafx00_ioctl_iosb_entry(scsi_qla_host_t *vha, struct req_que *req, struct bsg_job *bsg_job; struct fc_bsg_reply *bsg_reply; struct srb_iocb *iocb_job; - int res; + int res = 0; struct qla_mt_iocb_rsp_fx00 fstatus; uint8_t *fw_sts_ptr; -- 2.19.1.568.g152ad8e336-goog
[PATCH 4/7] qla2xxx: Make qla2x00_sysfs_write_nvram() easier to analyze
Modify the unlock statement such that it becomes easier for static
analyzers to analyze it. This patch does not change any functionality.
Cc: Himanshu Madhani
Signed-off-by: Bart Van Assche
---
drivers/scsi/qla2xxx/qla_attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index b28f159fdaee..0bb9ac6ece92 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -218,7 +218,7 @@ qla2x00_sysfs_write_nvram(struct file *filp, struct kobject
*kobj,
mutex_lock(>optrom_mutex);
if (qla2x00_chip_is_down(vha)) {
- mutex_unlock(>hw->optrom_mutex);
+ mutex_unlock(>optrom_mutex);
return -EAGAIN;
}
--
2.19.1.568.g152ad8e336-goog
[PATCH 7/7] qla2xxx: Remove two arguments from qlafx00_error_entry()
Move a debug statement from qlafx00_error_entry() into its caller. Remove
one unused argument from that function. This patch does not change the
behavior of the qla2xxx driver.
Cc: Himanshu Madhani
Signed-off-by: Bart Van Assche
---
drivers/scsi/qla2xxx/qla_mr.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c
index b8f967e61891..60f964c53c01 100644
--- a/drivers/scsi/qla2xxx/qla_mr.c
+++ b/drivers/scsi/qla2xxx/qla_mr.c
@@ -2681,12 +2681,10 @@ qlafx00_multistatus_entry(struct scsi_qla_host *vha,
* @vha: SCSI driver HA context
* @rsp: response queue
* @pkt: Entry pointer
- * @estatus:
- * @etype:
*/
static void
qlafx00_error_entry(scsi_qla_host_t *vha, struct rsp_que *rsp,
- struct sts_entry_fx00 *pkt, uint8_t estatus, uint8_t etype)
+ struct sts_entry_fx00 *pkt)
{
srb_t *sp;
struct qla_hw_data *ha = vha->hw;
@@ -2695,9 +2693,6 @@ qlafx00_error_entry(scsi_qla_host_t *vha, struct rsp_que
*rsp,
struct req_que *req = NULL;
int res = DID_ERROR << 16;
- ql_dbg(ql_dbg_async, vha, 0x507f,
- "type of error status in response: 0x%x\n", estatus);
-
req = ha->req_q_map[que];
sp = qla2x00_get_sp_from_handle(vha, func, req, pkt);
@@ -2745,9 +2740,11 @@ qlafx00_process_response_queue(struct scsi_qla_host *vha,
if (pkt->entry_status != 0 &&
pkt->entry_type != IOCTL_IOSB_TYPE_FX00) {
+ ql_dbg(ql_dbg_async, vha, 0x507f,
+ "type of error status in response: 0x%x\n",
+ pkt->entry_status);
qlafx00_error_entry(vha, rsp,
- (struct sts_entry_fx00 *)pkt, pkt->entry_status,
- pkt->entry_type);
+ (struct sts_entry_fx00 *)pkt);
continue;
}
--
2.19.1.568.g152ad8e336-goog
[PATCH 1/7] qla2xxx: Modify fall-through annotations
This patch avoids that the compiler complains about missing fall-through
annotations when building with W=1.
Cc: Himanshu Madhani
Signed-off-by: Bart Van Assche
---
drivers/scsi/qla2xxx/qla_init.c | 2 +-
drivers/scsi/qla2xxx/qla_target.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index c72d8012fe2a..2ccf9f190c68 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -680,7 +680,7 @@ static void qla24xx_handle_gnl_done_event(scsi_qla_host_t
*vha,
fcport);
break;
}
- /* drop through */
+ /* fall through */
default:
if (fcport_is_smaller(fcport)) {
/* local adapter is bigger */
diff --git a/drivers/scsi/qla2xxx/qla_target.c
b/drivers/scsi/qla2xxx/qla_target.c
index 3015f1bbcf1a..902f9c25da2e 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -4540,7 +4540,7 @@ static int qlt_issue_task_mgmt(struct fc_port *sess, u64
lun,
case QLA_TGT_CLEAR_TS:
case QLA_TGT_ABORT_TS:
abort_cmds_for_lun(vha, lun, a->u.isp24.fcp_hdr.s_id);
- /* drop through */
+ /* fall through */
case QLA_TGT_CLEAR_ACA:
h = qlt_find_qphint(vha, mcmd->unpacked_lun);
mcmd->qpair = h->qpair;
--
2.19.1.568.g152ad8e336-goog
[PATCH 3/7] qla2xxx: Declare local functions 'static'
This patch avoids that the compiler complains about missing declarations
when building with W=1.
Cc: Himanshu Madhani
Signed-off-by: Bart Van Assche
---
drivers/scsi/qla2xxx/qla_init.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 2ccf9f190c68..6fe20c27acc1 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -425,7 +425,7 @@ void qla24xx_handle_adisc_event(scsi_qla_host_t *vha,
struct event_arg *ea)
__qla24xx_handle_gpdb_event(vha, ea);
}
-int qla_post_els_plogi_work(struct scsi_qla_host *vha, fc_port_t *fcport)
+static int qla_post_els_plogi_work(struct scsi_qla_host *vha, fc_port_t
*fcport)
{
struct qla_work_evt *e;
@@ -1551,7 +1551,8 @@ void qla24xx_handle_relogin_event(scsi_qla_host_t *vha,
}
-void qla_handle_els_plogi_done(scsi_qla_host_t *vha, struct event_arg *ea)
+static void qla_handle_els_plogi_done(scsi_qla_host_t *vha,
+ struct event_arg *ea)
{
ql_dbg(ql_dbg_disc, vha, 0x2118,
"%s %d %8phC post PRLI\n",
--
2.19.1.568.g152ad8e336-goog
[PATCH 2/7] qla2xxx: Improve several kernel-doc headers
This patch avoids that complaints about kernel-doc headers are reported when building with W=1. Cc: Himanshu Madhani Signed-off-by: Bart Van Assche --- drivers/scsi/qla2xxx/qla_iocb.c | 4 ++-- drivers/scsi/qla2xxx/qla_isr.c| 6 +++--- drivers/scsi/qla2xxx/qla_mbx.c| 6 +++--- drivers/scsi/qla2xxx/qla_mr.c | 6 +++--- drivers/scsi/qla2xxx/qla_nx.c | 2 +- drivers/scsi/qla2xxx/qla_nx2.c| 2 +- drivers/scsi/qla2xxx/qla_sup.c| 2 +- drivers/scsi/qla2xxx/qla_target.c | 6 +++--- 8 files changed, 17 insertions(+), 17 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c index 86fb8b21aa71..032635321ad6 100644 --- a/drivers/scsi/qla2xxx/qla_iocb.c +++ b/drivers/scsi/qla2xxx/qla_iocb.c @@ -1195,8 +1195,8 @@ qla24xx_walk_and_build_prot_sglist(struct qla_hw_data *ha, srb_t *sp, * @sp: SRB command to process * @cmd_pkt: Command type 3 IOCB * @tot_dsds: Total number of segments to transfer - * @tot_prot_dsds: - * @fw_prot_opts: + * @tot_prot_dsds: Total number of segments with protection information + * @fw_prot_opts: Protection options to be passed to firmware */ inline int qla24xx_build_scsi_crc_2_iocbs(srb_t *sp, struct cmd_type_crc_2 *cmd_pkt, diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c index d73b04e40590..30d3090842f8 100644 --- a/drivers/scsi/qla2xxx/qla_isr.c +++ b/drivers/scsi/qla2xxx/qla_isr.c @@ -25,7 +25,7 @@ static int qla2x00_error_entry(scsi_qla_host_t *, struct rsp_que *, /** * qla2100_intr_handler() - Process interrupts for the ISP2100 and ISP2200. - * @irq: + * @irq: interrupt number * @dev_id: SCSI driver HA context * * Called by system whenever the host adapter generates an interrupt. @@ -144,7 +144,7 @@ qla2x00_check_reg16_for_disconnect(scsi_qla_host_t *vha, uint16_t reg) /** * qla2300_intr_handler() - Process interrupts for the ISP23xx and ISP63xx. - * @irq: + * @irq: interrupt number * @dev_id: SCSI driver HA context * * Called by system whenever the host adapter generates an interrupt. @@ -3109,7 +3109,7 @@ qla2xxx_check_risc_status(scsi_qla_host_t *vha) /** * qla24xx_intr_handler() - Process interrupts for the ISP23xx and ISP24xx. - * @irq: + * @irq: interrupt number * @dev_id: SCSI driver HA context * * Called by system whenever the host adapter generates an interrupt. diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c index bd8c86aeccc2..38ef06d613ba 100644 --- a/drivers/scsi/qla2xxx/qla_mbx.c +++ b/drivers/scsi/qla2xxx/qla_mbx.c @@ -3479,9 +3479,9 @@ qla8044_read_serdes_word(scsi_qla_host_t *vha, uint32_t addr, uint32_t *data) /** * qla2x00_set_serdes_params() - * @vha: HA context - * @sw_em_1g: - * @sw_em_2g: - * @sw_em_4g: + * @sw_em_1g: serial link options + * @sw_em_2g: serial link options + * @sw_em_4g: serial link options * * Returns */ diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c index 521a51370554..2d96f3b7e3e3 100644 --- a/drivers/scsi/qla2xxx/qla_mr.c +++ b/drivers/scsi/qla2xxx/qla_mr.c @@ -2624,7 +2624,7 @@ qlafx00_status_cont_entry(struct rsp_que *rsp, sts_cont_entry_t *pkt) * qlafx00_multistatus_entry() - Process Multi response queue entries. * @vha: SCSI driver HA context * @rsp: response queue - * @pkt: + * @pkt: received packet */ static void qlafx00_multistatus_entry(struct scsi_qla_host *vha, @@ -2867,7 +2867,7 @@ qlafx00_async_event(scsi_qla_host_t *vha) /** * qlafx00x_mbx_completion() - Process mailbox command completions. * @vha: SCSI driver HA context - * @mb0: + * @mb0: value to be written into mailbox register 0 */ static void qlafx00_mbx_completion(scsi_qla_host_t *vha, uint32_t mb0) @@ -2893,7 +2893,7 @@ qlafx00_mbx_completion(scsi_qla_host_t *vha, uint32_t mb0) /** * qlafx00_intr_handler() - Process interrupts for the ISPFX00. - * @irq: + * @irq: interrupt number * @dev_id: SCSI driver HA context * * Called by system whenever the host adapter generates an interrupt. diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c index de2bc78449e7..8dbe94c52f65 100644 --- a/drivers/scsi/qla2xxx/qla_nx.c +++ b/drivers/scsi/qla2xxx/qla_nx.c @@ -2010,7 +2010,7 @@ qla82xx_mbx_completion(scsi_qla_host_t *vha, uint16_t mb0) /** * qla82xx_intr_handler() - Process interrupts for the ISP23xx and ISP63xx. - * @irq: + * @irq: interrupt number * @dev_id: SCSI driver HA context * * Called by system whenever the host adapter generates an interrupt. diff --git a/drivers/scsi/qla2xxx/qla_nx2.c b/drivers/scsi/qla2xxx/qla_nx2.c index 3a2b0282df14..fe856b602e03 100644 --- a/drivers/scsi/qla2xxx/qla_nx2.c +++ b/drivers/scsi/qla2xxx/qla_nx2.c @@ -3878,7 +3878,7 @@ qla8044_write_optrom_data(struct scsi_qla_host *vha, uint8_t *buf, #define PF_BITS_MASK (0xF << 16) /** * qla8044_intr_handler() - Process interrupts for the ISP8044 - * @irq: + * @irq: interrupt number * @dev_id: SCSI driver HA c
[PATCH 0/7] qla2xxx patches for kernel v4.20
Hi Martin, This is a series with mostly trivial patches for the qla2xxx driver. These patches address warnings reported by gcc and by the smatch and sparse static analyzers. Please consider these patches for kernel v4.20. Thanks, Bart. Bart Van Assche (7): qla2xxx: Modify fall-through annotations qla2xxx: Improve several kernel-doc headers qla2xxx: Declare local functions 'static' qla2xxx: Make qla2x00_sysfs_write_nvram() easier to analyze qla2xxx: Remove a set-but-not-used variable qla2xxx: Make sure that qlafx00_ioctl_iosb_entry() initializes 'res' qla2xxx: Remove two arguments from qlafx00_error_entry() drivers/scsi/qla2xxx/qla_attr.c | 2 +- drivers/scsi/qla2xxx/qla_init.c | 7 --- drivers/scsi/qla2xxx/qla_iocb.c | 4 ++-- drivers/scsi/qla2xxx/qla_isr.c| 6 +++--- drivers/scsi/qla2xxx/qla_mbx.c| 6 +++--- drivers/scsi/qla2xxx/qla_mr.c | 21 + drivers/scsi/qla2xxx/qla_nx.c | 2 +- drivers/scsi/qla2xxx/qla_nx2.c| 2 +- drivers/scsi/qla2xxx/qla_os.c | 6 +++--- drivers/scsi/qla2xxx/qla_sup.c| 2 +- drivers/scsi/qla2xxx/qla_target.c | 8 11 files changed, 32 insertions(+), 34 deletions(-) -- 2.19.1.568.g152ad8e336-goog
Re: [PATCH v2 1/7] drivers/base: Fix a race condition in the device probing code
On Thu, 2018-10-18 at 07:10 +0200, Greg Kroah-Hartman wrote:
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index edfc9f0b1180..b4212154a94b 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -645,6 +645,14 @@ int driver_probe_device(struct device_driver *drv,
> > struct device *dev)
> > {
> > int ret = 0;
> >
> > + /*
> > +* Several callers check the driver pointer without holding the
> > +* device mutex. Hence check the driver pointer again while holding
> > +* the device mutex.
> > +*/
> > + if (dev->driver)
> > + return dev->driver == drv;
>
> I do not understand, who is calling probe twice? What is the sequence
> of events that is causing that error message being printed out? Wh is
> not grabbing the mutex properly?
Hi Greg,
If I drop patch 3/7 from my patch series and use Alexander Duyck's patches
instead I don't need this patch anymore. So I don't think we have to spend
more time on this patch.
Thanks,
Bart.
Re: [PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver
On Thu, 2018-10-18 at 08:25 -0700, Alexander Duyck wrote: > On 10/17/2018 5:54 PM, Dan Williams wrote: > > On Wed, Oct 17, 2018 at 4:41 PM Bart Van Assche wrote: > > > > > > Instead of probing devices sequentially in the PROBE_PREFER_ASYNCHRONOUS > > > mode, scan devices concurrently. This helps when the wall clock time for > > > a single probe is significantly above the CPU time needed for a single > > > probe, e.g. when scanning SCSI LUNs over a storage network. > > > > Alex had a similar patch here [1] that I don't think has been accepted > > yet, in any event some collaboration is needed: > > > > [1]: https://lkml.org/lkml/2018/9/27/14 > > The patch set referenced is a little out of date. The latest set is: > https://lore.kernel.org/lkml/20181015150305.29520.86363.stgit@localhost.localdomain/ > > I'm also not quite sure what the point of this patch is. I don't think > it is doing what it says it is doing. From what I can tell it is just > allowing the driver init code to ignore if the driver wants to be probed > asynchronously or not. Further comments inline below. Hi Alexander, Thanks for the pointer to the latest version of your patch series. I was not yet aware of your work when I posted this patch series. Now that I have had a look at your patch series I like your approach better than what I did in this patch. Since it could take a while before agreement is reached about the async domain patches in the same patch series, how about you submitting patches 3/6 and 4/6 from your patch series to Greg for kernel version v4.20? If I drop the driver core patches from my patch series and replace these with your driver core patches I achieve the same results. If you Cc me when you resubmit these patches I will review them. Thanks, Bart.
Re: [PATCH v2 2/7] drivers/base: Verify struct device locking requirements at runtime
On Thu, 2018-10-18 at 07:11 +0200, Greg Kroah-Hartman wrote:
> On Wed, Oct 17, 2018 at 04:40:01PM -0700, Bart Van Assche wrote:
> > Make sure that a complaint appears in the kernel log if the driver core
> > locking assumptions are violated.
> >
> > Cc: Lee Duncan
> > Cc: Hannes Reinecke
> > Cc: Luis Chamberlain
> > Cc: Johannes Thumshirn
> > Cc: Christoph Hellwig
> > Cc: Greg Kroah-Hartman
> > Signed-off-by: Bart Van Assche
> > ---
> > drivers/base/dd.c | 16
> > drivers/base/memory.c | 4
> > 2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index b4212154a94b..033382421351 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -315,11 +315,15 @@ __exitcall(deferred_probe_exit);
> > */
> > bool device_is_bound(struct device *dev)
> > {
> > + lockdep_assert_held(>mutex);
>
> With this patch applied, are you seeing lockdep messages anywhere?
No lockdep complaints appeared in my tests with this patch applied. I have
checked whether all callers of the modified functions hold dev->mutex. This
patch is most useful to check new callers of the modified functions. I came
up with this patch to verify the new code I added myself in the drivers/base
directory.
Bart.
[PATCH v2 6/7] sd: Rely on the driver core for asynchronous probing
As explained during the LSF/MM session about increasing SCSI disk
probing concurrency, the problems with the current probing approach
are as follows:
- The driver core is unaware of asynchronous SCSI LUN probing.
wait_for_device_probe() waits for all asynchronous probes except
asynchronous SCSI disk probes.
- There is unnecessary serialization between sd_probe() and sd_remove().
This can lead to a deadlock.
Hence this patch that modifies the sd driver such that it uses the
driver core framework for asynchronous probing. The async domains
and get_device()/put_device() pairs that became superfluous due to
this change are removed.
This patch reduces the time needed for loading the scsi_debug kernel
module with parameters delay=0 and max_luns=256 from 0.7s to 0.1s. In
other words, this specific test runs about seven times faster.
See also commit 3c31b52f96f7 ("scsi: async sd resume").
Cc: Lee Duncan
Cc: Hannes Reinecke
Cc: Luis Chamberlain
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
Cc: Dan Williams
Signed-off-by: Bart Van Assche
---
drivers/scsi/scsi.c | 14 --
drivers/scsi/scsi_pm.c | 22 ++
drivers/scsi/scsi_priv.h | 3 ---
drivers/scsi/sd.c| 13 +++--
4 files changed, 5 insertions(+), 47 deletions(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index fc1356d101b0..1205369ad44f 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -85,19 +85,6 @@ unsigned int scsi_logging_level;
EXPORT_SYMBOL(scsi_logging_level);
#endif
-/* sd, scsi core and power management need to coordinate flushing async
actions */
-ASYNC_DOMAIN(scsi_sd_probe_domain);
-EXPORT_SYMBOL(scsi_sd_probe_domain);
-
-/*
- * Separate domain (from scsi_sd_probe_domain) to maximize the benefit of
- * asynchronous system resume operations. It is marked 'exclusive' to avoid
- * being included in the async_synchronize_full() that is invoked by
- * dpm_resume()
- */
-ASYNC_DOMAIN_EXCLUSIVE(scsi_sd_pm_domain);
-EXPORT_SYMBOL(scsi_sd_pm_domain);
-
/**
* scsi_put_command - Free a scsi command block
* @cmd: command block to free
@@ -839,7 +826,6 @@ static void __exit exit_scsi(void)
scsi_exit_devinfo();
scsi_exit_procfs();
scsi_exit_queue();
- async_unregister_domain(_sd_probe_domain);
}
subsys_initcall(init_scsi);
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index b44c1bb687a2..f229208bfef3 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -54,9 +54,6 @@ static int scsi_dev_type_suspend(struct device *dev,
const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
int err;
- /* flush pending in-flight resume operations, suspend is synchronous */
- async_synchronize_full_domain(_sd_pm_domain);
-
err = scsi_device_quiesce(to_scsi_device(dev));
if (err == 0) {
err = cb(dev, pm);
@@ -149,18 +146,7 @@ static int scsi_bus_resume_common(struct device *dev,
if (scsi_is_sdev_device(dev) && pm_runtime_suspended(dev))
blk_set_runtime_active(to_scsi_device(dev)->request_queue);
- if (fn) {
- async_schedule_domain(fn, dev, _sd_pm_domain);
-
- /*
-* If a user has disabled async probing a likely reason
-* is due to a storage enclosure that does not inject
-* staggered spin-ups. For safety, make resume
-* synchronous as well in that case.
-*/
- if (strncmp(scsi_scan_type, "async", 5) != 0)
- async_synchronize_full_domain(_sd_pm_domain);
- } else {
+ if (!fn) {
pm_runtime_disable(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
@@ -170,11 +156,7 @@ static int scsi_bus_resume_common(struct device *dev,
static int scsi_bus_prepare(struct device *dev)
{
- if (scsi_is_sdev_device(dev)) {
- /* sd probing uses async_schedule. Wait until it finishes. */
- async_synchronize_full_domain(_sd_probe_domain);
-
- } else if (scsi_is_host_device(dev)) {
+ if (scsi_is_host_device(dev)) {
/* Wait until async scanning is finished */
scsi_complete_async_scans();
}
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 99f1db5e467e..b9fa363b4bbb 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -175,9 +175,6 @@ static inline int scsi_autopm_get_host(struct Scsi_Host *h)
{ return 0; }
static inline void scsi_autopm_put_host(struct Scsi_Host *h) {}
#endif /* CONFIG_PM */
-extern struct async_domain scsi_sd_pm_domain;
-extern struct async_domain scsi_sd_probe_domain;
-
/* scsi_dh.c */
#ifdef CONFIG_SCSI_DH
void scsi_dh_add_device(struct scsi_device *sdev);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
[PATCH v2 3/7] drivers/base: Probe devices concurrently if requested by the driver
Instead of probing devices sequentially in the PROBE_PREFER_ASYNCHRONOUS
mode, scan devices concurrently. This helps when the wall clock time for
a single probe is significantly above the CPU time needed for a single
probe, e.g. when scanning SCSI LUNs over a storage network.
Cc: Lee Duncan
Cc: Hannes Reinecke
Cc: Luis Chamberlain
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
Cc: Dan Williams
Signed-off-by: Bart Van Assche
---
drivers/base/bus.c | 3 +--
drivers/base/dd.c | 49 ++
2 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 8bfd27ec73d6..18ca1178821f 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -696,8 +696,7 @@ int bus_add_driver(struct device_driver *drv)
out_unregister:
kobject_put(>kobj);
- /* drv->p is freed in driver_release() */
- drv->p = NULL;
+
out_put_bus:
bus_put(bus);
return error;
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 033382421351..f8d645aa09be 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "base.h"
#include "power/power.h"
@@ -691,6 +692,49 @@ int driver_probe_device(struct device_driver *drv, struct
device *dev)
return ret;
}
+struct driver_and_dev {
+ struct device_driver*drv;
+ struct device *dev;
+};
+
+static void __driver_probe_device_async(void *data, async_cookie_t cookie)
+{
+ struct driver_and_dev *dd = data;
+ struct device_driver *drv = dd->drv;
+ struct device *dev = dd->dev;
+
+ device_lock(dev);
+ driver_probe_device(drv, dev);
+ device_unlock(dev);
+ kobject_put(>p->kobj);
+ module_put(drv->owner);
+ kfree(dd);
+}
+
+static void driver_probe_device_async(struct device_driver *drv,
+ struct device *dev)
+{
+ struct driver_and_dev *dd;
+
+ if (!try_module_get(drv->owner))
+ return;
+ dd = kmalloc(sizeof(*dd), GFP_KERNEL);
+ if (!dd) {
+ /* If out of memory, scan synchronously. */
+ device_lock(dev);
+ driver_probe_device(drv, dev);
+ device_unlock(dev);
+ module_put(drv->owner);
+ return;
+ }
+ *dd = (struct driver_and_dev){
+ .drv = drv,
+ .dev = dev,
+ };
+ kobject_get(>p->kobj);
+ async_schedule(__driver_probe_device_async, dd);
+}
+
bool driver_allows_async_probing(struct device_driver *drv)
{
switch (drv->probe_type) {
@@ -777,6 +821,11 @@ static int __device_attach_driver(struct device_driver
*drv, void *_data)
if (data->check_async && async_allowed != data->want_async)
return 0;
+ if (data->check_async) {
+ driver_probe_device_async(drv, dev);
+ return 0;
+ }
+
return driver_probe_device(drv, dev);
}
--
2.19.1.568.g152ad8e336-goog
[PATCH v2 7/7] sd: Inline sd_probe_part2()
This patch does not change any functionality.
Cc: Lee Duncan
Cc: Hannes Reinecke
Cc: Luis Chamberlain
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
Signed-off-by: Bart Van Assche
---
drivers/scsi/sd.c | 101 --
1 file changed, 43 insertions(+), 58 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 52eee36d13fb..2a96c2ed1b52 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3228,63 +3228,6 @@ static int sd_format_disk_name(char *prefix, int index,
char *buf, int buflen)
return 0;
}
-static void sd_probe_part2(struct scsi_disk *sdkp)
-{
- struct scsi_device *sdp;
- struct gendisk *gd;
- u32 index;
- struct device *dev;
-
- sdp = sdkp->device;
- gd = sdkp->disk;
- index = sdkp->index;
- dev = >sdev_gendev;
-
- gd->major = sd_major((index & 0xf0) >> 4);
- gd->first_minor = ((index & 0xf) << 4) | (index & 0xfff00);
-
- gd->fops = _fops;
- gd->private_data = >driver;
- gd->queue = sdkp->device->request_queue;
-
- /* defaults, until the device tells us otherwise */
- sdp->sector_size = 512;
- sdkp->capacity = 0;
- sdkp->media_present = 1;
- sdkp->write_prot = 0;
- sdkp->cache_override = 0;
- sdkp->WCE = 0;
- sdkp->RCD = 0;
- sdkp->ATO = 0;
- sdkp->first_scan = 1;
- sdkp->max_medium_access_timeouts = SD_MAX_MEDIUM_TIMEOUTS;
-
- sd_revalidate_disk(gd);
-
- gd->flags = GENHD_FL_EXT_DEVT;
- if (sdp->removable) {
- gd->flags |= GENHD_FL_REMOVABLE;
- gd->events |= DISK_EVENT_MEDIA_CHANGE;
- }
-
- blk_pm_runtime_init(sdp->request_queue, dev);
- device_add_disk(dev, gd);
- if (sdkp->capacity)
- sd_dif_config_host(sdkp);
-
- sd_revalidate_disk(gd);
-
- if (sdkp->security) {
- sdkp->opal_dev = init_opal_dev(sdp, _sec_submit);
- if (sdkp->opal_dev)
- sd_printk(KERN_NOTICE, sdkp, "supports TCG Opal\n");
- }
-
- sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
- sdp->removable ? "removable " : "");
- scsi_autopm_put_device(sdp);
-}
-
/**
* sd_probe - called during driver initialization and whenever a
* new scsi device is attached to the system. It is called once
@@ -3374,7 +3317,49 @@ static int sd_probe(struct device *dev)
get_device(dev);
dev_set_drvdata(dev, sdkp);
- sd_probe_part2(sdkp);
+ gd->major = sd_major((index & 0xf0) >> 4);
+ gd->first_minor = ((index & 0xf) << 4) | (index & 0xfff00);
+
+ gd->fops = _fops;
+ gd->private_data = >driver;
+ gd->queue = sdkp->device->request_queue;
+
+ /* defaults, until the device tells us otherwise */
+ sdp->sector_size = 512;
+ sdkp->capacity = 0;
+ sdkp->media_present = 1;
+ sdkp->write_prot = 0;
+ sdkp->cache_override = 0;
+ sdkp->WCE = 0;
+ sdkp->RCD = 0;
+ sdkp->ATO = 0;
+ sdkp->first_scan = 1;
+ sdkp->max_medium_access_timeouts = SD_MAX_MEDIUM_TIMEOUTS;
+
+ sd_revalidate_disk(gd);
+
+ gd->flags = GENHD_FL_EXT_DEVT;
+ if (sdp->removable) {
+ gd->flags |= GENHD_FL_REMOVABLE;
+ gd->events |= DISK_EVENT_MEDIA_CHANGE;
+ }
+
+ blk_pm_runtime_init(sdp->request_queue, dev);
+ device_add_disk(dev, gd);
+ if (sdkp->capacity)
+ sd_dif_config_host(sdkp);
+
+ sd_revalidate_disk(gd);
+
+ if (sdkp->security) {
+ sdkp->opal_dev = init_opal_dev(sdp, _sec_submit);
+ if (sdkp->opal_dev)
+ sd_printk(KERN_NOTICE, sdkp, "supports TCG Opal\n");
+ }
+
+ sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
+ sdp->removable ? "removable " : "");
+ scsi_autopm_put_device(sdp);
return 0;
--
2.19.1.568.g152ad8e336-goog
[PATCH v2 2/7] drivers/base: Verify struct device locking requirements at runtime
Make sure that a complaint appears in the kernel log if the driver core
locking assumptions are violated.
Cc: Lee Duncan
Cc: Hannes Reinecke
Cc: Luis Chamberlain
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
Signed-off-by: Bart Van Assche
---
drivers/base/dd.c | 16
drivers/base/memory.c | 4
2 files changed, 20 insertions(+)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index b4212154a94b..033382421351 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -315,11 +315,15 @@ __exitcall(deferred_probe_exit);
*/
bool device_is_bound(struct device *dev)
{
+ lockdep_assert_held(>mutex);
+
return dev->p && klist_node_attached(>p->knode_driver);
}
static void driver_bound(struct device *dev)
{
+ lockdep_assert_held(>mutex);
+
if (device_is_bound(dev)) {
printk(KERN_WARNING "%s: device %s already bound\n",
__func__, kobject_name(>kobj));
@@ -363,6 +367,8 @@ static int driver_sysfs_add(struct device *dev)
{
int ret;
+ lockdep_assert_held(>mutex);
+
if (dev->bus)
blocking_notifier_call_chain(>bus->p->bus_notifier,
BUS_NOTIFY_BIND_DRIVER, dev);
@@ -421,6 +427,8 @@ int device_bind_driver(struct device *dev)
{
int ret;
+ lockdep_assert_held(>mutex);
+
ret = driver_sysfs_add(dev);
if (!ret)
driver_bound(dev);
@@ -450,6 +458,8 @@ static int really_probe(struct device *dev, struct
device_driver *drv)
bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
!drv->suppress_bind_attrs;
+ lockdep_assert_held(>mutex);
+
if (defer_all_probes) {
/*
* Value of defer_all_probes can be set only by
@@ -589,6 +599,8 @@ static int really_probe_debug(struct device *dev, struct
device_driver *drv)
ktime_t calltime, delta, rettime;
int ret;
+ lockdep_assert_held(>mutex);
+
calltime = ktime_get();
ret = really_probe(dev, drv);
rettime = ktime_get();
@@ -645,6 +657,8 @@ int driver_probe_device(struct device_driver *drv, struct
device *dev)
{
int ret = 0;
+ lockdep_assert_held(>mutex);
+
/*
* Several callers check the driver pointer without holding the
* device mutex. Hence check the driver pointer again while holding
@@ -932,6 +946,8 @@ static void __device_release_driver(struct device *dev,
struct device *parent)
{
struct device_driver *drv;
+ lockdep_assert_held(>mutex);
+
drv = dev->driver;
if (drv) {
if (driver_allows_async_probing(drv))
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index c8a1cb0b6136..41a8454dabb5 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -283,6 +283,8 @@ static int memory_subsys_online(struct device *dev)
struct memory_block *mem = to_memory_block(dev);
int ret;
+ lockdep_assert_held(>mutex);
+
if (mem->state == MEM_ONLINE)
return 0;
@@ -307,6 +309,8 @@ static int memory_subsys_offline(struct device *dev)
{
struct memory_block *mem = to_memory_block(dev);
+ lockdep_assert_held(>mutex);
+
if (mem->state == MEM_OFFLINE)
return 0;
--
2.19.1.568.g152ad8e336-goog
[PATCH v2 4/7] drivers/base, __device_release_driver(): Do not wait for asynchronous probing
Since __device_release_driver() is called with the device lock held
and since the same device lock is obtained by the functions that
perform asynchronous probing (driver_attach_async() and
__device_attach_async_helper()), asynchronous probing is already
serialized against __device_release_driver(). Remove the
async_synchronize_full() call from __device_release_driver() to
avoid that a deadlock is triggered.
Fixes: 765230b5f084 ("driver-core: add asynchronous probing support for
drivers").
Acked-by: Dmitry Torokhov
Cc: Lee Duncan
Cc: Hannes Reinecke
Cc: Luis Chamberlain
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
Signed-off-by: Bart Van Assche
---
drivers/base/dd.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index f8d645aa09be..0a3c9f8702de 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -999,9 +999,6 @@ static void __device_release_driver(struct device *dev,
struct device *parent)
drv = dev->driver;
if (drv) {
- if (driver_allows_async_probing(drv))
- async_synchronize_full();
-
while (device_links_busy(dev)) {
device_unlock(dev);
if (parent)
--
2.19.1.568.g152ad8e336-goog
[PATCH v2 5/7] sd: Avoid that hibernation triggers a kernel warning
Although the power management code never calls the system-wide and runtime
suspend callbacks concurrently, runtime power state changes can happen
while the system is being suspended or resumed. See also the dpm_suspend()
and dpm_resume() calls in hibernation_snapshot(). Make sure the sd driver
supports this. This patch avoids that the following call trace is reported
during system-wide suspend:
WARNING: CPU: 0 PID: 701 at drivers/scsi/scsi_lib.c:3047
scsi_device_quiesce+0x4b/0xd0
Workqueue: events_unbound async_run_entry_fn
RIP: 0010:scsi_device_quiesce+0x4b/0xd0
Call Trace:
scsi_bus_suspend_common+0x71/0xe0
scsi_bus_freeze+0x15/0x20
dpm_run_callback+0x88/0x360
__device_suspend+0x1c4/0x840
async_suspend+0x1f/0xb0
async_run_entry_fn+0x6e/0x2c0
process_one_work+0x4ae/0xa20
worker_thread+0x63/0x5a0
kthread+0x1cf/0x1f0
ret_from_fork+0x24/0x30
Fixes: cd84a62e0078 ("block, scsi: Change the preempt-only flag into a counter")
Cc: Lee Duncan
Cc: Hannes Reinecke
Cc: Luis Chamberlain
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
Cc: Dan Williams
Cc: sta...@vger.kernel.org
Signed-off-by: Bart Van Assche
---
drivers/scsi/scsi_lib.c| 15 ++-
include/scsi/scsi_device.h | 1 -
2 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7db3c5fae469..6c18a61176e5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3052,11 +3052,12 @@ scsi_device_quiesce(struct scsi_device *sdev)
int err;
/*
-* It is allowed to call scsi_device_quiesce() multiple times from
-* the same context but concurrent scsi_device_quiesce() calls are
-* not allowed.
+* Since all scsi_device_quiesce() and scsi_device_resume() calls
+* are serialized it is safe here to check the device state without
+* holding the SCSI device state mutex.
*/
- WARN_ON_ONCE(sdev->quiesced_by && sdev->quiesced_by != current);
+ if (sdev->sdev_state == SDEV_QUIESCE)
+ return 0;
blk_set_preempt_only(q);
@@ -3072,9 +3073,7 @@ scsi_device_quiesce(struct scsi_device *sdev)
mutex_lock(>state_mutex);
err = scsi_device_set_state(sdev, SDEV_QUIESCE);
- if (err == 0)
- sdev->quiesced_by = current;
- else
+ if (err)
blk_clear_preempt_only(q);
mutex_unlock(>state_mutex);
@@ -3098,8 +3097,6 @@ void scsi_device_resume(struct scsi_device *sdev)
* device deleted during suspend)
*/
mutex_lock(>state_mutex);
- WARN_ON_ONCE(!sdev->quiesced_by);
- sdev->quiesced_by = NULL;
blk_clear_preempt_only(sdev->request_queue);
if (sdev->sdev_state == SDEV_QUIESCE)
scsi_device_set_state(sdev, SDEV_RUNNING);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 202f4d6a4342..ef86c8adc5d5 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -226,7 +226,6 @@ struct scsi_device {
unsigned char access_state;
struct mutexstate_mutex;
enum scsi_device_state sdev_state;
- struct task_struct *quiesced_by;
unsigned long sdev_data[0];
} __attribute__((aligned(sizeof(unsigned long;
--
2.19.1.568.g152ad8e336-goog
[PATCH v2 1/7] drivers/base: Fix a race condition in the device probing code
This patch avoids that complaints similar to the following appear in the
system log:
sysfs: cannot create duplicate filename
'/devices/pseudo_0/adapter0/host3/target3:0:0/3:0:0:133/driver'
Cc: Lee Duncan
Cc: Hannes Reinecke
Cc: Luis Chamberlain
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
Signed-off-by: Bart Van Assche
---
drivers/base/dd.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index edfc9f0b1180..b4212154a94b 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -645,6 +645,14 @@ int driver_probe_device(struct device_driver *drv, struct
device *dev)
{
int ret = 0;
+ /*
+* Several callers check the driver pointer without holding the
+* device mutex. Hence check the driver pointer again while holding
+* the device mutex.
+*/
+ if (dev->driver)
+ return dev->driver == drv;
+
if (!device_is_registered(dev))
return -ENODEV;
--
2.19.1.568.g152ad8e336-goog
[PATCH v2 0/7] Increase SCSI disk probing concurrency
Hello Martin, During the 2018 edition of LSF/MM there was a session about increasing SCSI disk probing concurrency. This patch series implements what has been proposed during that session, namely: - Make sure that the driver core is aware of asynchronous SCSI LUN probing. - Avoid unnecessary serialization between sd_probe() and sd_remove() because this could lead to a deadlock. This patch series makes SCSI LUN probing seven times faster for one particular test case. Please consider this patch series for kernel v4.20. Thanks, Bart. Changes compared to v1: - LUNs are now probed concurrently instead of sequentially. - Added several patches that fix bugs discovered while testing v1 of this patch series. - Included performance results in patch 6/7. Bart Van Assche (7): drivers/base: Fix a race condition in the device probing code drivers/base: Verify struct device locking requirements at runtime drivers/base: Probe devices concurrently if requested by the driver drivers/base, __device_release_driver(): Do not wait for asynchronous probing sd: Avoid that hibernation triggers a kernel warning sd: Rely on the driver core for asynchronous probing sd: Inline sd_probe_part2() drivers/base/bus.c | 3 +- drivers/base/dd.c | 76 - drivers/base/memory.c | 4 ++ drivers/scsi/scsi.c| 14 - drivers/scsi/scsi_lib.c| 15 ++--- drivers/scsi/scsi_pm.c | 22 +--- drivers/scsi/scsi_priv.h | 3 - drivers/scsi/sd.c | 110 +++-- include/scsi/scsi_device.h | 1 - 9 files changed, 130 insertions(+), 118 deletions(-) -- 2.19.1.568.g152ad8e336-goog
Re: [PATCH] scsi/sd: Avoid that hibernation triggers a kernel warning
On Wed, 2018-10-17 at 13:27 -0700, Bart Van Assche wrote: > Although the power management code never calls the system-wide and runtime > suspend callbacks concurrently, runtime power state changes can happen > while the system is being suspended or resumed. See also the dpm_suspend() > and dpm_resume() calls in hibernation_snapshot(). Make sure the sd driver > supports this. This patch avoids that the following call trace is reported > during system-wide suspend [ ... ] This patch does not apply cleanly on Martin's 4.20/scsi-queue tree. I will rebase this patch, retest and repost it. Bart.
[PATCH] scsi/sd: Avoid that hibernation triggers a kernel warning
Although the power management code never calls the system-wide and runtime
suspend callbacks concurrently, runtime power state changes can happen
while the system is being suspended or resumed. See also the dpm_suspend()
and dpm_resume() calls in hibernation_snapshot(). Make sure the sd driver
supports this. This patch avoids that the following call trace is reported
during system-wide suspend:
WARNING: CPU: 0 PID: 701 at drivers/scsi/scsi_lib.c:3047
scsi_device_quiesce+0x4b/0xd0
Workqueue: events_unbound async_run_entry_fn
RIP: 0010:scsi_device_quiesce+0x4b/0xd0
Call Trace:
scsi_bus_suspend_common+0x71/0xe0
scsi_bus_freeze+0x15/0x20
dpm_run_callback+0x88/0x360
__device_suspend+0x1c4/0x840
async_suspend+0x1f/0xb0
async_run_entry_fn+0x6e/0x2c0
process_one_work+0x4ae/0xa20
worker_thread+0x63/0x5a0
kthread+0x1cf/0x1f0
ret_from_fork+0x24/0x30
Fixes: cd84a62e0078 ("block, scsi: Change the preempt-only flag into a counter")
Cc: Lee Duncan
Cc: Hannes Reinecke
Cc: Luis Chamberlain
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
Cc: Dan Williams
Cc: sta...@vger.kernel.org
Signed-off-by: Bart Van Assche
---
drivers/scsi/scsi_lib.c| 16 +---
include/scsi/scsi_device.h | 1 -
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 62348412ed1b..3106e910e766 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3040,13 +3040,11 @@ scsi_device_quiesce(struct scsi_device *sdev)
int err;
/*
-* It is allowed to call scsi_device_quiesce() multiple times from
-* the same context but concurrent scsi_device_quiesce() calls are
-* not allowed.
+* Since all scsi_device_quiesce() and scsi_device_resume() calls
+* are serialized it is safe to check the device state without holding
+* the SCSI device state mutex.
*/
- WARN_ON_ONCE(sdev->quiesced_by && sdev->quiesced_by != current);
-
- if (sdev->quiesced_by == current)
+ if (sdev->sdev_state == SDEV_QUIESCE)
return 0;
blk_set_pm_only(q);
@@ -3063,9 +3061,7 @@ scsi_device_quiesce(struct scsi_device *sdev)
mutex_lock(>state_mutex);
err = scsi_device_set_state(sdev, SDEV_QUIESCE);
- if (err == 0)
- sdev->quiesced_by = current;
- else
+ if (err)
blk_clear_pm_only(q);
mutex_unlock(>state_mutex);
@@ -3089,8 +3085,6 @@ void scsi_device_resume(struct scsi_device *sdev)
* device deleted during suspend)
*/
mutex_lock(>state_mutex);
- WARN_ON_ONCE(!sdev->quiesced_by);
- sdev->quiesced_by = NULL;
blk_clear_pm_only(sdev->request_queue);
if (sdev->sdev_state == SDEV_QUIESCE)
scsi_device_set_state(sdev, SDEV_RUNNING);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 202f4d6a4342..ef86c8adc5d5 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -226,7 +226,6 @@ struct scsi_device {
unsigned char access_state;
struct mutexstate_mutex;
enum scsi_device_state sdev_state;
- struct task_struct *quiesced_by;
unsigned long sdev_data[0];
} __attribute__((aligned(sizeof(unsigned long;
--
2.19.1.568.g152ad8e336-goog
Re: [PATCHv2] ib_srp: Remove WARN_ON in srp_terminate_io()
On 10/17/18 12:20 AM, Hannes Reinecke wrote:
The WARN_ON() is pointless as the rport is placed in SDEV_TRANSPORT_OFFLINE
at that time, so no new commands can be submitted via srp_queuecommand()
Signed-off-by: Hannes Reinecke
Reviewed-by: Jens Axboe
Reviewed-by: Johannes Thumshirn
---
drivers/infiniband/ulp/srp/ib_srp.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
b/drivers/infiniband/ulp/srp/ib_srp.c
index 0b34e909505f..5a79444c2f3c 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1334,13 +1334,6 @@ static void srp_terminate_io(struct srp_rport *rport)
struct scsi_device *sdev;
int i, j;
- /*
-* Invoking srp_terminate_io() while srp_queuecommand() is running
-* is not safe. Hence the warning statement below.
-*/
- shost_for_each_device(sdev, shost)
- WARN_ON_ONCE(sdev->request_queue->request_fn_active);
-
for (i = 0; i < target->ch_count; i++) {
ch = >ch[i];
Although I had explained before why I think that warning is not
pointless, I agree with this change because the legacy block layer is
going away. Anyway:
Acked-by: Bart Van Assche
Re: [PATCH 0/3] sd: Rely on the driver core for asynchronous probing
On Tue, 2018-10-16 at 01:19 -0400, Martin K. Petersen wrote:
> Bart,
>
> > During the 2018 edition of LSF/MM there was a session about increasing
> > SCSI disk probing concurrency. This patch series implements what has
> > been proposed during that session, namely: - Make sure that the driver
> > core is aware of asynchronous SCSI LUN probing. - Avoid unnecessary
> > serialization between sd_probe() and sd_remove() because this could
> > lead to a deadlock.
>
> I like it.
>
> What kind of testing have you done with $BIGNUM devices? Got any numbers
> to share?
Hi Martin,
For the following test:
modprobe -r scsi_debug; time modprobe scsi_debug delay=0 max_luns=256
I obtained the following time measurements:
* Kernel 4.15.0: 2.2s.
* Kernel 4.19-rc6 with this patch series applied: 2.3s.
This is not what I had expected. I think this small increase is because the
current sd code scans multiple LUNs associated with the same SCSI host
concurrently while apparently the device core scans such LUNs sequentially.
>From the driver core:
void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
{
[ ... ]
bus_for_each_drv(dev->bus, NULL, , __device_attach_driver);
[ ... ]
}
static int __device_attach(struct device *dev, bool allow_async)
{
[ ... ]
async_schedule(__device_attach_async_helper, dev);
[ ... ]
}
Do you want me to look into modifying __device_attach_async_helper() such
that it calls __device_attach_driver() concurrently instead of sequentially
in case of multiple LUNs?
Thanks,
Bart.
Re: [PATCH] IB/srp: remove old request_fn_active check
On Tue, 2018-10-16 at 08:31 -0600, Jens Axboe wrote:
> This check is only viable for non scsi-mq. Since that is going away,
> kill this legacy check.
>
> Cc: Bart Van Assche
> Cc: Parav Pandit
> Cc: linux-scsi@vger.kernel.org
> Signed-off-by: Jens Axboe
> ---
> drivers/infiniband/ulp/srp/ib_srp.c | 7 ---
> 1 file changed, 7 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
> b/drivers/infiniband/ulp/srp/ib_srp.c
> index 0b34e909505f..5a79444c2f3c 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1334,13 +1334,6 @@ static void srp_terminate_io(struct srp_rport *rport)
> struct scsi_device *sdev;
> int i, j;
>
> - /*
> - * Invoking srp_terminate_io() while srp_queuecommand() is running
> - * is not safe. Hence the warning statement below.
> - */
> - shost_for_each_device(sdev, shost)
> - WARN_ON_ONCE(sdev->request_queue->request_fn_active);
> -
> for (i = 0; i < target->ch_count; i++) {
> ch = >ch[i];
How about adding Hannes' Signed-off-by? See also
https://www.spinics.net/lists/linux-scsi/msg123488.html.
Anyway:
Reviewed-by: Bart Van Assche
Re: [PATCH 0/3] sd: Rely on the driver core for asynchronous probing
On 10/2/18 2:52 PM, Bart Van Assche wrote: During the 2018 edition of LSF/MM there was a session about increasing SCSI disk probing concurrency. This patch series implements what has been proposed during that session, namely: - Make sure that the driver core is aware of asynchronous SCSI LUN probing. - Avoid unnecessary serialization between sd_probe() and sd_remove() because this could lead to a deadlock. Has anyone already had the time to have a closer look at this patch series? Thanks, Bart.
Re: SCSI qla2xxx: tcm_qla2xxx server code seems to have regressed quite badly with latest testing
On 10/12/18 1:36 PM, Laurence Oberman wrote: > While I have for the longest time used 4.5 as a base for my F/C jammer > that I use every day here in our lab I recently added more jammer code > so I decided to test this all on latest upstream. > > Booting the target server on my 4.5 kernel with jammer code is > flawless and serves LUNS with no issues and handles the jamming also > fine. > > However just building a 4.19.0_rc7+-1 (I left the jammer stuff out) > its pretty broken. A large number of patches went upstream between these two kernel versions for both the QLogic initiator and target drivers. From the logs it seems like you were using QLogic hardware at both the initiator and target side? If so, which kernel version was running at the initiator side during these tests? 4.5, 4.19-rc7+ or yet another version? Thanks, Bart.
Re: [PATCH v2 4/5] target: split out helper for cxn timeout error stashing
On Fri, 2018-10-12 at 12:01 +0200, David Disseldorp wrote:
> +void iscsit_fill_cxn_timeout_err_stats(struct iscsi_session *sess)
> +{
> + struct iscsi_portal_group *tpg = sess->tpg;
> + struct iscsi_tiqn *tiqn = tpg->tpg_tiqn;
> +
> + if (!tiqn)
> + return;
> +
> + spin_lock_bh(>sess_err_stats.lock);
> + strcpy(tiqn->sess_err_stats.last_sess_fail_rem_name,
> + sess->sess_ops->InitiatorName);
There have been too many problems with strcpy() and buffer overflows in the
past. If the source and destination strings both have the same size, please
add a BUILD_BUG_ON() statement that verifies that at compile time. If that
not's the case, how about using strlcpy() to make it easy for anyone who
reads the source code that no output buffer overflow will occur?
Thanks,
Bart.
[PATCH 0/3] sd: Rely on the driver core for asynchronous probing
Hello Martin, During the 2018 edition of LSF/MM there was a session about increasing SCSI disk probing concurrency. This patch series implements what has been proposed during that session, namely: - Make sure that the driver core is aware of asynchronous SCSI LUN probing. - Avoid unnecessary serialization between sd_probe() and sd_remove() because this could lead to a deadlock. Please consider this patch series for kernel v4.20. Thanks, Bart. Bart Van Assche (3): sd: Rely on the driver core for asynchronous probing sd: Inline sd_probe_part2() __device_release_driver(): Do not wait for asynchronous probing drivers/base/dd.c| 3 -- drivers/scsi/scsi.c | 14 - drivers/scsi/scsi_pm.c | 22 +--- drivers/scsi/scsi_priv.h | 3 -- drivers/scsi/sd.c| 110 --- 5 files changed, 46 insertions(+), 106 deletions(-) -- 2.19.0.605.g01d371f741-goog
[PATCH 3/3] __device_release_driver(): Do not wait for asynchronous probing
Since __device_release_driver() is called with the device lock held
and since the same device lock is obtained by the functions that
perform asynchronous probing (driver_attach_async() and
__device_attach_async_helper()), asynchronous probing is already
serialized against __device_release_driver(). Remove the
async_synchronize_full() call from __device_release_driver() to
avoid that a deadlock is triggered.
See also commit 765230b5f084 ("driver-core: add asynchronous probing
support for drivers").
Signed-off-by: Bart Van Assche
Cc: Dmitry Torokhov
Cc: Lee Duncan
Cc: Hannes Reinecke
Cc: Luis R. Rodriguez
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
---
drivers/base/dd.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index edfc9f0b1180..d6520de659bd 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -926,9 +926,6 @@ static void __device_release_driver(struct device *dev,
struct device *parent)
drv = dev->driver;
if (drv) {
- if (driver_allows_async_probing(drv))
- async_synchronize_full();
-
while (device_links_busy(dev)) {
device_unlock(dev);
if (parent)
--
2.19.0.605.g01d371f741-goog
[PATCH 1/3] sd: Rely on the driver core for asynchronous probing
As explained during the LSF/MM session about increasing SCSI disk
probing concurrency, the problems with the current probing approach
are as follows:
- The driver core is unaware of asynchronous SCSI LUN probing.
wait_for_device_probe() waits for all asynchronous probes except
asynchronous SCSI disk probes.
- There is unnecessary serialization between sd_probe() and sd_remove().
This can lead to a deadlock.
Hence this patch that modifies the sd driver such that it uses the
driver core framework for asynchronous probing. The async domains
and get_device()/put_device() pairs that became superfluous due to
this change are removed.
See also commit 3c31b52f96f7 ("scsi: async sd resume").
Signed-off-by: Bart Van Assche
Cc: Lee Duncan
Cc: Hannes Reinecke
Cc: Luis R. Rodriguez
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
Cc: Dan Williams
---
drivers/scsi/scsi.c | 14 --
drivers/scsi/scsi_pm.c | 22 ++
drivers/scsi/scsi_priv.h | 3 ---
drivers/scsi/sd.c| 13 +++--
4 files changed, 5 insertions(+), 47 deletions(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index fc1356d101b0..1205369ad44f 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -85,19 +85,6 @@ unsigned int scsi_logging_level;
EXPORT_SYMBOL(scsi_logging_level);
#endif
-/* sd, scsi core and power management need to coordinate flushing async
actions */
-ASYNC_DOMAIN(scsi_sd_probe_domain);
-EXPORT_SYMBOL(scsi_sd_probe_domain);
-
-/*
- * Separate domain (from scsi_sd_probe_domain) to maximize the benefit of
- * asynchronous system resume operations. It is marked 'exclusive' to avoid
- * being included in the async_synchronize_full() that is invoked by
- * dpm_resume()
- */
-ASYNC_DOMAIN_EXCLUSIVE(scsi_sd_pm_domain);
-EXPORT_SYMBOL(scsi_sd_pm_domain);
-
/**
* scsi_put_command - Free a scsi command block
* @cmd: command block to free
@@ -839,7 +826,6 @@ static void __exit exit_scsi(void)
scsi_exit_devinfo();
scsi_exit_procfs();
scsi_exit_queue();
- async_unregister_domain(_sd_probe_domain);
}
subsys_initcall(init_scsi);
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index b44c1bb687a2..f229208bfef3 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -54,9 +54,6 @@ static int scsi_dev_type_suspend(struct device *dev,
const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
int err;
- /* flush pending in-flight resume operations, suspend is synchronous */
- async_synchronize_full_domain(_sd_pm_domain);
-
err = scsi_device_quiesce(to_scsi_device(dev));
if (err == 0) {
err = cb(dev, pm);
@@ -149,18 +146,7 @@ static int scsi_bus_resume_common(struct device *dev,
if (scsi_is_sdev_device(dev) && pm_runtime_suspended(dev))
blk_set_runtime_active(to_scsi_device(dev)->request_queue);
- if (fn) {
- async_schedule_domain(fn, dev, _sd_pm_domain);
-
- /*
-* If a user has disabled async probing a likely reason
-* is due to a storage enclosure that does not inject
-* staggered spin-ups. For safety, make resume
-* synchronous as well in that case.
-*/
- if (strncmp(scsi_scan_type, "async", 5) != 0)
- async_synchronize_full_domain(_sd_pm_domain);
- } else {
+ if (!fn) {
pm_runtime_disable(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
@@ -170,11 +156,7 @@ static int scsi_bus_resume_common(struct device *dev,
static int scsi_bus_prepare(struct device *dev)
{
- if (scsi_is_sdev_device(dev)) {
- /* sd probing uses async_schedule. Wait until it finishes. */
- async_synchronize_full_domain(_sd_probe_domain);
-
- } else if (scsi_is_host_device(dev)) {
+ if (scsi_is_host_device(dev)) {
/* Wait until async scanning is finished */
scsi_complete_async_scans();
}
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 99f1db5e467e..b9fa363b4bbb 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -175,9 +175,6 @@ static inline int scsi_autopm_get_host(struct Scsi_Host *h)
{ return 0; }
static inline void scsi_autopm_put_host(struct Scsi_Host *h) {}
#endif /* CONFIG_PM */
-extern struct async_domain scsi_sd_pm_domain;
-extern struct async_domain scsi_sd_probe_domain;
-
/* scsi_dh.c */
#ifdef CONFIG_SCSI_DH
void scsi_dh_add_device(struct scsi_device *sdev);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4a57ffecc7e6..48a133a3aab8 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -559,6 +559,7 @@ static struct scsi_driver sd_template = {
.name = "sd&qu
[PATCH 2/3] sd: Inline sd_probe_part2()
This patch does not change any functionality.
Signed-off-by: Bart Van Assche
Cc: Lee Duncan
Cc: Hannes Reinecke
Cc: Luis R. Rodriguez
Cc: Johannes Thumshirn
Cc: Christoph Hellwig
Cc: Greg Kroah-Hartman
---
drivers/scsi/sd.c | 101 --
1 file changed, 43 insertions(+), 58 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 48a133a3aab8..93732f1fb551 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3232,63 +3232,6 @@ static int sd_format_disk_name(char *prefix, int index,
char *buf, int buflen)
return 0;
}
-static void sd_probe_part2(struct scsi_disk *sdkp)
-{
- struct scsi_device *sdp;
- struct gendisk *gd;
- u32 index;
- struct device *dev;
-
- sdp = sdkp->device;
- gd = sdkp->disk;
- index = sdkp->index;
- dev = >sdev_gendev;
-
- gd->major = sd_major((index & 0xf0) >> 4);
- gd->first_minor = ((index & 0xf) << 4) | (index & 0xfff00);
-
- gd->fops = _fops;
- gd->private_data = >driver;
- gd->queue = sdkp->device->request_queue;
-
- /* defaults, until the device tells us otherwise */
- sdp->sector_size = 512;
- sdkp->capacity = 0;
- sdkp->media_present = 1;
- sdkp->write_prot = 0;
- sdkp->cache_override = 0;
- sdkp->WCE = 0;
- sdkp->RCD = 0;
- sdkp->ATO = 0;
- sdkp->first_scan = 1;
- sdkp->max_medium_access_timeouts = SD_MAX_MEDIUM_TIMEOUTS;
-
- sd_revalidate_disk(gd);
-
- gd->flags = GENHD_FL_EXT_DEVT;
- if (sdp->removable) {
- gd->flags |= GENHD_FL_REMOVABLE;
- gd->events |= DISK_EVENT_MEDIA_CHANGE;
- }
-
- blk_pm_runtime_init(sdp->request_queue, dev);
- device_add_disk(dev, gd);
- if (sdkp->capacity)
- sd_dif_config_host(sdkp);
-
- sd_revalidate_disk(gd);
-
- if (sdkp->security) {
- sdkp->opal_dev = init_opal_dev(sdp, _sec_submit);
- if (sdkp->opal_dev)
- sd_printk(KERN_NOTICE, sdkp, "supports TCG Opal\n");
- }
-
- sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
- sdp->removable ? "removable " : "");
- scsi_autopm_put_device(sdp);
-}
-
/**
* sd_probe - called during driver initialization and whenever a
* new scsi device is attached to the system. It is called once
@@ -3378,7 +3321,49 @@ static int sd_probe(struct device *dev)
get_device(dev);
dev_set_drvdata(dev, sdkp);
- sd_probe_part2(sdkp);
+ gd->major = sd_major((index & 0xf0) >> 4);
+ gd->first_minor = ((index & 0xf) << 4) | (index & 0xfff00);
+
+ gd->fops = _fops;
+ gd->private_data = >driver;
+ gd->queue = sdkp->device->request_queue;
+
+ /* defaults, until the device tells us otherwise */
+ sdp->sector_size = 512;
+ sdkp->capacity = 0;
+ sdkp->media_present = 1;
+ sdkp->write_prot = 0;
+ sdkp->cache_override = 0;
+ sdkp->WCE = 0;
+ sdkp->RCD = 0;
+ sdkp->ATO = 0;
+ sdkp->first_scan = 1;
+ sdkp->max_medium_access_timeouts = SD_MAX_MEDIUM_TIMEOUTS;
+
+ sd_revalidate_disk(gd);
+
+ gd->flags = GENHD_FL_EXT_DEVT;
+ if (sdp->removable) {
+ gd->flags |= GENHD_FL_REMOVABLE;
+ gd->events |= DISK_EVENT_MEDIA_CHANGE;
+ }
+
+ blk_pm_runtime_init(sdp->request_queue, dev);
+ device_add_disk(dev, gd);
+ if (sdkp->capacity)
+ sd_dif_config_host(sdkp);
+
+ sd_revalidate_disk(gd);
+
+ if (sdkp->security) {
+ sdkp->opal_dev = init_opal_dev(sdp, _sec_submit);
+ if (sdkp->opal_dev)
+ sd_printk(KERN_NOTICE, sdkp, "supports TCG Opal\n");
+ }
+
+ sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
+ sdp->removable ? "removable " : "");
+ scsi_autopm_put_device(sdp);
return 0;
--
2.19.0.605.g01d371f741-goog
Re: [PATCH v7 3/8] uapi: ufs: Make utp_upiu_req visible to user space
On Sun, 2018-09-30 at 08:45 +0300, Avri Altman wrote: > +++ b/include/uapi/scsi/scsi_bsg_ufs.h > @@ -0,0 +1,74 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * TBD - UFS Transport SGIO v4 BSG Message Support > + * > + * Copyright (C) 2011-2013 Samsung India Software Operations > + */ > +#ifndef SCSI_BSG_UFS_H > +#define SCSI_BSG_UFS_H > + > +/* > + * This file intended to be included by both kernel and user space > + */ > + > +#define MAX_CDB_SIZE 16 Please rename this constant such that it has either "UFS" or "UTP" in its name. This name is too generic for a protocol-specific header file. Thanks, Bart.
Re: aacraid: latest driver results in Host adapter abort request. / Outstanding commands on (0,0,0,0):
On 9/18/18 11:10 PM, Stefan Priebe - Profihost AG wrote:
after upgrading the aacraid driver / kernel from aacraid 50792 to
aacraid 50877.
The aacraid driver version was updated to 50792 in commit 0662cc968ace
("scsi: aacraid: Update driver version") and to 50877 in commit
1cdb74b80f93 ("scsi: aacraid: Update driver version to 50877"). That
means that the regression you encountered got introduced after commit
0662cc968ace. 114 changes got checked in after that commit. That's too
much to find the root cause by rereading all these changes. Is there any
way to trigger the problem faster such that it becomes feasible to run a
bisect?
$ git log 0662cc968ace..master drivers/scsi/aacraid | grep -c ^commit
114
Bart.
Re: [PATCH] ib_srp: Remove WARN_ON in srp_terminate_io()
On 9/21/18 5:15 AM, Hannes Reinecke wrote:
The WARN_ON() is pointless as the rport is placed in SDEV_TRANSPORT_OFFLINE
at that time, so no new commands can be submitted via srp_queuecomment()
Signed-off-by: Hannes Reinecke
---
drivers/infiniband/ulp/srp/ib_srp.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
b/drivers/infiniband/ulp/srp/ib_srp.c
index 444d16520506..4f6e88136da3 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1334,13 +1334,6 @@ static void srp_terminate_io(struct srp_rport *rport)
struct scsi_device *sdev;
int i, j;
- /*
-* Invoking srp_terminate_io() while srp_queuecommand() is running
-* is not safe. Hence the warning statement below.
-*/
- shost_for_each_device(sdev, shost)
- WARN_ON_ONCE(sdev->request_queue->request_fn_active);
-
for (i = 0; i < target->ch_count; i++) {
ch = >ch[i];
Since this function is not in the hot path and since this function is
called indirectly, I'd like to keep the above code as documentation of
the requirements for callers of this function.
Please also Cc the linux-rdma mailing list for SRP initiator changes.
Thanks,
Bart.
Re: [PATCH v5 3/6] scsi: ufs: Add ufs-bsg module
On 9/17/18 9:08 AM, Avri Altman wrote: +*Caveat emptor*: The driver makes no further input validations and sends the +UPIU to the device as it is. Open the bsg device in /dev/ufs-bsg-%0:0 and +send SG_IO with the applicable sg_io_v4: It's not clear to me what %0:0 stands for. Otherwise this patch series looks good to me. Thanks, Bart.
Re: [PATCH 2/6] qla2xxx_nvmet: Added Makefile and Kconfig changes
On 9/17/18 12:07 PM, Madhani, Himanshu wrote: On Sep 15, 2018, at 9:53 PM, Bart Van Assche wrote: External Email On 09/14/18 14:28, Himanshu Madhani wrote: diff --git a/drivers/scsi/qla2xxx/Kconfig b/drivers/scsi/qla2xxx/Kconfig index 036cc3f217b1..f1539d8b68ef 100644 --- a/drivers/scsi/qla2xxx/Kconfig +++ b/drivers/scsi/qla2xxx/Kconfig @@ -3,6 +3,7 @@ config SCSI_QLA_FC depends on PCI && SCSI depends on SCSI_FC_ATTRS depends on NVME_FC || !NVME_FC + depends on NVME_TARGET_FC || !NVME_TARGET_FC Why has this line been added? We’ve added this to indicate NVMe Target support with the driver. anything that I am missing? Hello Himanshu, Does the qla2xxx driver build with CONFIG_NVME_TARGET_FC=y, CONFIG_NVME_TARGET_FC=m and CONFIG_NVME_TARGET_FC=n? If so, why do you think that the line "depends on NVME_TARGET_FC || !NVME_TARGET_FC" is necessary? Thanks, Bart.
Re: [PATCH v4 0/7] scsi: ufs bsg endpoint
On 09/15/18 23:41, Avri Altman wrote: Since this patch series does not touch any include/uapi header and since no uapi UFS header files already exist, how is user space software expected to know which message format it should use for communicating over the UFS BSG endpoint? I don't think that "read the source" is an acceptable answer. I was thinking in V5 to move some of the notations from include/uapi to ufs_bsg.h. Or, I can add a "bsg support" paragraph to Documentation/scsi/ufs.txt, Or even a new entry to Documentation/ABI/testing/ - whatever you think is best? Adding a paragraph to Documentation/scsi/ufs.txt sounds like a good idea to me. Thanks, Bart.
Re: [PATCH 5/6] qla2xxx_nvmet: Add SysFS node for FC-NVMe Target
On 09/14/18 14:28, Himanshu Madhani wrote:
From: Anil Gurumurthy
This patch adds SysFS node for NVMe Target configuration
Please elaborate the description of this patch. Are NVMe initiator and
target mode mutually exclusive or can both be enabled at the same time?
What is the impact of enabling NVMe target mode on FC target mode? I was
surprised to see the following in patch 4/6:
- if (qla_tgt_mode_enabled(vha) ||
- qla_dual_mode_enabled(vha))
+ if ((qla_tgt_mode_enabled(vha) ||
+qla_dual_mode_enabled(vha)) &&
+ qlt_op_target_mode) {
Does this mean that with NVMe target mode disabled that FC target mode
is also disabled?
Thanks,
Bart.
Re: [PATCH 3/6] qla2xxx_nvmet: Add FC-NVMe Target LS request handling
On 09/14/18 14:28, Himanshu Madhani wrote: From: Anil Gurumurthy This patch adds LS handling into driver Please make the patch description more clear. What does "LS" stand for? What is the relationship between LS and NVMe target support? Thanks, Bart.
Re: [PATCH 5/6] qla2xxx_nvmet: Add SysFS node for FC-NVMe Target
On 09/14/18 14:28, Himanshu Madhani wrote: @@ -686,7 +686,7 @@ static void qla_nvmet_send_resp_ctio(struct qla_qpair *qpair, ctio->u.nvme_status_mode1.transfer_len = cpu_to_be32(ersp->xfrd_len); - ql_log(ql_log_info, vha, 0x1100f, +ql_dbg(ql_dbg_nvme, vha, 0x1100f, "op: %#x, rsplen: %#x\n", rsp_buf->op, rsp_buf->rsplen); } else Have you verified this patch series with checkpatch? I think the above change introduces a whitespace violation. Bart.
Re: [PATCH 2/6] qla2xxx_nvmet: Added Makefile and Kconfig changes
On 09/14/18 14:28, Himanshu Madhani wrote: diff --git a/drivers/scsi/qla2xxx/Kconfig b/drivers/scsi/qla2xxx/Kconfig index 036cc3f217b1..f1539d8b68ef 100644 --- a/drivers/scsi/qla2xxx/Kconfig +++ b/drivers/scsi/qla2xxx/Kconfig @@ -3,6 +3,7 @@ config SCSI_QLA_FC depends on PCI && SCSI depends on SCSI_FC_ATTRS depends on NVME_FC || !NVME_FC + depends on NVME_TARGET_FC || !NVME_TARGET_FC Why has this line been added? Please also consider to fold this patch into the first patch. I don't think it makes sense that patches 1/6 and 2/6 are separate. Bart.
Re: [PATCH 1/6] qla2xxx_nvmet: Add files for FC-NVMe Target support
On 09/14/18 14:28, Himanshu Madhani wrote: diff --git a/drivers/scsi/qla2xxx/qla_nvmet.c b/drivers/scsi/qla2xxx/qla_nvmet.c new file mode 100644 [ ... ] +#ifIS_ENABLED(CONFIG_NVME_TARGET_FC) [ ... ] +#endif This style of using #if / #endif is not acceptable. Instead, drivers/scsi/qla2xxx/Makefile should be modified such that qla_nvmet.c is only built if CONFIG_NVME_TARGET_FC has been set. Bart.
Re: [PATCH v3 1/7] scsi: ufs: Add ufs-bsg module
On Mon, 2018-09-03 at 13:33 +0300, Avri Altman wrote:
> [ ... ]
> +++ b/include/uapi/scsi/scsi_bsg_ufs.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * UFS Transport SGIO v4 BSG Message Support
> + *
> + * Copyright (C) 2018 Western Digital Corporation
> + */
> +#ifndef SCSI_BSG_UFS_H
> +#define SCSI_BSG_UFS_H
> +
> +/*
> + * This file intended to be included by both kernel and user space
> + */
> +
> +
> +/**
> + * struct ufs_bsg_upiu - upiu transaction structure
> + *
> + * @header: upiu header
> + * @tsf: Transaction Specific Fields
> + * @data: payload pointer
> + *
> + * This structure supports all ufs transaction types per JEDEC
> UFSv2.1
> + * paragraph 10.7
> + */
> +struct ufs_bsg_upiu {
> + uint32_t header[3];
> + uint32_t tsf[5];
> + uint8_t data[0];
> +};
In addition to Christoph's comments: is this a redefinition of an
existing data structure (struct utp_upiu_header)? Please do not
introduce variants of existing data structures but instead proceed as
follows:
- Move the relevant existing data structures (utp_upiu_header,
utp_upiu_query, ...) from drivers/scsi/ufs/ufs.h into a header file
under include/uapi.
- Add a new patch at the beginning of this series that does nothing
else than moving these data structures.
- Use the existing data structures instead of introducing struct ufs_bsg_upiu.
Thanks,
Bart.
Re: [PATCH v2 1/8] scsi: Add ufs transport class
On Tue, 2018-08-14 at 11:42 +, Stanislav Nijnikov wrote: > From: Bart Van Assche > > Are you perhaps referring to the transport_class_register() calls in SCSI > > transport drivers? From what I see in existing SCSI transport drivers the > > transport_class_register() function is used to register link, port, host, > > vport, rport and other objects. I don't think that a SCSI transport driver > > is required to register host and port objects. > > > > Maybe we should take a step back and discuss first why the new bsg queues > > are registered by a transport driver? Since in case of UFS as far as I can > > see there is no real need to introduce a transport driver other than for > > creating the bsg device nodes, have you considered to add the code for > > creating bsg device nodes to the UFS driver instead of in a UFS transport > > driver? I think transport drivers were introduced as a way to share code > > between multiple SCSI LLDs that use the same transport mechanism. In the > > case of UFS there is only one SCSI LLD. Hence I'm wondering whether we > > really need an UFS transport driver. > > At the moment, the SCSI transport related code could be found at > driver/scsi/scsi_transport_* files. > What is a point of hiding the UFS transport code inside the UFS driver? Have you tried to implement the approach I proposed? If so, did you encounter any issues that made it impossible to implement that approach? If you have not yet tried to implement the above proposal, what are you waiting for? Bart.
Re: BUG in slab_free after iSCSI login timeout
On Sat, 2018-08-11 at 09:36 +, Vincent Pelletier wrote: > What can I try to help debug this further ? Can you try to reproduce this with KASAN enabled in the kernel config? Thanks, Bart.
Re: [PATCH v2 1/8] scsi: Add ufs transport class
On Thu, 2018-08-09 at 23:32 +, Avri Altman wrote: > And as I said before, we think that maintaining the flexibility to have > more-than-one > bsg device nodes, will be useful serving as a testing and validation > environment. That is a very vague statement. Please clarify. > >> "... > >> In addition to the basic SCSI core objects this transport class > >> introduces two additional (currently empty) class objects: > >> “ufs-host” and “ufs-port”. There is only one “ufs-host” in the > >> system, but can be more-than-one “ufs-ports”. > >> > >> ..." > > >Since both the ufs-host and ufs-port objects are empty, can both be left out? > But mustn't I declare those classes for the various components of the scsi > transport to work? Are you perhaps referring to the transport_class_register() calls in SCSI transport drivers? From what I see in existing SCSI transport drivers the transport_class_register() function is used to register link, port, host, vport, rport and other objects. I don't think that a SCSI transport driver is required to register host and port objects. Maybe we should take a step back and discuss first why the new bsg queues are registered by a transport driver? Since in case of UFS as far as I can see there is no real need to introduce a transport driver other than for creating the bsg device nodes, have you considered to add the code for creating bsg device nodes to the UFS driver instead of in a UFS transport driver? I think transport drivers were introduced as a way to share code between multiple SCSI LLDs that use the same transport mechanism. In the case of UFS there is only one SCSI LLD. Hence I'm wondering whether we really need an UFS transport driver. Thanks, Bart.
Re: [PATCH v2 1/8] scsi: Add ufs transport class
On Thu, 2018-08-09 at 16:22 +, Avri Altman wrote: > > -Original Message- > > From: Bart Van Assche > > Sent: Wednesday, August 08, 2018 7:58 PM > > To: h...@lst.de; Avri Altman ; linux-scsi@vger.kernel.org; > > jthumsh...@suse.de; h...@suse.com; martin.peter...@oracle.com; > > j...@linux.vnet.ibm.com > > Cc: Vinayak Holikatti ; Avi Shchislowski ; Alex Lemberg ; Stanislav > > Nijnikov ; > > subha...@codeaurora.org > > Subject: Re: [PATCH v2 1/8] scsi: Add ufs transport class > > > > On Sun, 2018-08-05 at 14:39 +0300, Avri Altman wrote: > > > A “ufs-port” is purely a software object. Evidently, the function > > > template takes no port as an argument, as the driver has no concept > > > of "port". We only need it as a hanging point in the bsg device tree, > > > so maybe a more appropriate term would be: "ufs-bsg-dev-node". > > > > > > The ufs-port has a pretty lean structure. This is because we are > > > using upiu transactions that contains the outmost detailed info, > > > so we don't really need complex constructs to support it. > > > > > > The transport will keep track of its ufs-ports via its scsi host. > > > > Since no port concept has been defined in the UFS standard, can the port > > concept be left out? Have you considered to attach the bsg queue directly > > to the UFS SCSI host? There are two struct device instances in each > > Scsi_Host > > structure, namely shost_gendev and shost_dev. I think the former > > corresponds > > to /sys/bus/scsi/devices/host and the latter corresponds to > > /sys/class/scsi_host/host. The bsg queue probably can be attached to the > > latter. Several SCSI drivers already add additional sysfs attributes to the > > /sys/class/scsi_host/host. > > Yeah - so today we are using shost_gendev, and the newly created classes > points to /sys/devices/... : > > htc_ocnwhl:/sys/class/ufs_host # ls -la > lrwxrwxrwx 1 root root 0 2018-06-17 13:15 host0 -> > ../../devices/soc/1da4000.ufshc/host0/ufs_host/host0 > > htc_ocnwhl:/sys/class/ufs_ports # ls -la > lrwxrwxrwx 1 root root 0 2018-06-17 13:05 ufs-port-0:1 -> > ../../devices/soc/1da4000.ufshc/host0/ufs-port-0:1/ufs_ports/ufs-port-0:0 > > and under /dev/ we get: > htc_ocnwhl:/dev # ls -la | grep ufs > crw--- 1 root root 246, 3 1970-02-26 03:12 ufs-port-0:0 > > We didn't add any sysfs attribute to those classes, and I don't expect any to > be added, > as I tried to explain in the commit: > "Those classes are left empty for now, as ufs-sysfs already contains > an abundant amount of attributes." > > Practically, this infrastructure provides the bsg device files /dev/. > > Anyway, if you think it's better, I will try to switch it as you suggested. My concern is that no port concept is defined in the UFS standard and hence that users will be confused if they see a UFS port object in sysfs. > Can you please refer me to those scsi drivers that you mentioned? The following command will yield plenty of examples: git grep -nHE 'device_(create|remove)_file|shost_attrs' drivers/scsi drivers/infiniband Thanks, Bart.
Re: [PATCH v2 1/8] scsi: Add ufs transport class
On Sun, 2018-08-05 at 14:39 +0300, Avri Altman wrote: > A “ufs-port” is purely a software object. Evidently, the function > template takes no port as an argument, as the driver has no concept > of "port". We only need it as a hanging point in the bsg device tree, > so maybe a more appropriate term would be: "ufs-bsg-dev-node". > > The ufs-port has a pretty lean structure. This is because we are > using upiu transactions that contains the outmost detailed info, > so we don't really need complex constructs to support it. > > The transport will keep track of its ufs-ports via its scsi host. Since no port concept has been defined in the UFS standard, can the port concept be left out? Have you considered to attach the bsg queue directly to the UFS SCSI host? There are two struct device instances in each Scsi_Host structure, namely shost_gendev and shost_dev. I think the former corresponds to /sys/bus/scsi/devices/host and the latter corresponds to /sys/class/scsi_host/host. The bsg queue probably can be attached to the latter. Several SCSI drivers already add additional sysfs attributes to the /sys/class/scsi_host/host. Thanks, Bart.
Re: [PATCH v2 7/8] scsi: ufs-bsg: Add support for raw upiu in ufs_bsg_request()
On Sun, 2018-08-05 at 14:39 +0300, Avri Altman wrote: > + if (ret || !(*desc_len)) > + return -EINVAL; No parentheses around *desc_len please. Thanks, Bart.
Re: [PATCH v2 6/8] scsi: ufs: Add API to execute raw upiu commands
On Sun, 2018-08-05 at 14:39 +0300, Avri Altman wrote: > + lrbp->command_type = (hba->ufs_version == UFSHCI_VERSION_10 || > + hba->ufs_version == UFSHCI_VERSION_11) ? > + UTP_CMD_TYPE_DEV_MANAGE : > + UTP_CMD_TYPE_UFS_STORAGE; Parentheses are not needed around the logical or expression because logical or has a higher precedence than ?: so please leave the parentheses out. Thanks, Bart.
Re: [PATCH v2 2/8] scsi: ufs: Add ufs-bsg module
On Sun, 2018-08-05 at 14:39 +0300, Avri Altman wrote: > A LLD companion for the ufs scsi transport. This description is misleading. How about changing this into "Add a bsg endpoint that supports UPIUs"? diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c > new file mode 100644 > index 000..6f1941b > --- /dev/null > +++ b/drivers/scsi/ufs/ufs_bsg.c > @@ -0,0 +1,127 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * SCSI transport companion for UFS HBA This description is misleading too. Please consider to change this into something like "bsg endpoint that supports UPIUs". Thanks, Bart.
Re: [PATCH] qla2xxx: Fix issue reported by static checker for qla2x00_els_dcmd2_sp_done()
On Tue, 2018-08-07 at 20:39 -0700, Himanshu Madhani wrote: > From: Quinn Tran To me this seems to be a real bug fix rather than a patch that only suppresses a static checker complaint. If that is the case, please mention this in the patch subject. Thanks, Bart.
Re: [PATCH] mpt3sas: correct reset of smid while clearing scsi tracker
On Wed, 2018-07-18 at 01:22 -0400, Sreekanth Reddy wrote: > In mpt3sas_base_clear_st() function smid value is reseted in wrong line, > i.e. driver should reset smid value to zero after decrementing chain_offset > counter in chain_lookup table but in current code, driver is resetting smid > value before decrementing the chain_offset counter. which we are correcting > with this patch. > > Signed-off-by: Sreekanth Reddy > --- > drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c > b/drivers/scsi/mpt3sas/mpt3sas_base.c > index 902610d..94b939b 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -3283,8 +3283,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > return; > st->cb_idx = 0xFF; > st->direct_io = 0; > - st->smid = 0; > atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); > + st->smid = 0; > } > > /** Reviewed-by: Bart Van Assche
Re: [PATCH v3] mpt3sas: correct reset of smid while clearing scsi tracker
On Tue, 2018-08-07 at 21:46 +0530, Sreekanth Reddy wrote: > [Sreekanth] In the patch description I mentioned that I have done a > manual mistake and I am correcting with this patch. > > Hope I have answered all of your quires. Not yet unfortunately. Why do you consider the current implementation a wrong? Why is the patch at the start of this e-mail thread considered a fix? Which behavior changes due to this patch? If this patch is a bug fix, how can the bug be triggered and why is this patch considered the right way to fix that bug? Thanks, Bart.
Re: [PATCH v3] mpt3sas: correct reset of smid while clearing scsi tracker
On Tue, 2018-08-07 at 20:03 +0530, Sreekanth Reddy wrote: > The main intention of this patch to reset the smid to zero after > resetting the corresponding smid entry's chain_offset to zero. While > posting "mpt3sas: Fix calltrace observed while running IO & reset" > patch I have done manual mistake that I reset the smid to zero before > resetting the chain_offset which is wrong. Due to which driver > always's reset the chain_offset of zero th entry of chain_lookup[] > table which is wrong and hence I am correcting it with this patch. > > After that you asked me to add memory barriers and hence I have added > memory barriers in subsequent versions of the patch. Hello Sreekanth, Please answer the questions I already asked you twice instead of sidestepping these. Thanks, Bart.
Re: [PATCH v3] mpt3sas: correct reset of smid while clearing scsi tracker
On Tue, 2018-08-07 at 12:10 +0530, Sreekanth Reddy wrote: > This function _base_get_chain_buffer_tracker() is called only during > the IO submission time and mpt3sas_base_clear_st() is called during IO > completion time and hence I don't see any race condition here. > > Currently this patch is very much needed, so please consider this > patch. May be we can start new email thread to discuss on the possible > race conditions you are observing and I can clear your quires or I can > fix them as on we can find the real race conditions. Hello Sreekanth, How can this patch be necessary if there are no races between I/O submission and I/O completion? Changing the order of two memory writes only makes sense if there is a race condition involved. Since the block layer allocates a request tag before scsih_qcmd() is called and since mpt3sas_base_free_smid() is called before scmd->scsi_done(scmd) in _scsih_io_done(), the request tag is freed after the smid has been freed when I/O completes in a normal way. So there shouldn't be a race condition in that scenario. By the way, you have not answered my question about why development of this patch started. Does this patch address a theoretical concern or a real bug? In the latter case, which scenario triggers the addressed bug? If you want this patch to go upstream you should be able to explain why you think it is useful, and that description should be included in the patch description. Thanks, Bart.
Re: [PATCH v2] target: fix tcm_loop build errors when SCSI=m
On Mon, 2018-08-06 at 16:29 -0700, Randy Dunlap wrote:
> On 08/06/2018 04:26 PM, Bart Van Assche wrote:
> > On Mon, 2018-08-06 at 16:20 -0700, Randy Dunlap wrote:
> > > Fixes: 3703b2c5d041 ("[SCSI] tcm_loop: Add multi-fabric Linux/SCSI LLD
> > >fabric module")
> >
> > From drivers/target/Kconfig on Linus' master branch:
> >
> > menuconfig TARGET_CORE
> > tristate "Generic Target Core Mod (TCM) and ConfigFS Infrastructure"
> > depends on SCSI && BLOCK
> >
> > if TARGET_CORE
> > [ ... ]
> > source "drivers/target/loopback/Kconfig"
> > endif
> >
> > In other words, the loopback driver already depends on SCSI. So I doubt that
> > this is a longstanding issue. Did you encounter this with Linus' master
> > branch
> > or rather with linux-next?
>
> Darn. Thanks for the info.
>
> I encountered it in linux-next. I'll dig deeper.
This patch series may be what you are looking for: "[PATCH v2 0/9] block:
Consolidate scsi sense buffer usage" (https://lkml.org/lkml/2018/7/31/829).
Bart.
Re: [PATCH v2] target: fix tcm_loop build errors when SCSI=m
On Mon, 2018-08-06 at 16:20 -0700, Randy Dunlap wrote:
> Fixes: 3703b2c5d041 ("[SCSI] tcm_loop: Add multi-fabric Linux/SCSI LLD
>fabric module")
>From drivers/target/Kconfig on Linus' master branch:
menuconfig TARGET_CORE
tristate "Generic Target Core Mod (TCM) and ConfigFS Infrastructure"
depends on SCSI && BLOCK
if TARGET_CORE
[ ... ]
source "drivers/target/loopback/Kconfig"
endif
In other words, the loopback driver already depends on SCSI. So I doubt that
this is a longstanding issue. Did you encounter this with Linus' master branch
or rather with linux-next?
Thanks,
Bart.
Re: [PATCH v3] mpt3sas: correct reset of smid while clearing scsi tracker
On Sat, 2018-08-04 at 18:56 +0530, Sreekanth Reddy wrote: > No Bart, their is no race condition here. Since chain lookup table > entry is uniquely accessed using smid value. And this smid (which is > scmd->request->tag +1) is unique for each IO request. And > _base_get_chain_buffer_tracker() function is called only during IO > submission time (i.e. before submitting the IO to the IOC firmware) > and mpt3sas_base_clear_st() function is called in the ISR (i.e during > IO completion) path. So for a particular IO these two functions called > at two different instances of time. Hello Sreekanth, In mpt3sas_base_get_smid_scsiio() I found the following code: struct scsiio_tracker *request = scsi_cmd_priv(scmd); u16 smid = scmd->request->tag + 1; request->smid = smid; That confirms what you wrote about smid being unique for each I/O request. However, this also raises new questions. As far as I can see all code in the I/O path that accesses the chain_lookup[] array uses index smid - 1. The patch at the start of this e-mail thread modifies two functions, namely mpt3sas_remove_dead_ioc_func() and mpt3sas_base_clear_st(). Since the chain_lookup[] array is indexed with the smid and hence contains request- private data, how is it even possible that these functions race against each other? Is there perhaps code that accesses the chain_lookup[] array and that is called from another context than the I/O completion path? Is the patch at the start of this e-mail thread the result of a theoretical concern or of a real failure? In the latter case, which sequence of actions triggered the failure? The answer to this question should already have been mentioned in the description of v1 of this patch. Thanks, Bart.
Re: [PATCH v3] mpt3sas: correct reset of smid while clearing scsi tracker
On Fri, 2018-08-03 at 12:16 -0400, Sreekanth Reddy wrote: > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c > b/drivers/scsi/mpt3sas/mpt3sas_base.c > index 902610d..2c5a5b4 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -1702,6 +1702,13 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) > return NULL; > > chain_req = >chain_lookup[smid - 1].chains_per_smid[chain_offset]; > + > + /* > + * Added memory barrier to make sure that correct chain tracker > + * is retrieved before incrementing the smid pool's chain_offset > + * value in chain lookup table. > + */ > + smp_mb(); > atomic_inc(>chain_lookup[smid - 1].chain_offset); > return chain_req; > } > @@ -3283,8 +3290,15 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > return; > st->cb_idx = 0xFF; > st->direct_io = 0; > - st->smid = 0; > atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); > + > + /* > + * Added memory barrier to make sure that smid is set to zero > + * only after resetting corresponding smid pool's chain_offset to zero > + * in chain lookup table. > + */ > + smp_mb(); > + st->smid = 0; > } Thanks for having addressed previous review comments. Hence: Reviewed-by: Bart Van Assche However, I'm not yet convinced that this patch is sufficient to fix the race between _base_get_chain_buffer_tracker() and mpt3sas_base_clear_st(). Can e.g. the atomic_set() that resets chain_offset occur after it has been read by _base_get_chain_buffer_tracker() and before that function increments the chain_offset member variable? Thanks, Bart.
Re: [PATCH v2] mpt3sas: correct reset of smid while clearing scsi tracker
On Fri, 2018-08-03 at 18:55 +0300, Andy Shevchenko wrote: > On Fri, Aug 3, 2018 at 5:06 PM, Bart Van Assche > wrote: > > On Fri, 2018-08-03 at 06:31 -0400, Sreekanth Reddy wrote: > > > + /* Added memory barrier to make sure that correct chain tracker > > > + * is retrieved before incrementing the smid pool's chain_offset > > > + * value in chain lookup table. > > > + */ > > > > Please verify your patch with checkpatch and use the proper kernel comment > > style. > > This kind of style is what net subsystem is using. Last time I heard > about this was that checkpatch doesn't distinguish subsystem and thus > allows this kind of style. Since consistency is appreciated in Linux kernel code and since most other comments in SCSI code start with "/*" on a line by itself I think that's the preferred style for SCSI code. Thanks, Bart.
Re: [PATCH v2] mpt3sas: correct reset of smid while clearing scsi tracker
On Fri, 2018-08-03 at 06:31 -0400, Sreekanth Reddy wrote: > In mpt3sas_base_clear_st() function smid value is reseted in ^^^ That's not an English word. Please read https://forum.wordreference.com/threads/set-reset-vs-setted-resetted-programming.70776/ > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c > b/drivers/scsi/mpt3sas/mpt3sas_base.c > index 902610d..94f7236 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -1702,6 +1702,11 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) > return NULL; > > chain_req = >chain_lookup[smid - 1].chains_per_smid[chain_offset]; > + /* Added memory barrier to make sure that correct chain tracker > + * is retrieved before incrementing the smid pool's chain_offset > + * value in chain lookup table. > + */ Please verify your patch with checkpatch and use the proper kernel comment style. > + smp_mb__before_atomic(); The purpose of smp_mb__before_atomic() / smp_mb__after_atomic() is to implement acquire / release semantics. I think what you need here is a regular barrier (smp_mb()). > @@ -3283,8 +3288,13 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > return; > st->cb_idx = 0xFF; > st->direct_io = 0; > - st->smid = 0; > atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); > + /* Added memory barrier to make sure that smid is set to zero > + * only after resetting corresponding smid pool's chain_offset to zero > + * in chain lookup table. > + */ > + smp_mb__after_atomic(); > + st->smid = 0; The two above comments also apply to these two changes. Thanks, Bart.
Re: [PATCH v1] mpt3sas: correct reset of smid while clearing scsi tracker
On Tue, 2018-07-31 at 01:35 -0400, Sreekanth Reddy wrote: > In mpt3sas_base_clear_st() function smid value is reseted in wrong line, > i.e. driver should reset smid value to zero after decrementing chain_offset > counter in chain_lookup table but in current code, driver is resetting smid > value before decrementing the chain_offset counter. which we are correcting > with this patch. > > v1 changelog: > Added memory barriers before & after atomic_set() API. > > Signed-off-by: Sreekanth Reddy > --- > drivers/scsi/mpt3sas/mpt3sas_base.c | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c > b/drivers/scsi/mpt3sas/mpt3sas_base.c > index 902610d..94359d8 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -1702,6 +1702,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) > return NULL; > > chain_req = >chain_lookup[smid - 1].chains_per_smid[chain_offset]; > + /* Adding memory barrier before atomic operation. */ > + smp_mb__before_atomic(); I know that checkpatch complains if it encounters a barrier without preceding comment. The comment above this barrier doesn't seem useful to me - it is so general that that comment could be added above every smp_mb__before_atomic() call. The comment above a barrier should explain which other code needs the barrier in order to execute correctly. > atomic_inc(>chain_lookup[smid - 1].chain_offset); > return chain_req; > } > @@ -3283,8 +3285,12 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > return; > st->cb_idx = 0xFF; > st->direct_io = 0; > - st->smid = 0; > + /* Adding memory barrier before atomic operation. */ > + smp_mb__before_atomic(); > atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); > + /* Adding memory barrier after atomic operation. */ > + smp_mb__after_atomic(); > + st->smid = 0; Same comment here: the comments that have been added are not useful. I think it is clear that you want to enforce the order in which .chain_offset and .smid are written. But which is the other code that can race with this code and that depends on this write order? I think this information should have been mentioned in the patch description. Thanks, Bart.
Re: [PATCH v1] mpt3sas: correct reset of smid while clearing scsi tracker
On Thu, 2018-08-02 at 16:11 -0400, Martin K. Petersen wrote: > > In mpt3sas_base_clear_st() function smid value is reseted in wrong > > line, i.e. driver should reset smid value to zero after decrementing > > chain_offset counter in chain_lookup table but in current code, driver > > is resetting smid value before decrementing the chain_offset > > counter. which we are correcting with this patch. > > You asked for Sreekanth to make changes. Please review the result. Hello Martin, That update had escaped from my attention because I wasn't Cc-ed on the update. Anyway, I will review that patch first. Bart.
[PATCH 2/2] Avoid that SCSI device removal through sysfs triggers a deadlock
A long time ago the unfortunate decision was taken to add a self-
deletion attribute to the sysfs SCSI device directory. That decision
was unfortunate because self-deletion is really tricky. We can't drop
that attribute because widely used user space software depends on it,
namely the rescan-scsi-bus.sh script. Hence this patch that avoids
that writing into that attribute triggers a deadlock. See also commit
7973cbd9fbd9 ("[PATCH] add sysfs attributes to scan and delete
scsi_devices").
This patch avoids that self-removal triggers the following deadlock:
==
WARNING: possible circular locking dependency detected
4.18.0-rc2-dbg+ #5 Not tainted
--
modprobe/6539 is trying to acquire lock:
8323c4cd (kn->count#202){}, at: kernfs_remove_by_name_ns+0x45/0x90
but task is already holding lock:
a6ec2c69 (>scan_mutex){+.+.}, at: scsi_remove_host+0x21/0x150
[scsi_mod]
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (>scan_mutex){+.+.}:
__mutex_lock+0xfe/0xc70
mutex_lock_nested+0x1b/0x20
scsi_remove_device+0x26/0x40 [scsi_mod]
sdev_store_delete+0x27/0x30 [scsi_mod]
dev_attr_store+0x3e/0x50
sysfs_kf_write+0x87/0xa0
kernfs_fop_write+0x190/0x230
__vfs_write+0xd2/0x3b0
vfs_write+0x101/0x270
ksys_write+0xab/0x120
__x64_sys_write+0x43/0x50
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #0 (kn->count#202){}:
lock_acquire+0xd2/0x260
__kernfs_remove+0x424/0x4a0
kernfs_remove_by_name_ns+0x45/0x90
remove_files.isra.1+0x3a/0x90
sysfs_remove_group+0x5c/0xc0
sysfs_remove_groups+0x39/0x60
device_remove_attrs+0x82/0xb0
device_del+0x251/0x580
__scsi_remove_device+0x19f/0x1d0 [scsi_mod]
scsi_forget_host+0x37/0xb0 [scsi_mod]
scsi_remove_host+0x9b/0x150 [scsi_mod]
sdebug_driver_remove+0x4b/0x150 [scsi_debug]
device_release_driver_internal+0x241/0x360
device_release_driver+0x12/0x20
bus_remove_device+0x1bc/0x290
device_del+0x259/0x580
device_unregister+0x1a/0x70
sdebug_remove_adapter+0x8b/0xf0 [scsi_debug]
scsi_debug_exit+0x76/0xe8 [scsi_debug]
__x64_sys_delete_module+0x1c1/0x280
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0CPU1
lock(>scan_mutex);
lock(kn->count#202);
lock(>scan_mutex);
lock(kn->count#202);
*** DEADLOCK ***
2 locks held by modprobe/6539:
#0: efaf9298 (>mutex){}, at:
device_release_driver_internal+0x68/0x360
#1: a6ec2c69 (>scan_mutex){+.+.}, at:
scsi_remove_host+0x21/0x150 [scsi_mod]
stack backtrace:
CPU: 10 PID: 6539 Comm: modprobe Not tainted 4.18.0-rc2-dbg+ #5
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
1.0.0-prebuilt.qemu-project.org 04/01/2014
Call Trace:
dump_stack+0xa4/0xf5
print_circular_bug.isra.34+0x213/0x221
__lock_acquire+0x1a7e/0x1b50
lock_acquire+0xd2/0x260
__kernfs_remove+0x424/0x4a0
kernfs_remove_by_name_ns+0x45/0x90
remove_files.isra.1+0x3a/0x90
sysfs_remove_group+0x5c/0xc0
sysfs_remove_groups+0x39/0x60
device_remove_attrs+0x82/0xb0
device_del+0x251/0x580
__scsi_remove_device+0x19f/0x1d0 [scsi_mod]
scsi_forget_host+0x37/0xb0 [scsi_mod]
scsi_remove_host+0x9b/0x150 [scsi_mod]
sdebug_driver_remove+0x4b/0x150 [scsi_debug]
device_release_driver_internal+0x241/0x360
device_release_driver+0x12/0x20
bus_remove_device+0x1bc/0x290
device_del+0x259/0x580
device_unregister+0x1a/0x70
sdebug_remove_adapter+0x8b/0xf0 [scsi_debug]
scsi_debug_exit+0x76/0xe8 [scsi_debug]
__x64_sys_delete_module+0x1c1/0x280
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
See also https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg54525.html.
Fixes: ac0ece9174ac ("scsi: use device_remove_file_self() instead of
device_schedule_callback()")
Signed-off-by: Bart Van Assche
Cc: Greg Kroah-Hartman
Acked-by: Tejun Heo
Cc: Johannes Thumshirn
Cc:
---
drivers/scsi/scsi_sysfs.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index de122354d09a..3aee9464a7bf 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -722,8 +722,24 @@ static ssize_t
sdev_store_delete(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- if (device_remove_file_self(dev, attr))
- scsi_remove_device(to_scsi_device(dev));
+ struct kernfs_node *kn;
+
