On Fri, Aug 18, 2023 at 01:36:39AM +0200, Niklas Cassel wrote: > On Thu, Aug 17, 2023 at 02:41:36PM -0700, Igor Pylypiv wrote: (snip) > 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 && > Note that some drivers, e.g. aic94xx, already seem to: always copy to ending_fis, and sets SAS_PROTO_RESPONSE, both for successful and error commands. So it would already work. Other drivers like isci, hisi, and mvsas seem to always copy to ending_fis, but incorrectly sets status to SAS_PROTO_RESPONSE only when there was an error. They should probably also set status to SAS_PROTO_RESPONSE in the success case, since they did copy the FIS also in the success case. Kind regards, Niklas
