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