[PATCH] normalize fixed and descriptor sense data

[Douglas Gilbert <dougg@torque.net>]

[-- Attachment #1: Type: text/plain, Size: 1479 bytes --]

This is a retransmission. My previous post on this
subject doesn't seem to have got through to the list.


After the comments on the RFC for this thread, here is
a patch against lk 2.6.9-rc1 to kick around.

The patch only touches two files: scsi.h and scsi_lib.c
It adds the proposed facility and then uses it in scsi_lib
in roughly 4 locations. IMO there were 3 sense processing errors:
    - block SG_IO did not get passed back deferred errors
      [SG_IO is a __pass-through__ interface!!]
    - MEDIUM_ERRORs _do_ get processed for deferred sense errors
      in scsi_io_completion() which seems unintended
      [I did not fix this one.]
    - invalid command operation code handling in
      __scsi_mode_sense() was just wrong

If people think this is a reasonable approach, then the rest of
the scsi mid-level and upper level driver could be converted.
As Kai pointed we may need some general routines to pick up the
sense data "extras".

The benefit of doing this conversion is that it may well
highlight a lot more sense data processing errors (if the
above is any guide).

Changes:
     - add structure to receive normalized sense data from either
       fixed or descriptor format sense data
     - add scsi_normalize_sense() function to populate that structure
     - add scsi_sense_is_deferred() function so deferred errors can
       be ignored in many contexts
     - apply new mechanism to sense data processing within the
       scsi_lib.c file

Doug Gilbert


[-- Attachment #2: scsi_sdesc269rc1.diff --]
[-- Type: text/x-patch, Size: 6281 bytes --]

--- linux/include/scsi/scsi.h	2004-08-25 10:06:42.000000000 +1000
+++ linux/include/scsi/scsi.h269rc1sdesc	2004-08-27 14:35:42.000000000 +1000
@@ -236,6 +236,33 @@
 	__u8 scsi_lun[8];
 };
 
+/* This is a slightly modified SCSI sense "descriptor" format header.
+ * The addition is to allow the 0x70 and 0x71 response codes. The idea
+ * is to place the salient data from either "fixed" or "descriptor" sense
+ * format into one structure to ease application processing.
+ * The original sense buffer should be kept around for those cases
+ * in which more information is required (e.g. the LBA of a MEDIUM ERROR).
+ */
+struct scsi_sense_hd {     /* See SPC-3 section 4.5 */
+    uint8_t response_code; /* permit: 0x0, 0x70, 0x71, 0x72, 0x73 */
+    uint8_t sense_key;
+    uint8_t asc;
+    uint8_t ascq;
+    uint8_t byte4;
+    uint8_t byte5;
+    uint8_t byte6;
+    uint8_t additional_length; /* always 0 for fixed sense format */
+};
+
+extern int scsi_normalize_sense(const uint8_t * sense_buffer, int sb_len,
+                                struct scsi_sense_hd * sshd);
+
+static inline int scsi_sense_is_deferred(struct scsi_sense_hd * sshd)
+{
+	return (sshd && (sshd->response_code >= 0x70) && 
+		(sshd->response_code & 1)) ? 1 : 0;
+}
+ 
 /*
  *  MESSAGE CODES
  */
--- linux/drivers/scsi/scsi_lib.c	2004-08-25 10:06:39.000000000 +1000
+++ linux/drivers/scsi/scsi_lib.c269rc1sdesc	2004-08-27 15:38:03.000000000 +1000
@@ -692,6 +692,7 @@
 	request_queue_t *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int clear_errors = 1;
+	struct scsi_sense_hd sshd;
 
 	/*
 	 * Free up any indirection buffers we allocated for DMA purposes. 
@@ -715,7 +716,9 @@
 			      (CHECK_CONDITION << 1) : (result & 0xff);
 		if (result) {
 			clear_errors = 0;
-			if (cmd->sense_buffer[0] & 0x70) {
+			if (scsi_normalize_sense(cmd->sense_buffer,
+					sizeof cmd->sense_buffer, &sshd)) {
+				/* SG_IO wants to know about deferred errors */
 				int len = 8 + cmd->sense_buffer[7];
 
 				if (len > SCSI_SENSE_BUFFERSIZE)
@@ -774,17 +777,18 @@
 	 * can choose a block to remap, etc.
 	 */
 	if (driver_byte(result) != 0) {
-		if ((cmd->sense_buffer[0] & 0x7f) == 0x70) {
+		if (scsi_normalize_sense(cmd->sense_buffer,
+					 SCSI_SENSE_BUFFERSIZE, &sshd) &&
+		    (! scsi_sense_is_deferred(&sshd))) {
 			/*
 			 * If the device is in the process of becoming ready,
 			 * retry.
 			 */
-			if (cmd->sense_buffer[12] == 0x04 &&
-			    cmd->sense_buffer[13] == 0x01) {
+			if (sshd.asc == 0x04 && sshd.ascq == 0x01) {
 				scsi_requeue_command(q, cmd);
 				return;
 			}
-			if ((cmd->sense_buffer[2] & 0xf) == UNIT_ATTENTION) {
+			if (sshd.sense_key == UNIT_ATTENTION) {
 				if (cmd->device->removable) {
 					/* detected disc change.  set a bit 
 					 * and quietly refuse further access.
@@ -813,7 +817,9 @@
 		 * failed, we may have read past the end of the disk.
 		 */
 
-		switch (cmd->sense_buffer[2]) {
+/* Following is broken since deferred errors fall through. [dpg 20040827] */
+
+		switch (sshd.sense_key) {
 		case ILLEGAL_REQUEST:
 			if (cmd->device->use_10_for_rw &&
 			    (cmd->cmnd[0] == READ_10 ||
@@ -1496,8 +1502,7 @@
 	}
 
 	sreq->sr_cmd_len = 0;
-	sreq->sr_sense_buffer[0] = 0;
-	sreq->sr_sense_buffer[2] = 0;
+	memset(sreq->sr_sense_buffer, 0, sizeof(sreq->sr_sense_buffer));
 	sreq->sr_data_direction = DMA_FROM_DEVICE;
 
 	memset(buffer, 0, len);
@@ -1508,14 +1513,20 @@
 	 * ILLEGAL REQUEST sense return identifies the actual command
 	 * byte as the problem.  MODE_SENSE commands can return
 	 * ILLEGAL REQUEST if the code page isn't supported */
-	if (use_10_for_ms && ! scsi_status_is_good(sreq->sr_result) &&
-	    (driver_byte(sreq->sr_result) & DRIVER_SENSE) &&
-	    sreq->sr_sense_buffer[2] == ILLEGAL_REQUEST &&
-	    (sreq->sr_sense_buffer[4] & 0x40) == 0x40 &&
-	    sreq->sr_sense_buffer[5] == 0 &&
-	    sreq->sr_sense_buffer[6] == 0 ) {
-		sreq->sr_device->use_10_for_ms = 0;
-		goto retry;
+
+	if (use_10_for_ms && (! scsi_status_is_good(sreq->sr_result)) &&
+	    (driver_byte(sreq->sr_result) & DRIVER_SENSE)) {
+		struct scsi_sense_hd sshd;
+
+		if (scsi_normalize_sense(sreq->sr_sense_buffer,
+			 sizeof sreq->sr_sense_buffer, &sshd)) {
+			if ((sshd.sense_key == ILLEGAL_REQUEST) &&
+			    (sshd.asc == 0x20)) {
+				/* Invalid command operation code */
+				sreq->sr_device->use_10_for_ms = 0;
+				goto retry;
+			}
+		}
 	}
 
 	if(scsi_status_is_good(sreq->sr_result)) {
@@ -1711,3 +1722,55 @@
 }
 EXPORT_SYMBOL(scsi_device_resume);
 
+/**
+ *	scsi_normalize_sense - normalize main elements from either fixed
+ *		or descriptor sense data format into a common format.
+ *	@sense_buffer:	byte array containing sense data returned by
+ *			device
+ *	@sb_len:	number of valid bytes in sense_buffer
+ *	@sshd:		pointer to instance of structure that common
+ *			elements are written to. Ignored if NULL.
+ *
+ *	The "main elements" from sense data are: response_code, sense_key,
+ *	asc, ascq and additional_length (only for descriptor format).
+ *	Typically this function can be called after a device has
+ *	responded to a SCSI command with the CHECK_CONDITION status.
+ *
+ *	Returns 1 if valid sense data information found, else 0;
+ **/
+int scsi_normalize_sense(const uint8_t * sense_buffer, int sb_len,
+                         struct scsi_sense_hd * sshd)
+{
+	if (sshd)
+		memset(sshd, 0, sizeof(struct scsi_sense_hd));
+	if ((NULL == sense_buffer) || (0 == sb_len) ||
+	    (0x70 != (0x70 & sense_buffer[0])))
+		return 0;
+	if (NULL == sshd)
+		return 1;
+	sshd->response_code = (0x7f & sense_buffer[0]);
+	if (sshd->response_code >= 0x72) {	/* descriptor format */
+		if (sb_len > 1)
+			sshd->sense_key = (0xf & sense_buffer[1]);
+		if (sb_len > 2)
+			sshd->asc = sense_buffer[2];
+		if (sb_len > 3)
+			sshd->ascq = sense_buffer[3];
+		if (sb_len > 7)
+			sshd->additional_length = sense_buffer[7];
+	} else {				/* fixed format */
+		if (sb_len > 2)
+			sshd->sense_key = (0xf & sense_buffer[2]);
+		if (sb_len > 7) {
+			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
+					 sb_len : (sense_buffer[7] + 8);
+			if (sb_len > 12)
+				sshd->asc = sense_buffer[12];
+			if (sb_len > 13)
+				sshd->ascq = sense_buffer[13];
+		}
+	}
+	return 1;
+}
+EXPORT_SYMBOL(scsi_normalize_sense);
+