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. 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). > + > if (qc->scsicmd) > ASSIGN_SAS_TASK(qc->scsicmd, task); > > diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h > index 159823e0afbf..9e2c69c13dd3 100644 > --- a/include/scsi/libsas.h > +++ b/include/scsi/libsas.h > @@ -550,6 +550,7 @@ struct sas_ata_task { > u8 use_ncq:1; > u8 set_affil_pol:1; > u8 stp_affil_pol:1; > + u8 return_fis_on_success:1; > > u8 device_control_reg_update:1; > > -- > 2.42.0.rc1.204.g551eb34607-goog > Considering that libsas return value is defined like this: https://github.com/torvalds/linux/blob/v6.5-rc6/include/scsi/libsas.h#L507 Basically, if you returned a FIS in resp->ending_fis, you should return SAS_PROTO_RESPONSE. If you didn't return a FIS for your command, then you return SAS_SAM_STAT_GOOD (or if it is an error, a SAS_ error code that is not SAS_PROTO_RESPONSE, as you didn't return a FIS). Since you have implemented this only for pm80xx, how about adding something like this to sas_ata_task_done: diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index 77714a495cbb..e1c56c2c00a5 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -114,6 +114,15 @@ static void sas_ata_task_done(struct sas_task *task) } } + /* + * If a FIS was requested for a good command, and the lldd returned + * SAS_SAM_STAT_GOOD instead of SAS_PROTO_RESPONSE, then the lldd + * has not implemented support for sas_ata_task.return_fis_on_success + * Warn about this once. If we don't return FIS on success, then we + * won't be able to return an up to date TF.status and TF.error. + */ + WARN_ON_ONCE(ata_qc_wants_result(qc) && stat->stat == SAS_SAM_STAT_GOOD); + if (stat->stat == SAS_PROTO_RESPONSE || stat->stat == SAS_SAM_STAT_GOOD || (stat->stat == SAS_SAM_STAT_CHECK_CONDITION && Kind regards, Niklas
