| Message ID | 20230817214137.462044-2-ipylypiv@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] ata: libata: Introduce ata_qc_has_cdl() | expand |
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);
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). > + > 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); + if (stat->stat == SAS_PROTO_RESPONSE || stat->stat == SAS_SAM_STAT_GOOD || (stat->stat == SAS_SAM_STAT_CHECK_CONDITION && Kind regards, Niklas
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
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@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. 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
On 2023/08/18 8:36, 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. I do not think we need that helper. Testing the flag directly would be fine. If you really insist on introducing the helper, then at least go through libata and replace all direct tests of that flag with the helper. But I do not think it is worth the churn. > > (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). > > >> + >> 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); > + > if (stat->stat == SAS_PROTO_RESPONSE || > stat->stat == SAS_SAM_STAT_GOOD || > (stat->stat == SAS_SAM_STAT_CHECK_CONDITION && > > > > > Kind regards, > Niklas
On 2023/08/18 8:36, 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. +1 > > 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).
On Fri, Aug 18, 2023 at 09:08:26AM +0900, Damien Le Moal wrote: > On 2023/08/18 8:36, 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. > > I do not think we need that helper. Testing the flag directly would be fine. > If you really insist on introducing the helper, then at least go through libata > and replace all direct tests of that flag with the helper. But I do not think it > is worth the churn. I agree that there is no need for a helper. Kind regards, Niklas
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
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
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); + 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;
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(+)