Re: [PATCH 35/37] cdrom: use 16-bits defined sense codes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Tracepoint scsi_cmd_done_timeout_template incorrectly logs the 'ascq' field as the 'asc' value.
--

commit 7e6312a47a6eeb4644f3eba5f9acd48a9a7fd4c4
Author: Damien Le Moal <dlemoal@kernel.org>

    cdrom: use 16-bits defined sense codes
    
    Refactor the cdrom driver 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.

> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 4f1fd389260f5..c717e35ea2afc 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -345,9 +345,8 @@ int cdrom_dummy_generic_packet(struct cdrom_device_info *cdi,
>  			       struct packet_command *cgc)
>  {
>  	if (cgc->sshdr) {
> -		cgc->sshdr->sense_key = 0x05;
> -		cgc->sshdr->asc = 0x20;
> -		cgc->sshdr->ascq = 0x00;
> +		cgc->sshdr->sense_key = ILLEGAL_REQUEST;
> +		cgc->sshdr->sense_code = INVALID_COMMAND_OP_CODE;
>  	}

[Severity: Medium]
While reviewing the sense code changes, I noticed a related issue in
include/trace/events/scsi.h. Does the scsi_cmd_done_timeout_template
tracepoint incorrectly log the 'ascq' field?

include/trace/events/scsi.h: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 = scsi_sense_asc(&sshdr);
			__entry->ascq = scsi_sense_asc(&sshdr);
		} else {
...
}

Any time a tracepoint based on scsi_cmd_done_timeout_template is hit and a
valid SCSI sense buffer is present, it looks like scsi_sense_asc(&sshdr) is
called twice, assigning its return value to both __entry->asc and
__entry->ascq.

Does this irrevocably lose the Additional Sense Code Qualifier (ascq) value
in trace events (like scsi_dispatch_cmd_done and scsi_dispatch_cmd_timeout),
corrupting the trace output and hampering debugging?

>  
>  	cgc->stat = -EIO;
>  	return -EIO;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831020451.585944-1-dlemoal@kernel.org?part=35