| Message ID | 20240624221211.2593736-7-ipylypiv@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | ATA PASS-THROUGH sense data fixes | expand |
On 6/25/24 00:12, Igor Pylypiv wrote: > qc->result_tf contents are only valid when the ATA_QCFLAG_RTF_FILLED flag > is set. The ATA_QCFLAG_RTF_FILLED flag should be always set for commands > that failed or for commands that have the ATA_QCFLAG_RESULT_TF flag set. > > For ATA errors and ATA PASS-THROUGH commands the ATA_QCFLAG_RTF_FILLED > flag should be always set. Added WARN_ON_ONCE() checks to generate > a warning when ATA_QCFLAG_RTF_FILLED is not set and libata needs to > generate sense data. > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/ata/libata-scsi.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index e5669a296d81..7a8a08692ce9 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -246,6 +246,9 @@ static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc) > struct ata_taskfile *tf = &qc->result_tf; > unsigned char *sb = cmd->sense_buffer; > > + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) > + return; > + > if ((sb[0] & 0x7f) >= 0x72) { > unsigned char *desc; > u8 len; > @@ -928,6 +931,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > unsigned char *sb = cmd->sense_buffer; > u8 sense_key, asc, ascq; > > + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) > + return; > + > /* > * Use ata_to_sense_error() to map status register bits > * onto sense key, asc & ascq. > @@ -971,6 +977,10 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) > ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21); > return; > } > + > + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) > + return; > + > /* Use ata_to_sense_error() to map status register bits > * onto sense key, asc & ascq. > */ Hmm. Not sure if we really need the WARN_ON() here or whether a simple logging message wouldn't be sufficient; after all, we continue fine here. Cheers, Hannes
On Tue, Jun 25, 2024 at 08:26:59AM +0200, Hannes Reinecke wrote: > On 6/25/24 00:12, Igor Pylypiv wrote: > > qc->result_tf contents are only valid when the ATA_QCFLAG_RTF_FILLED flag > > is set. The ATA_QCFLAG_RTF_FILLED flag should be always set for commands > > that failed or for commands that have the ATA_QCFLAG_RESULT_TF flag set. > > > > For ATA errors and ATA PASS-THROUGH commands the ATA_QCFLAG_RTF_FILLED > > flag should be always set. Added WARN_ON_ONCE() checks to generate > > a warning when ATA_QCFLAG_RTF_FILLED is not set and libata needs to > > generate sense data. > > > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > --- > > drivers/ata/libata-scsi.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > > index e5669a296d81..7a8a08692ce9 100644 > > --- a/drivers/ata/libata-scsi.c > > +++ b/drivers/ata/libata-scsi.c > > @@ -246,6 +246,9 @@ static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc) > > struct ata_taskfile *tf = &qc->result_tf; > > unsigned char *sb = cmd->sense_buffer; > > > > + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) > > + return; > > + > > if ((sb[0] & 0x7f) >= 0x72) { > > unsigned char *desc; > > u8 len; > > @@ -928,6 +931,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > > unsigned char *sb = cmd->sense_buffer; > > u8 sense_key, asc, ascq; > > + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) > > + return; > > + > > /* > > * Use ata_to_sense_error() to map status register bits > > * onto sense key, asc & ascq. > > @@ -971,6 +977,10 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) > > ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21); > > return; > > } > > + > > + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) > > + return; > > + > > /* Use ata_to_sense_error() to map status register bits > > * onto sense key, asc & ascq. > > */ > > Hmm. Not sure if we really need the WARN_ON() here or whether a simple > logging message wouldn't be sufficient; after all, we continue fine here. > My worry about adding a simple log statement is that it might cause a log spam if things go wrong for some reason. This code is more like a "this should never happen" comment and we always expect ATA_QCFLAG_RTF_FILLED to be present when generating sense data based on ATA registers. If WARN_ON_ONCE() is too much for this case I guess we can just remove it and silently return? Damien, Niklas, what are your thoughts on this? Thanks, Igor > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Kernel Storage Architect > hare@suse.de +49 911 74053 688 > SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg > HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich >
On 6/26/24 09:30, Igor Pylypiv wrote: > On Tue, Jun 25, 2024 at 08:26:59AM +0200, Hannes Reinecke wrote: >> On 6/25/24 00:12, Igor Pylypiv wrote: >>> qc->result_tf contents are only valid when the ATA_QCFLAG_RTF_FILLED flag >>> is set. The ATA_QCFLAG_RTF_FILLED flag should be always set for commands >>> that failed or for commands that have the ATA_QCFLAG_RESULT_TF flag set. >>> >>> For ATA errors and ATA PASS-THROUGH commands the ATA_QCFLAG_RTF_FILLED >>> flag should be always set. Added WARN_ON_ONCE() checks to generate >>> a warning when ATA_QCFLAG_RTF_FILLED is not set and libata needs to >>> generate sense data. >>> >>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com> >>> --- >>> drivers/ata/libata-scsi.c | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >>> index e5669a296d81..7a8a08692ce9 100644 >>> --- a/drivers/ata/libata-scsi.c >>> +++ b/drivers/ata/libata-scsi.c >>> @@ -246,6 +246,9 @@ static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc) >>> struct ata_taskfile *tf = &qc->result_tf; >>> unsigned char *sb = cmd->sense_buffer; >>> >>> + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) >>> + return; >>> + >>> if ((sb[0] & 0x7f) >= 0x72) { >>> unsigned char *desc; >>> u8 len; >>> @@ -928,6 +931,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) >>> unsigned char *sb = cmd->sense_buffer; >>> u8 sense_key, asc, ascq; >>> + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) >>> + return; >>> + >>> /* >>> * Use ata_to_sense_error() to map status register bits >>> * onto sense key, asc & ascq. >>> @@ -971,6 +977,10 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) >>> ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21); >>> return; >>> } >>> + >>> + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) >>> + return; >>> + >>> /* Use ata_to_sense_error() to map status register bits >>> * onto sense key, asc & ascq. >>> */ >> >> Hmm. Not sure if we really need the WARN_ON() here or whether a simple >> logging message wouldn't be sufficient; after all, we continue fine here. >> > > My worry about adding a simple log statement is that it might cause a log > spam if things go wrong for some reason. > > This code is more like a "this should never happen" comment and we always > expect ATA_QCFLAG_RTF_FILLED to be present when generating sense data > based on ATA registers. > > If WARN_ON_ONCE() is too much for this case I guess we can just remove it > and silently return? what about ata_dev_dbg() or ata_port_dbg() ? No message spamming by default and if problems are detected, they can be turned on to figure out what is going on. > > Damien, Niklas, what are your thoughts on this? > > Thanks, > Igor > >> Cheers, >> >> Hannes >> -- >> Dr. Hannes Reinecke Kernel Storage Architect >> hare@suse.de +49 911 74053 688 >> SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg >> HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich >>
On Wed, Jun 26, 2024 at 03:21:58PM +0900, Damien Le Moal wrote: > On 6/26/24 09:30, Igor Pylypiv wrote: > > On Tue, Jun 25, 2024 at 08:26:59AM +0200, Hannes Reinecke wrote: > >> On 6/25/24 00:12, Igor Pylypiv wrote: > >>> qc->result_tf contents are only valid when the ATA_QCFLAG_RTF_FILLED flag > >>> is set. The ATA_QCFLAG_RTF_FILLED flag should be always set for commands > >>> that failed or for commands that have the ATA_QCFLAG_RESULT_TF flag set. > >>> > >>> For ATA errors and ATA PASS-THROUGH commands the ATA_QCFLAG_RTF_FILLED > >>> flag should be always set. Added WARN_ON_ONCE() checks to generate > >>> a warning when ATA_QCFLAG_RTF_FILLED is not set and libata needs to > >>> generate sense data. > >>> > >>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > >>> --- > >>> drivers/ata/libata-scsi.c | 10 ++++++++++ > >>> 1 file changed, 10 insertions(+) > >>> > >>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > >>> index e5669a296d81..7a8a08692ce9 100644 > >>> --- a/drivers/ata/libata-scsi.c > >>> +++ b/drivers/ata/libata-scsi.c > >>> @@ -246,6 +246,9 @@ static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc) > >>> struct ata_taskfile *tf = &qc->result_tf; > >>> unsigned char *sb = cmd->sense_buffer; > >>> > >>> + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) > >>> + return; > >>> + > >>> if ((sb[0] & 0x7f) >= 0x72) { > >>> unsigned char *desc; > >>> u8 len; > >>> @@ -928,6 +931,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > >>> unsigned char *sb = cmd->sense_buffer; > >>> u8 sense_key, asc, ascq; > >>> + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) > >>> + return; > >>> + > >>> /* > >>> * Use ata_to_sense_error() to map status register bits > >>> * onto sense key, asc & ascq. > >>> @@ -971,6 +977,10 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) > >>> ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21); > >>> return; > >>> } > >>> + > >>> + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) > >>> + return; > >>> + > >>> /* Use ata_to_sense_error() to map status register bits > >>> * onto sense key, asc & ascq. > >>> */ > >> > >> Hmm. Not sure if we really need the WARN_ON() here or whether a simple > >> logging message wouldn't be sufficient; after all, we continue fine here. > >> > > > > My worry about adding a simple log statement is that it might cause a log > > spam if things go wrong for some reason. > > > > This code is more like a "this should never happen" comment and we always > > expect ATA_QCFLAG_RTF_FILLED to be present when generating sense data > > based on ATA registers. > > > > If WARN_ON_ONCE() is too much for this case I guess we can just remove it > > and silently return? > > what about ata_dev_dbg() or ata_port_dbg() ? > No message spamming by default and if problems are detected, they can be turned > on to figure out what is going on. ata_dev_dbg() should work. Updated the patch in v3. Thank you! Igor > > > > > Damien, Niklas, what are your thoughts on this? > > > > Thanks, > > Igor > > > >> Cheers, > >> > >> Hannes > >> -- > >> Dr. Hannes Reinecke Kernel Storage Architect > >> hare@suse.de +49 911 74053 688 > >> SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg > >> HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich > >> > > -- > Damien Le Moal > Western Digital Research >
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index e5669a296d81..7a8a08692ce9 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -246,6 +246,9 @@ static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc) struct ata_taskfile *tf = &qc->result_tf; unsigned char *sb = cmd->sense_buffer; + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) + return; + if ((sb[0] & 0x7f) >= 0x72) { unsigned char *desc; u8 len; @@ -928,6 +931,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) unsigned char *sb = cmd->sense_buffer; u8 sense_key, asc, ascq; + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) + return; + /* * Use ata_to_sense_error() to map status register bits * onto sense key, asc & ascq. @@ -971,6 +977,10 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21); return; } + + if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED))) + return; + /* Use ata_to_sense_error() to map status register bits * onto sense key, asc & ascq. */
qc->result_tf contents are only valid when the ATA_QCFLAG_RTF_FILLED flag is set. The ATA_QCFLAG_RTF_FILLED flag should be always set for commands that failed or for commands that have the ATA_QCFLAG_RESULT_TF flag set. For ATA errors and ATA PASS-THROUGH commands the ATA_QCFLAG_RTF_FILLED flag should be always set. Added WARN_ON_ONCE() checks to generate a warning when ATA_QCFLAG_RTF_FILLED is not set and libata needs to generate sense data. Signed-off-by: Igor Pylypiv <ipylypiv@google.com> --- drivers/ata/libata-scsi.c | 10 ++++++++++ 1 file changed, 10 insertions(+)