On 2023/08/18 8:50, Niklas Cassel wrote: > 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@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); >> >> 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. Right. Forgot about non NCQ commands :) So no need for renaming to sdb_fis. Apologies for the noise. > > 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? Yes. > > > Kind regards, > Niklas -- Damien Le Moal Western Digital Research
