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

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?

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.

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.

> +		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.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6