Re: [PATCH v1 6/6] ata: libata-scsi: support the all command format for reporting supported commands

On 7/2/26 11:11, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Incorrect bit-shifting and field conflation for the CDLP and RWCDLP fields corrupts Command Duration Limits (CDL) reporting.
> - [Medium] ZBC_IN and VARIABLE_LENGTH_CMD are incorrectly reported as lacking service actions in the All Commands format.
> --
> 
> commit f7b18a5d6dd37eabc2de02696bfa0461178f0b87
> Author: Damien Le Moal <[email protected]>
> 
> ata: libata-scsi: support the all command format for reporting supported commands
> 
> This commit introduces support for the "All Commands" format (reporting option 0)
> in ata_scsi_report_supported_opcodes(). It adds ata_scsi_report_all_supported_opcodes()
> to iterate through ata_supported_cmds and report supported operations.
> 
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 3f313050c63f3..30e2cf3b03dc7 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -3731,6 +3731,42 @@ static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op, u16 sa,
> [ ... ]
>> +static unsigned int
>> +ata_scsi_report_all_supported_opcodes(struct ata_device *dev, u8 *rbuf)
>> +{
> [ ... ]
>> +		/* All command format */
>> +		if (!ata_scsi_cmd_is_supported(dev, cmd->op, cmd->sa, &sup))
>> +			continue;
>> +
>> +		buf[0] = cmd->op;
>> +		put_unaligned_be16(cmd->sa, &buf[2]);
>> +		buf[5] |= (sup.rwcdlp << 6) | (sup.cdlp << 2);
> 
> [Severity: High]
> Does this code correctly shift and map the CDLP and RWCDLP fields?

No it does not.

> It looks like sup.rwcdlp is a 1-bit boolean flag (0 for A/B, 1 for T2A/T2B)
> used in the One Command format, not the 2-bit RWCDLP enum (01b=Read, 10b=Write)
> required for the All Commands format. Shifting 1 by 6 sets bit 6, resulting in
> 10b, which would incorrectly report READ_16 as a Write command.

rdwcdlp is always 1 bit, for both the one command format and the all command format.

> 
> Additionally, the CDLP field is defined at bits 4-3, but sup.cdlp is shifted
> by 2, placing it at bits 3-2. For WRITE_16 (sup.cdlp=2), shifting by 2 results
> in setting bit 3, which falls into the CDLP field as 01b instead of 10b.

cdlp in bits 4-3 of byte 1 is for the one command format, which is different
from the all command format which has cdlp in bits 3-2 of byte 5 of a descriptor.

> 
>> +		if (cmd->sa)
>> +			buf[5] |= 0x01;
> 
> [Severity: Medium]
> Will this correctly set the SERVACTV bit for all commands that require service
> actions?
> 
> Commands like ZBC_IN (0x95) and VARIABLE_LENGTH_CMD (0x7F) have their service
> action explicitly defined as 0x00 in ata_supported_cmds. Since cmd->sa evaluates
> to 0, the SERVACTV bit won't be set. This incorrectly tells the host that these
> commands do not use the SERVICE ACTION field.

Will fix that, despite the fact that this is originally a problem with the specs
making a mess with service action "0".

> 
>> +		put_unaligned_be16(cmd->cdb_len, &buf[6]);
>> +
>> +		/* CTDP == 0 */
>> +		len += 8;
>> +		buf += 8;
>> +	}
> 


-- 
Damien Le Moal
Western Digital Research