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 08:12:02AM +0900, Damien Le Moal wrote:
> On 2023/08/18 6:41, Igor Pylypiv wrote:
> > 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);
> 
> From the comments on patch 1, this should be:
> 
> 	if (qc->flags & ATA_QCFLAG_HAS_CDL)
> 		task->ata_task.return_sdb_fis_on_success = 1;
> 
> Note the renaming to "return_sdb_fis_on_success" to be clear about the FIS type.

I'm not sure if I agree with this comment.

According to the pm80xx programmers manual,
setting the RETFIS bit enables the HBA to return a FIS for a successful
command:

PIO Read: Nothing is returned
PIO Write: Device To Host FIS received from the device.
DMA Read: Device To Host FIS received from the device.
DMA Write: Device To Host FIS received from the device.
FPDMA Read: Set Device Bit FIS received from the device.
FPDMA Write: Set Device Bit FIS received from the device.
Non-Data: Device To Host FIS received from the device.

So the FIS you get back can also be e.g. a D2H FIS, if you send
e.g. a DMA read command.

If the RETFIS bit is not set, and the command was successful,
no FIS will be returned.

So if you really want to rename this bit, then we would also need to
change the logic in pm80xx to be something like:
if (ata_is_ncq() && task->ata_task.return_sdb_fis_on_success)
	set_RETFIS_bit;

Doesn't it make more sense for this generic libsas flag to keep its
current name, i.e. it means that we enable FIS reception for successful
commands, regardless of FIS type?


Kind regards,
Niklas