| Message ID | 20250422172123.281387-1-ipylypiv@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] ata: libata-scsi: Do not set the INFORMATION field twice for ATA PT | expand |
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>
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 !
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);
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(-)