Use normalized sense when emulating autosense

PATCH In review Luben Tuikov 3 participants 10 messages Oct 22, 2010 – Nov 11, 2010

Subsystems (3)

SCSI USB USB mass storage driver

lore

Share & tools

lore

Luben Tuikov

author

2010-10-22 21:12 UTC #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

Signed-off-by Luben Tuikov <[email protected]>

---
 drivers/usb/storage/transport.c |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)

drivers/usb/storage/transport.c +19 ~~-15~~

index 64ec073..cb04664 100644
@@ -691,6 +691,9 @@ 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;
+		const u8 *scdd;
+		u8 fm_ili;
 
 		/* device supports and needs bigger sense buffer */
 		if (us->fflags & US_FL_SANE_SENSE)

@@ -774,32 +777,30 @@ 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 */
 		srb->result = SAM_STAT_CHECK_CONDITION;
 
+		scdd = scsi_sense_desc_find(srb->sense_buffer,
+					    SCSI_SENSE_BUFFERSIZE, 4);
+		fm_ili = (scdd ? scdd[3] : srb->sense_buffer[2]) & 0xA0;
+
 		/* We often get empty sense data.  This could indicate that
 		 * 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 &&
+		    fm_ili == 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 +815,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

Greg KH

2010-11-11 13:44 UTC #1 lore raw

Quoted 8 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 7 lines

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

Signed-off-by Luben Tuikov <[email protected]>

I need to get the ack from the usb-storage maintainer. Can you get that so that I can apply this?

thanks,

greg k-h

Luben Tuikov

author

2010-11-11 16:49 UTC #2 lore raw

Quoted 19 lines · Greg KH

Quoted 9 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:
Oct 21 14:11:07 localhost kernel: sd 7:0:0:0:
[sdc] Sense Key : Recovered Error [current]
[descriptor]
Quoted 1 line
Oct 21 14:11:07 localhost kernel: Descriptor sense
data with sense descriptors (in hex):
Quoted 1 line
Oct 21 14:11:07 localhost kernel:     
72 01 04 1d 00 00 00 0e 09 0c 00 00 00 00 00 00
Quoted 1 line
Oct 21 14:11:07 localhost kernel:     
00 4f 00 c2 00 50

Quoted 8 lines

Quoted 1 line
Oct 21 14:11:07 localhost kernel: sd 7:0:0:0:
[sdc] ASC=0x4 ASCQ=0x1d
Quoted 2 lines
Signed-off-by Luben Tuikov <[email protected]>
I need to get the ack from the usb-storage
maintainer. Can you get that
so that I can apply this?

Hi Greg,

I don't know who the usb-storage maintainer is, as I've mostly contributed to linux-scsi. Do you? I know Alan acked this patch. Is it him? Any reason you didn't CC the usb-storage maintainer in this email? Can you reply back to this thread CC-ing him? The only email I have in my address book is yours.

Thank you for being so nice and patient in this long, difficult, arduous and intimidating[1] process.

Luben
[1]linux-2.6/Documentation/development-process/6.Followthrough:22:
"Working with reviewers can be, for many developers, the most intimidating part of the kernel development process."

--
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

Greg KH

2010-11-11 16:56 UTC #3 lore raw

Quoted 25 lines · Luben Tuikov

Quoted 24 lines · Greg KH
Quoted 9 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:
Oct 21 14:11:07 localhost kernel: sd 7:0:0:0:
[sdc]? Sense Key : Recovered Error [current]
[descriptor]
Quoted 1 line
Oct 21 14:11:07 localhost kernel: Descriptor sense
data with sense descriptors (in hex):
Quoted 1 line
Oct 21 14:11:07 localhost kernel:? ? ?
? 72 01 04 1d 00 00 00 0e 09 0c 00 00 00 00 00 00
Quoted 1 line
Oct 21 14:11:07 localhost kernel:? ? ?
? 00 4f 00 c2 00 50
Quoted 1 line
Oct 21 14:11:07 localhost kernel: sd 7:0:0:0:
[sdc]? ASC=0x4 ASCQ=0x1d
Quoted 2 lines
Signed-off-by Luben Tuikov <[email protected]>

Quoted 8 lines

Quoted 3 lines
I need to get the ack from the usb-storage
maintainer.? Can you get that
so that I can apply this?
Hi Greg,
I don't know who the usb-storage maintainer is, as I've mostly contributed to linux-scsi. Do you?

$ ./scripts/get_maintainer.pl --roles --file drivers/usb/storage/usb.c
Matthew Dharm <[email protected]> (maintainer:USB MASS STORAGE...)
Greg Kroah-Hartman <[email protected]> (supporter:USB SUBSYSTEM)
[email protected] (open list:USB MASS STORAGE...)
[email protected] (open list:USB MASS STORAGE...)
[email protected] (open list)

Or you can just look in the MAINTAINERS file :)

Quoted 3 lines

I know Alan acked this patch. Is it him? Any reason you didn't CC the usb-storage maintainer in this email? Can you reply back to this thread CC-ing him? The only email I have in my address book is yours.

Please resend your patch to Matthew, and the people and lists listed above. I need his ack before I can take it.

thanks,

greg k-h

Luben Tuikov

author

2010-11-11 17:07 UTC #4 lore raw

Quoted 59 lines · Greg KH

Quoted 1 line · Luben Tuikov
--- On Thu, 11/11/10, Greg KH <[email protected]>
Quoted 4 lines · wrote:
Quoted 4 lines
Quoted 2 lines · Luben Tuikov
This patch solves two things:
1) Enables autosense emulation code to
correctly
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
interpret descriptor format sense data, and
2) Fixes a bug whereby the autosense
emulation
Quoted 1 line
Quoted 1 line
Quoted 1 line
code would overwrite descriptor format sense
data
Quoted 1 line
Quoted 1 line
Quoted 1 line
with SENSE KEY HARDWARE ERROR in fixed
format, to
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
incorrectly look like this:
Oct 21 14:11:07 localhost kernel: sd
7:0:0:0:
Quoted 3 lines
Quoted 3 lines
[sdc]? Sense Key : Recovered Error [current]
[descriptor]
Quoted 1 line
Oct 21 14:11:07 localhost kernel: Descriptor
sense
Quoted 3 lines
Quoted 3 lines
data with sense descriptors (in hex):
Quoted 1 line
Oct 21 14:11:07 localhost kernel:? ? ?
? 72 01 04 1d 00 00 00 0e 09 0c 00 00 00 00 00
00
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Oct 21 14:11:07 localhost kernel:? ? ?
? 00 4f 00 c2 00 50
Quoted 1 line
Oct 21 14:11:07 localhost kernel: sd
7:0:0:0:
Quoted 11 lines
Quoted 7 lines
[sdc]? ASC=0x4 ASCQ=0x1d
Quoted 2 lines
Signed-off-by Luben Tuikov <ltuikov-/[email protected]>
I need to get the ack from the usb-storage
maintainer.? Can you get that
so that I can apply this?
Hi Greg,
I don't know who the usb-storage maintainer is, as
I've mostly
Quoted 1 line
contributed to linux-scsi. Do you?
$ ./scripts/get_maintainer.pl --roles --file
drivers/usb/storage/usb.c
Matthew Dharm <[email protected]>
(maintainer:USB MASS STORAGE...)
Greg Kroah-Hartman <[email protected]>
(supporter:USB SUBSYSTEM)
[email protected]
(open list:USB MASS STORAGE...)
[email protected]
(open list:USB MASS STORAGE...)
[email protected]
(open list)

Quoted 1 line

Or you can just look in the MAINTAINERS file :)

Oh, and I was looking for the MAINTAINERS file in Documentation/.

Wow! There is actually a "get_maintainer" script!? Unbelievable!

Quoted 10 lines

Quoted 1 line
I know Alan acked this patch. Is it him? Any reason
you didn't CC the
Quoted 1 line
usb-storage maintainer in this email? Can you
reply back to this
Quoted 1 line
thread CC-ing him? The only email I have in my address
book is yours.
Please resend your patch to Matthew, and the people and lists listed
above. I need his ack before I can take it.

Oh, I see. I'll re-post the patch then to Matthew and the people and lists you listed above.

--
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

Matthew Dharm

2010-11-11 17:43 UTC #5 lore raw

Quoted 2 lines · Luben Tuikov

Oh, I see. I'll re-post the patch then to Matthew and the people and lists you listed above.

Now that I'm on the CC, I see this.

What's the 10k-foot level summary here? Is there an alternate sense-data format that uses some sort of "descriptor" structure? I'm assuming this is a new SCSI-III thing, or did I just miss it in SCSI-II?

This *looks* like you're using functions provided by the SCSI core to access and modify the sense data... so I'm assuming that's because there are alternate formats...

Matt

Signature · 6 lines

Matthew Dharm                              Home: [email protected] 
Maintainer, Linux USB Mass Storage Driver

It was a new hope.
					-- Dust Puppy
User Friendly, 12/25/1998

Luben Tuikov

author

2010-11-11 18:27 UTC #6 lore raw

Quoted 8 lines · Matthew Dharm

Quoted 2 lines · Luben Tuikov
Oh, I see. I'll re-post the patch then to Matthew and
the people and lists you listed above.
Now that I'm on the CC, I see this.
What's the 10k-foot level summary here?

I'm not sure either. It's apparently the procedure of getting a patch into the linux kernel nowadays. Last time I did this was 5 years ago and in linux-scsi--I remember it being easier, again that was 5 years ago.

Quoted 1 line

Is there an alternate sense-data format that uses some sort of "descriptor" structure?

Yes. Some devices return this format of sense data for some commands.

Quoted 7 lines

I'm assuming this is
a new SCSI-III thing, or did I just miss it in SCSI-II? This *looks* like you're using functions provided by the SCSI core to
access and modify the sense data... so I'm assuming that's because there
are alternate formats...

The patch doesn't add code that modifies the sense data. That modification code is already there, the patch fixes the modification which assumed only one type of format of sense data.

The patch uses SCSI functions to abstract the sense information irrespective of the type of format of sense data.

Quoted 15 lines

Matt

Signature · 10 lines

Matthew Dharm           
               
  Home: [email protected]

Maintainer, Linux USB Mass Storage Driver

It was a new hope.
           
        -- Dust Puppy
User Friendly, 12/25/1998

--
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

Matthew Dharm

Acked-by

2010-11-11 18:49 UTC #7 lore raw

Quoted 1 line · Luben Tuikov

The patch uses SCSI functions to abstract the sense information irrespective of the type of format of sense data.

Looks reasonable. The underlying SCSI functions seem to all have the tests to distinguish between sense format types. Let's just hope there are no devices out there which can confuse those tests, or we'll be buried in regressions.

Acked-by Matthew Dharm <[email protected]>

Matt

Signature · 7 lines

Matthew Dharm                              Home: [email protected] 
Maintainer, Linux USB Mass Storage Driver

My mother not mind to die for stoppink Windows NT!  She is rememberink 
Stalin!
					-- Pitr
User Friendly, 9/6/1998

Greg KH

2010-11-11 19:26 UTC #8 lore raw

Quoted 9 lines · Matthew Dharm

Quoted 1 line · Luben Tuikov
The patch uses SCSI functions to abstract the sense information irrespective of the type of format of sense data.
Looks reasonable. The underlying SCSI functions seem to all have the tests to distinguish between sense format types. Let's just hope there are no devices out there which can confuse those tests, or we'll be buried in regressions.
Acked-by Matthew Dharm <[email protected]>

Luben, can you resend me the original patch with Matthew's ack on it? I no longer have it in my mboxes.

thanks,

greg k-h

Luben Tuikov

author

2010-11-11 19:50 UTC #9 lore raw

Quoted 9 lines · Greg KH

Quoted 8 lines · Matthew Dharm
Quoted 1 line · Luben Tuikov
The patch uses SCSI functions to abstract the sense information irrespective of the type of format of sense data.
Looks reasonable. The underlying SCSI functions seem to all have the tests to distinguish between sense format types. Let's just hope there are no devices out there which can confuse those tests, or we'll be buried in regressions.

Quoted 5 lines

Quoted 1 line
Acked-by Matthew Dharm <[email protected]>
Luben, can you resend me the original patch with Matthew's ack on it? I no longer have it in my mboxes.

No prob.

Quoted 3 lines

thanks,
greg k-h

--
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