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
