diff mbox series

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

Message ID 20260831020451.585944-13-dlemoal@kernel.org
State New
Headers show
Series Use defined 16-bits ASC/ASCQ combinations | expand

Commit Message

Damien Le Moal Aug. 31, 2026, 2:04 a.m. UTC 
Refactor the SCSI tape 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/scsi/st.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

Comments

sashiko-bot@kernel.org Aug. 31, 2026, 2:19 a.m. UTC | #1 
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);
                                        ^^^^^^^^^^^^^^

[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?
diff mbox series

Patch

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index f1c3c4946637..1ca9753cb734 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -421,11 +421,13 @@  static int st_chk_result(struct scsi_tape *STp, struct st_request * SRpnt)
 					       STp->cln_sense_mask) != 0);
 	}
 	if (cmdstatp->have_sense &&
-	    cmdstatp->sense_hdr.asc == 0 && cmdstatp->sense_hdr.ascq == 0x17)
-		STp->cleaning_req = 1; /* ASC and ASCQ => cleaning requested */
+	    cmdstatp->sense_hdr.sense_code == CLEANING_REQUESTED)
+		STp->cleaning_req = 1;
 	if (cmdstatp->have_sense && scode == UNIT_ATTENTION &&
-		cmdstatp->sense_hdr.asc == 0x29 && !STp->pos_unknown) {
-		STp->pos_unknown = 1; /* ASC => power on / reset */
+	    scsi_sense_asc(&cmdstatp->sense_hdr) ==
+	    ASC_POWER_ON_RESET_OR_BUS_DEVICE_RESET_OCCURRED &&
+	    !STp->pos_unknown) {
+		STp->pos_unknown = 1;
 		st_printk(KERN_WARNING, STp, "Power on/reset recognized.");
 	}
 
@@ -1003,7 +1005,8 @@  static int test_ready(struct scsi_tape *STp, int do_wait)
 			scode = cmdstatp->sense_hdr.sense_key;
 
 			if (scode == UNIT_ATTENTION) { /* New media? */
-				if (cmdstatp->sense_hdr.asc == 0x28) { /* New media */
+				if (scsi_sense_asc(&cmdstatp->sense_hdr) ==
+				    ASC_NOT_READY_TO_READY_CHANGE_MEDIUM_MAY_HAVE_CHANGED) {
 					new_session = 1;
 					DEBC_printk(STp, "New tape session.");
 				}
@@ -1026,14 +1029,13 @@  static int test_ready(struct scsi_tape *STp, int do_wait)
 					waits++;
 					continue;
 				}
-				else {
-					if ((STp->device)->scsi_level >= SCSI_2 &&
-					    cmdstatp->sense_hdr.asc == 0x3a)	/* Check ASC */
-						retval = CHKRES_NO_TAPE;
-					else
-						retval = CHKRES_NOT_READY;
-					break;
-				}
+				if ((STp->device)->scsi_level >= SCSI_2 &&
+				    scsi_sense_asc(&cmdstatp->sense_hdr) ==
+				    ASC_MEDIUM_NOT_PRESENT)
+					retval = CHKRES_NO_TAPE;
+				else
+					retval = CHKRES_NOT_READY;
+				break;
 			}
 		}
 
@@ -3105,9 +3107,10 @@  static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
 			   cmd_in == MTSETDRVBUFFER ||
 			   cmd_in == SET_DENS_AND_BLK) {
 			if (cmdstatp->sense_hdr.sense_key == ILLEGAL_REQUEST &&
-				cmdstatp->sense_hdr.asc == 0x24 &&
-				(STp->device)->scsi_level <= SCSI_2 &&
-				!(STp->use_pf & PF_TESTED)) {
+			    scsi_sense_asc(&cmdstatp->sense_hdr) ==
+			    ASC_INVALID_FIELD_IN_CDB &&
+			    (STp->device)->scsi_level <= SCSI_2 &&
+			    !(STp->use_pf & PF_TESTED)) {
 				/* Try the other possible state of Page Format if not
 				   already tried */
 				STp->use_pf = (STp->use_pf ^ USE_PF) | PF_TESTED;