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

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(-)

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.

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