[patch] scsi: ten -> use_10_for_rw / use_10_for_ms

[patch] scsi: ten -> use_10_for_rw / use_10_for_ms

[Andries.Brouwer]

In the old days, ancient scsi devices understood 6-byte commands
and more recent ones also understood 10-byte commands.
Thus, we had a "ten" flag indicating that 10-byte commands worked.

These days, especially for usb-storage devices, the opposite
sometimes holds - 10-byte commands are supported, but 6-byte commands
are not.

The patch below changes the field ten into the pair of fields
use_10_for_rw, use_10_for_ms set initially when the driver
thinks these are supported. Ifthe device returns ILLEGAL_REQUEST
they are cleared.

This patch obsoletes a large amount of code in usb-storage,
and not only that, once the subsequent patch removes all this
usb-storage code many devices will work that hang today.


Andries

diff -u --recursive --new-file -X /linux/dontdiff a/drivers/scsi/scsi.h b/drivers/scsi/scsi.h
--- a/drivers/scsi/scsi.h	Thu May 22 13:17:01 2003
+++ b/drivers/scsi/scsi.h	Fri May 23 10:02:30 2003
@@ -383,9 +383,10 @@
 				 * time. */
 	unsigned was_reset:1;	/* There was a bus reset on the bus for 
 				 * this device */
-	unsigned expecting_cc_ua:1;	/* Expecting a CHECK_CONDITION/UNIT_ATTN
-					 * because we did a bus reset. */
-	unsigned ten:1;		/* support ten byte read / write */
+	unsigned expecting_cc_ua:1; /* Expecting a CHECK_CONDITION/UNIT_ATTN
+				     * because we did a bus reset. */
+	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
+	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
 	unsigned remap:1;	/* support remapping  */
 //	unsigned sync:1;	/* Sync transfer state, managed by host */
 //	unsigned wide:1;	/* WIDE transfer state, managed by host */
@@ -402,10 +403,6 @@
 	container_of(d, struct scsi_device, sdev_driverfs_dev)
 
 
-/*
- * The Scsi_Cmnd structure is used by scsi.c internally, and for communication
- * with low level drivers that support multiple outstanding commands.
- */
 typedef struct scsi_pointer {
 	char *ptr;		/* data pointer */
 	int this_residual;	/* left in this buffer */
@@ -458,12 +455,13 @@
 };
 
 /*
- * FIXME(eric) - one of the great regrets that I have is that I failed to define
- * these structure elements as something like sc_foo instead of foo.  This would
- * make it so much easier to grep through sources and so forth.  I propose that
- * all new elements that get added to these structures follow this convention.
- * As time goes on and as people have the stomach for it, it should be possible to 
- * go back and retrofit at least some of the elements here with with the prefix.
+ * FIXME(eric) - one of the great regrets that I have is that I failed to
+ * define these structure elements as something like sc_foo instead of foo.
+ * This would make it so much easier to grep through sources and so forth.
+ * I propose that all new elements that get added to these structures follow
+ * this convention.  As time goes on and as people have the stomach for it,
+ * it should be possible to go back and retrofit at least some of the elements
+ * here with with the prefix.
  */
 struct scsi_cmnd {
 	int     sc_magic;
diff -u --recursive --new-file -X /linux/dontdiff a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c	Thu May 22 13:17:01 2003
+++ b/drivers/scsi/scsi_lib.c	Fri May 23 10:02:30 2003
@@ -662,7 +662,6 @@
  *
  *		b) We can just use scsi_requeue_command() here.  This would
  *		   be used if we just wanted to retry, for example.
- *
  */
 void scsi_io_completion(struct scsi_cmnd *cmd, int good_sectors,
 			int block_sectors)
@@ -796,17 +795,20 @@
 				}
 			}
 		}
-		/* If we had an ILLEGAL REQUEST returned, then we may have
-		 * performed an unsupported command.  The only thing this should be
-		 * would be a ten byte read where only a six byte read was supported.
-		 * Also, on a system where READ CAPACITY failed, we have have read
-		 * past the end of the disk.
+		/*
+		 * If we had an ILLEGAL REQUEST returned, then we may have
+		 * performed an unsupported command.  The only thing this
+		 * should be would be a ten byte read where only a six byte
+		 * read was supported.  Also, on a system where READ CAPACITY
+		 * failed, we may have read past the end of the disk.
 		 */
 
 		switch (cmd->sense_buffer[2]) {
 		case ILLEGAL_REQUEST:
-			if (cmd->device->ten) {
-				cmd->device->ten = 0;
+			if (cmd->device->use_10_for_rw &&
+			    (cmd->cmnd[0] == READ_10 ||
+			     cmd->cmnd[0] == WRITE_10)) {
+				cmd->device->use_10_for_rw = 0;
 				/*
 				 * This will cause a retry with a 6-byte
 				 * command.
diff -u --recursive --new-file -X /linux/dontdiff a/drivers/scsi/sd.c b/drivers/scsi/sd.c
--- a/drivers/scsi/sd.c	Thu May 22 13:17:01 2003
+++ b/drivers/scsi/sd.c	Fri May 23 10:02:30 2003
@@ -320,7 +320,8 @@
 		SCpnt->cmnd[12] = (unsigned char) (this_count >> 8) & 0xff;
 		SCpnt->cmnd[13] = (unsigned char) this_count & 0xff;
 		SCpnt->cmnd[14] = SCpnt->cmnd[15] = 0;
-	} else if (((this_count > 0xff) || (block > 0x1fffff)) || SCpnt->device->ten) {
+	} else if ((this_count > 0xff) || (block > 0x1fffff) ||
+		   SCpnt->device->use_10_for_rw) {
 		if (this_count > 0xffff)
 			this_count = 0xffff;
 
@@ -768,11 +769,14 @@
 			break;
 
 		case ILLEGAL_REQUEST:
-			if (SCpnt->device->ten == 1) {
-				if (SCpnt->cmnd[0] == READ_10 ||
-				    SCpnt->cmnd[0] == WRITE_10)
-					SCpnt->device->ten = 0;
-			}
+			if (SCpnt->device->use_10_for_rw &&
+			    (SCpnt->cmnd[0] == READ_10 ||
+			     SCpnt->cmnd[0] == WRITE_10))
+				SCpnt->device->use_10_for_rw = 0;
+			if (SCpnt->device->use_10_for_ms &&
+			    (SCpnt->cmnd[0] == MODE_SENSE_10 ||
+			     SCpnt->cmnd[0] == MODE_SELECT_10))
+				SCpnt->device->use_10_for_ms = 0;
 			break;
 
 		default:
@@ -1093,16 +1097,29 @@
 	sdkp->device->sector_size = sector_size;
 }
 
+/* called with buffer of length 512 */
 static int
-sd_do_mode_sense6(struct scsi_device *sdp, struct scsi_request *SRpnt,
-		  int dbd, int modepage, unsigned char *buffer, int len) {
-	unsigned char cmd[8];
+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, 8);
-	cmd[0] = MODE_SENSE;
+	memset((void *) &cmd[0], 0, 12);
 	cmd[1] = dbd;
 	cmd[2] = modepage;
-	cmd[4] = len;
+
+	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;
@@ -1119,11 +1136,11 @@
 
 /*
  * read write protect setting, if possible - called only in sd_init_onedisk()
+ * called with buffer of length 512
  */
 static void
 sd_read_write_protect_flag(struct scsi_disk *sdkp, char *diskname,
 		   struct scsi_request *SRpnt, unsigned char *buffer) {
-	struct scsi_device *sdp = sdkp->device;
 	int res;
 
 	/*
@@ -1131,7 +1148,7 @@
 	 * We have to start carefully: some devices hang if we ask
 	 * for more than is available.
 	 */
-	res = sd_do_mode_sense6(sdp, SRpnt, 0, 0x3F, buffer, 4);
+	res = sd_do_mode_sense(SRpnt, 0, 0x3F, buffer, 4);
 
 	/*
 	 * Second attempt: ask for page 0
@@ -1139,13 +1156,13 @@
 	 * Sense Key 5: Illegal Request, Sense Code 24: Invalid field in CDB.
 	 */
 	if (res)
-		res = sd_do_mode_sense6(sdp, SRpnt, 0, 0, buffer, 4);
+		res = sd_do_mode_sense(SRpnt, 0, 0, buffer, 4);
 
 	/*
 	 * Third attempt: ask 255 bytes, as we did earlier.
 	 */
 	if (res)
-		res = sd_do_mode_sense6(sdp, SRpnt, 0, 0x3F, buffer, 255);
+		res = sd_do_mode_sense(SRpnt, 0, 0x3F, buffer, 255);
 
 	if (res) {
 		printk(KERN_WARNING
@@ -1161,25 +1178,25 @@
 
 /*
  * sd_read_cache_type - called only from sd_init_onedisk()
+ * called with buffer of length 512
  */
 static void
 sd_read_cache_type(struct scsi_disk *sdkp, char *diskname,
 		   struct scsi_request *SRpnt, unsigned char *buffer) {
-	struct scsi_device *sdp = sdkp->device;
 	int len = 0, res;
 
 	const int dbd = 0x08;	   /* DBD */
 	const int modepage = 0x08; /* current values, cache page */
 
 	/* cautiously ask */
-	res = sd_do_mode_sense6(sdp, SRpnt, dbd, modepage, buffer, 4);
+	res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer, 4);
 
 	if (res == 0) {
 		/* that went OK, now ask for the proper length */
 		len = buffer[0] + 1;
 		if (len > 128)
 			len = 128;
-		res = sd_do_mode_sense6(sdp, SRpnt, dbd, modepage, buffer, len);
+		res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer, len);
 	}
 
 	if (res == 0 && buffer[3] + 6 < len) {
@@ -1278,7 +1295,8 @@
 	if (sdkp->media_present)
 		sd_read_cache_type(sdkp, disk->disk_name, SRpnt, buffer);
 		
-	SRpnt->sr_device->ten = 1;
+	SRpnt->sr_device->use_10_for_rw = 1;
+	SRpnt->sr_device->use_10_for_ms = 0;
 	SRpnt->sr_device->remap = 1;
 
  leave:
diff -u --recursive --new-file -X /linux/dontdiff a/drivers/scsi/sr.c b/drivers/scsi/sr.c
--- a/drivers/scsi/sr.c	Thu May 22 13:17:01 2003
+++ b/drivers/scsi/sr.c	Fri May 23 10:02:30 2003
@@ -559,7 +559,8 @@
 	sprintf(cd->cdi.name, "sr%d", minor);
 
 	sdev->sector_size = 2048;	/* A guess, just in case */
-	sdev->ten = 1;
+	sdev->use_10_for_rw = 1;
+	sdev->use_10_for_ms = 0;
 	sdev->remap = 1;
 
 	/* FIXME: need to handle a get_capabilities failure properly ?? */

Re: [patch] scsi: ten -> use_10_for_rw / use_10_for_ms

[Matthew Dharm]

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

On Fri, May 23, 2003 at 09:22:44AM +0200, Andries.Brouwer@cwi.nl wrote:
> In the old days, ancient scsi devices understood 6-byte commands
> and more recent ones also understood 10-byte commands.
> Thus, we had a "ten" flag indicating that 10-byte commands worked.
> 
> These days, especially for usb-storage devices, the opposite
> sometimes holds - 10-byte commands are supported, but 6-byte commands
> are not.
> 
> The patch below changes the field ten into the pair of fields
> use_10_for_rw, use_10_for_ms set initially when the driver
> thinks these are supported. Ifthe device returns ILLEGAL_REQUEST
> they are cleared.
> 
> This patch obsoletes a large amount of code in usb-storage,
> and not only that, once the subsequent patch removes all this
> usb-storage code many devices will work that hang today.

Any idea when you'll submit that patch to usb-storage?

Do you perhaps just want to send what you have to me for integration?

Matt

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

Sir, for the hundreth time, we do NOT carry 600-round boxes of belt-fed 
suction darts!
					-- Salesperson to Greg
User Friendly, 12/30/1997

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