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. > > (ata_set_tf_cdl() will set both ATA_QCFLAG_HAS_CDL and ATA_QCFLAG_RESULT_TF). > > That way, e.g. an internal command (i.e. a command issued by > ata_exec_internal_sg()), which always has ATA_QCFLAG_RESULT_TF set, > will always gets an up to date tf status and tf error value back, > because the SAS HBA will send a FIS back. +1 > > If we don't do this, then libsas will instead fill in the tf status and > tf error from the last command that returned a FIS (which might be out > of date). -- Damien Le Moal Western Digital Research
