Re: [PATCH 0/3] ata: libata-scsi: fix ATA PASS-THROUGH(32) handling

[Niklas Cassel <cassel@kernel.org>]

On Wed, Nov 04, 2026 at 10:08:32PM +0800, shao.mingyin@zte.com.cn wrote:
> ATA PASS-THROUGH(32) uses the variable length CDB opcode 7Fh with
> service action 1FF0h.  Commit b1ffbf854e08 added support for it, but
> three paths still use the offsets of the 12- and 16-byte CDBs, or do
> not recognize the 32-byte form at all.
> 
> Patch 1 fixes the PIO multi-sector sanity check and the multi_count
> computation, which read cdb[1] instead of cdb[1 + cdb_offset], and
> reports the field pointer for the same byte.
> 
> Patch 2 makes an unsupported service action fail with INVALID FIELD IN
> CDB.  It currently returns 1 without setting sense data, so the command
> completes with SAM_STAT_GOOD even though no ATA command was issued.
> 
> Patch 3 teaches the completion path to recognize ATA PASS-THROUGH(32)
> and to read CK_COND from byte 11.  Without it, CK_COND is ignored and
> the ATA Status Return sense descriptor is not built.
> 
> The three patches are independent and can be applied in any order.  All
> were tested with a 32-byte CDB sent through SG_IO to a QEMU IDE disk;
> the per-patch test descriptions are included after the --- line.
> 
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
> ---
> base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c

Hello Shao,


LLM found some review comments.

I looked at the SAT specs, and the comments appears to be valid:



commit d693212f2d5e4f09a56a4883db4172f7b5892673
Author: Shao Mingyin <shao.mingyin@zte.com.cn>

ata: libata-scsi: fix the multi_count check for 32-byte passthru

This patch makes the PIO multi-sector sanity check and the multi_count
computation in ata_scsi_pass_thru() read byte 1 + cdb_offset of the CDB
instead of byte 1, and reports the field pointer for the same byte.

> For a 32-byte CDB, cdb[1] is the variable length CDB control byte and
> its bits 7:5 do not carry MULTIPLE_COUNT.

[ ... ]

> Read the count field from cdb[1 + cdb_offset], matching the rest of the
> function, and report the field pointer relative to the same byte so that
> a rejected multi-sector count points at the MULTIPLE_COUNT field instead
> of byte 1.

Does the ATA PASS-THROUGH (32) CDB have a MULTIPLE_COUNT field at all?

The ATA PASS-THROUGH (32) CDB defined in SAT-4, SAT-5 and SAT-6 (table 210
in SAT-6 r02) has:

  10 | Reserved (7:5) | PROTOCOL (4:1) | EXTEND (0)

so bits 7:5 of byte 10 are reserved, not a count. The 32-byte CDB was added
in SAT-4, where bits 7:5 of byte 1 of the 12 and 16-byte CDBs, the former
MULTIPLE_COUNT field, are already obsolete.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a61fb0331279..927e75b7498d 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3574,13 +3574,13 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
>  	}
>  
>  	/* sanity check for pio multi commands */
> -	if ((cdb[1] & 0xe0) && !is_multi_taskfile(tf)) {
> +	if ((cdb[1 + cdb_offset] & 0xe0) && !is_multi_taskfile(tf)) {
> -		fp = 1;
> +		fp = 1 + cdb_offset;

[Severity: Low]
Was this field pointer change part of the patch that was tested?

The index line of this patch records 927e75b7498d as the resulting
libata-scsi.c, but applying the posted diff to next-20261030 results in
20ba572c0e37. 927e75b7498d is what the diff gives without this change, so
it looks like the change was added after the patch was generated. That
would also explain the interleaved - and + lines above, which git does not
produce.

The test results below the --- line only list the sense key and the
additional sense code, not the field pointer.

>  		goto invalid_fld;
>  	}
>  
>  	if (is_multi_taskfile(tf)) {
> -		unsigned int multi_count = 1 << (cdb[1] >> 5);
> +		unsigned int multi_count = 1 << (cdb[1 + cdb_offset] >> 5);

[Severity: Low]
With a 32-byte CDB that leaves the reserved bits zero, doesn't this still
compute multi_count = 1 << 0 = 1 for READ MULTIPLE and WRITE MULTIPLE?

ata_scsi_pass_thru() {
    ...
	if (is_multi_taskfile(tf)) {
		unsigned int multi_count = 1 << (cdb[1 + cdb_offset] >> 5);
    ...
		if (multi_count != dev->multi_count)
			ata_dev_warn(dev, "invalid multi_count %u ignored\n",
				     multi_count);
	}
    ...
}

So the "invalid multi_count 1 ignored" warning that the commit message
describes is still emitted for devices with dev->multi_count != 1. Only an
initiator that sets the reserved bits gets a different multi_count.

The change that does take effect is that the CONTROL byte, whose bits 7:6
are vendor specific, is no longer interpreted as a multiple count.

Would it be more accurate to treat bits 7:5 of byte 10 as reserved, and to
skip the multi_count comparison for the 32-byte CDB, which cannot specify
a multiple count?


commit 5bd933578ed43952ff6544aadda2394a021acb83
Author: Shao Mingyin <shao.mingyin@zte.com.cn>

ata: libata-scsi: generate SAT sense data for ATA PASS-THROUGH(32)

This patch makes ata_scsi_qc_complete() recognize ATA PASS-THROUGH (32)
commands by their service action, and read CK_COND from byte 11 of the
32-byte CDB, so that CK_COND is honored and the ATA Status Return sense
descriptor is built for these commands.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a61fb0331279..97adeecc7736 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c

[ ... ]

> @@ -3389,6 +3392,6 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
>  	tf->protocol = ata_scsi_map_proto(cdb[1 + cdb_offset]);
>  	if (tf->protocol == ATA_PROT_UNKNOWN) {
> -		fp = 1;
> +		fp = 1 + cdb_offset;
>  		goto invalid_fld;
>  	}

[Severity: Low]
This isn't a bug, but this hunk changes the field pointer that
ata_scsi_pass_thru() reports for an invalid PROTOCOL field, and the commit
message only describes ata_scsi_qc_complete().

Would this fit better in the first patch of the series, which changes the
field pointer for the same byte, or in a patch of its own?

[Severity: Low]
Was this hunk part of the patch that was tested?

As with the first patch, the index line records 97adeecc7736 as the
resulting libata-scsi.c, which is what this patch gives without this hunk.
Applying the posted diff to next-20261030 results in c23b110dae63, so it
looks like the hunk was added after the patch was generated.

[Severity: Low]
Should the remaining field pointers in ata_scsi_pass_thru() also take
cdb_offset into account? They still use the offsets of the 12 and 16-byte
CDBs:

ata_scsi_pass_thru() {
    ...
		if (tf->protocol != ATA_PROT_PIO || tf->nsect != 1) {
			fp = 1;
			goto invalid_fld;
		}
    ...
	if (tf->protocol == ATA_PROT_DMA && !ata_dma_enabled(dev)) {
		fp = 1;
		goto invalid_fld;
	}
    ...
	if (ata_is_ncq(tf->protocol) && !ata_ncq_enabled(dev)) {
		fp = 1;
		goto invalid_fld;
	}
    ...
	    tf->feature == SETFEATURES_XFER) {
		fp = (cdb[0] == ATA_16) ? 4 : 3;
    ...
	if (tf->command >= 0x5C && tf->command <= 0x5F && !libata_allow_tpm) {
		fp = (cdb[0] == ATA_16) ? 14 : 9;
    ...
}

In the 32-byte CDB, PROTOCOL is in byte 10, FEATURES (7:0) in byte 21 and
COMMAND in byte 25. The cover letter says "three paths still use the
offsets of the 12- and 16-byte CDBs", but these five still do after the
series.




Kind regards,
Niklas