| Message ID | 20230915022034.678121-1-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: Do no try to probe for CDL on old drives | expand |
On 9/14/23 19:20, Damien Le Moal wrote: > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 92ae4b4f30ac..7aa70af1fc07 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -1828,6 +1828,9 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf) > hdr[2] = 0x7; /* claim SPC-5 version compatibility */ > } > > + if (args->dev->flags & ATA_DFLAG_CDL) > + hdr[2] = 0xd; /* claim SPC-6 version compatibility */ How about using the symbolic name SCSI_SPC_6 - 1 instead of the literal constant 0xd? > - sdev->scsi_level = inq_result[2] & 0x07; > + sdev->scsi_level = inq_result[2] & 0x0f; > if (sdev->scsi_level >= 2 || > (sdev->scsi_level == 1 && (inq_result[3] & 0x0f) == 1)) > sdev->scsi_level++; Can support for inq_result[3] & 0x0f == 1 be dropped? From an SPC-2 draft from 2001: "A RESPONSE DATA FORMAT field value of two indicates that the data shall be in the format specified in this standard. Response data format values less than two are obsolete. Response data format values greater than two are reserved." > @@ -157,6 +157,9 @@ enum scsi_disposition { > #define SCSI_3 4 /* SPC */ > #define SCSI_SPC_2 5 > #define SCSI_SPC_3 6 > +#define SCSI_SPC_4 7 > +#define SCSI_SPC_5 8 > +#define SCSI_SPC_6 14 Please consider changing the SCSI_SPC_* constants such that these match the SPC standard. Having numerical values that do not match the standard is confusing. Thanks, Bart.
On 9/16/23 00:06, Bart Van Assche wrote: > On 9/14/23 19:20, Damien Le Moal wrote: >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index 92ae4b4f30ac..7aa70af1fc07 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -1828,6 +1828,9 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf) >> hdr[2] = 0x7; /* claim SPC-5 version compatibility */ >> } >> >> + if (args->dev->flags & ATA_DFLAG_CDL) >> + hdr[2] = 0xd; /* claim SPC-6 version compatibility */ > > How about using the symbolic name SCSI_SPC_6 - 1 instead of the literal > constant 0xd? I tried to stay consistent with the code in that function which has all the versions hard coded. I can do a cleanup with a followup patch to replace the version values with the "macro - 1" names. I would not want this to block this patch as it is a regression fix confirmed to solve issues for several (and growing number of) users. > >> - sdev->scsi_level = inq_result[2] & 0x07; >> + sdev->scsi_level = inq_result[2] & 0x0f; >> if (sdev->scsi_level >= 2 || >> (sdev->scsi_level == 1 && (inq_result[3] & 0x0f) == 1)) >> sdev->scsi_level++; > > Can support for inq_result[3] & 0x0f == 1 be dropped? From an SPC-2 > draft from 2001: "A RESPONSE DATA FORMAT field value of two indicates > that the data shall be in the format specified in this standard. > Response data format values less than two are obsolete. Response data > format values greater than two are reserved." I did not check. But that is a change outside of the scope of this fix patch. > >> @@ -157,6 +157,9 @@ enum scsi_disposition { >> #define SCSI_3 4 /* SPC */ >> #define SCSI_SPC_2 5 >> #define SCSI_SPC_3 6 >> +#define SCSI_SPC_4 7 >> +#define SCSI_SPC_5 8 >> +#define SCSI_SPC_6 14 > > Please consider changing the SCSI_SPC_* constants such that these match > the SPC standard. Having numerical values that do not match the standard > is confusing. I agree. I do not know why this was done like this. This has been around as is for a long time. The problem though with changing this is that scsi_level is exposed in sysfs, so if we change that now, that could break some user things unless we keep exposing sysfs value as scsi_level + 1. We could also add a scsi_level_name attribute which gives the name, e.g. "spc-6" to be clear. In any case, such a change is outside the scope of this patch and should be a followup. > > Thanks, > > Bart.
Le 2023/09/15 à 10:20, Damien Le Moal a écrit : > Some old drives (e.g. an Ultra320 SCSI disk as reported by John) do not > seem to execute MAINTENANCE_IN / MI_REPORT_SUPPORTED_OPERATION_CODES > commands correctly and hang when a non-zero service action is specified > (one command format with service action case in scsi_report_opcode()). > > Currently, CDL probing with scsi_cdl_check_cmd() is the only caller > using a non zero service action for scsi_report_opcode(). To avoid > issues with these old drives, do not attempt CDL probe if the device > reports support for an SPC version lower than 5 (CDL was introduced in > SPC-5). To keep things working with ATA devices which probe for the CDL > T2A and T2B pages introduced with SPC-6, modify ata_scsiop_inq_std() to > claim SPC-6 version compatibility for ATA drives supporting CDL. > > SPC-6 standard version number is defined as Dh (= 13) in SPC-6 r09. Fix > scsi_probe_lun() to correctly capture this value by changing the bit > mask for the second byte of the INQUIRY response from 0x7 to 0xf. > include/scsi/scsi.h is modified to add the definition SCSI_SPC_6 with > the value 14 (Dh + 1). The missing definitions for the SCSI_SPC_4 and > SCSI_SPC_5 versions are also added. > > Reported-by: John David Anglin <dave.anglin@bell.net> > Fixes: 624885209f31 ("scsi: core: Detect support for command duration limits") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- This fixes the log spam I was seeing on a Marvell 88SE9230, which had looked like this: [ 1.744094] ata14: SATA link up 1.5 Gbps (SStatus 113 SControl 300) [ 1.744368] ata14.00: configured for UDMA/66 It now just gets configured the once at boot, as it did before the CDL patch. Tested-by: David Gow <david@davidgow.net> Thanks a lot, -- David
On 9/15/23 15:21, Damien Le Moal wrote: > In any case, such a change is outside the scope of this patch and > should be a followup. That sounds fair to me. Hence: Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Fri, Sep 15, 2023 at 11:20:34AM +0900, Damien Le Moal wrote: > Some old drives (e.g. an Ultra320 SCSI disk as reported by John) do not > seem to execute MAINTENANCE_IN / MI_REPORT_SUPPORTED_OPERATION_CODES > commands correctly and hang when a non-zero service action is specified > (one command format with service action case in scsi_report_opcode()). > > Currently, CDL probing with scsi_cdl_check_cmd() is the only caller > using a non zero service action for scsi_report_opcode(). To avoid > issues with these old drives, do not attempt CDL probe if the device > reports support for an SPC version lower than 5 (CDL was introduced in > SPC-5). To keep things working with ATA devices which probe for the CDL > T2A and T2B pages introduced with SPC-6, modify ata_scsiop_inq_std() to > claim SPC-6 version compatibility for ATA drives supporting CDL. > > SPC-6 standard version number is defined as Dh (= 13) in SPC-6 r09. Fix > scsi_probe_lun() to correctly capture this value by changing the bit > mask for the second byte of the INQUIRY response from 0x7 to 0xf. > include/scsi/scsi.h is modified to add the definition SCSI_SPC_6 with > the value 14 (Dh + 1). The missing definitions for the SCSI_SPC_4 and > SCSI_SPC_5 versions are also added. > > Reported-by: John David Anglin <dave.anglin@bell.net> > Fixes: 624885209f31 ("scsi: core: Detect support for command duration limits") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > drivers/ata/libata-scsi.c | 3 +++ > drivers/scsi/scsi.c | 11 +++++++++++ > drivers/scsi/scsi_scan.c | 2 +- > include/scsi/scsi.h | 3 +++ > 4 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 92ae4b4f30ac..7aa70af1fc07 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -1828,6 +1828,9 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf) > hdr[2] = 0x7; /* claim SPC-5 version compatibility */ > } > > + if (args->dev->flags & ATA_DFLAG_CDL) > + hdr[2] = 0xd; /* claim SPC-6 version compatibility */ > + > memcpy(rbuf, hdr, sizeof(hdr)); > memcpy(&rbuf[8], "ATA ", 8); > ata_id_string(args->id, &rbuf[16], ATA_ID_PROD, 16); > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > index d0911bc28663..89367c4bf0ef 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -613,6 +613,17 @@ void scsi_cdl_check(struct scsi_device *sdev) > bool cdl_supported; > unsigned char *buf; > > + /* > + * Support for CDL was defined in SPC-5. Ignore devices reporting an > + * lower SPC version. This also avoids problems with old drives choking > + * on MAINTENANCE_IN / MI_REPORT_SUPPORTED_OPERATION_CODES with a > + * service action specified, as done in scsi_cdl_check_cmd(). > + */ > + if (sdev->scsi_level < SCSI_SPC_5) { > + sdev->cdl_supported = 0; > + return; > + } > + > buf = kmalloc(SCSI_CDL_CHECK_BUF_LEN, GFP_KERNEL); > if (!buf) { > sdev->cdl_supported = 0; > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c > index 6650f63afec9..37dd6bbcffd3 100644 > --- a/drivers/scsi/scsi_scan.c > +++ b/drivers/scsi/scsi_scan.c > @@ -822,7 +822,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result, > * device is attached at LUN 0 (SCSI_SCAN_TARGET_PRESENT) so > * non-zero LUNs can be scanned. > */ > - sdev->scsi_level = inq_result[2] & 0x07; > + sdev->scsi_level = inq_result[2] & 0x0f; > if (sdev->scsi_level >= 2 || > (sdev->scsi_level == 1 && (inq_result[3] & 0x0f) == 1)) > sdev->scsi_level++; > diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h > index ec093594ba53..4498f845b112 100644 > --- a/include/scsi/scsi.h > +++ b/include/scsi/scsi.h > @@ -157,6 +157,9 @@ enum scsi_disposition { > #define SCSI_3 4 /* SPC */ > #define SCSI_SPC_2 5 > #define SCSI_SPC_3 6 > +#define SCSI_SPC_4 7 > +#define SCSI_SPC_5 8 > +#define SCSI_SPC_6 14 > > /* > * INQ PERIPHERAL QUALIFIERS > -- > 2.41.0 > Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
On Fri, 15 Sep 2023 11:20:34 +0900, Damien Le Moal wrote: > Some old drives (e.g. an Ultra320 SCSI disk as reported by John) do not > seem to execute MAINTENANCE_IN / MI_REPORT_SUPPORTED_OPERATION_CODES > commands correctly and hang when a non-zero service action is specified > (one command format with service action case in scsi_report_opcode()). > > Currently, CDL probing with scsi_cdl_check_cmd() is the only caller > using a non zero service action for scsi_report_opcode(). To avoid > issues with these old drives, do not attempt CDL probe if the device > reports support for an SPC version lower than 5 (CDL was introduced in > SPC-5). To keep things working with ATA devices which probe for the CDL > T2A and T2B pages introduced with SPC-6, modify ata_scsiop_inq_std() to > claim SPC-6 version compatibility for ATA drives supporting CDL. > > [...] Applied to 6.6/scsi-fixes, thanks! [1/1] scsi: Do no try to probe for CDL on old drives https://git.kernel.org/mkp/scsi/c/2132df16f53b
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 92ae4b4f30ac..7aa70af1fc07 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1828,6 +1828,9 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf) hdr[2] = 0x7; /* claim SPC-5 version compatibility */ } + if (args->dev->flags & ATA_DFLAG_CDL) + hdr[2] = 0xd; /* claim SPC-6 version compatibility */ + memcpy(rbuf, hdr, sizeof(hdr)); memcpy(&rbuf[8], "ATA ", 8); ata_id_string(args->id, &rbuf[16], ATA_ID_PROD, 16); diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index d0911bc28663..89367c4bf0ef 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -613,6 +613,17 @@ void scsi_cdl_check(struct scsi_device *sdev) bool cdl_supported; unsigned char *buf; + /* + * Support for CDL was defined in SPC-5. Ignore devices reporting an + * lower SPC version. This also avoids problems with old drives choking + * on MAINTENANCE_IN / MI_REPORT_SUPPORTED_OPERATION_CODES with a + * service action specified, as done in scsi_cdl_check_cmd(). + */ + if (sdev->scsi_level < SCSI_SPC_5) { + sdev->cdl_supported = 0; + return; + } + buf = kmalloc(SCSI_CDL_CHECK_BUF_LEN, GFP_KERNEL); if (!buf) { sdev->cdl_supported = 0; diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index 6650f63afec9..37dd6bbcffd3 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -822,7 +822,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result, * device is attached at LUN 0 (SCSI_SCAN_TARGET_PRESENT) so * non-zero LUNs can be scanned. */ - sdev->scsi_level = inq_result[2] & 0x07; + sdev->scsi_level = inq_result[2] & 0x0f; if (sdev->scsi_level >= 2 || (sdev->scsi_level == 1 && (inq_result[3] & 0x0f) == 1)) sdev->scsi_level++; diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h index ec093594ba53..4498f845b112 100644 --- a/include/scsi/scsi.h +++ b/include/scsi/scsi.h @@ -157,6 +157,9 @@ enum scsi_disposition { #define SCSI_3 4 /* SPC */ #define SCSI_SPC_2 5 #define SCSI_SPC_3 6 +#define SCSI_SPC_4 7 +#define SCSI_SPC_5 8 +#define SCSI_SPC_6 14 /* * INQ PERIPHERAL QUALIFIERS
Some old drives (e.g. an Ultra320 SCSI disk as reported by John) do not seem to execute MAINTENANCE_IN / MI_REPORT_SUPPORTED_OPERATION_CODES commands correctly and hang when a non-zero service action is specified (one command format with service action case in scsi_report_opcode()). Currently, CDL probing with scsi_cdl_check_cmd() is the only caller using a non zero service action for scsi_report_opcode(). To avoid issues with these old drives, do not attempt CDL probe if the device reports support for an SPC version lower than 5 (CDL was introduced in SPC-5). To keep things working with ATA devices which probe for the CDL T2A and T2B pages introduced with SPC-6, modify ata_scsiop_inq_std() to claim SPC-6 version compatibility for ATA drives supporting CDL. SPC-6 standard version number is defined as Dh (= 13) in SPC-6 r09. Fix scsi_probe_lun() to correctly capture this value by changing the bit mask for the second byte of the INQUIRY response from 0x7 to 0xf. include/scsi/scsi.h is modified to add the definition SCSI_SPC_6 with the value 14 (Dh + 1). The missing definitions for the SCSI_SPC_4 and SCSI_SPC_5 versions are also added. Reported-by: John David Anglin <dave.anglin@bell.net> Fixes: 624885209f31 ("scsi: core: Detect support for command duration limits") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-scsi.c | 3 +++ drivers/scsi/scsi.c | 11 +++++++++++ drivers/scsi/scsi_scan.c | 2 +- include/scsi/scsi.h | 3 +++ 4 files changed, 18 insertions(+), 1 deletion(-)