PATCH; make sr.c respect use_10_for_ms

PATCH; make sr.c respect use_10_for_ms

[Matthew Dharm]

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

This patch makes sr.c respect the use_10_for_ms flag.

The old behavior was to try the 6-byte command, and if it fails make some
assumptions.

The new behavior is to first try the 10-bit version (only if use_10_for_ms
is set), the fall back to the old behavior.

With this patch, we should be able to cut out a few hundred lines of
problematic code in usb-storage.

Linus, please apply.

Matt

# This is a BitKeeper generated patch for the following project:
# Project Name: greg k-h's linux 2.5 USB kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.709   -> 1.710  
#	   drivers/scsi/sr.c	1.47    -> 1.48   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/06/20	mdharm@zen.san.one-eyed-alien.net	1.710
# Make SCSI CD-ROM driver respect the use_10_for_ms flag.
# --------------------------------------------
#
diff -Nru a/drivers/scsi/sr.c b/drivers/scsi/sr.c
--- a/drivers/scsi/sr.c	Fri Jun 20 17:34:56 2003
+++ b/drivers/scsi/sr.c	Fri Jun 20 17:34:56 2003
@@ -663,7 +663,7 @@
 {
 	struct cdrom_generic_command cgc;
 	unsigned char *buffer;
-	int rc, n;
+	int n, rc = 0;
 
 	static char *loadmech[] =
 	{
@@ -682,16 +682,38 @@
 		printk(KERN_ERR "sr: out of memory.\n");
 		return;
 	}
-	memset(&cgc, 0, sizeof(struct cdrom_generic_command));
-	cgc.cmd[0] = MODE_SENSE;
-	cgc.cmd[2] = 0x2a;
-	cgc.cmd[4] = 128;
-	cgc.buffer = buffer;
-	cgc.buflen = 128;
-	cgc.quiet = 1;
-	cgc.data_direction = SCSI_DATA_READ;
-	cgc.timeout = SR_TIMEOUT;
-	rc = sr_do_ioctl(cd, &cgc);
+
+	/* first, attempt 10-byte command */
+	if (cd->device->use_10_for_ms) {
+		memset(&cgc, 0, sizeof(struct cdrom_generic_command));
+		cgc.cmd[0] = MODE_SENSE_10;
+		cgc.cmd[2] = 0x2a;		/* page code */
+		cgc.cmd[8] = 128;		/* allocation length */
+		cgc.buffer = buffer;
+		cgc.buflen = 128;
+		cgc.quiet = 1;
+		cgc.data_direction = SCSI_DATA_READ;
+		cgc.timeout = SR_TIMEOUT;
+		rc = sr_do_ioctl(cd, &cgc);
+	}
+
+	/* if we got an error, return to old behavior */
+	if (rc)
+		cd->device->use_10_for_ms = 0;
+
+	/* issue 6-byte command */
+	if (!cd->device->use_10_for_ms) {
+		memset(&cgc, 0, sizeof(struct cdrom_generic_command));
+		cgc.cmd[0] = MODE_SENSE;
+		cgc.cmd[2] = 0x2a;		/* page code */
+		cgc.cmd[4] = 128;		/* allocation length */
+		cgc.buffer = buffer;
+		cgc.buflen = 128;
+		cgc.quiet = 1;
+		cgc.data_direction = SCSI_DATA_READ;
+		cgc.timeout = SR_TIMEOUT;
+		rc = sr_do_ioctl(cd, &cgc);
+	}
 
 	if (rc) {
 		/* failed, drive doesn't have capabilities mode page */
@@ -703,7 +725,13 @@
 		printk("%s: scsi-1 drive\n", cd->cdi.name);
 		return;
 	}
-	n = buffer[3] + 4;
+
+	/* calculate the start of the page */
+	if (cd->device->use_10_for_ms)
+		n = (buffer[6] << 8) + buffer[7] + 8;
+	else
+		n = buffer[3] + 4;
+
 	cd->cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
 	cd->readcd_known = 1;
 	cd->readcd_cdda = buffer[n + 5] & 0x01;

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

P:  How about "Web Designer"?
DP: I'd like a name that people won't laugh at.
					-- Pitr and Dust Puppy
User Friendly, 12/6/1997

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH; make sr.c respect use_10_for_ms

[Linus Torvalds]

On Sat, 21 Jun 2003, Matthew Dharm wrote:
>
> This patch makes sr.c respect the use_10_for_ms flag.

Ouch.

Is there some SCSI person out there that would be willing to make sr.c 
look a bit more like sd.c in this respect? This is a bit hacky, I think, 
but I guess the sr.c layout forces that..

		Linus

Re: PATCH; make sr.c respect use_10_for_ms

[Matthew Dharm]

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

On Sat, Jun 21, 2003 at 05:07:48PM -0700, Linus Torvalds wrote:
> On Sat, 21 Jun 2003, Matthew Dharm wrote:
> >
> > This patch makes sr.c respect the use_10_for_ms flag.
> 
> Ouch.
> 
> Is there some SCSI person out there that would be willing to make sr.c 
> look a bit more like sd.c in this respect? This is a bit hacky, I think, 
> but I guess the sr.c layout forces that..

I actually think sd.c looks hacky in the way it handles this...

The other difference is that sd.c wants to issue MODE_SENSE in a couple of
places, where sr.c only wants to do it once.  So, creating an entirely
separate function to figure out what type of MODE_SENSE to send seems kinda
overkill to me.

I mean, this works, it's not difficult to follow, and meets all the
operational goals.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

G:  Money isn't everything, A.J.
AJ: Who convinced you of that?
G:  The Chief, at my last salary review.
					-- Mike and Greg
User Friendly, 11/3/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH; make sr.c respect use_10_for_ms

[Linus Torvalds]

On Sat, 21 Jun 2003, Matthew Dharm wrote:
> 
> I mean, this works, it's not difficult to follow, and meets all the
> operational goals.

As far as I can tell, your patch is not actually all that stable.

Imagine a flaky SCSI connection (think iscsi or whatever), where your
commands are getting lost or corrupted. The first mode-sense command goes 
out, it takes a while to come back, another one goes out for some other 
reason (like scsi-generic), that other one failes due to something else, 
and look what happens: two bugs in one schenario:

 - your code will clear "use_10_for_ms" even though the failure code 
   wasn't due to an unrecognized command, but due to something else 
   (timeout, out-of-memory, whatever)

 - when the other sense command comes back, it was sent with 
   "use_10_for_ms" active, but by the time the result comes back it has
   been cleared, and now we'll use that flag to test what the sense data 
   means. So now we'll get the size wrong.

That sounds flaky to me.

			Linus

Re: PATCH; make sr.c respect use_10_for_ms

[James Bottomley]

In general, you would have been a bit better off just doing a

	if(cd->device->use_10_for_ms) {
		cgc.cmd[0] = MODE_SENSE_10;
		cgc.cmd[8] = 128;
	} else {
		cdc.cmd[0] = MODE_SENSE;
		cgc.cmd[4] = 128;
	}

Rather than duplicating so much code.

This: 

> +	if (cd->device->use_10_for_ms) {
> +		memset(&cgc, 0, sizeof(struct cdrom_generic_command));
> +		cgc.cmd[0] = MODE_SENSE_10;
> +		cgc.cmd[2] = 0x2a;		/* page code */
> +		cgc.cmd[8] = 128;		/* allocation length */
> +		cgc.buffer = buffer;
> +		cgc.buflen = 128;
> +		cgc.quiet = 1;
> +		cgc.data_direction = SCSI_DATA_READ;
> +		cgc.timeout = SR_TIMEOUT;
> +		rc = sr_do_ioctl(cd, &cgc);
> +	}
> +
> +	/* if we got an error, return to old behavior */
> +	if (rc)
> +		cd->device->use_10_for_ms = 0;

Is slightly incorrect.  Mode sense commands can return errors for many
reasons, you need to check for ILLEGAL_REQUEST sense before assuming it
was a problem with the command.

What probably should happen is that the resetting of the flag should be
in scsi_io_completion (like it is for the use_10_for_rw flag), then the
retry and the flag reset would come for free.

James

Re: PATCH; make sr.c respect use_10_for_ms

[Matthew Dharm]

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

On Sat, Jun 21, 2003 at 05:21:53PM -0700, Linus Torvalds wrote:
> On Sat, 21 Jun 2003, Matthew Dharm wrote:
> > I mean, this works, it's not difficult to follow, and meets all the
> > operational goals.
> 
> As far as I can tell, your patch is not actually all that stable.
> 
> Imagine a flaky SCSI connection (think iscsi or whatever), where your
> commands are getting lost or corrupted. The first mode-sense command goes 
> out, it takes a while to come back, another one goes out for some other 
> reason (like scsi-generic), that other one failes due to something else, 
> and look what happens: two bugs in one schenario:
> 
>  - your code will clear "use_10_for_ms" even though the failure code 
>    wasn't due to an unrecognized command, but due to something else 
>    (timeout, out-of-memory, whatever)

True.  With this scenario and the old code, the code just makes an
assumption and goes on -- I did not aim to improve robustness, only
maintain it.  You can make the same argument about the existing code
falling back to the dumb-assumption condition under a transient error
condition.

So if this is a problem, then this entire section of code needed a rewrite
before I ever looked at it.  Until that rewrite occurs, I think my patch
adds value.

>  - when the other sense command comes back, it was sent with 
>    "use_10_for_ms" active, but by the time the result comes back it has
>    been cleared, and now we'll use that flag to test what the sense data 
>    means. So now we'll get the size wrong.

I don't see how a sense command sent from the scsi-generic interface will
affect (or be affected by) use_10_for_ms -- if scsi-generic issues the
command, this code path won't try to interpret the data at all.  Perhaps
I'm confused?

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

C:  Like the Furby?
DP: He gives me the creeps.  Think the SPCA will take him?
					-- Cobb and Dust Puppy
User Friendly, 1/2/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH; make sr.c respect use_10_for_ms

[Linus Torvalds]

On Sat, 21 Jun 2003, Matthew Dharm wrote:
> 
> I don't see how a sense command sent from the scsi-generic interface will
> affect (or be affected by) use_10_for_ms -- if scsi-generic issues the
> command, this code path won't try to interpret the data at all.  Perhaps
> I'm confused?

Yeah, it only works if the first command was sent through sr.c, and the 
second command also clears the use_10_for_ms bit (ie it has to be sent 
through the sr.c or sd.c codepaths). 

The basic idea stands, though: your code depends on global device state 
not to change between the creation of the command and the parsing of it. 
Which just isn't a good thing to do.

sd.c gets all of these things right (including re-transmission etc), 
simply because of how it does things. Which is why I prefer the sd.c 
approach over the simplistic patch. sd.c just looks a lot more robust.

(Maybe that one has bugs too, but they aren't immediately obvious).

		Linus

Re: PATCH; make sr.c respect use_10_for_ms

[Matthew Dharm]

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

On Sat, Jun 21, 2003 at 05:38:44PM -0700, Linus Torvalds wrote:
> The basic idea stands, though: your code depends on global device state 
> not to change between the creation of the command and the parsing of it. 
> Which just isn't a good thing to do.
> 
> sd.c gets all of these things right (including re-transmission etc), 
> simply because of how it does things. Which is why I prefer the sd.c 
> approach over the simplistic patch. sd.c just looks a lot more robust.
> 
> (Maybe that one has bugs too, but they aren't immediately obvious).

Okay, I can buy this.  I don't suppose you have someone you can sign-up to
fix this mess?  I really want to get rid of all the mode-sense translation
code in usb-storage.

I'm just not qualified to put in what it's going to take to fix this.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Now payink attention, please.  This is mouse.  Click-click. Easy to 
use, da? Now you try...
					-- Pitr to Miranda
User Friendly, 10/11/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH; make sr.c respect use_10_for_ms

[Matthew Dharm]

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

On Sat, Jun 21, 2003 at 07:25:49PM -0500, James Bottomley wrote:
> What probably should happen is that the resetting of the flag should be
> in scsi_io_completion (like it is for the use_10_for_rw flag), then the
> retry and the flag reset would come for free.

Since you seem to have a handle on this, can you fix it?

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

S:  Another stupid question?
G:  There's no such thing as a stupid question, only stupid people.
					-- Stef and Greg
User Friendly, 7/15/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH; make sr.c respect use_10_for_ms

[James Bottomley]

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

On Sat, 2003-06-21 at 19:46, Matthew Dharm wrote:
> Since you seem to have a handle on this, can you fix it?

OK, try the attached.  I abstracted out the mode_sense routines into the
SCSI function library.  It now employs the correct retry policy and
sweeps up several bugs in the sd.c and sr.c use of mode sense along the
way.  I killed all the spurious uses of DBD, which makes calculating the
offsets much easier.

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 8843 bytes --]

===== drivers/scsi/sd.c 1.123 vs edited =====
--- 1.123/drivers/scsi/sd.c	Tue Jun 17 04:17:15 2003
+++ edited/drivers/scsi/sd.c	Sat Jun 21 21:24:27 2003
@@ -1062,40 +1062,12 @@
 }
 
 /* called with buffer of length 512 */
-static int
+static inline int
 sd_do_mode_sense(struct scsi_request *SRpnt, int dbd, int modepage,
-		 unsigned char *buffer, int len) {
-	unsigned char cmd[12];
-
-	memset((void *) &cmd[0], 0, 12);
-	cmd[1] = dbd;
-	cmd[2] = modepage;
-
-	if (SRpnt->sr_device->use_10_for_ms) {
-		if (len < 8)
-			len = 8;
-
-		cmd[0] = MODE_SENSE_10;
-		cmd[8] = len;
-	} else {
-		if (len < 4)
-			len = 4;
-
-		cmd[0] = MODE_SENSE;
-		cmd[4] = len;
-	}
-
-	SRpnt->sr_cmd_len = 0;
-	SRpnt->sr_sense_buffer[0] = 0;
-	SRpnt->sr_sense_buffer[2] = 0;
-	SRpnt->sr_data_direction = SCSI_DATA_READ;
-
-	memset((void *) buffer, 0, len);
-
-	scsi_wait_req(SRpnt, (void *) cmd, (void *) buffer,
-		      len, SD_TIMEOUT, SD_MAX_RETRIES);
-
-	return SRpnt->sr_result;
+		 unsigned char *buffer, int len)
+{
+	return scsi_do_mode_sense(SRpnt, dbd, modepage, buffer, len,
+				  SD_TIMEOUT, SD_MAX_RETRIES);
 }
 
 /*
@@ -1119,20 +1091,20 @@
 	 * When only page 0 is implemented, a request for page 3F may return
 	 * Sense Key 5: Illegal Request, Sense Code 24: Invalid field in CDB.
 	 */
-	if (res)
+	if (res == 0)
 		res = sd_do_mode_sense(SRpnt, 0, 0, buffer, 4);
 
 	/*
 	 * Third attempt: ask 255 bytes, as we did earlier.
 	 */
-	if (res)
+	if (res == 0)
 		res = sd_do_mode_sense(SRpnt, 0, 0x3F, buffer, 255);
 
-	if (res) {
+	if (res == 0) {
 		printk(KERN_WARNING
 		       "%s: test WP failed, assume Write Enabled\n", diskname);
 	} else {
-		sdkp->write_prot = ((buffer[2] & 0x80) != 0);
+		sdkp->write_prot = ((buffer[res == 4 ? 2 : 3] & 0x80) != 0);
 		printk(KERN_NOTICE "%s: Write Protect is %s\n", diskname,
 		       sdkp->write_prot ? "on" : "off");
 		printk(KERN_DEBUG "%s: Mode Sense: %02x %02x %02x %02x\n",
@@ -1149,43 +1121,43 @@
 		   struct scsi_request *SRpnt, unsigned char *buffer) {
 	int len = 0, res;
 
-	const int dbd = 0x08;	   /* DBD */
+	const int dbd = 0;	   /* DBD */
 	const int modepage = 0x08; /* current values, cache page */
 
 	/* cautiously ask */
 	res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer, 4);
 
-	if (res == 0) {
+	if (res != 0) {
 		/* that went OK, now ask for the proper length */
-		len = buffer[0] + 1;
+		if(res == 4) 
+			len = buffer[0] + 1;
+		else
+			len = buffer[0]*256 + buffer[1] + 1;
 		if (len > 128)
 			len = 128;
 		res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer, len);
 	}
 
-	if (res == 0 && buffer[3] + 6 < len) {
+	if (res != 0) {
 		const char *types[] = {
 			"write through", "none", "write back",
 			"write back, no read (daft)"
 		};
 		int ct = 0;
-		int offset = buffer[3] + 4; /* start of mode page */
 
-		sdkp->WCE = ((buffer[offset + 2] & 0x04) != 0);
-		sdkp->RCD = ((buffer[offset + 2] & 0x01) != 0);
+		sdkp->WCE = ((buffer[res + 2] & 0x04) != 0);
+		sdkp->RCD = ((buffer[res + 2] & 0x01) != 0);
 
 		ct =  sdkp->RCD + 2*sdkp->WCE;
 
 		printk(KERN_NOTICE "SCSI device %s: drive cache: %s\n",
 		       diskname, types[ct]);
 	} else {
-		if (res == 0 ||
-		    (status_byte(res) == CHECK_CONDITION
-		     && (SRpnt->sr_sense_buffer[0] & 0x70) == 0x70
+		if ((SRpnt->sr_sense_buffer[0] & 0x70) == 0x70
 		     && (SRpnt->sr_sense_buffer[2] & 0x0f) == ILLEGAL_REQUEST
 		     /* ASC 0x24 ASCQ 0x00: Invalid field in CDB */
 		     && SRpnt->sr_sense_buffer[12] == 0x24
-		     && SRpnt->sr_sense_buffer[13] == 0x00)) {
+		     && SRpnt->sr_sense_buffer[13] == 0x00) {
 			printk(KERN_NOTICE "%s: cache data unavailable\n",
 			       diskname);
 		} else {
===== drivers/scsi/sr.c 1.82 vs edited =====
--- 1.82/drivers/scsi/sr.c	Mon Jun 16 19:22:18 2003
+++ edited/drivers/scsi/sr.c	Sat Jun 21 21:25:54 2003
@@ -660,7 +660,6 @@
 
 static void get_capabilities(struct scsi_cd *cd)
 {
-	struct cdrom_generic_command cgc;
 	unsigned char *buffer;
 	int rc, n;
 
@@ -681,18 +680,9 @@
 		printk(KERN_ERR "sr: out of memory.\n");
 		return;
 	}
-	memset(&cgc, 0, sizeof(struct cdrom_generic_command));
-	cgc.cmd[0] = MODE_SENSE;
-	cgc.cmd[2] = 0x2a;
-	cgc.cmd[4] = 128;
-	cgc.buffer = buffer;
-	cgc.buflen = 128;
-	cgc.quiet = 1;
-	cgc.data_direction = SCSI_DATA_READ;
-	cgc.timeout = SR_TIMEOUT;
-	rc = sr_do_ioctl(cd, &cgc);
+	rc = scsi_mode_sense(cd->device, 0, 0x2a, buffer, 128, SR_TIMEOUT, 3);
 
-	if (rc) {
+	if (rc == 0) {
 		/* failed, drive doesn't have capabilities mode page */
 		cd->cdi.speed = 1;
 		cd->cdi.mask |= (CDC_CD_R | CDC_CD_RW | CDC_DVD_R |
@@ -702,7 +692,7 @@
 		printk("%s: scsi-1 drive\n", cd->cdi.name);
 		return;
 	}
-	n = buffer[3] + 4;
+	n = rc;
 	cd->cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
 	cd->readcd_known = 1;
 	cd->readcd_cdda = buffer[n + 5] & 0x01;
===== drivers/scsi/scsi_lib.c 1.95 vs edited =====
--- 1.95/drivers/scsi/scsi_lib.c	Wed Jun  4 06:43:01 2003
+++ edited/drivers/scsi/scsi_lib.c	Sat Jun 21 20:40:22 2003
@@ -1336,3 +1336,105 @@
 		kmem_cache_destroy(sgp->slab);
 	}
 }
+/**	scsi_do_mode_sense - issue a mode sense, falling back from 10 to 
+ *		six bytes if necessary.
+ *	@SRpnt:	SCSI request to fill in with the MODE_SENSE
+ *	@dbd:	set if mode sense will allow block descriptors to be returned
+ *	@modepage: mode page being requested
+ *	@buffer: request buffer (may not be smaller than eight bytes)
+ *	@len:	length of request buffer.
+ *	@timeout: command timeout
+ *	@retries: number of retries before failing
+ *
+ *	Returns zero if unsuccessful, or the header offset (either 4
+ *	or 8 depending on whether a six or ten byte command was
+ *	issued) if successful.
+ **/
+int
+scsi_do_mode_sense(struct scsi_request *SRpnt, int dbd, int modepage,
+		   unsigned char *buffer, int len, int timeout, int retries) {
+	unsigned char cmd[12];
+	int use_10_for_ms;
+	int header_offset;
+
+	memset((void *) &cmd[0], 0, 12);
+	cmd[1] = dbd;
+	cmd[2] = modepage;
+
+ retry:
+	use_10_for_ms = SRpnt->sr_device->use_10_for_ms;
+
+	if (use_10_for_ms) {
+		if (len < 8)
+			len = 8;
+
+		cmd[0] = MODE_SENSE_10;
+		cmd[8] = len;
+		header_offset = 8;
+	} else {
+		if (len < 4)
+			len = 4;
+
+		cmd[0] = MODE_SENSE;
+		cmd[4] = len;
+		header_offset = 4;
+	}
+
+	SRpnt->sr_cmd_len = 0;
+	SRpnt->sr_sense_buffer[0] = 0;
+	SRpnt->sr_sense_buffer[2] = 0;
+	SRpnt->sr_data_direction = SCSI_DATA_READ;
+
+	memset((void *) buffer, 0, len);
+
+	scsi_wait_req(SRpnt, (void *) cmd, (void *) buffer,
+		      len, timeout, retries);
+
+	/* This code looks awful: what it's doing is making sure an
+	 * 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(SRpnt->sr_result) &&
+	    (driver_byte(SRpnt->sr_result) & DRIVER_SENSE) &&
+	    SRpnt->sr_sense_buffer[2] == ILLEGAL_REQUEST &&
+	    (SRpnt->sr_sense_buffer[4] & 0x40) == 0x40 &&
+	    SRpnt->sr_sense_buffer[5] == 0 &&
+	    SRpnt->sr_sense_buffer[6] == 0 ) {
+		SRpnt->sr_device->use_10_for_ms = 0;
+		goto retry;
+	}
+
+	return scsi_status_is_good(SRpnt->sr_result) ? header_offset : 0;
+}
+
+/**	scsi_mode_sense - issue a mode sense, falling back from 10 to 
+ *		six bytes if necessary.
+ *	@sdev:	scsi device to send command to.
+ *	@dbd:	set if mode sense will allow block descriptors to be returned
+ *	@modepage: mode page being requested
+ *	@buffer: request buffer (may not be smaller than eight bytes)
+ *	@len:	length of request buffer.
+ *	@timeout: command timeout
+ *	@retries: number of retries before failing
+ *
+ *	Returns zero if unsuccessful, or the header offset (either 4
+ *	or 8 depending on whether a six or ten byte command was
+ *	issued) if successful.
+ **/
+int
+scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
+		unsigned char *buffer, int len, int timeout, int retries)
+{
+	struct scsi_request *sreq = scsi_allocate_request(sdev);
+	int ret;
+
+	if (!sreq)
+		return 0;
+
+	ret = scsi_do_mode_sense(sreq, dbd, modepage, buffer, len,
+				 timeout, retries);
+
+	scsi_release_request(sreq);
+
+	return ret;
+}
===== drivers/scsi/scsi_syms.c 1.40 vs edited =====
--- 1.40/drivers/scsi/scsi_syms.c	Thu Jun  5 06:26:04 2003
+++ edited/drivers/scsi/scsi_syms.c	Sat Jun 21 20:52:12 2003
@@ -86,6 +86,9 @@
 EXPORT_SYMBOL(scsi_remove_device);
 EXPORT_SYMBOL(scsi_set_device_offline);
 
+EXPORT_SYMBOL(scsi_do_mode_sense);
+EXPORT_SYMBOL(scsi_mode_sense);
+
 /*
  * This symbol is for the highlevel drivers (e.g. sg) only.
  */

Re: PATCH; make sr.c respect use_10_for_ms

[Matthew Dharm]

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

On Sat, Jun 21, 2003 at 09:54:46PM -0500, James Bottomley wrote:
> @@ -702,7 +692,7 @@
>  		printk("%s: scsi-1 drive\n", cd->cdi.name);
>  		return;
>  	}
> -	n = buffer[3] + 4;
> +	n = rc;
>  	cd->cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
>  	cd->readcd_known = 1;
>  	cd->readcd_cdda = buffer[n + 5] & 0x01;

This bit isn't right.  n is supposed to point to the start of the page
data, not the page header.  The header is a different size if the command
is 6-byte or 10-byte.

If you look at my patch, you can see the correct offsets you need.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

We can customize our colonels.
					-- Tux
User Friendly, 12/1/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH; make sr.c respect use_10_for_ms

[Douglas Gilbert]

Matthew Dharm wrote:
> On Sat, Jun 21, 2003 at 09:54:46PM -0500, James Bottomley wrote:
> 
>>@@ -702,7 +692,7 @@
>> 		printk("%s: scsi-1 drive\n", cd->cdi.name);
>> 		return;
>> 	}
>>-	n = buffer[3] + 4;
>>+	n = rc;
>> 	cd->cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
>> 	cd->readcd_known = 1;
>> 	cd->readcd_cdda = buffer[n + 5] & 0x01;
> 
> 
> This bit isn't right.  n is supposed to point to the start of the page
> data, not the page header.  The header is a different size if the command
> is 6-byte or 10-byte.
> 
> If you look at my patch, you can see the correct offsets you need.

Matt,
Here is a small function to calculate the mode sense reponse
offset to the first page (borrowed from smartmontools, GPL-ed
code):


/* Offset into mode sense (6 or 10 byte) response that actual mode page
  * starts at (relative to resp[0]). Returns -1 if problem */
int scsiModePageOffset(const UINT8 * resp, int len, int modese_10)
{
     int resp_len, bd_len;
     int offset = -1;
 

     if (resp) {
         if (modese_10) {
             resp_len = (resp[0] << 8) + resp[1] + 2;
             bd_len = (resp[6] << 8) + resp[7];
             offset = bd_len + 8;
         } else {
             resp_len = resp[0] + 1;
             bd_len = resp[3];
             offset = bd_len + 4;
         }
         if ((offset + 2) > len) {
             pout("scsiModePageOffset: page too small, offset=%d "
                  "resp_len=%d bd_len=%d\n", offset, resp_len, bd_len);
             offset = -1;
         } else if ((offset + 2) > resp_len) {
             pout("scsiModePageOffset: response length too short, 
resp_len=%d"
                  " offset=%d bd_len=%d\n", resp_len, offset, bd_len);
             offset = -1;
         }
     }
     return offset;
}


BTW The sanity checks can be useful, Recently, I have been
trying to solve problems with the aacraid which returns
garbage to MODE SENSE commands (and locks up when given
LOG SENSE commands).
My USB mass storage enclosure is quirky as well: when
it gets a MODE_SENSE_10 it swaps the response_length bytes
(bytes 0 and 1) producing an exaggerated length. The mode
page is still readable and valid.

Doug Gilbert

Re: PATCH; make sr.c respect use_10_for_ms

[James Bottomley]

On Sat, 2003-06-21 at 23:24, Matthew Dharm wrote:
> On Sat, Jun 21, 2003 at 09:54:46PM -0500, James Bottomley wrote:
> >  	}
> > -	n = buffer[3] + 4;
> > +	n = rc;
> >  	cd->cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
> 
> This bit isn't right.  n is supposed to point to the start of the page
> data, not the page header.  The header is a different size if the command
> is 6-byte or 10-byte.

Yes it is.

That's why I eliminated the dbd bit.  Your patch was calculating the
offsets past the dbd headers.  However, if you tell the mode sense not
to send any dbd headers (which are pointless, because the routine isn't
interested in them), then you don't have to skip past them. and the mode
data begins just past the mode header.

James

Re: PATCH; make sr.c respect use_10_for_ms

[Matthew Dharm]

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

On Sun, Jun 22, 2003 at 08:58:00AM -0500, James Bottomley wrote:
> On Sat, 2003-06-21 at 23:24, Matthew Dharm wrote:
> > On Sat, Jun 21, 2003 at 09:54:46PM -0500, James Bottomley wrote:
> > >  	}
> > > -	n = buffer[3] + 4;
> > > +	n = rc;
> > >  	cd->cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
> > 
> > This bit isn't right.  n is supposed to point to the start of the page
> > data, not the page header.  The header is a different size if the command
> > is 6-byte or 10-byte.
> 
> Yes it is.
> 
> That's why I eliminated the dbd bit.  Your patch was calculating the
> offsets past the dbd headers.  However, if you tell the mode sense not
> to send any dbd headers (which are pointless, because the routine isn't
> interested in them), then you don't have to skip past them. and the mode
> data begins just past the mode header.

But (as I read it -- remember I'm not an expert), the old sr.c code didn't
set the DBD bit, just like the new code.  So whatever formula applied to
the old code should apply to the new code, yes?

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Stef, you just got beaten by a ball of DIRT.
					-- Greg
User Friendly, 12/7/1997

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH; make sr.c respect use_10_for_ms

[Matthew Dharm]

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

On Sun, Jun 22, 2003 at 12:49:17PM -0700, Matthew Dharm wrote:
> On Sun, Jun 22, 2003 at 08:58:00AM -0500, James Bottomley wrote:
> > On Sat, 2003-06-21 at 23:24, Matthew Dharm wrote:
> > > On Sat, Jun 21, 2003 at 09:54:46PM -0500, James Bottomley wrote:
> > > >  	}
> > > > -	n = buffer[3] + 4;
> > > > +	n = rc;
> > > >  	cd->cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
> > > 
> > > This bit isn't right.  n is supposed to point to the start of the page
> > > data, not the page header.  The header is a different size if the command
> > > is 6-byte or 10-byte.
> > 
> > Yes it is.
> > 
> > That's why I eliminated the dbd bit.  Your patch was calculating the
> > offsets past the dbd headers.  However, if you tell the mode sense not
> > to send any dbd headers (which are pointless, because the routine isn't
> > interested in them), then you don't have to skip past them. and the mode
> > data begins just past the mode header.
> 
> But (as I read it -- remember I'm not an expert), the old sr.c code didn't
> set the DBD bit, just like the new code.  So whatever formula applied to
> the old code should apply to the new code, yes?

Okay, according to my copy of the SCSI-2 spec:

 A disable block descriptors (DBD) bit of zero indicates that the target
 may return zero or more block descriptors in the returned MODE SENSE data
 (see 8.3.3), at the target's discretion. A DBD bit of one specifies that
 the target shall not return any block descriptors in the returned MODE
 SENSE data.

The code James sent sets DBD to 0 -- I like that, as many usb-storage
devices choke when DBD is set to 1.  I believe in avoiding the DBD bit as
much as possible, and James seems to have eliminated it.

However, DBD==0 means that a block descriptor is likely to be returned --
so we need to add in the size of the block descriptor header.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Hi.  I have my back hairs caught in my computer fan.
					-- Customer
User Friendly, 8/20/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH; make sr.c respect use_10_for_ms

[James Bottomley]

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

On Sun, 2003-06-22 at 14:56, Matthew Dharm wrote:
> > But (as I read it -- remember I'm not an expert), the old sr.c code didn't
> > set the DBD bit, just like the new code.  So whatever formula applied to
> > the old code should apply to the new code, yes?

Hmm, the diff I sent was an older one which had the dbd meaning
inverted.  The attached is the one I'm actually testing.

> The code James sent sets DBD to 0 -- I like that, as many usb-storage
> devices choke when DBD is set to 1.  I believe in avoiding the DBD bit as
> much as possible, and James seems to have eliminated it.
> 
> However, DBD==0 means that a block descriptor is likely to be returned --
> so we need to add in the size of the block descriptor header.

OK, now I'm confused.  the write caching determination code that you and
Andries spent a while getting to work has DBD==1, and I thought we'd
eliminated all the USB problems with that code...

Since the BD skip code is quite a wodge of complexity, I'd like to know
what devices fail on DBD==1 before trying to add it all in, especially
as we (fortunatly) have nothing in the kernel that actually wants to see
the BDs.

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 8644 bytes --]

===== drivers/scsi/sd.c 1.123 vs edited =====
--- 1.123/drivers/scsi/sd.c	Tue Jun 17 04:17:15 2003
+++ edited/drivers/scsi/sd.c	Sun Jun 22 09:21:16 2003
@@ -1062,40 +1062,12 @@
 }
 
 /* called with buffer of length 512 */
-static int
+static inline int
 sd_do_mode_sense(struct scsi_request *SRpnt, int dbd, int modepage,
-		 unsigned char *buffer, int len) {
-	unsigned char cmd[12];
-
-	memset((void *) &cmd[0], 0, 12);
-	cmd[1] = dbd;
-	cmd[2] = modepage;
-
-	if (SRpnt->sr_device->use_10_for_ms) {
-		if (len < 8)
-			len = 8;
-
-		cmd[0] = MODE_SENSE_10;
-		cmd[8] = len;
-	} else {
-		if (len < 4)
-			len = 4;
-
-		cmd[0] = MODE_SENSE;
-		cmd[4] = len;
-	}
-
-	SRpnt->sr_cmd_len = 0;
-	SRpnt->sr_sense_buffer[0] = 0;
-	SRpnt->sr_sense_buffer[2] = 0;
-	SRpnt->sr_data_direction = SCSI_DATA_READ;
-
-	memset((void *) buffer, 0, len);
-
-	scsi_wait_req(SRpnt, (void *) cmd, (void *) buffer,
-		      len, SD_TIMEOUT, SD_MAX_RETRIES);
-
-	return SRpnt->sr_result;
+		 unsigned char *buffer, int len)
+{
+	return scsi_do_mode_sense(SRpnt, dbd, modepage, buffer, len,
+				  SD_TIMEOUT, SD_MAX_RETRIES);
 }
 
 /*
@@ -1119,20 +1091,20 @@
 	 * When only page 0 is implemented, a request for page 3F may return
 	 * Sense Key 5: Illegal Request, Sense Code 24: Invalid field in CDB.
 	 */
-	if (res)
+	if (res == 0)
 		res = sd_do_mode_sense(SRpnt, 0, 0, buffer, 4);
 
 	/*
 	 * Third attempt: ask 255 bytes, as we did earlier.
 	 */
-	if (res)
+	if (res == 0)
 		res = sd_do_mode_sense(SRpnt, 0, 0x3F, buffer, 255);
 
-	if (res) {
+	if (res == 0) {
 		printk(KERN_WARNING
 		       "%s: test WP failed, assume Write Enabled\n", diskname);
 	} else {
-		sdkp->write_prot = ((buffer[2] & 0x80) != 0);
+		sdkp->write_prot = ((buffer[res == 4 ? 2 : 3] & 0x80) != 0);
 		printk(KERN_NOTICE "%s: Write Protect is %s\n", diskname,
 		       sdkp->write_prot ? "on" : "off");
 		printk(KERN_DEBUG "%s: Mode Sense: %02x %02x %02x %02x\n",
@@ -1155,37 +1127,37 @@
 	/* cautiously ask */
 	res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer, 4);
 
-	if (res == 0) {
+	if (res != 0) {
 		/* that went OK, now ask for the proper length */
-		len = buffer[0] + 1;
+		if(res == 4) 
+			len = buffer[0] + 1;
+		else
+			len = buffer[0]*256 + buffer[1] + 2;
 		if (len > 128)
 			len = 128;
 		res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer, len);
 	}
 
-	if (res == 0 && buffer[3] + 6 < len) {
+	if (res != 0) {
 		const char *types[] = {
 			"write through", "none", "write back",
 			"write back, no read (daft)"
 		};
 		int ct = 0;
-		int offset = buffer[3] + 4; /* start of mode page */
 
-		sdkp->WCE = ((buffer[offset + 2] & 0x04) != 0);
-		sdkp->RCD = ((buffer[offset + 2] & 0x01) != 0);
+		sdkp->WCE = ((buffer[res + 2] & 0x04) != 0);
+		sdkp->RCD = ((buffer[res + 2] & 0x01) != 0);
 
 		ct =  sdkp->RCD + 2*sdkp->WCE;
 
 		printk(KERN_NOTICE "SCSI device %s: drive cache: %s\n",
 		       diskname, types[ct]);
 	} else {
-		if (res == 0 ||
-		    (status_byte(res) == CHECK_CONDITION
-		     && (SRpnt->sr_sense_buffer[0] & 0x70) == 0x70
+		if ((SRpnt->sr_sense_buffer[0] & 0x70) == 0x70
 		     && (SRpnt->sr_sense_buffer[2] & 0x0f) == ILLEGAL_REQUEST
 		     /* ASC 0x24 ASCQ 0x00: Invalid field in CDB */
 		     && SRpnt->sr_sense_buffer[12] == 0x24
-		     && SRpnt->sr_sense_buffer[13] == 0x00)) {
+		     && SRpnt->sr_sense_buffer[13] == 0x00) {
 			printk(KERN_NOTICE "%s: cache data unavailable\n",
 			       diskname);
 		} else {
===== drivers/scsi/sr.c 1.82 vs edited =====
--- 1.82/drivers/scsi/sr.c	Mon Jun 16 19:22:18 2003
+++ edited/drivers/scsi/sr.c	Sun Jun 22 09:06:02 2003
@@ -660,7 +660,6 @@
 
 static void get_capabilities(struct scsi_cd *cd)
 {
-	struct cdrom_generic_command cgc;
 	unsigned char *buffer;
 	int rc, n;
 
@@ -681,18 +680,10 @@
 		printk(KERN_ERR "sr: out of memory.\n");
 		return;
 	}
-	memset(&cgc, 0, sizeof(struct cdrom_generic_command));
-	cgc.cmd[0] = MODE_SENSE;
-	cgc.cmd[2] = 0x2a;
-	cgc.cmd[4] = 128;
-	cgc.buffer = buffer;
-	cgc.buflen = 128;
-	cgc.quiet = 1;
-	cgc.data_direction = SCSI_DATA_READ;
-	cgc.timeout = SR_TIMEOUT;
-	rc = sr_do_ioctl(cd, &cgc);
+	rc = scsi_mode_sense(cd->device, 0x08, 0x2a, buffer, 128,
+			     SR_TIMEOUT, 3);
 
-	if (rc) {
+	if (rc == 0) {
 		/* failed, drive doesn't have capabilities mode page */
 		cd->cdi.speed = 1;
 		cd->cdi.mask |= (CDC_CD_R | CDC_CD_RW | CDC_DVD_R |
@@ -702,7 +693,7 @@
 		printk("%s: scsi-1 drive\n", cd->cdi.name);
 		return;
 	}
-	n = buffer[3] + 4;
+	n = rc;
 	cd->cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
 	cd->readcd_known = 1;
 	cd->readcd_cdda = buffer[n + 5] & 0x01;
===== drivers/scsi/scsi_lib.c 1.95 vs edited =====
--- 1.95/drivers/scsi/scsi_lib.c	Wed Jun  4 06:43:01 2003
+++ edited/drivers/scsi/scsi_lib.c	Sun Jun 22 15:26:15 2003
@@ -1336,3 +1336,105 @@
 		kmem_cache_destroy(sgp->slab);
 	}
 }
+/**	scsi_do_mode_sense - issue a mode sense, falling back from 10 to 
+ *		six bytes if necessary.
+ *	@SRpnt:	SCSI request to fill in with the MODE_SENSE
+ *	@dbd:	set if mode sense will allow block descriptors to be returned
+ *	@modepage: mode page being requested
+ *	@buffer: request buffer (may not be smaller than eight bytes)
+ *	@len:	length of request buffer.
+ *	@timeout: command timeout
+ *	@retries: number of retries before failing
+ *
+ *	Returns zero if unsuccessful, or the header offset (either 4
+ *	or 8 depending on whether a six or ten byte command was
+ *	issued) if successful.
+ **/
+int
+scsi_do_mode_sense(struct scsi_request *SRpnt, int dbd, int modepage,
+		   unsigned char *buffer, int len, int timeout, int retries) {
+	unsigned char cmd[12];
+	int use_10_for_ms;
+	int header_offset;
+
+	memset((void *) &cmd[0], 0, 12);
+	cmd[1] = dbd ? 0x08: 0;
+	cmd[2] = modepage;
+
+ retry:
+	use_10_for_ms = SRpnt->sr_device->use_10_for_ms;
+
+	if (use_10_for_ms) {
+		if (len < 8)
+			len = 8;
+
+		cmd[0] = MODE_SENSE_10;
+		cmd[8] = len;
+		header_offset = 8;
+	} else {
+		if (len < 4)
+			len = 4;
+
+		cmd[0] = MODE_SENSE;
+		cmd[4] = len;
+		header_offset = 4;
+	}
+
+	SRpnt->sr_cmd_len = 0;
+	SRpnt->sr_sense_buffer[0] = 0;
+	SRpnt->sr_sense_buffer[2] = 0;
+	SRpnt->sr_data_direction = SCSI_DATA_READ;
+
+	memset((void *) buffer, 0, len);
+
+	scsi_wait_req(SRpnt, (void *) cmd, (void *) buffer,
+		      len, timeout, retries);
+
+	/* This code looks awful: what it's doing is making sure an
+	 * 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(SRpnt->sr_result) &&
+	    (driver_byte(SRpnt->sr_result) & DRIVER_SENSE) &&
+	    SRpnt->sr_sense_buffer[2] == ILLEGAL_REQUEST &&
+	    (SRpnt->sr_sense_buffer[4] & 0x40) == 0x40 &&
+	    SRpnt->sr_sense_buffer[5] == 0 &&
+	    SRpnt->sr_sense_buffer[6] == 0 ) {
+		SRpnt->sr_device->use_10_for_ms = 0;
+		goto retry;
+	}
+
+	return scsi_status_is_good(SRpnt->sr_result) ? header_offset : 0;
+}
+
+/**	scsi_mode_sense - issue a mode sense, falling back from 10 to 
+ *		six bytes if necessary.
+ *	@sdev:	scsi device to send command to.
+ *	@dbd:	set if mode sense will allow block descriptors to be returned
+ *	@modepage: mode page being requested
+ *	@buffer: request buffer (may not be smaller than eight bytes)
+ *	@len:	length of request buffer.
+ *	@timeout: command timeout
+ *	@retries: number of retries before failing
+ *
+ *	Returns zero if unsuccessful, or the header offset (either 4
+ *	or 8 depending on whether a six or ten byte command was
+ *	issued) if successful.
+ **/
+int
+scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
+		unsigned char *buffer, int len, int timeout, int retries)
+{
+	struct scsi_request *sreq = scsi_allocate_request(sdev);
+	int ret;
+
+	if (!sreq)
+		return 0;
+
+	ret = scsi_do_mode_sense(sreq, dbd, modepage, buffer, len,
+				 timeout, retries);
+
+	scsi_release_request(sreq);
+
+	return ret;
+}
===== drivers/scsi/scsi_syms.c 1.40 vs edited =====
--- 1.40/drivers/scsi/scsi_syms.c	Thu Jun  5 06:26:04 2003
+++ edited/drivers/scsi/scsi_syms.c	Sat Jun 21 20:52:12 2003
@@ -86,6 +86,9 @@
 EXPORT_SYMBOL(scsi_remove_device);
 EXPORT_SYMBOL(scsi_set_device_offline);
 
+EXPORT_SYMBOL(scsi_do_mode_sense);
+EXPORT_SYMBOL(scsi_mode_sense);
+
 /*
  * This symbol is for the highlevel drivers (e.g. sg) only.
  */

Re: PATCH; make sr.c respect use_10_for_ms

[Matthew Dharm]

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

On Sun, Jun 22, 2003 at 03:37:28PM -0500, James Bottomley wrote:
> On Sun, 2003-06-22 at 14:56, Matthew Dharm wrote:
> > > But (as I read it -- remember I'm not an expert), the old sr.c code didn't
> > > set the DBD bit, just like the new code.  So whatever formula applied to
> > > the old code should apply to the new code, yes?
> 
> Hmm, the diff I sent was an older one which had the dbd meaning
> inverted.  The attached is the one I'm actually testing.

Ah.. I'll review this.

> > The code James sent sets DBD to 0 -- I like that, as many usb-storage
> > devices choke when DBD is set to 1.  I believe in avoiding the DBD bit as
> > much as possible, and James seems to have eliminated it.
> > 
> > However, DBD==0 means that a block descriptor is likely to be returned --
> > so we need to add in the size of the block descriptor header.
> 
> OK, now I'm confused.  the write caching determination code that you and
> Andries spent a while getting to work has DBD==1, and I thought we'd
> eliminated all the USB problems with that code...

Yeah, it's confusing.

Andries and I spent a great deal of time getting the use_10_for_ms worked
out -- and that's helped greatly.  The DBD=1 for cache determination was
there before that, and is still there, and I would still like it removed.
However, with the use_10_for_ms code in place, many more devices don't
-fatally- choke when DBD=1.  Lots still do, however.

Up until now, I really didn't understand what DBD did... now that I do, I
really don't understand why we use it at all.  It's a matter of
convenience to parse the data, I guess, but it comes at a great expense of
incompatiblity.

> Since the BD skip code is quite a wodge of complexity, I'd like to know
> what devices fail on DBD==1 before trying to add it all in, especially
> as we (fortunatly) have nothing in the kernel that actually wants to see
> the BDs.

The problem as I see it is ATAPI devices.  That means that sbp2, ide-scsi,
and usb-storage (all of which bridge ATAPI devices to the SCSI layer) will
be affected.  These devices don't include the DBD bit in their command
specification -- the bit is reserved.

Unfortunately, more data is difficult to come by -- the cache-detect code
is still relatively new, and the use_10_for_ms is even newer (read: last
week).  We haven't had the opportunity to get the bug reports back from
everywhere to find out how bad the problem is.  The devices I have avoid
the fatal condition by rejecting all commands with the DBD bit set, but
it is still an error condition I'd like to avoid.

And I'm pretty certain that there are devices for which setting DBD is
fatal still.  I just don't have any hard evidence to prove I'm right.  Tho
I have yet to see a USB device answer the cache-detect query with a
meaningful answer.

And, I don't see DBD as a required element.  It's a convenience item, which
will create a great deal of headache for us.

I don't see how the BD code is complex.  If we change it so that DBD==0
always, then this problem goes away completely.  Just change the calculation
of header_offset to include the DBD size (taken from the header) and we're
done -- the callers still get an offset into the data which shows them
where the mode page starts.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: PATCH; make sr.c respect use_10_for_ms

[James Bottomley]

On Sun, 2003-06-22 at 16:06, Matthew Dharm wrote:
> The problem as I see it is ATAPI devices.  That means that sbp2, ide-scsi,
> and usb-storage (all of which bridge ATAPI devices to the SCSI layer) will
> be affected.  These devices don't include the DBD bit in their command
> specification -- the bit is reserved.

That's SFF-8020i?  The bit is listed as reserved, *but* the
implementation defined behaves as if DBD were specified.  The BD header
component of the mode sense header is specifically listed as "reserved"
and the spec doesn't allow for block descriptors to be returned.

The current code for ATAPI CD-ROMS must just be lucky because the
reserved fields are zero filled.

the SFF-8020i says that the recipient "shall not check" reserved fields,
so the standard makes it sound like we can get away with the correct
behaviour in all cases by setting DBD.

Have you actually heard of setting DBD causing a failure on ATAPI
devices?

James

Re: PATCH; make sr.c respect use_10_for_ms

[Matthew Dharm]

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

On Mon, Jun 23, 2003 at 09:33:45AM -0500, James Bottomley wrote:
> Have you actually heard of setting DBD causing a failure on ATAPI
> devices?

I believe I have, but I can't site anything solid.  Which means (likely)
that there were multiple possible explanations.

We've changed a lot of MODE_SENSE code, and it's difficult to separate out
what was causing the problem.  And I'm constantly finding devices with new
and more creative ways to not work right.

In short: I guess I'm not really certain.  So let's go ahead and try it.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

What, are you one of those Microsoft-bashing Linux freaks?
					-- Customer to Greg
User Friendly, 2/10/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]