Maximum_transfer_size and SCSI request buffer size parameters

Maximum_transfer_size and SCSI request buffer size parameters

[Alan Stern]

I'm trying to solve the problem of a user whose USB disk drive crashes
when it is asked to write 512K bytes in a single command.  A good way to
solve this problem would be to have a maximum_transfer_size field as part 
of the scsi_device structure.  Is it worthwhile adding such a feature?

More importantly, however, I noticed the following additional problem in
the sd.c and sr.c files (presumably also present in st.c and sg.c, but I
didn't look).  It has to do with the scmd->bufflen and
scmd->request_bufflen fields are used.  Now they aren't very well
documented to begin with, but even so their usage is inconsistent and
liable to crash a USB storage device.

In sd.c and sr.c, scmd->request_bufflen is the number of bytes to
transfer.  Both files use the variable this_count to compute the number
of sectors, stored in the command's CDB.  this_count is based on
request_bufflen, but the values can disagree.  For example, if READ(6)  
is used and request_bufflen corresponds to more than 255 sectors, 
this_count will be reduced to 255.

Furthermore, sr.c goes to the trouble of adding up the lengths of all the
scatter-gather blocks, and if the total disagrees with request_bufflen it
sets request_bufflen equal to the total -- but it does this after storing
this_count in the CDB.

This combination is deadly for usb-storage!  It relies on having 
request_bufflen be the exact byte count for the command stored in the CDB.

A related issue is that in both sd.c and sr.c, the rw_intr() routine
identifies the number of sectors correctly transferred as scmd->bufflen
(if no error occurred).  Normally bufflen is initialized equal to
request_bufflen.  But if this_count has been altered, the number of
sectors will be different from bufflen.

If nobody objects, I will work on a patch to fix these problems.

Alan Stern

Re: Maximum_transfer_size and SCSI request buffer size parameters

[Jens Axboe]

On Thu, Jul 24 2003, Alan Stern wrote:
> I'm trying to solve the problem of a user whose USB disk drive crashes
> when it is asked to write 512K bytes in a single command.  A good way to
> solve this problem would be to have a maximum_transfer_size field as part 
> of the scsi_device structure.  Is it worthwhile adding such a feature?

First of all, you don't mention what kernel version you are talking
about...

Anyways, the feature is already there. It's called max_sectors, and it's
even in the host template for you to set. That handles block layer
requests, at least.

> In sd.c and sr.c, scmd->request_bufflen is the number of bytes to
> transfer.  Both files use the variable this_count to compute the number
> of sectors, stored in the command's CDB.  this_count is based on
> request_bufflen, but the values can disagree.  For example, if READ(6)  
> is used and request_bufflen corresponds to more than 255 sectors, 
> this_count will be reduced to 255.

That is in purpose, for cases like that you have to only do a part of
the entire request.

> Furthermore, sr.c goes to the trouble of adding up the lengths of all the
> scatter-gather blocks, and if the total disagrees with request_bufflen it
> sets request_bufflen equal to the total -- but it does this after storing
> this_count in the CDB.

It's a bug if they differ, that's why there's a printk there. Might be a
good idea to make that a harder error, though.

> A related issue is that in both sd.c and sr.c, the rw_intr() routine
> identifies the number of sectors correctly transferred as scmd->bufflen
> (if no error occurred).  Normally bufflen is initialized equal to
> request_bufflen.  But if this_count has been altered, the number of
> sectors will be different from bufflen.

The device will not transfer more data than what it is told from the
cdb. Aren't you getting a completed byte count from your hardware? Or
are you blindly assuming the device transferred what you initially told
you?

> If nobody objects, I will work on a patch to fix these problems.

Well..

-- 
Jens Axboe

Re: Maximum_transfer_size and SCSI request buffer size parameters

[Alan Stern]

On Thu, 24 Jul 2003, Jens Axboe wrote:

> On Thu, Jul 24 2003, Alan Stern wrote:
> > I'm trying to solve the problem of a user whose USB disk drive crashes
> > when it is asked to write 512K bytes in a single command.  A good way to
> > solve this problem would be to have a maximum_transfer_size field as part 
> > of the scsi_device structure.  Is it worthwhile adding such a feature?
> 
> First of all, you don't mention what kernel version you are talking
> about...

And I often make the same mental complaint about other writers!  I'm 
referring to 2.6, although it wouldn't hurt to work on 2.4 as well...

> Anyways, the feature is already there. It's called max_sectors, and it's
> even in the host template for you to set. That handles block layer
> requests, at least.

Great!  I should have seen that.  Thanks for the pointer.

> > In sd.c and sr.c, scmd->request_bufflen is the number of bytes to
> > transfer.  Both files use the variable this_count to compute the number
> > of sectors, stored in the command's CDB.  this_count is based on
> > request_bufflen, but the values can disagree.  For example, if READ(6)  
> > is used and request_bufflen corresponds to more than 255 sectors, 
> > this_count will be reduced to 255.
> 
> That is in purpose, for cases like that you have to only do a part of
> the entire request.

Certainly.  But when this_count is reduced, request_bufflen should be 
reduced as well.

> > Furthermore, sr.c goes to the trouble of adding up the lengths of all the
> > scatter-gather blocks, and if the total disagrees with request_bufflen it
> > sets request_bufflen equal to the total -- but it does this after storing
> > this_count in the CDB.
> 
> It's a bug if they differ, that's why there's a printk there. Might be a
> good idea to make that a harder error, though.

But if they differ and you're trying to correct the error, the correction 
should be made _before_ this_count is set to request_bufflen, not after.

> > A related issue is that in both sd.c and sr.c, the rw_intr() routine
> > identifies the number of sectors correctly transferred as scmd->bufflen
> > (if no error occurred).  Normally bufflen is initialized equal to
> > request_bufflen.  But if this_count has been altered, the number of
> > sectors will be different from bufflen.
> 
> The device will not transfer more data than what it is told from the
> cdb. Aren't you getting a completed byte count from your hardware? Or
> are you blindly assuming the device transferred what you initially told
> you?

If no errors occur we do not get a completed byte count; we have to assume
that all the requested data was transferred.  It seems like a reasonable
assumption under the circumstances.  That's not the problem; the problem
is that the value passed to scsi_io_completion() for the number of sectors
completed will be the value in scmd->bufflen, not the amount in the CDB
which was actually transferred.

Alan Stern

PATCH: (as70) SCSI request buffer size parameters

[Alan Stern]

Here is my proposed patch to fix up inconsistent values in 
scmd->request_bufflen, scmd->bufflen, and scmd->cmnd[].  Only sd.c and 
sr.c are affected; I didn't see anything that needed changing in st.c and 
sg.c.

Alan Stern


===== sd.c 1.52 vs edited =====
--- 1.52/drivers/scsi/sd.c	Tue Jul  1 17:54:19 2003
+++ edited/drivers/scsi/sd.c	Thu Jul 24 15:33:25 2003
@@ -308,6 +308,8 @@
 		SCpnt->cmnd[4] = (unsigned char) this_count;
 		SCpnt->cmnd[5] = 0;
 	}
+	SCpnt->request_bufflen = SCpnt->bufflen =
+			this_count * sdp->sector_size;
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb
===== sr.c 1.50 vs edited =====
--- 1.50/drivers/scsi/sr.c	Tue Jul  1 17:54:19 2003
+++ edited/drivers/scsi/sr.c	Thu Jul 24 15:38:04 2003
@@ -315,6 +315,19 @@
 		return 0;
 	}
 
+	{
+		struct scatterlist *sg = SCpnt->request_buffer;
+		int i, size = 0;
+		for (i = 0; i < SCpnt->use_sg; i++)
+			size += sg[i].length;
+
+		if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
+			printk(KERN_ERR "sr: mismatch count %d, bytes %d\n", size, SCpnt->request_bufflen);
+			if (SCpnt->request_bufflen > size)
+				SCpnt->request_bufflen = SCpnt->bufflen = size;
+		}
+	}
+
 	/*
 	 * request doesn't start on hw block boundary, add scatter pads
 	 */
@@ -336,8 +349,11 @@
 	SCpnt->cmnd[1] = 0;
 	block = (unsigned int)SCpnt->request->sector / (s_size >> 9);
 
-	if (this_count > 0xffff)
+	if (this_count > 0xffff) {
 		this_count = 0xffff;
+		SCpnt->request_bufflen = SCpnt->bufflen =
+				this_count * s_size;
+	}
 
 	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
 	SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
@@ -364,18 +380,6 @@
 	 * of capability to this function.
 	 */
 	SCpnt->done = rw_intr;
-
-	{
-		struct scatterlist *sg = SCpnt->request_buffer;
-		int i, size = 0;
-		for (i = 0; i < SCpnt->use_sg; i++)
-			size += sg[i].length;
-
-		if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
-			printk("sr: mismatch count %d, bytes %d\n", size, SCpnt->request_bufflen);
-			SCpnt->request_bufflen = size;
-		}
-	}
 
 	/*
 	 * This indicates that the command is ready from our end to be

Retry scsi_mode_sense if UNIT ATTENTION occurs

[Alan Stern]

Jens:

This patch changes the scsi_mode_sense() routine to retry up to a total of 
3 times if a command fails with a UNIT ATTENTION sense key value.

Without this change, sr.c does not properly read the capabilities mode 
page for a just-powered-up cdrom drive -- it gives up upon receiving a 
Device Reset Occurred code.

The patch looks lengthy, but most of it just changes the indentation 
level.

Alan Stern


--- 2.6.0-orig/drivers/scsi/scsi_lib.c	Thu Jul 17 15:49:29 2003
+++ 2.6.0/drivers/scsi/scsi_lib.c	Sat Jul 26 10:30:49 2003
@@ -1375,55 +1375,63 @@
 	unsigned char cmd[12];
 	int use_10_for_ms;
 	int header_length;
+	int my_retries;
 
 	memset(data, 0, sizeof(*data));
 	memset(&cmd[0], 0, 12);
 	cmd[1] = dbd & 0x18;	/* allows DBD and LLBA bits */
 	cmd[2] = modepage;
 
- retry:
-	use_10_for_ms = sreq->sr_device->use_10_for_ms;
+	for (my_retries = 0; my_retries < 3; ++my_retries) {
+		use_10_for_ms = sreq->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_length = 8;
-	} else {
-		if (len < 4)
-			len = 4;
-
-		cmd[0] = MODE_SENSE;
-		cmd[4] = len;
-		header_length = 4;
+		if (use_10_for_ms) {
+			if (len < 8)
+				len = 8;
+
+			cmd[0] = MODE_SENSE_10;
+			cmd[8] = len;
+			header_length = 8;
+		} else {
+			if (len < 4)
+				len = 4;
+
+			cmd[0] = MODE_SENSE;
+			cmd[4] = len;
+			header_length = 4;
+		}
+
+		sreq->sr_cmd_len = 0;
+		sreq->sr_sense_buffer[0] = 0;
+		sreq->sr_sense_buffer[2] = 0;
+		sreq->sr_data_direction = DMA_FROM_DEVICE;
+
+		memset(buffer, 0, len);
+
+		scsi_wait_req(sreq, cmd, buffer, len, timeout, retries);
+
+		if ((driver_byte(sreq->sr_result) & DRIVER_SENSE) &&
+		    sreq->sr_sense_buffer[2] == UNIT_ATTENTION)
+			continue;
+
+		/* 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(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;
+			continue;
+		}
+		if (scsi_status_is_good(sreq->sr_result))
+			break;
 	}
 
-	sreq->sr_cmd_len = 0;
-	sreq->sr_sense_buffer[0] = 0;
-	sreq->sr_sense_buffer[2] = 0;
-	sreq->sr_data_direction = DMA_FROM_DEVICE;
-
-	memset(buffer, 0, len);
-
-	scsi_wait_req(sreq, cmd, 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(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(scsi_status_is_good(sreq->sr_result)) {
+	if (scsi_status_is_good(sreq->sr_result)) {
 		data->header_length = header_length;
 		if(use_10_for_ms) {
 			data->length = buffer[0]*256 + buffer[1];

Re: Retry scsi_mode_sense if UNIT ATTENTION occurs

[Jeff Garzik]

On Mon, Jul 28, 2003 at 10:10:33AM -0400, Alan Stern wrote:
> Jens:
> 
> This patch changes the scsi_mode_sense() routine to retry up to a total of 
> 3 times if a command fails with a UNIT ATTENTION sense key value.
> 
> Without this change, sr.c does not properly read the capabilities mode 
> page for a just-powered-up cdrom drive -- it gives up upon receiving a 
> Device Reset Occurred code.

I'm curious, are you testing against a real SCSI device, or a faked one
as via ide-scsi.c?

	Jeff

Re: Retry scsi_mode_sense if UNIT ATTENTION occurs

[Alan Stern]

On Mon, 28 Jul 2003, Jeff Garzik wrote:

> On Mon, Jul 28, 2003 at 10:10:33AM -0400, Alan Stern wrote:
> > Jens:
> > 
> > This patch changes the scsi_mode_sense() routine to retry up to a total of 
> > 3 times if a command fails with a UNIT ATTENTION sense key value.
> > 
> > Without this change, sr.c does not properly read the capabilities mode 
> > page for a just-powered-up cdrom drive -- it gives up upon receiving a 
> > Device Reset Occurred code.
> 
> I'm curious, are you testing against a real SCSI device, or a faked one
> as via ide-scsi.c?

Sort of in-between.  It's a USB CD-RW drive.  So it's not attached to a 
real SCSI bus, but it does use the genuine SCSI command set.

Alan Stern

Re: Retry scsi_mode_sense if UNIT ATTENTION occurs

[Kai Makisara]

On Mon, 28 Jul 2003, Alan Stern wrote:

> Jens:
>
> This patch changes the scsi_mode_sense() routine to retry up to a total of
> 3 times if a command fails with a UNIT ATTENTION sense key value.
>
This is dangerous or makes __scsi_mode_sense usable only in special cases.
Some high-level drivers need to process the UNIT ATTENTION sense data.

> Without this change, sr.c does not properly read the capabilities mode
> page for a just-powered-up cdrom drive -- it gives up upon receiving a
> Device Reset Occurred code.
>
One alternative is to move the retry code upwards to scsi_mode_sense and
document that it may discard UNIT ATTENTION. Those drivers that can't
tolerate this can then use __scsi_mode_sense. This would solve the sr
problem.

-- 
Kai

Re: Retry scsi_mode_sense if UNIT ATTENTION occurs

[Alan Stern]

On Tue, 29 Jul 2003, Kai Makisara wrote:

> On Mon, 28 Jul 2003, Alan Stern wrote:
> 
> > Jens:
> >
> > This patch changes the scsi_mode_sense() routine to retry up to a total of
> > 3 times if a command fails with a UNIT ATTENTION sense key value.
> >
> This is dangerous or makes __scsi_mode_sense usable only in special cases.
> Some high-level drivers need to process the UNIT ATTENTION sense data.

I don't know what you're talking about.  __scsi_mode_sense() is called
only from a very few places within sr.c and sd.c, and none of the calling
routines pays any notice to the sense data.

> > Without this change, sr.c does not properly read the capabilities mode
> > page for a just-powered-up cdrom drive -- it gives up upon receiving a
> > Device Reset Occurred code.
> >
> One alternative is to move the retry code upwards to scsi_mode_sense and
> document that it may discard UNIT ATTENTION. Those drivers that can't
> tolerate this can then use __scsi_mode_sense. This would solve the sr
> problem.

But scsi_mode_sense() allocates its own struct scsi_request, whereas the 
callers in sd.c already have one available.

I wouldn't object to documenting in both scsi_mode_sense() and 
__scsi_mode_sense() that UNIT ATTENTION sense data is discarded.

Alan Stern

Re: Retry scsi_mode_sense if UNIT ATTENTION occurs

[James Bottomley]

On Mon, 2003-07-28 at 10:10, Alan Stern wrote:
> Jens:
> 
> This patch changes the scsi_mode_sense() routine to retry up to a total of 
> 3 times if a command fails with a UNIT ATTENTION sense key value.
> 
> Without this change, sr.c does not properly read the capabilities mode 
> page for a just-powered-up cdrom drive -- it gives up upon receiving a 
> Device Reset Occurred code.
> 
> The patch looks lengthy, but most of it just changes the indentation 
> level.

Actually, this isn't really the correct place for it.  The problem
occurs in sr not sd because sd already does an initial TUR to clear the
unit attention.

Before OLS got in the way and I got submerged in writing presentations,
I had some patches that Matt Dharm was testing out to add the TUR to the
initial sr code.

I'll pick that up (...er, when I get caught back up at work).

James

Re: Retry scsi_mode_sense if UNIT ATTENTION occurs

[Alan Stern]

On 29 Jul 2003, James Bottomley wrote:

> On Mon, 2003-07-28 at 10:10, Alan Stern wrote:
> > Jens:
> > 
> > This patch changes the scsi_mode_sense() routine to retry up to a total of 
> > 3 times if a command fails with a UNIT ATTENTION sense key value.
> > 
> > Without this change, sr.c does not properly read the capabilities mode 
> > page for a just-powered-up cdrom drive -- it gives up upon receiving a 
> > Device Reset Occurred code.
> > 
> > The patch looks lengthy, but most of it just changes the indentation 
> > level.
> 
> Actually, this isn't really the correct place for it.  The problem
> occurs in sr not sd because sd already does an initial TUR to clear the
> unit attention.
> 
> Before OLS got in the way and I got submerged in writing presentations,
> I had some patches that Matt Dharm was testing out to add the TUR to the
> initial sr code.
> 
> I'll pick that up (...er, when I get caught back up at work).

Okay, that ought to take care of it just as well.

Thanks.

Alan Stern

Re: Retry scsi_mode_sense if UNIT ATTENTION occurs

[Matthew Dharm]

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

On Wed, Jul 30, 2003 at 10:09:23AM -0400, Alan Stern wrote:
> On 29 Jul 2003, James Bottomley wrote:
> 
> > On Mon, 2003-07-28 at 10:10, Alan Stern wrote:
> > > Jens:
> > > 
> > > This patch changes the scsi_mode_sense() routine to retry up to a total of 
> > > 3 times if a command fails with a UNIT ATTENTION sense key value.
> > > 
> > > Without this change, sr.c does not properly read the capabilities mode 
> > > page for a just-powered-up cdrom drive -- it gives up upon receiving a 
> > > Device Reset Occurred code.
> > > 
> > > The patch looks lengthy, but most of it just changes the indentation 
> > > level.
> > 
> > Actually, this isn't really the correct place for it.  The problem
> > occurs in sr not sd because sd already does an initial TUR to clear the
> > unit attention.
> > 
> > Before OLS got in the way and I got submerged in writing presentations,
> > I had some patches that Matt Dharm was testing out to add the TUR to the
> > initial sr code.
> > 
> > I'll pick that up (...er, when I get caught back up at work).
> 
> Okay, that ought to take care of it just as well.

I had been wondering what ever happened to that... James, I presume you
still have my last set of data, regarding my CD-RW initialization?

Matt

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

G:  Let me guess, you started on the 'net with AOL, right?
C:  WOW! d00d! U r leet!
					-- Greg and Customer 
User Friendly, 2/12/1999

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