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

[Damien Le Moal <dlemoal@kernel.org>]

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