SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results"
specifies that SATL shall generate sense data for ATA PASS-THROUGH
commands when either CK_COND is set or when ATA_ERR or ATA_DF status
bits are set.
ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR
or ATA_DF bits are set. It looks like qc->err_mask can be used as
an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit
when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error
indication if no other bits were set in qc->err_mask.
ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH
commands because qc->err_mask can be zero (i.e. "no error") even when
the corresponding command has failed with ATA_ERR/ATA_DF bits set.
Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID)
should not prevent SATL from generating sense data for ATA PASS-THROUGH.
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
drivers/ata/libata-scsi.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 032cf11d0bcc..79e8103ef3a9 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
!(qc->flags & ATA_QCFLAG_SENSE_VALID);
/* For ATA pass thru (SAT) commands, generate a sense block if
- * user mandated it or if there's an error. Note that if we
- * generate because the user forced us to [CK_COND =1], a check
+ * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that
+ * if we generate because the user forced us to [CK_COND=1], a check
* condition is generated and the ATA register values are returned
* whether the command completed successfully or not. If there
* was no error, we use the following sense data:
@@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
* asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
*/
if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
- ((cdb[2] & 0x20) || need_sense))
+ ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF))))
ata_gen_passthru_sense(qc);
else if (need_sense)
ata_gen_ata_sense(qc);
--
2.45.2.627.g7a2c4fd464-googOn Fri, Jun 14, 2024 at 07:18:33PM +0000, Igor Pylypiv wrote: > SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results" > specifies that SATL shall generate sense data for ATA PASS-THROUGH > commands when either CK_COND is set or when ATA_ERR or ATA_DF status > bits are set. > > ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR > or ATA_DF bits are set. It looks like qc->err_mask can be used as > an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit > when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error > indication if no other bits were set in qc->err_mask. The reason why libata clears the err_mask when having sense data, is because the upper layer, i.e. SCSI, should determine what to do with the command, if there is sense data. For a non-passthrough command, this will be done by scsi_io_completion_action(): https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1084-L1087 However, if there is any bits set in cmd->result, scsi_io_completion_nz_result() will be called: https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1052-L1053 which will do the following for a passthrough command: https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L969-L978 which will set blk_stat. After that, scsi_io_completion() which check blk_stat and if it is a scsi_noretry_cmd(): https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1073-L1078 A passthrough command will return true for scsi_noretry_cmd(), so scsi_io_completion_action() should NOT get called for a passthough command. So IIUC, for a non-passthrough command, scsi_io_completion_action() will decide what to do depending on the sense data, but a passthrough command will get finished with the sense data, leaving the user to decide what to do. > > ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH > commands because qc->err_mask can be zero (i.e. "no error") even when > the corresponding command has failed with ATA_ERR/ATA_DF bits set. > > Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID) > should not prevent SATL from generating sense data for ATA PASS-THROUGH. > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/ata/libata-scsi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 032cf11d0bcc..79e8103ef3a9 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) > !(qc->flags & ATA_QCFLAG_SENSE_VALID); > > /* For ATA pass thru (SAT) commands, generate a sense block if > - * user mandated it or if there's an error. Note that if we > - * generate because the user forced us to [CK_COND =1], a check > + * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that > + * if we generate because the user forced us to [CK_COND=1], a check > * condition is generated and the ATA register values are returned > * whether the command completed successfully or not. If there > * was no error, we use the following sense data: > @@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) > * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE > */ > if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) && > - ((cdb[2] & 0x20) || need_sense)) > + ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF)))) qc->result_tf can only be used if qc->flags has ATA_QCFLAG_RESULT_TF set, otherwise it can contain bogus data. You don't seem to check if ATA_QCFLAG_RESULT_TF is set. ata_scsi_pass_thru() does set ATA_QCFLAG_RESULT_TF. > ata_gen_passthru_sense(qc); > else if (need_sense) > ata_gen_ata_sense(qc); > -- > 2.45.2.627.g7a2c4fd464-goog >
On Mon, Jun 17, 2024 at 01:29:25PM +0200, Niklas Cassel wrote: > On Fri, Jun 14, 2024 at 07:18:33PM +0000, Igor Pylypiv wrote: > > SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results" > > specifies that SATL shall generate sense data for ATA PASS-THROUGH > > commands when either CK_COND is set or when ATA_ERR or ATA_DF status > > bits are set. > > > > ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR > > or ATA_DF bits are set. It looks like qc->err_mask can be used as > > an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit > > when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error > > indication if no other bits were set in qc->err_mask. > > The reason why libata clears the err_mask when having sense data, > is because the upper layer, i.e. SCSI, should determine what to do > with the command, if there is sense data. > > For a non-passthrough command, this will be done by > scsi_io_completion_action(): > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1084-L1087 > > > However, if there is any bits set in cmd->result, > scsi_io_completion_nz_result() will be called: > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1052-L1053 > > which will do the following for a passthrough command: > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L969-L978 > which will set blk_stat. > > After that, scsi_io_completion() which check blk_stat and if it is a > scsi_noretry_cmd(): > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1073-L1078 > > A passthrough command will return true for scsi_noretry_cmd(), so > scsi_io_completion_action() should NOT get called for a passthough command. > > So IIUC, for a non-passthrough command, scsi_io_completion_action() will > decide what to do depending on the sense data, but a passthrough command will > get finished with the sense data, leaving the user to decide what to do. > Thank you for the detailed explanation, Niklas! I was looking at a related logic in ata_eh_link_report(): https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-eh.c#L2359-L2360 Is my understanding correct that if we have ATA_QCFLAG_SENSE_VALID set and qc->err_mask is zero then we don't want to report the error to user since SCSI might decide that it is not an error based on the sense data? > > > > > ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH > > commands because qc->err_mask can be zero (i.e. "no error") even when > > the corresponding command has failed with ATA_ERR/ATA_DF bits set. > > > > Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID) > > should not prevent SATL from generating sense data for ATA PASS-THROUGH. > > > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > --- > > drivers/ata/libata-scsi.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > > index 032cf11d0bcc..79e8103ef3a9 100644 > > --- a/drivers/ata/libata-scsi.c > > +++ b/drivers/ata/libata-scsi.c > > @@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) > > !(qc->flags & ATA_QCFLAG_SENSE_VALID); > > > > /* For ATA pass thru (SAT) commands, generate a sense block if > > - * user mandated it or if there's an error. Note that if we > > - * generate because the user forced us to [CK_COND =1], a check > > + * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that > > + * if we generate because the user forced us to [CK_COND=1], a check > > * condition is generated and the ATA register values are returned > > * whether the command completed successfully or not. If there > > * was no error, we use the following sense data: > > @@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) > > * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE > > */ > > if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) && > > - ((cdb[2] & 0x20) || need_sense)) > > + ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF)))) > > qc->result_tf can only be used if qc->flags has ATA_QCFLAG_RESULT_TF set, > otherwise it can contain bogus data. > You don't seem to check if ATA_QCFLAG_RESULT_TF is set. > > ata_scsi_pass_thru() does set ATA_QCFLAG_RESULT_TF. > Thanks for pointing this out! Looks like ATA PASS-TRHOUGH case is fine since the flag is always set by ata_scsi_pass_thru() as you pointed out. Do we still want to add the check even though we know that it is always set by ata_scsi_pass_thru()? If the answer is "yes", I wonder if we should use the ATA_QCFLAG_RTF_FILLED flag instead? Currently it is used for ahci only but looks like it can be expanded to other drivers. inic_qc_fill_rtf() will benefit from this change because it is not always setting the status/error values: https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/sata_inic162x.c#L583-L586 For the non passthough case qc->result_tf in ata_gen_ata_sense() is also valid because fill_result_tf() is being called for failed commands regardless of the ATA_QCFLAG_RESULT_TF flag: https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-core.c#L4856-L4873 In this case using ATA_QCFLAG_RTF_FILLED will be more accurate because fill_result_tf() is being called even when ATA_QCFLAG_RESULT_TF is not set. With that said I'm not sure if it makes sense to update all of the ATA error handling to start checking for the ATA_QCFLAG_RTF_FILLED flag. What are your thoughts on this? > > > ata_gen_passthru_sense(qc); > > else if (need_sense) > > ata_gen_ata_sense(qc); > > -- > > 2.45.2.627.g7a2c4fd464-goog > > Thank you, Igor
On Tue, Jun 18, 2024 at 09:51:50PM +0000, Igor Pylypiv wrote: > On Mon, Jun 17, 2024 at 01:29:25PM +0200, Niklas Cassel wrote: > > On Fri, Jun 14, 2024 at 07:18:33PM +0000, Igor Pylypiv wrote: > > > SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results" > > > specifies that SATL shall generate sense data for ATA PASS-THROUGH > > > commands when either CK_COND is set or when ATA_ERR or ATA_DF status > > > bits are set. > > > > > > ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR > > > or ATA_DF bits are set. It looks like qc->err_mask can be used as > > > an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit > > > when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error > > > indication if no other bits were set in qc->err_mask. > > > > The reason why libata clears the err_mask when having sense data, > > is because the upper layer, i.e. SCSI, should determine what to do > > with the command, if there is sense data. > > > > For a non-passthrough command, this will be done by > > scsi_io_completion_action(): > > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1084-L1087 > > > > > > However, if there is any bits set in cmd->result, > > scsi_io_completion_nz_result() will be called: > > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1052-L1053 > > > > which will do the following for a passthrough command: > > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L969-L978 > > which will set blk_stat. > > > > After that, scsi_io_completion() which check blk_stat and if it is a > > scsi_noretry_cmd(): > > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1073-L1078 > > > > A passthrough command will return true for scsi_noretry_cmd(), so > > scsi_io_completion_action() should NOT get called for a passthough command. > > > > So IIUC, for a non-passthrough command, scsi_io_completion_action() will > > decide what to do depending on the sense data, but a passthrough command will > > get finished with the sense data, leaving the user to decide what to do. > > > > Thank you for the detailed explanation, Niklas! > I was looking at a related logic in ata_eh_link_report(): > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-eh.c#L2359-L2360 > > Is my understanding correct that if we have ATA_QCFLAG_SENSE_VALID set and > qc->err_mask is zero then we don't want to report the error to user since > SCSI might decide that it is not an error based on the sense data? I'm assuming that that was the reasoning. However, IIUC, passthrough commands should never be retried by SCSI, it should always be reported back to the user. > > > > > > > > > ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH > > > commands because qc->err_mask can be zero (i.e. "no error") even when > > > the corresponding command has failed with ATA_ERR/ATA_DF bits set. > > > > > > Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID) > > > should not prevent SATL from generating sense data for ATA PASS-THROUGH. > > > > > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > > --- > > > drivers/ata/libata-scsi.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > > > index 032cf11d0bcc..79e8103ef3a9 100644 > > > --- a/drivers/ata/libata-scsi.c > > > +++ b/drivers/ata/libata-scsi.c > > > @@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) > > > !(qc->flags & ATA_QCFLAG_SENSE_VALID); > > > > > > /* For ATA pass thru (SAT) commands, generate a sense block if > > > - * user mandated it or if there's an error. Note that if we > > > - * generate because the user forced us to [CK_COND =1], a check > > > + * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that > > > + * if we generate because the user forced us to [CK_COND=1], a check > > > * condition is generated and the ATA register values are returned > > > * whether the command completed successfully or not. If there > > > * was no error, we use the following sense data: > > > @@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) > > > * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE > > > */ > > > if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) && > > > - ((cdb[2] & 0x20) || need_sense)) > > > + ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF)))) > > > > qc->result_tf can only be used if qc->flags has ATA_QCFLAG_RESULT_TF set, > > otherwise it can contain bogus data. > > You don't seem to check if ATA_QCFLAG_RESULT_TF is set. > > > > ata_scsi_pass_thru() does set ATA_QCFLAG_RESULT_TF. > > > > Thanks for pointing this out! Looks like ATA PASS-TRHOUGH case is fine > since the flag is always set by ata_scsi_pass_thru() as you pointed out. > Do we still want to add the check even though we know that it is always > set by ata_scsi_pass_thru()? > > If the answer is "yes", I wonder if we should use the ATA_QCFLAG_RTF_FILLED > flag instead? Currently it is used for ahci only but looks like it can be > expanded to other drivers. inic_qc_fill_rtf() will benefit from this change > because it is not always setting the status/error values: > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/sata_inic162x.c#L583-L586 > > For the non passthough case qc->result_tf in ata_gen_ata_sense() is also valid > because fill_result_tf() is being called for failed commands regardless of > the ATA_QCFLAG_RESULT_TF flag: > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-core.c#L4856-L4873 > > In this case using ATA_QCFLAG_RTF_FILLED will be more accurate because > fill_result_tf() is being called even when ATA_QCFLAG_RESULT_TF is not set. > > With that said I'm not sure if it makes sense to update all of the ATA > error handling to start checking for the ATA_QCFLAG_RTF_FILLED flag. > > What are your thoughts on this? I see your point, we will fill the result if there is an error, even if ATA_QCFLAG_RESULT_TF wasn't set. Perhaps we should modify fill_result_tf() to set ATA_QCFLAG_RTF_FILLED, after it has called ap->ops->qc_fill_rtf(qc); Then this code can check if ATA_QCFLAG_RTF_FILLED is set, like you suggested. Kind regards, Niklas
On 6/20/24 21:55, Niklas Cassel wrote: >> Thanks for pointing this out! Looks like ATA PASS-TRHOUGH case is fine >> since the flag is always set by ata_scsi_pass_thru() as you pointed out. >> Do we still want to add the check even though we know that it is always >> set by ata_scsi_pass_thru()? >> >> If the answer is "yes", I wonder if we should use the ATA_QCFLAG_RTF_FILLED >> flag instead? Currently it is used for ahci only but looks like it can be >> expanded to other drivers. inic_qc_fill_rtf() will benefit from this change >> because it is not always setting the status/error values: >> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/sata_inic162x.c#L583-L586 >> >> For the non passthough case qc->result_tf in ata_gen_ata_sense() is also valid >> because fill_result_tf() is being called for failed commands regardless of >> the ATA_QCFLAG_RESULT_TF flag: >> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-core.c#L4856-L4873 >> >> In this case using ATA_QCFLAG_RTF_FILLED will be more accurate because >> fill_result_tf() is being called even when ATA_QCFLAG_RESULT_TF is not set. >> >> With that said I'm not sure if it makes sense to update all of the ATA >> error handling to start checking for the ATA_QCFLAG_RTF_FILLED flag. >> >> What are your thoughts on this? > > I see your point, we will fill the result if there is an error, > even if ATA_QCFLAG_RESULT_TF wasn't set. > > Perhaps we should modify fill_result_tf() to set ATA_QCFLAG_RTF_FILLED, > after it has called ap->ops->qc_fill_rtf(qc); Yes, let's do that. > Then this code can check if ATA_QCFLAG_RTF_FILLED is set, like you suggested. And I wonder if we should not just drop ATA_QCFLAG_RESULT_TF and *always* set the result tf for all commands. I fail to see why this is conditional to that flag. -- Damien Le Moal Western Digital Research
On Fri, Jun 21, 2024 at 09:05:33AM +0900, Damien Le Moal wrote: > On 6/20/24 21:55, Niklas Cassel wrote: > > > > Perhaps we should modify fill_result_tf() to set ATA_QCFLAG_RTF_FILLED, > > after it has called ap->ops->qc_fill_rtf(qc); > > Yes, let's do that. > > > Then this code can check if ATA_QCFLAG_RTF_FILLED is set, like you suggested. > > And I wonder if we should not just drop ATA_QCFLAG_RESULT_TF and *always* set > the result tf for all commands. I fail to see why this is conditional to that flag. I'm guessing that originally this was just an optimization, that you did not read the taskfile register for a command that was completed successfully. (Since they did not see a need for it.) And a command that failed would have gotten an error IRQ anyway, so the result TF would be populated for those. I'm not sure how much time we save by not reading the TF register for non-NCQ commands... Most likely it would be possible to read the TF register for all drivers for non-NCQ commands on completion. E.g. we set the ATA_QCFLAG_RESULT_TF flag for internal commands and for ATA-passthru commands, however, both of these are non-NCQ commands. I think it is NCQ commands that are the problem... For AHCI it is possible to get ATA status and ATA error, for NCQ commands, if we extract it from the FIS rather than reading the PxTFD register, and this is what we do in ahci_qc_ncq_fill_rtf(). Probably, most other drivers could also extract this from the FIS, if we spend the effort on implementing that for every driver. But if we don't do that, the drivers will read the TF register, which e.g. for: https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/sata_nv.c#L1400-L1407 doesn't seem to work for NCQ commands. So I'm not sure if we can remove ATA_QCFLAG_RESULT_TF, but we could definitely set it unconditionally for non-NCQ commands if we want. Kind regards, Niklas
On Mon, Jun 17, 2024 at 01:29:25PM +0200, Niklas Cassel wrote: > > However, if there is any bits set in cmd->result, > scsi_io_completion_nz_result() will be called: > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1052-L1053 If forgot to say that, if ATA_ERR and Sense Data Available bit is set: ata_eh_analyze_tf() will call scsi_check_sense() which will set the SCSI ML bit for many ASC/ASCQ codes. If Sense Data Available bit is set, but ATA_ERR is not set: -For non-NCQ commands, ata_eh_read_sense_success_non_ncq() will call scsi_check_sense() which will set the SCSI ML bit for many ASC/ASCQ codes. -For NCQ commands, ata_eh_read_sense_success_ncq_log() will call scsi_check_sense() which will set the SCSI ML bit for many ASC/ASCQ codes. The SCSI ML bit is stored in cmd->result, so if scsi_check_sense() did set the SCSI ML bit, cmd->result will be non-zero, which means that once scsi_io_completion() is called, it will call scsi_io_completion_nz_result(). Kind regards, Niklas
On 6/15/24 04:18, Igor Pylypiv wrote: > SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results" > specifies that SATL shall generate sense data for ATA PASS-THROUGH > commands when either CK_COND is set or when ATA_ERR or ATA_DF status > bits are set. > > ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR > or ATA_DF bits are set. It looks like qc->err_mask can be used as > an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit > when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error > indication if no other bits were set in qc->err_mask. > > ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH > commands because qc->err_mask can be zero (i.e. "no error") even when > the corresponding command has failed with ATA_ERR/ATA_DF bits set. If there was a failed command, qc->err_mask will be non-zero since the command completion will be signaled by an error interrupt. So I am confused here. Are you seeing this happening in practice ? If yes, doing what with which adapter ? > Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID) > should not prevent SATL from generating sense data for ATA PASS-THROUGH. Same here, this is strange: ATA_QCFLAG_SENSE_VALID indicates that we have sense data for the command, regardless if the command is passthrough or not. So if that flag is set, we should not overwrite the already valid sense data with sense data generated from the error and status bits. Do you see an issue without this change ? If yes, what are the adapter and operations you are running ? > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/ata/libata-scsi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 032cf11d0bcc..79e8103ef3a9 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) > !(qc->flags & ATA_QCFLAG_SENSE_VALID); > > /* For ATA pass thru (SAT) commands, generate a sense block if > - * user mandated it or if there's an error. Note that if we > - * generate because the user forced us to [CK_COND =1], a check > + * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that > + * if we generate because the user forced us to [CK_COND=1], a check > * condition is generated and the ATA register values are returned > * whether the command completed successfully or not. If there > * was no error, we use the following sense data: > @@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) > * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE > */ > if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) && > - ((cdb[2] & 0x20) || need_sense)) > + ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF)))) > ata_gen_passthru_sense(qc); > else if (need_sense) > ata_gen_ata_sense(qc); -- Damien Le Moal Western Digital Research
On Mon, Jun 17, 2024 at 08:22:07AM +0900, Damien Le Moal wrote:
> On 6/15/24 04:18, Igor Pylypiv wrote:
> > SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results"
> > specifies that SATL shall generate sense data for ATA PASS-THROUGH
> > commands when either CK_COND is set or when ATA_ERR or ATA_DF status
> > bits are set.
> >
> > ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR
> > or ATA_DF bits are set. It looks like qc->err_mask can be used as
> > an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit
> > when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error
> > indication if no other bits were set in qc->err_mask.
> >
> > ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH
> > commands because qc->err_mask can be zero (i.e. "no error") even when
> > the corresponding command has failed with ATA_ERR/ATA_DF bits set.
>
> If there was a failed command, qc->err_mask will be non-zero since the command
> completion will be signaled by an error interrupt. So I am confused here. Are
> you seeing this happening in practice ? If yes, doing what with which adapter ?
>
Yes, this is happening in practice with the PM8006 adapter. ata_eh_analyze_tf()
sets AC_ERR_DEV and later ata_eh_link_autopsy() clears AC_ERR_DEV making
qc->err_mask == 0.
ata_eh_link_autopsy()
\
`ata_eh_analyze_tf()
if (stat & (ATA_ERR | ATA_DF)) {
qc->err_mask |= AC_ERR_DEV; <<< set AC_ERR_DEV
/*
* Sense data reporting does not work if the
* device fault bit is set.
*/
if (stat & ATA_DF)
stat &= ~ATA_SENSE;
...
<cont. ata_eh_link_autopsy()>
/*
* SENSE_VALID trumps dev/unknown error and revalidation. Upper
* layers will determine whether the command is worth retrying
* based on the sense data and device class/type. Otherwise,
* determine directly if the command is worth retrying using its
* error mask and flags.
*/
if (qc->flags & ATA_QCFLAG_SENSE_VALID)
qc->err_mask &= ~(AC_ERR_DEV | AC_ERR_OTHER); <<< clear AC_ERR_DEV
> > Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID)
> > should not prevent SATL from generating sense data for ATA PASS-THROUGH.
>
> Same here, this is strange: ATA_QCFLAG_SENSE_VALID indicates that we have sense
> data for the command, regardless if the command is passthrough or not. So if
> that flag is set, we should not overwrite the already valid sense data with
> sense data generated from the error and status bits.
Sorry about the confusion. I meant that the "ATA Status Return sense data
descriptor" is not getting populated when ATA_QCFLAG_SENSE_VALID is set
and CK_COND is 0.
What you described is true when CK_COND is 0, however, when CK_COND is 1,
existing code will call ata_gen_passthru_sense() without checking
the "need_sense" value. This will generate fake sk/asc/ascq regardless of
the ATA_QCFLAG_SENSE_VALID flag.
> Do you see an issue without this change ? If yes, what are the adapter and
> operations you are running ?
>
Yes, we see the issue on PM8006 adapter.
Any failed ATA PASS-THROUGH command with CK_COND=1 has this issue. For example,
Sending READ SECTOR(S) EXT via ATA PASS-THROUGH with CK_COND=1 to an lba
that was previously corrupted by WRITE UNCORRECTABLE EXT is supposed to
return "READ ERROR - LBA MARKED BAD BY APPLICATION CLIENT" but instead it
returns generated "UNRECOVERED READ ERROR - AUTO REALLOCATE FAILED".
> >
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> > drivers/ata/libata-scsi.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 032cf11d0bcc..79e8103ef3a9 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> > !(qc->flags & ATA_QCFLAG_SENSE_VALID);
> >
> > /* For ATA pass thru (SAT) commands, generate a sense block if
> > - * user mandated it or if there's an error. Note that if we
> > - * generate because the user forced us to [CK_COND =1], a check
> > + * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that
> > + * if we generate because the user forced us to [CK_COND=1], a check
> > * condition is generated and the ATA register values are returned
> > * whether the command completed successfully or not. If there
> > * was no error, we use the following sense data:
> > @@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> > * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
> > */
> > if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> > - ((cdb[2] & 0x20) || need_sense))
> > + ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF))))
> > ata_gen_passthru_sense(qc);
> > else if (need_sense)
> > ata_gen_ata_sense(qc);
>
> --
> Damien Le Moal
> Western Digital Research
>
Thank you,
Igor
On Tue, Jun 18, 2024 at 01:13:24AM +0000, Igor Pylypiv wrote: > > Any failed ATA PASS-THROUGH command with CK_COND=1 has this issue. For example, > Sending READ SECTOR(S) EXT via ATA PASS-THROUGH with CK_COND=1 to an lba > that was previously corrupted by WRITE UNCORRECTABLE EXT is supposed to > return "READ ERROR - LBA MARKED BAD BY APPLICATION CLIENT" but instead it > returns generated "UNRECOVERED READ ERROR - AUTO REALLOCATE FAILED". I assume that the drive generated correct sense data, but that ata_gen_passthru_sense() overwrites/overwrote the sense data with generated sense data based on taskfile registers? Kind regards, Niklas
On Thu, Jun 20, 2024 at 03:12:37PM +0200, Niklas Cassel wrote: > On Tue, Jun 18, 2024 at 01:13:24AM +0000, Igor Pylypiv wrote: > > > > Any failed ATA PASS-THROUGH command with CK_COND=1 has this issue. For example, > > Sending READ SECTOR(S) EXT via ATA PASS-THROUGH with CK_COND=1 to an lba > > that was previously corrupted by WRITE UNCORRECTABLE EXT is supposed to > > return "READ ERROR - LBA MARKED BAD BY APPLICATION CLIENT" but instead it > > returns generated "UNRECOVERED READ ERROR - AUTO REALLOCATE FAILED". > > I assume that the drive generated correct sense data, but that > ata_gen_passthru_sense() overwrites/overwrote the sense data with > generated sense data based on taskfile registers? > Yes, that is correct. Thanks, Igor > > Kind regards, > Niklas
© 2016 - 2025 Red Hat, Inc.