Skip to contentSkip to conversation

Fix device not ready printk

RFC In review Matthew Wilcox 3 participants 5 messages Jun 29, 2007 – Sep 23, 2007 SCSI

lore
Share & tools
lore

Matthew Wilcox

author
#0 lore raw

RFC 1/1 Fix device not ready printk

scsi_print_sense_hdr prints KERN_INFO, making the output from scsi_io_completion ugly:

sd 0:0:0:0: [sdb] Device not ready: <6>: Sense Key : 0x2 [current] : ASC=0x4 ASCQ=0x3

By duplicating some of the code from it, we can get the nicer output:

sd 0:0:0:0: [sdb] Device not ready: Sense Key : Not Ready [current]
sd 0:0:0:0: [sdb] Device not ready: Add. Sense: Logical unit not ready, manual intervention required

(the observant will notice that I turned on SCSI_CONSTANTS between the two kernels)

I'm in two minds about printing the 'Device not ready' twice. On the one hand, it's redundant information. On the other hand, it helps the harried sysadmin with multiple simultaneous disc failures tie the two messages together.

So I'm not signing off on this because I think it warrants further discussion.

drivers/scsi/scsi_lib.c +4 -1
index 1f5a07b..d0b8608 100644
@@ -940,7 +940,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 			if (!(req->cmd_flags & REQ_QUIET)) {
 				scmd_printk(KERN_INFO, cmd,
 					    "Device not ready: ");
-				scsi_print_sense_hdr("", &sshdr);
+				scsi_show_sense_hdr(&sshdr);
+				scmd_printk(KERN_INFO, cmd,
+					    "Device not ready: ");
+				scsi_show_extd_sense(sshdr.asc, sshdr.ascq);
 			}
 			scsi_end_request(cmd, 0, this_count, 1);
 			return;
Signature · 3 lines
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
author
#1 lore raw
Quoted 6 lines · Matthew Wilcox

I'm in two minds about printing the 'Device not ready' twice. On the one hand, it's redundant information. On the other hand, it helps the harried sysadmin with multiple simultaneous disc failures tie the two messages together.

So I'm not signing off on this because I think it warrants further discussion.

After three weeks with no discussion, I guess it may as well be applied.

----

scsi: Better error messages when device not ready

Because scsi_print_sense_hdr prefixes with KERN_INFO, the output from scsi_io_completion looks like:

sd 0:0:0:0: [sdb] Device not ready: <6>: Sense Key : 0x2 [current] : ASC=0x4 ASCQ=0x3

By using scsi_show_sense_hdr, we can get the much more appealing output:

sd 0:0:0:0: [sdb] Device not ready: Sense Key : 0x2 [current] sd 0:0:0:0: [sdb] Device not ready: ASC=0x4 ASCQ=0x3

Signed-off-by Matthew Wilcox <[email protected]>
drivers/scsi/scsi_lib.c +4 -1
index 1f5a07b..d0b8608 100644
@@ -940,7 +940,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 			if (!(req->cmd_flags & REQ_QUIET)) {
 				scmd_printk(KERN_INFO, cmd,
 					    "Device not ready: ");
-				scsi_print_sense_hdr("", &sshdr);
+				scsi_show_sense_hdr(&sshdr);
+				scmd_printk(KERN_INFO, cmd,
+					    "Device not ready: ");
+				scsi_show_extd_sense(sshdr.asc, sshdr.ascq);
 			}
 			scsi_end_request(cmd, 0, this_count, 1);
 			return;
Signature · 3 lines
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
Acked-by
#2 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Matthew" == Matthew Wilcox <[email protected]> writes:

Matthew> After three weeks with no discussion, I guess it may as well Matthew> be applied.

Matthew> scsi: Better error messages when device not ready

Looks good to me.

Acked-by Martin K. Petersen <[email protected]>
Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
#3 lore raw
In reply to Matthew Wilcox
Quoted 9 lines · Matthew Wilcox
Quoted 6 lines · Matthew Wilcox

I'm in two minds about printing the 'Device not ready' twice. On the one hand, it's redundant information. On the other hand, it helps the harried sysadmin with multiple simultaneous disc failures tie the two messages together.

So I'm not signing off on this because I think it warrants further discussion.

After three weeks with no discussion, I guess it may as well be applied.

Sorry ... busy with other things.

The main thing I don't like is that we've spent a lot of time moving sense prints into a library so that if someone gullible^Wsensible enough to want to do a reporting infrastructure comes along, we have all the entry points nicely laid out, this is a retrograde step on that because it reintroduces the open coding.

How about something like the attached. It does exactly what you want but retains the tap into the constants file for logging?

James

drivers/scsi/constants.c +14 -0
Index: BUILD-2.6/drivers/scsi/constants.c
===================================================================
@@ -1235,6 +1235,20 @@ scsi_print_sense_hdr(const char *name, s
 }
 EXPORT_SYMBOL(scsi_print_sense_hdr);
 
+/*
+ * Print normalized SCSI sense header with device information and a prefix.
+ */
+void
+scsi_cmd_print_sense_hdr(struct scsi_cmnd *scmd, const char *name,
+			  struct scsi_sense_hdr *sshdr)
+{
+	scmd_printk(KERN_INFO, scmd, "%s: ", name);
+	scsi_show_sense_hdr(sshdr);
+	scmd_printk(KERN_INFO, scmd, "%s: ", name);
+	scsi_show_extd_sense(sshdr->asc, sshdr->ascq);
+}
+EXPORT_SYMBOL(scsi_cmd_print_sense_hdr);
+
 static void
 scsi_decode_sense_buffer(const unsigned char *sense_buffer, int sense_len,
 		       struct scsi_sense_hdr *sshdr)
drivers/scsi/scsi_lib.c +5 -5
Index: BUILD-2.6/drivers/scsi/scsi_lib.c
===================================================================
@@ -944,11 +944,11 @@ void scsi_io_completion(struct scsi_cmnd
 					break;
 				}
 			}
-			if (!(req->cmd_flags & REQ_QUIET)) {
-				scmd_printk(KERN_INFO, cmd,
-					    "Device not ready: ");
-				scsi_print_sense_hdr("", &sshdr);
-			}
+			if (!(req->cmd_flags & REQ_QUIET))
+				scsi_cmd_print_sense_hdr(cmd,
+							 "Device not ready",
+							 &sshdr);
+
 			scsi_end_request(cmd, 0, this_count, 1);
 			return;
 		case VOLUME_OVERFLOW:
include/scsi/scsi_dbg.h +2 -0
Index: BUILD-2.6/include/scsi/scsi_dbg.h
===================================================================
@@ -9,6 +9,8 @@ extern void __scsi_print_command(unsigne
 extern void scsi_show_extd_sense(unsigned char, unsigned char);
 extern void scsi_show_sense_hdr(struct scsi_sense_hdr *);
 extern void scsi_print_sense_hdr(const char *, struct scsi_sense_hdr *);
+extern void scsi_cmd_print_sense_hdr(struct scsi_cmnd *, const char *,
+				     struct scsi_sense_hdr *);
 extern void scsi_print_sense(char *, struct scsi_cmnd *);
 extern void __scsi_print_sense(const char *name,
 			       const unsigned char *sense_buffer,
author
#4 lore raw
Quoted 5 lines · James Bottomley

The main thing I don't like is that we've spent a lot of time moving sense prints into a library so that if someone gullible^Wsensible enough to want to do a reporting infrastructure comes along, we have all the entry points nicely laid out, this is a retrograde step on that because it reintroduces the open coding.

Your patch is certainly an improvement from that point of view; it consolidates two calls (which would have to be tied together by said infrastructure) into one.

Quoted 5 lines

How about something like the attached. It does exactly what you want but retains the tap into the constants file for logging?

drivers/scsi/constants.c +2 -0
+/*
+ * Print normalized SCSI sense header with device information and a prefix.

drivers/scsi/constants.c scsi_print_sense_hdr(const char *name, s · line 1249

Quoted 10 lines
drivers/scsi/constants.c +10 -0
+ */
+void
+scsi_cmd_print_sense_hdr(struct scsi_cmnd *scmd, const char *name,
+			  struct scsi_sense_hdr *sshdr)
+{
+	scmd_printk(KERN_INFO, scmd, "%s: ", name);
+	scsi_show_sense_hdr(sshdr);
+	scmd_printk(KERN_INFO, scmd, "%s: ", name);
+	scsi_show_extd_sense(sshdr->asc, sshdr->ascq);
+}

I think 'name' is wrong. It's not a name, it's a description. How about s/name/desc/g ?

With that change, Acked-by: Matthew Wilcox <[email protected]>

Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."