Re: [PATCH 12/37] scsi: st: use 16-bits defined sense codes

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

On 2026/09/16 22:29, Laurence Oberman wrote:
> On Mon, 2026-08-31 at 02:19 +0000, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential
>> issue(s) to consider:
>> - [Medium] Incorrectly assigning ASC value to ASCQ in SCSI
>> tracepoint.
>> --
>>
>> commit c1f7c0aa9501c3562bd45de8a85431d940fa338b
>> Author: Damien Le Moal <dlemoal@kernel.org>
>> Subject: scsi: st: use 16-bits defined sense codes
>>
>> This commit refactors the SCSI tape driver to use the 16-bit
>> sense_code
>> field of struct scsi_sense_hdr. It replaces hard-coded additional
>> sense
>> codes and qualifiers with enum values defined in
>> include/scsi/scsi_sense.h
>> to improve code clarity.
>>
>> While reviewing this patch series, I noticed an issue introduced in
>> the
>> related change to include/trace/events/scsi.h:
>>
>>> diff --git a/include/trace/events/scsi.h
>>> b/include/trace/events/scsi.h
>>> --- a/include/trace/events/scsi.h
>>> +++ b/include/trace/events/scsi.h
>>> @@ -303,8 +303,8 @@
>>> DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>>>  		if (cmd->sense_buffer && SCSI_SENSE_VALID(cmd) &&
>>>  		    scsi_command_normalize_sense(cmd, &sshdr)) {
>>>  			__entry->sense_key = sshdr.sense_key;
>>> -			__entry->asc = sshdr.asc;
>>> -			__entry->ascq = sshdr.ascq;
>>> +			__entry->asc = scsi_sense_asc(&sshdr);
>>> +			__entry->ascq = scsi_sense_asc(&sshdr); <-
>>> --- here
>>                                         ^^^^^^^^^^^^^^
>>
>> [Severity: Medium]
>> Is there a typo in this assignment?
>>
>> In the scsi_cmd_done_timeout_template tracepoint, __entry->ascq is
>> being
>> assigned scsi_sense_asc(&sshdr) instead of the ASCQ value. This
>> appears to
>> log the Additional Sense Code for both the asc and ascq fields, which
>> will
>> result in lost diagnostic data when userspace tools analyze SCSI
>> errors via
>> tracepoints.
>>
>> Should this use scsi_sense_ascq(&sshdr) for the ascq field instead?
> 
> Hello Damien
> This seems to be real issue here that sashiko found
> 
> We assign the same for __entry->ascq  as we do for __entry->asc i.e.
> scsi_sense_asc(&sshdr)
> 
> We need to have it as
> 
> +			__entry->asc = scsi_sense_asc(&sshdr);
> +			__entry->ascq = scsi_sense_ascq(&sshdr); <--
> 
> Otherwise we will miss the ASCQ

Yes. That was fixed in later versions. The series that Martin queued up has this
fix :)



-- 
Damien Le Moal
Western Digital Research