ata: libata-sata: Use LBA from sense data descriptor

The definition of the LBA field in the sense data descriptor is:

"If definition of the sense data to be returned when a command completes without an error includes an LBA value, then the LBA field contains the defined value. Otherwise, the LBA field contains a copy of the LBA field in the command inputs for the command that completed without an error and returned sense data."

Thus, the LBA field in the sense data descriptor can contain a LBA value that is different from the LBA field in the command input.

Therefore, just like how ata_eh_read_log_10h() overrides qc->result_tf with the LBA in the NCQ Command Error log, override qc->result_tf with the LBA in the Successful NCQ Commands log.

Fixes 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL commands using policy 0xD")

Signed-off-by Niklas Cassel <[email protected]>

---
 drivers/ata/libata-sata.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/ata/libata-sata.c +8 -0

index ba300cc0a3a3..c21fdacd0777 100644
@@ -1556,6 +1556,14 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
 			continue;
 		}
 
+		/* LBA in sense data desc can be different from LBA in qc->tf */
+		qc->result_tf.lbal = sense[8];
+		qc->result_tf.lbam = sense[9];
+		qc->result_tf.lbah = sense[10];
+		qc->result_tf.hob_lbal = sense[11];
+		qc->result_tf.hob_lbam = sense[12];
+		qc->result_tf.hob_lbah = sense[13];
+
 		/* Set sense without also setting scsicmd->result */
 		scsi_build_sense_buffer(dev->flags & ATA_DFLAG_D_SENSE,
 					qc->scsicmd->sense_buffer, sk,

Signature · 1 line

2.49.0

Quoted 8 lines · Niklas Cassel

The definition of the LBA field in the sense data descriptor is:

"If definition of the sense data to be returned when a command completes without an error includes an LBA value, then the LBA field contains the defined value. Otherwise, the LBA field contains a copy of the LBA field in the command inputs for the command that completed without an error and returned sense data."

Quoted 6 lines

Thus, the LBA field in the sense data descriptor can contain a LBA value that is different from the LBA field in the command input.

Therefore, just like how ata_eh_read_log_10h() overrides qc->result_tf with the LBA in the NCQ Command Error log, override qc->result_tf with the LBA in the Successful NCQ Commands log.

Hi Niklas,

Should we also override other fields e.g. COMMAND, FEATURE, COUNT, AUXILIARY? I understand that per current SAT spec those fields contain data from command inputs so we can get the same data directly from qc->tf and technically don't need to fill qc->result_tf with the same.

If I understand correctly, qc->result_tf contains data filled from a shared FIS so it is likely that it contains data that belongs to some other command, is that true? If that's true, should we clear the qc->result_tf before filling the fields with data from the Successful NCQ Commands log?

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.

Quoted 8 lines · Niklas Cassel

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.

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

Quoted 13 lines · Niklas Cassel

Quoted 8 lines · Niklas Cassel

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

Quoted 8 lines

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.

Quoted 8 lines · Niklas Cassel

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.

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