Re: [PATCH v2] ata: libata-sata: Use LBA from sense data descriptor

[Niklas Cassel <cassel@kernel.org>]

On Thu, Apr 10, 2025 at 10:00:29AM -0700, Igor Pylypiv wrote:
> On Thu, Apr 10, 2025 at 04:04:29PM +0200, Niklas Cassel wrote:
> > On Thu, Apr 10, 2025 at 03:02:33PM +0200, Niklas Cassel wrote:
> > > 
> > > I guess now when we do have access to the information, the most consistent
> > > thing would be to fill all field we can in qc->result_tf... but, to do this
> > > for every IO might slow things down.
> > > 
> > > So is there perhaps some logic to only filling LBA (in addition to STATUS
> > > and ERROR, which are filled for all NCQ commands), since that is the only
> > > field that can change, as per the specs.
> > 
> > Looking at this more closely:
> > https://github.com/torvalds/linux/blob/v6.15-rc1/include/linux/libata.h#L574-L577
> > 
> > FEATURE is a union with ERROR, so we cannot save it in qc->result_tf.
> > 
> > COMMAND is a union with STATUS, so we cannot save it in qc->result_tf.
> > 
> > 
> > The sense data descriptor does not provide AUXILIARY, nor DEVICE,
> > so we cannot save these.
> > 
> 
> Successful Sense Data descriptor provides AUXILIARY field as well:
> 
> ACS-6 (revision 10)
> 
>         Table 339 — Successful Sense Data descriptor format
>  +--------+------+-----------------------------------------------------+
>  | Offset | Type | Description                                         |
>  +--------+------+-----------------------------------------------------+
>  | 0      | Byte | SENSE KEY field (see 9.29.3.2)                      |
>  | 1      | Byte | ADDITIONAL SENSE CODE field (see 9.29.3.2)          |
>  | 2      | Byte | ADDITIONAL SENSE CODE QUALIFIER field (see 9.29.3.2)|
>  | 3      | Byte | COMMAND field (see 9.29.3.3)                        |
>  | 4      | Byte | FEATURE field (7:0) (see 9.29.3.3)                  |
>  | 5      | Byte | FEATURE field (15:8) (see 9.29.3.3)                 |
>  | 6      | Byte | COUNT field (7:0) (see 9.29.3.3)                    |
>  | 7      | Byte | COUNT field (15:8) (see 9.29.3.3)                   |
>  | 8      | Byte | LBA field (7:0) (see 9.29.3.4)                      |
>  | 9      | Byte | LBA field (15:8) (see 9.29.3.4)                     |
>  | 10     | Byte | LBA field (23:16) (see 9.29.3.4)                    |
>  | 11     | Byte | LBA field (31:24) (see 9.29.3.4)                    |
>  | 12     | Byte | LBA field (39:32) (see 9.29.3.4)                    |
>  | 13     | Byte | LBA field (47:40) (see 9.29.3.4)                    |
>  | 14     | Byte | INFORMATION field (7:0) (see 9.29.3.5)              |
>  | 15     | Byte | INFORMATION field (15:8) (see 9.29.3.5)             |
>  | 16     | Byte | AUXILIARY field (7:0) (see 9.29.3.6)                |
>  | 17     | Byte | AUXILIARY field (15:8) (see 9.29.3.6)               |
>  | 18     | Byte | AUXILIARY field (23:16) (see 9.29.3.6)              |
>  | 19     | Byte | AUXILIARY field (31:24) (see 9.29.3.6)              |
>  | 20     | Byte | ICC field (7:0) (see 9.29.3.6)                      |
>  | 21..23 | Bytes| Reserved                                            |
>  +--------+------+-----------------------------------------------------+  
> 
> The data in AUXILIARY and ICC is only valid when the AUXILIARY AND ICC FIELDS
> VALID bit is set. Similarly to other fields, AUXILIARY contains a copy of
> command inputs.

I was working on my laptop, and was not looking at the latest ACS-6 draft.
ACS-6 r2 has bytes 16 to 13 marked as Reserved.

But indeed, in the latest draft we do also have the AUX field, thank you
for noticing!


Kind regards,
Niklas