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

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

[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."

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 <cassel@kernel.org>
---
 drivers/ata/libata-sata.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index ba300cc0a3a3..c21fdacd0777 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -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,
-- 
2.49.0

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

[Igor Pylypiv]

On Wed, Apr 09, 2025 at 10:45:47AM +0200, Niklas Cassel wrote:
> 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.

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?

Thanks,
Igor

> 
> Fixes: 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL commands using policy 0xD")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/libata-sata.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index ba300cc0a3a3..c21fdacd0777 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -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,
> -- 
> 2.49.0
> 

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

[Niklas Cassel]

On Wed, Apr 09, 2025 at 10:29:59AM -0700, Igor Pylypiv wrote:
> On Wed, Apr 09, 2025 at 10:45:47AM +0200, Niklas Cassel wrote:
> > 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.
> 
> 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?

For AHCI, for a successful NCQ command, we will fill the result TF from the
SDB FIS in the FIS Receive Area, and will set ATA_QCFLAG_RTF_FILLED:
https://github.com/torvalds/linux/blob/v6.15-rc1/drivers/ata/libahci.c#L2153-L2158

The FIS Receive Area will get overwritten with each new received SDB FIS,
as it can only hold one SDB FIS.
(For libsas drivers, usually each completion can access the exact FIS that
completed the IO.)


A CDL command will set ATA_QCFLAG_RESULT_TF, but since ATA_QCFLAG_RTF_FILLED
is already set, fill_result_tf() will be a no-op.

If it was an NCQ error, ata_eh_read_log_10h() will set/overwrite qc->result_tf
with the information from the NCQ Command Error log, but for an NCQ error
there can be only one tag that caused the error:
https://github.com/torvalds/linux/blob/v6.15-rc1/drivers/ata/libata-sata.c#L1472-L1482

Both for a failed and a successful command qc->result_tf should be cleared
for a new QC, so I don't think we need to clear anything.
(And as you saw, ahci_qc_ncq_fill_rtf() only fills status, error, and flags.)


I chose to only fill LBA from the sense data descriptor, because, as you
said, "9.29.3 Successful Sense Data descriptor" says that:
COMMAND field, FEATURE field, and COUNT field are copies of the input command.

Sure, ata_eh_read_log_10h() does fill in all these fields, so strictly
speaking, we probably should fill qc->result_tf with COMMAND, FEATURE,
and COUNT, even if they will always have the same values as qc->tf...

But... even for a NCQ QC with ATA_QCFLAG_RESULT_TF, we have so far only
been filling STATUS, ERROR and flags, basically because that is the only
information that is available in the Set Device Bits (SDB) FIS that is
received on NCQ completion (and no one has complained yet...)

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.

Damien, thoughts?


Kind regards,
Niklas

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

[Niklas Cassel]

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.


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

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

[Igor Pylypiv]

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

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

[Damien Le Moal]

On 4/10/25 23:04, 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.
> 
> 
> 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.

+1

> 
> 
> Kind regards,
> Niklas


-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

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