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

[Damien Le Moal <dlemoal@kernel.org>]

On 9/2/26 20:29, Niklas Cassel wrote:
>> +		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.

Yes. I considered keeping the original call to scsi_build_sense_buffer() but
went with using scsi_set_sense_buffer() to be consistent with the fact that we
have the 16-bits sense_code and to try to stick with using it as much as we can.
I do understand that this is a bit silly to combined and split again the
asc/ascq, but this is all done only if there is a sense, meaning there was an
error and so things went through EH already and thus have been slow anyway. So I
do not see this as a perf problem at all and prefer to priviledge
readability/code simplicity.

> 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() ?

The target code (and ATA code) fill the sense buffer from asc and ascq obtained
directly from command results, not from a sense code. So I kept that function to
avoid doing the combine/split asc/ascq everywhere. This is not great: blame the
scsi specs that define asc/ascq as separate fields while the sense codes are
essentially defined as a 16-bits value combining both :)

> 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?

Yeah, I wanted to, but that would mean having a single gigantic patch that
change the function use is all scsi and ata code in one go. I did not want to do
that to facilitate review. But if that's the preferred path, I will do that.

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

Yes, I can completely drop it if everyone is OK with a few unnecessary
combine/split asc/ascq in some places.

-- 
Damien Le Moal
Western Digital Research