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

[Niklas Cassel <cassel@kernel.org>]

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