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

[Igor Pylypiv <ipylypiv@google.com>]

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.

Thanks,
Igor

> 
> I will send a v3 that does populate COUNT (7:0) and COUNT (15:8),
> since it is only so few fields that we do have, we might as well
> populate them properly.
> 
> 
> Kind regards,
> Niklas