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@xxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxx>
