Re: PATCH; make sr.c respect use_10_for_ms

[James Bottomley <James.Bottomley@steeleye.com>]

[-- 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.
  */