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

[Damien Le Moal <dlemoal@kernel.org>]

On 6/20/24 21:55, Niklas Cassel wrote:
>> Thanks for pointing this out! Looks like ATA PASS-TRHOUGH case is fine
>> since the flag is always set by ata_scsi_pass_thru() as you pointed out.
>> Do we still want to add the check even though we know that it is always
>> set by ata_scsi_pass_thru()?
>>
>> If the answer is "yes", I wonder if we should use the ATA_QCFLAG_RTF_FILLED
>> flag instead? Currently it is used for ahci only but looks like it can be
>> expanded to other drivers. inic_qc_fill_rtf() will benefit from this change
>> because it is not always setting the status/error values:
>> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/sata_inic162x.c#L583-L586
>>
>> For the non passthough case qc->result_tf in ata_gen_ata_sense() is also valid
>> because fill_result_tf() is being called for failed commands regardless of
>> the ATA_QCFLAG_RESULT_TF flag:
>> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-core.c#L4856-L4873
>>
>> In this case using ATA_QCFLAG_RTF_FILLED will be more accurate because
>> fill_result_tf() is being called even when ATA_QCFLAG_RESULT_TF is not set.
>>
>> With that said I'm not sure if it makes sense to update all of the ATA
>> error handling to start checking for the ATA_QCFLAG_RTF_FILLED flag.
>>
>> What are your thoughts on this?
> 
> I see your point, we will fill the result if there is an error,
> even if ATA_QCFLAG_RESULT_TF wasn't set.
> 
> 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.


-- 
Damien Le Moal
Western Digital Research