* [PATCH v3] ata: libata-scsi: Do not set the INFORMATION field twice for ATA PT
@ 2025-04-22 17:21 Igor Pylypiv
2025-04-24 11:29 ` Niklas Cassel
2025-04-25 1:26 ` Damien Le Moal
0 siblings, 2 replies; 3+ messages in thread
From: Igor Pylypiv @ 2025-04-22 17:21 UTC (permalink / raw)
To: Damien Le Moal, Niklas Cassel; +Cc: linux-ide, linux-kernel, Igor Pylypiv
For ATA PASS-THROUGH + fixed format sense data + NCQ autosense
the INFORMATION sense data field is being written twice:
- 1st write: (redundant)
scsi_set_sense_information() sets the INFORMATION field to ATA LBA.
This is incorrect for ATA PASS-THROUGH.
- 2nd write: (correct)
ata_scsi_set_passthru_sense_fields() sets the INFORMATION field
to ATA ERROR/STATUS/DEVICE/COUNT(7:0) as per SAT spec.
There is no user-visible issue because second write overwrites
the incorrect data from the first write.
This patch eliminates the reduntant write by moving the INFORMATION
sense data field population logic to ata_scsi_qc_complete().
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
Changes in v3:
- Rewrote the commit message to clearly point that this patch removes
a redundant write to the INFORMATION sense data field.
Changes in v2:
- Rephrased commit message to make it clearer.
- Dropped kernel-doc comment for the ata_scsi_set_sense_information().
drivers/ata/libata-sata.c | 2 --
drivers/ata/libata-scsi.c | 31 ++++++++++++++-----------------
drivers/ata/libata.h | 3 ---
3 files changed, 14 insertions(+), 22 deletions(-)
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 2e4463d3a356..74a0af2888fa 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1659,8 +1659,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..ef117a0bc248 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -216,17 +216,21 @@ 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)
+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 +975,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 +985,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 +1015,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 +1674,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.850.g28803427d3-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] ata: libata-scsi: Do not set the INFORMATION field twice for ATA PT
2025-04-22 17:21 [PATCH v3] ata: libata-scsi: Do not set the INFORMATION field twice for ATA PT Igor Pylypiv
@ 2025-04-24 11:29 ` Niklas Cassel
2025-04-25 1:26 ` Damien Le Moal
1 sibling, 0 replies; 3+ messages in thread
From: Niklas Cassel @ 2025-04-24 11:29 UTC (permalink / raw)
To: Igor Pylypiv; +Cc: Damien Le Moal, linux-ide, linux-kernel
On Tue, Apr 22, 2025 at 10:21:23AM -0700, Igor Pylypiv wrote:
> For ATA PASS-THROUGH + fixed format sense data + NCQ autosense
> the INFORMATION sense data field is being written twice:
>
> - 1st write: (redundant)
> scsi_set_sense_information() sets the INFORMATION field to ATA LBA.
> This is incorrect for ATA PASS-THROUGH.
>
> - 2nd write: (correct)
> ata_scsi_set_passthru_sense_fields() sets the INFORMATION field
> to ATA ERROR/STATUS/DEVICE/COUNT(7:0) as per SAT spec.
>
> There is no user-visible issue because second write overwrites
> the incorrect data from the first write.
>
> This patch eliminates the reduntant write by moving the INFORMATION
> sense data field population logic to ata_scsi_qc_complete().
>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>
> Changes in v3:
> - Rewrote the commit message to clearly point that this patch removes
> a redundant write to the INFORMATION sense data field.
>
> Changes in v2:
> - Rephrased commit message to make it clearer.
> - Dropped kernel-doc comment for the ata_scsi_set_sense_information().
>
>
> drivers/ata/libata-sata.c | 2 --
> drivers/ata/libata-scsi.c | 31 ++++++++++++++-----------------
> drivers/ata/libata.h | 3 ---
> 3 files changed, 14 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 2e4463d3a356..74a0af2888fa 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1659,8 +1659,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..ef117a0bc248 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -216,17 +216,21 @@ 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)
> +static void ata_scsi_set_sense_information(struct ata_queued_cmd *qc)
Nice refactoring of ata_scsi_set_sense_information()!
However, since it is a separate logical change, I would have prefered for it
to be in a separate patch, but I don't think you need to respin just for this.
Reviewed-by: Niklas Cassel <cassel@kernel.org>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] ata: libata-scsi: Do not set the INFORMATION field twice for ATA PT
2025-04-22 17:21 [PATCH v3] ata: libata-scsi: Do not set the INFORMATION field twice for ATA PT Igor Pylypiv
2025-04-24 11:29 ` Niklas Cassel
@ 2025-04-25 1:26 ` Damien Le Moal
1 sibling, 0 replies; 3+ messages in thread
From: Damien Le Moal @ 2025-04-25 1:26 UTC (permalink / raw)
To: Igor Pylypiv, Niklas Cassel; +Cc: linux-ide, linux-kernel
On 4/23/25 02:21, Igor Pylypiv wrote:
> For ATA PASS-THROUGH + fixed format sense data + NCQ autosense
> the INFORMATION sense data field is being written twice:
>
> - 1st write: (redundant)
> scsi_set_sense_information() sets the INFORMATION field to ATA LBA.
> This is incorrect for ATA PASS-THROUGH.
>
> - 2nd write: (correct)
> ata_scsi_set_passthru_sense_fields() sets the INFORMATION field
> to ATA ERROR/STATUS/DEVICE/COUNT(7:0) as per SAT spec.
>
> There is no user-visible issue because second write overwrites
> the incorrect data from the first write.
>
> This patch eliminates the reduntant write by moving the INFORMATION
> sense data field population logic to ata_scsi_qc_complete().
>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
Applied to for-6.16. Thanks !
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-04-25 1:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-22 17:21 [PATCH v3] ata: libata-scsi: Do not set the INFORMATION field twice for ATA PT Igor Pylypiv
2025-04-24 11:29 ` Niklas Cassel
2025-04-25 1:26 ` Damien Le Moal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).