Re: PATCH; make sr.c respect use_10_for_ms

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

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