Re: [PATCH v3] ata: libata-scsi: Do not set the INFORMATION field twice for ATA PT

[Niklas Cassel <cassel@kernel.org>]

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>