Skip to contentSkip to conversation

Use normalized sense when emulating autosense

PATCH In review Luben Tuikov 2 participants 6 messages Oct 21, 2010 – Oct 23, 2010

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Use normalized sense when emulating autosense

This patch solves two things:
1) Enables autosense emulation code to correctly interpret descriptor format sense data, and
2) Fixes a bug whereby the autosense emulation code would overwrite descriptor format sense data with SENSE KEY HARDWARE ERROR in fixed format, to incorrectly look like this:

Oct 21 14:11:07 localhost kernel: sd 7:0:0:0: [sdc]  Sense Key : Recovered Error [current] [descriptor]
Oct 21 14:11:07 localhost kernel: Descriptor sense data with sense descriptors (in hex):
Oct 21 14:11:07 localhost kernel:        72 01 04 1d 00 00 00 0e 09 0c 00 00 00 00 00 00
Oct 21 14:11:07 localhost kernel:        00 4f 00 c2 00 50
Oct 21 14:11:07 localhost kernel: sd 7:0:0:0: [sdc]  ASC=0x4 ASCQ=0x1d
---
 drivers/usb/storage/transport.c |   27 ++++++++++++---------------
 1 files changed, 12 insertions(+), 15 deletions(-)
drivers/usb/storage/transport.c +12 -15
index 64ec073..46165de 100644
@@ -691,6 +691,7 @@ void usb_stor_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)
 		int temp_result;
 		struct scsi_eh_save ses;
 		int sense_size = US_SENSE_SIZE;
+		struct scsi_sense_hdr sshdr;
 
 		/* device supports and needs bigger sense buffer */
 		if (us->fflags & US_FL_SANE_SENSE)
@@ -774,17 +775,15 @@ Retry_Sense:
 			srb->sense_buffer[7] = (US_SENSE_SIZE - 8);
 		}
 
+		scsi_normalize_sense(srb->sense_buffer, SCSI_SENSE_BUFFERSIZE,
+				     &sshdr);
+
 		US_DEBUGP("-- Result from auto-sense is %d\n", temp_result);
 		US_DEBUGP("-- code: 0x%x, key: 0x%x, ASC: 0x%x, ASCQ: 0x%x\n",
-			  srb->sense_buffer[0],
-			  srb->sense_buffer[2] & 0xf,
-			  srb->sense_buffer[12], 
-			  srb->sense_buffer[13]);
+			  sshdr.response_code, sshdr.sense_key,
+			  sshdr.asc, sshdr.ascq);
 #ifdef CONFIG_USB_STORAGE_DEBUG
-		usb_stor_show_sense(
-			  srb->sense_buffer[2] & 0xf,
-			  srb->sense_buffer[12], 
-			  srb->sense_buffer[13]);
+		usb_stor_show_sense(sshdr.sense_key, sshdr.asc, sshdr.ascq);
 #endif
 
 		/* set the result so the higher layers expect this data */
@@ -794,12 +793,7 @@ Retry_Sense:
 		 * everything worked or that there was an unspecified
 		 * problem.  We have to decide which.
 		 */
-		if (	/* Filemark 0, ignore EOM, ILI 0, no sense */
-				(srb->sense_buffer[2] & 0xaf) == 0 &&
-			/* No ASC or ASCQ */
-				srb->sense_buffer[12] == 0 &&
-				srb->sense_buffer[13] == 0) {
-
+		if (sshdr.sense_key == 0 && sshdr.asc == 0 && sshdr.ascq == 0) {
 			/* If things are really okay, then let's show that.
 			 * Zero out the sense buffer so the higher layers
 			 * won't realize we did an unsolicited auto-sense.
@@ -814,7 +808,10 @@ Retry_Sense:
 			 */
 			} else {
 				srb->result = DID_ERROR << 16;
-				srb->sense_buffer[2] = HARDWARE_ERROR;
+				if ((sshdr.response_code & 0x72) == 0x72)
+					srb->sense_buffer[1] = HARDWARE_ERROR;
+				else
+					srb->sense_buffer[2] = HARDWARE_ERROR;
 			}
 		}
 	}
Signature · 1 line
1.7.0.1
maintainer
#1 lore raw
Quoted 7 lines · Luben Tuikov

This patch solves two things:
1) Enables autosense emulation code to correctly interpret descriptor format sense data, and
2) Fixes a bug whereby the autosense emulation code would overwrite descriptor format sense data with SENSE KEY HARDWARE ERROR in fixed format, to incorrectly look like this:

...

Quoted 4 lines
drivers/usb/storage/transport.c +0 -0
@@ -794,12 +793,7 @@ Retry_Sense:
 		 * everything worked or that there was an unspecified
 		 * problem.  We have to decide which.
 		 */

drivers/usb/storage/transport.c Retry_Sense: · line 799

Quoted 10 lines
drivers/usb/storage/transport.c +1 -6
-		if (	/* Filemark 0, ignore EOM, ILI 0, no sense */
-				(srb->sense_buffer[2] & 0xaf) == 0 &&
-			/* No ASC or ASCQ */
-				srb->sense_buffer[12] == 0 &&
-				srb->sense_buffer[13] == 0) {
-
+		if (sshdr.sense_key == 0 && sshdr.asc == 0 && sshdr.ascq == 0) {
 			/* If things are really okay, then let's show that.
 			 * Zero out the sense buffer so the higher layers
 			 * won't realize we did an unsolicited auto-sense.

What about the Filemark and ILI tests? You mustn't get rid of them; they are needed for tape drives.

Alan Stern

maintainer
#3 lore raw
Quoted 6 lines · Luben Tuikov
Quoted 3 lines · Alan Stern

What about the Filemark and ILI tests?  You mustn't get rid of them;
they are needed for tape drives.

I've resubmitted the patch.

Yeah, the new one is better. I would have preferred to see fm_ili explained in a comment, but this is okay.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw
Quoted 3 lines · Alan Stern

Yeah, the new one is better.  I would have preferred to see fm_ili
explained in a comment, but this is okay.

Thanks. I tried to preserve the logic and the comments as much as possible and tried not to introduce neither additional comment nor additional logic. Is this an implicit ACK then? Or does Greg needs an explicit one? Will he or I have to ask for an explicit ACK? From whom else?

maintainer
#5 lore raw
Quoted 6 lines · Luben Tuikov
Quoted 3 lines · Alan Stern

Yeah, the new one is better.  I would have preferred to see fm_ili
explained in a comment, but this is okay.

Thanks. I tried to preserve the logic and the comments as much as possible and tried not to introduce neither additional comment nor additional logic. Is this an implicit ACK then? Or does Greg needs an explicit one? Will he or I have to ask for an explicit ACK? From whom else?

You may consider it an ACK from me. However this needs to be approved by the usb-storage maintainer.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html