On Fri, Aug 18, 2023 at 09:08:26AM +0900, Damien Le Moal wrote: > On 2023/08/18 8:36, Niklas Cassel wrote: > > On Thu, Aug 17, 2023 at 02:41:36PM -0700, Igor Pylypiv wrote: > > > > Hello Igor, > > > >> For Command Duration Limits policy 0xD (command completes without > >> an error) libata needs FIS in order to detect the ATA_SENSE bit and > >> read the Sense Data for Successful NCQ Commands log (0Fh). > >> > >> Set return_fis_on_success for commands that have a CDL descriptor > >> since any CDL descriptor can be configured with policy 0xD. > >> > >> Signed-off-by: Igor Pylypiv <ipylypiv@xxxxxxxxxx> > >> --- > >> drivers/scsi/libsas/sas_ata.c | 3 +++ > >> include/scsi/libsas.h | 1 + > >> 2 files changed, 4 insertions(+) > >> > >> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c > >> index 77714a495cbb..da67c4f671b2 100644 > >> --- a/drivers/scsi/libsas/sas_ata.c > >> +++ b/drivers/scsi/libsas/sas_ata.c > >> @@ -207,6 +207,9 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc) > >> task->ata_task.use_ncq = ata_is_ncq(qc->tf.protocol); > >> task->ata_task.dma_xfer = ata_is_dma(qc->tf.protocol); > >> > >> + /* CDL policy 0xD requires FIS for successful (no error) completions */ > >> + task->ata_task.return_fis_on_success = ata_qc_has_cdl(qc); > > > > In ata_qc_complete(), for a successful command, we call fill_result_tf() > > if (qc->flags & ATA_QCFLAG_RESULT_TF): > > https://github.com/torvalds/linux/blob/v6.5-rc6/drivers/ata/libata-core.c#L4926 > > > > My point is, I think that you should set > > task->ata_task.return_fis_on_success = ata_qc_wants_result(qc); > > > > where ata_qc_wants_result() > > returns true if ATA_QCFLAG_RESULT_TF is set. > > I do not think we need that helper. Testing the flag directly would be fine. > If you really insist on introducing the helper, then at least go through libata > and replace all direct tests of that flag with the helper. But I do not think it > is worth the churn. I agree that there is no need for a helper. Kind regards, Niklas
