drivers/ata/libata-sata.c | 2 -- drivers/ata/libata-scsi.c | 40 ++++++++++++++++++++++----------------- drivers/ata/libata.h | 3 --- 3 files changed, 23 insertions(+), 22 deletions(-)
Populate the INFORMATION field with ATA LBA when sense data is obtained
by the ata_eh_request_sense(). Kernel already populates the INFORMATION
field when sense data is reported via autosense or when it is generated
by the ata_gen_ata_sense().
ATA PASS-THROUGH commands, unlike regular ATA commands, populate
the INFORMATION field with ATA ERROR, STATUS, DEVICE, and COUNT(7:0)
fields thus setting ATA LBA into the INFORMATION field is incorrect.
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
drivers/ata/libata-sata.c | 2 --
drivers/ata/libata-scsi.c | 40 ++++++++++++++++++++++-----------------
drivers/ata/libata.h | 3 ---
3 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index ba300cc0a3a3..b01b52e95352 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1644,8 +1644,6 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
if (ata_scsi_sense_is_valid(sense_key, asc, ascq)) {
ata_scsi_set_sense(dev, qc->scsicmd, sense_key, asc,
ascq);
- ata_scsi_set_sense_information(dev, qc->scsicmd,
- &qc->result_tf);
qc->flags |= ATA_QCFLAG_SENSE_VALID;
}
}
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 2796c0da8257..7e93581439b2 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -216,17 +216,30 @@ void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd,
scsi_build_sense(cmd, d_sense, sk, asc, ascq);
}
-void ata_scsi_set_sense_information(struct ata_device *dev,
- struct scsi_cmnd *cmd,
- const struct ata_taskfile *tf)
+/**
+ * ata_scsi_set_sense_information - Populate INFORMATION field
+ * @qc: ATA command
+ *
+ * Populates the INFORMATION field with ATA LBA.
+ *
+ * LOCKING:
+ * None.
+ */
+static void ata_scsi_set_sense_information(struct ata_queued_cmd *qc)
{
u64 information;
- information = ata_tf_read_block(tf, dev);
+ if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
+ ata_dev_dbg(qc->dev,
+ "missing result TF: can't set INFORMATION sense field\n");
+ return;
+ }
+
+ information = ata_tf_read_block(&qc->result_tf, qc->dev);
if (information == U64_MAX)
return;
- scsi_set_sense_information(cmd->sense_buffer,
+ scsi_set_sense_information(qc->scsicmd->sense_buffer,
SCSI_SENSE_BUFFERSIZE, information);
}
@@ -971,8 +984,7 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
* ata_gen_ata_sense - generate a SCSI fixed sense block
* @qc: Command that we are erroring out
*
- * Generate sense block for a failed ATA command @qc. Descriptor
- * format is used to accommodate LBA48 block address.
+ * Generate sense block for a failed ATA command @qc.
*
* LOCKING:
* None.
@@ -982,8 +994,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
struct ata_device *dev = qc->dev;
struct scsi_cmnd *cmd = qc->scsicmd;
struct ata_taskfile *tf = &qc->result_tf;
- unsigned char *sb = cmd->sense_buffer;
- u64 block;
u8 sense_key, asc, ascq;
if (ata_dev_disabled(dev)) {
@@ -1014,12 +1024,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
return;
}
-
- block = ata_tf_read_block(&qc->result_tf, dev);
- if (block == U64_MAX)
- return;
-
- scsi_set_sense_information(sb, SCSI_SENSE_BUFFERSIZE, block);
}
void ata_scsi_sdev_config(struct scsi_device *sdev)
@@ -1679,8 +1683,10 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
ata_scsi_set_passthru_sense_fields(qc);
if (is_ck_cond_request)
set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
- } else if (is_error && !have_sense) {
- ata_gen_ata_sense(qc);
+ } else if (is_error) {
+ if (!have_sense)
+ ata_gen_ata_sense(qc);
+ ata_scsi_set_sense_information(qc);
}
ata_qc_done(qc);
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 0337be4faec7..ce5c628fa6fd 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -141,9 +141,6 @@ extern int ata_scsi_offline_dev(struct ata_device *dev);
extern bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq);
extern void ata_scsi_set_sense(struct ata_device *dev,
struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq);
-extern void ata_scsi_set_sense_information(struct ata_device *dev,
- struct scsi_cmnd *cmd,
- const struct ata_taskfile *tf);
extern void ata_scsi_media_change_notify(struct ata_device *dev);
extern void ata_scsi_hotplug(struct work_struct *work);
extern void ata_scsi_dev_rescan(struct work_struct *work);
--
2.49.0.472.ge94155a9ec-googOn 3/29/25 03:30, Igor Pylypiv wrote:
> Populate the INFORMATION field with ATA LBA when sense data is obtained
> by the ata_eh_request_sense(). Kernel already populates the INFORMATION
s/by the/with
s/Kernel/The kernel or s/Kernel/libata-eh
> field when sense data is reported via autosense or when it is generated
> by the ata_gen_ata_sense().
>
> ATA PASS-THROUGH commands, unlike regular ATA commands, populate
> the INFORMATION field with ATA ERROR, STATUS, DEVICE, and COUNT(7:0)
> fields thus setting ATA LBA into the INFORMATION field is incorrect.
Could you rephrase all of this is a manner that is clearer ? That is, describe
the problem first and then how you fix it. As-is, this is hard to understand.
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 2796c0da8257..7e93581439b2 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -216,17 +216,30 @@ void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd,
> scsi_build_sense(cmd, d_sense, sk, asc, ascq);
> }
>
> -void ata_scsi_set_sense_information(struct ata_device *dev,
> - struct scsi_cmnd *cmd,
> - const struct ata_taskfile *tf)
> +/**
> + * ata_scsi_set_sense_information - Populate INFORMATION field
> + * @qc: ATA command
> + *
> + * Populates the INFORMATION field with ATA LBA.
> + *
> + * LOCKING:
> + * None.
> + */
The function name is clear enough. We do not need this kdoc block to clarify it
and this is a static function anyway.
> +static void ata_scsi_set_sense_information(struct ata_queued_cmd *qc)
> {
> u64 information;
>
> - information = ata_tf_read_block(tf, dev);
> + if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> + ata_dev_dbg(qc->dev,
> + "missing result TF: can't set INFORMATION sense field\n");
Why ata_dev_dbg() ? This should be ata_dev_err(), no ? With your change, this is
not supposed to be called without the rtf filled...
> + return;
> + }
> +
> + information = ata_tf_read_block(&qc->result_tf, qc->dev);
> if (information == U64_MAX)
> return;
>
> - scsi_set_sense_information(cmd->sense_buffer,
> + scsi_set_sense_information(qc->scsicmd->sense_buffer,
> SCSI_SENSE_BUFFERSIZE, information);
> }
>
> @@ -971,8 +984,7 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> * ata_gen_ata_sense - generate a SCSI fixed sense block
> * @qc: Command that we are erroring out
> *
> - * Generate sense block for a failed ATA command @qc. Descriptor
> - * format is used to accommodate LBA48 block address.
> + * Generate sense block for a failed ATA command @qc.
> *
> * LOCKING:
> * None.
> @@ -982,8 +994,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> struct ata_device *dev = qc->dev;
> struct scsi_cmnd *cmd = qc->scsicmd;
> struct ata_taskfile *tf = &qc->result_tf;
> - unsigned char *sb = cmd->sense_buffer;
> - u64 block;
> u8 sense_key, asc, ascq;
>
> if (ata_dev_disabled(dev)) {
> @@ -1014,12 +1024,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
> return;
> }
> -
> - block = ata_tf_read_block(&qc->result_tf, dev);
> - if (block == U64_MAX)
> - return;
> -
> - scsi_set_sense_information(sb, SCSI_SENSE_BUFFERSIZE, block);
> }
>
> void ata_scsi_sdev_config(struct scsi_device *sdev)
> @@ -1679,8 +1683,10 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> ata_scsi_set_passthru_sense_fields(qc);
> if (is_ck_cond_request)
> set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
> - } else if (is_error && !have_sense) {
> - ata_gen_ata_sense(qc);
> + } else if (is_error) {
> + if (!have_sense)
> + ata_gen_ata_sense(qc);
> + ata_scsi_set_sense_information(qc);
> }
>
> ata_qc_done(qc);
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 0337be4faec7..ce5c628fa6fd 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -141,9 +141,6 @@ extern int ata_scsi_offline_dev(struct ata_device *dev);
> extern bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq);
> extern void ata_scsi_set_sense(struct ata_device *dev,
> struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq);
> -extern void ata_scsi_set_sense_information(struct ata_device *dev,
> - struct scsi_cmnd *cmd,
> - const struct ata_taskfile *tf);
> extern void ata_scsi_media_change_notify(struct ata_device *dev);
> extern void ata_scsi_hotplug(struct work_struct *work);
> extern void ata_scsi_dev_rescan(struct work_struct *work);
--
Damien Le Moal
Western Digital Research
On Tue, Apr 01, 2025 at 04:33:55PM +0900, Damien Le Moal wrote:
> On 3/29/25 03:30, Igor Pylypiv wrote:
> > Populate the INFORMATION field with ATA LBA when sense data is obtained
> > by the ata_eh_request_sense(). Kernel already populates the INFORMATION
>
> s/by the/with
> s/Kernel/The kernel or s/Kernel/libata-eh
>
> > field when sense data is reported via autosense or when it is generated
> > by the ata_gen_ata_sense().
> >
> > ATA PASS-THROUGH commands, unlike regular ATA commands, populate
> > the INFORMATION field with ATA ERROR, STATUS, DEVICE, and COUNT(7:0)
> > fields thus setting ATA LBA into the INFORMATION field is incorrect.
>
> Could you rephrase all of this is a manner that is clearer ? That is, describe
> the problem first and then how you fix it. As-is, this is hard to understand.
Thanks, Damien. I'll rephrase it in v2.
>
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 2796c0da8257..7e93581439b2 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -216,17 +216,30 @@ void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd,
> > scsi_build_sense(cmd, d_sense, sk, asc, ascq);
> > }
> >
> > -void ata_scsi_set_sense_information(struct ata_device *dev,
> > - struct scsi_cmnd *cmd,
> > - const struct ata_taskfile *tf)
> > +/**
> > + * ata_scsi_set_sense_information - Populate INFORMATION field
> > + * @qc: ATA command
> > + *
> > + * Populates the INFORMATION field with ATA LBA.
> > + *
> > + * LOCKING:
> > + * None.
> > + */
>
> The function name is clear enough. We do not need this kdoc block to clarify it
> and this is a static function anyway.
>
> > +static void ata_scsi_set_sense_information(struct ata_queued_cmd *qc)
> > {
> > u64 information;
> >
> > - information = ata_tf_read_block(tf, dev);
> > + if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> > + ata_dev_dbg(qc->dev,
> > + "missing result TF: can't set INFORMATION sense field\n");
>
> Why ata_dev_dbg() ? This should be ata_dev_err(), no ? With your change, this is
> not supposed to be called without the rtf filled...
I used ata_dev_dbg() to match ata_scsi_set_passthru_sense_fields(),
ata_gen_passthru_sense(), and ata_gen_ata_sense().
Last year we settled on using the ata_dev_dbg() to avoid a potential log spam:
https://lore.kernel.org/lkml/ab9f6564-3df1-4061-93e7-32a59aacb205@kernel.org/
Thanks,
Igor
> > + return;
> > + }
> > +
> > + information = ata_tf_read_block(&qc->result_tf, qc->dev);
> > if (information == U64_MAX)
> > return;
> >
> > - scsi_set_sense_information(cmd->sense_buffer,
> > + scsi_set_sense_information(qc->scsicmd->sense_buffer,
> > SCSI_SENSE_BUFFERSIZE, information);
> > }
> >
> > @@ -971,8 +984,7 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> > * ata_gen_ata_sense - generate a SCSI fixed sense block
> > * @qc: Command that we are erroring out
> > *
> > - * Generate sense block for a failed ATA command @qc. Descriptor
> > - * format is used to accommodate LBA48 block address.
> > + * Generate sense block for a failed ATA command @qc.
> > *
> > * LOCKING:
> > * None.
> > @@ -982,8 +994,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> > struct ata_device *dev = qc->dev;
> > struct scsi_cmnd *cmd = qc->scsicmd;
> > struct ata_taskfile *tf = &qc->result_tf;
> > - unsigned char *sb = cmd->sense_buffer;
> > - u64 block;
> > u8 sense_key, asc, ascq;
> >
> > if (ata_dev_disabled(dev)) {
> > @@ -1014,12 +1024,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> > ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
> > return;
> > }
> > -
> > - block = ata_tf_read_block(&qc->result_tf, dev);
> > - if (block == U64_MAX)
> > - return;
> > -
> > - scsi_set_sense_information(sb, SCSI_SENSE_BUFFERSIZE, block);
> > }
> >
> > void ata_scsi_sdev_config(struct scsi_device *sdev)
> > @@ -1679,8 +1683,10 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> > ata_scsi_set_passthru_sense_fields(qc);
> > if (is_ck_cond_request)
> > set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
> > - } else if (is_error && !have_sense) {
> > - ata_gen_ata_sense(qc);
> > + } else if (is_error) {
> > + if (!have_sense)
> > + ata_gen_ata_sense(qc);
> > + ata_scsi_set_sense_information(qc);
> > }
> >
> > ata_qc_done(qc);
> > diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> > index 0337be4faec7..ce5c628fa6fd 100644
> > --- a/drivers/ata/libata.h
> > +++ b/drivers/ata/libata.h
> > @@ -141,9 +141,6 @@ extern int ata_scsi_offline_dev(struct ata_device *dev);
> > extern bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq);
> > extern void ata_scsi_set_sense(struct ata_device *dev,
> > struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq);
> > -extern void ata_scsi_set_sense_information(struct ata_device *dev,
> > - struct scsi_cmnd *cmd,
> > - const struct ata_taskfile *tf);
> > extern void ata_scsi_media_change_notify(struct ata_device *dev);
> > extern void ata_scsi_hotplug(struct work_struct *work);
> > extern void ata_scsi_dev_rescan(struct work_struct *work);
>
>
> --
> Damien Le Moal
> Western Digital Research
© 2016 - 2025 Red Hat, Inc.