| Message ID | 20260831020451.585944-36-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Use defined 16-bits ASC/ASCQ combinations | expand |
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;
> }
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c index 4f1fd389260f..c717e35ea2af 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; } cgc->stat = -EIO; @@ -2979,9 +2978,8 @@ static noinline int mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi, cgc->sshdr = &sshdr; cgc->data_direction = CGC_DATA_READ; ret = cdrom_read_block(cdi, cgc, lba, 1, format, blocksize); - if (ret && sshdr.sense_key == 0x05 && - sshdr.asc == 0x20 && - sshdr.ascq == 0x00) { + if (ret && sshdr.sense_key == ILLEGAL_REQUEST && + sshdr.sense_code == INVALID_COMMAND_OP_CODE) { /* * SCSI-II devices are not required to support * READ_CD, so let's try switching block size
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. No functional change intended. Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/cdrom/cdrom.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)