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

[Igor Pylypiv <ipylypiv@google.com>]

On Thu, Aug 17, 2023 at 11:36:43PM +0000, 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@google.com>
> > ---
> >  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).

Hi Niklas,

Thanks for the suggestion! I'll update the check to ATA_QCFLAG_RESULT_TF in v2.

> 
> 
> > +
> >  	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);

I'm a bit hesitant about adding this warning. pm80xx manual states that FIS
is not getting returned for PIO Read commands. ata_dev_read_id() sends
ATA_CMD_ID_ATA (0xEC) PIO command during bus probe resulting in this warning
getting triggered every time. Checking for !PIO doesn't seem to be the right
thing to do. I'll hold off from adding the warning.

> +
>         if (stat->stat == SAS_PROTO_RESPONSE ||
>             stat->stat == SAS_SAM_STAT_GOOD ||
>             (stat->stat == SAS_SAM_STAT_CHECK_CONDITION &&
> 

Thanks,
Igor