Re: [PATCH 2/3] scsi: libsas: Add return_fis_on_success to sas_ata_task

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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