Re: [PATCH v2] ata: libata-sata: Use LBA from sense data descriptor

[Igor Pylypiv <ipylypiv@google.com>]

On Wed, Apr 09, 2025 at 10:45:47AM +0200, Niklas Cassel wrote:
> The definition of the LBA field in the sense data descriptor is:
> 
> "If definition of the sense data to be returned when a command completes
> without an error includes an LBA value, then the LBA field contains the
> defined value. Otherwise, the LBA field contains a copy of the LBA field
> in the command inputs for the command that completed without an error
> and returned sense data."
> 
> Thus, the LBA field in the sense data descriptor can contain a LBA value
> that is different from the LBA field in the command input.
> 
> Therefore, just like how ata_eh_read_log_10h() overrides qc->result_tf
> with the LBA in the NCQ Command Error log, override qc->result_tf with
> the LBA in the Successful NCQ Commands log.

Hi Niklas,

Should we also override other fields e.g. COMMAND, FEATURE, COUNT, AUXILIARY?
I understand that per current SAT spec those fields contain data from command
inputs so we can get the same data directly from qc->tf and technically don't
need to fill qc->result_tf with the same.

If I understand correctly, qc->result_tf contains data filled from a shared
FIS so it is likely that it contains data that belongs to some other command,
is that true? If that's true, should we clear the qc->result_tf before filling
the fields with data from the Successful NCQ Commands log?

Thanks,
Igor

> 
> Fixes: 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL commands using policy 0xD")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/libata-sata.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index ba300cc0a3a3..c21fdacd0777 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1556,6 +1556,14 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
>  			continue;
>  		}
>  
> +		/* LBA in sense data desc can be different from LBA in qc->tf */
> +		qc->result_tf.lbal = sense[8];
> +		qc->result_tf.lbam = sense[9];
> +		qc->result_tf.lbah = sense[10];
> +		qc->result_tf.hob_lbal = sense[11];
> +		qc->result_tf.hob_lbam = sense[12];
> +		qc->result_tf.hob_lbah = sense[13];
> +
>  		/* Set sense without also setting scsicmd->result */
>  		scsi_build_sense_buffer(dev->flags & ATA_DFLAG_D_SENSE,
>  					qc->scsicmd->sense_buffer, sk,
> -- 
> 2.49.0
>