Re: [PATCH 36/37] ata: libata: use 16-bits defined sense codes

[Niklas Cassel <cassel@kernel.org>]

On Mon, Aug 31, 2026 at 11:04:50AM +0900, Damien Le Moal wrote:
> Refactor libata core code to use the 16-bits sense_code field of
> struct scsi_sense_hdr and replace all hard-coded additional sense codes
> and additional sense code qualifiers with the enum values defined in
> include/scsi/scsi_sense.h. This helps with code clarity as the sense codes
> being processed are easier to test and self-documented.
> 
> No functional change intended.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-eh.c   |  13 +-
>  drivers/ata/libata-sata.c |  14 +-
>  drivers/ata/libata-scsi.c | 346 +++++++++++++++++++++++---------------
>  drivers/ata/libata.h      |   5 +-
>  4 files changed, 226 insertions(+), 152 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index c93c24b22d9c..71961fbaa42f 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -1516,11 +1516,13 @@ static bool ata_eh_request_sense(struct ata_queued_cmd *qc)
>  	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
>  	/* Ignore err_mask; ATA_ERR might be set */
>  	if (tf.status & ATA_SENSE) {
> -		if (ata_scsi_sense_is_valid(tf.lbah, tf.lbam, tf.lbal)) {
> +		u16 sense_code = scsi_sense_code(tf.lbam, tf.lbal);

Here you take asc and ascq and combine them to one.
I see the reason, since ata_scsi_sense_is_valid() takes a u16 sense code.
That is fine, and I think makes ata_scsi_sense_is_valid() easier to read.


> +
> +		if (ata_scsi_sense_is_valid(tf.lbah, sense_code)) {
>  			/* Set sense without also setting scsicmd->result */
> -			scsi_build_sense_buffer(dev->flags & ATA_DFLAG_D_SENSE,
> -						cmd->sense_buffer, tf.lbah,
> -						tf.lbam, tf.lbal);
> +			scsi_set_sense_buffer(dev->flags & ATA_DFLAG_D_SENSE,
> +					      cmd->sense_buffer, tf.lbah,
> +					      sense_code);

However, here you call scsi_set_sense_buffer() which is just an inline
function that calls:
scsi_build_sense_buffer(desc, buf, key, scsi_sense_code_asc(code),
			scsi_sense_code_ascq(code));

which will call scsi_sense_code_asc() and scsi_sense_code_ascq() to
split sense_code to asc and ascq again.

It seems that you series has decided to not kill scsi_build_sense_buffer().

As long as scsi_build_sense_buffer() exists, and since we already have asc
and ascq stored in separate variables, I think we should let this code
continue calling scsi_build_sense_buffer().

Especially since, even after this series, we still will have a call to
scsi_build_sense_buffer() in libata-sata.c:ata_eh_get_ncq_success_sense().

I think either this function and ata_eh_get_ncq_success_sense() should both
call scsi_build_sense_buffer(), or both should call scsi_set_sense_buffer().



Is there a reason why you don't convert all scsi_build_sense_buffer() users
to scsi_set_sense_buffer() and drop scsi_build_sense_buffer() ?
You could even keep the same function name (scsi_build_sense_buffer()) and
just have it take an u16 sense_code instead of u8 asc + u8 ascq?

(Doing a git grep scsi_build_sense_buffer shows a few users even after this
series.)


Kind regards,
Niklas