[PATCH] scsi: Do no try to probe for CDL on old drives

[PATCH] scsi: Do no try to probe for CDL on old drives

[Damien Le Moal]

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

Re: [PATCH] scsi: Do no try to probe for CDL on old drives

[Bart Van Assche]

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.

Re: [PATCH] scsi: Do no try to probe for CDL on old drives

[Damien Le Moal]

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.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] scsi: Do no try to probe for CDL on old drives

[David Gow]

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

Re: [PATCH] scsi: Do no try to probe for CDL on old drives

[Bart Van Assche]

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>

Re: [PATCH] scsi: Do no try to probe for CDL on old drives

[Niklas Cassel]

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>

Re: [PATCH] scsi: Do no try to probe for CDL on old drives

[Martin K. Petersen]

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

-- 
Martin K. Petersen	Oracle Linux Engineering