Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Niklas Cassel <cassel@kernel.org>]

On Fri, Jun 21, 2024 at 09:05:33AM +0900, Damien Le Moal wrote:
> On 6/20/24 21:55, Niklas Cassel wrote:
> > 
> > Perhaps we should modify fill_result_tf() to set ATA_QCFLAG_RTF_FILLED,
> > after it has called ap->ops->qc_fill_rtf(qc);
> 
> Yes, let's do that.
> 
> > Then this code can check if ATA_QCFLAG_RTF_FILLED is set, like you suggested.
> 
> And I wonder if we should not just drop ATA_QCFLAG_RESULT_TF and *always* set
> the result tf for all commands. I fail to see why this is conditional to that flag.

I'm guessing that originally this was just an optimization, that you did
not read the taskfile register for a command that was completed successfully.
(Since they did not see a need for it.)

And a command that failed would have gotten an error IRQ anyway, so the
result TF would be populated for those.

I'm not sure how much time we save by not reading the TF register for non-NCQ
commands... Most likely it would be possible to read the TF register for all
drivers for non-NCQ commands on completion.

E.g. we set the ATA_QCFLAG_RESULT_TF flag for internal commands and for
ATA-passthru commands, however, both of these are non-NCQ commands.



I think it is NCQ commands that are the problem...

For AHCI it is possible to get ATA status and ATA error, for NCQ commands,
if we extract it from the FIS rather than reading the PxTFD register,
and this is what we do in ahci_qc_ncq_fill_rtf().

Probably, most other drivers could also extract this from the FIS,
if we spend the effort on implementing that for every driver.

But if we don't do that, the drivers will read the TF register,
which e.g. for:
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/sata_nv.c#L1400-L1407
doesn't seem to work for NCQ commands.

So I'm not sure if we can remove ATA_QCFLAG_RESULT_TF, but we could
definitely set it unconditionally for non-NCQ commands if we want.


Kind regards,
Niklas