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
