adding flag for 192 byte mode sense

Following are patches to support a flag for a 192 byte MODE SENSE, for use by USB.

First patch replaces the scsi_host flags with a per sdev bflag, and sets values in a new USB slave_alloc rather than the scsi_host template. I grepped code for other users of scsi_host flags, and did not find any, and do not recall seeing any added (flags is a bad name). slave_alloc is used so that if needed the devinfo code can overwrite any settings.

Second patch adds the 192 flag.

With both patches we should get the exact behaviour we have now.

USB patches are still needed to set the BLIST_MS_192_BYTES_FOR_3F flag, and to conditionally use the BLIST_USE_10_BYTE_MS based on the transparent scsi versus all others. Alan or Matthew - can you send patches for that?

The scsi core usage of use_10_for_ms might need to change to behave the way usb wants, though it probably will not matter. It currently demotes from 10 byte to 6 byte if there is an illegal request on the 10 byte command. Based on Alan's information, it sounds like USB wants the commands to always be 6 or 10 bytes, with no change on an illegal request.

I tested with these usb mass storage devices: sony camera, olympus camera, and dazzle card reader with no problems. I thought the sony used to choke on the MODE SENSE, but it worked for both the short 8 byte and long 192 byte requests (did get sense key Illegal Request but no hang on allow medium removal).

Some log output with command logging (sysctl -w dev.scsi.logging_level=0x2400) on as follows.

Default USB behaviour, no MODE SENSE is sent:

kernel: scsi <1:0:0:0> send                  Inquiry 00 00 00 24 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Inquiry 00 00 00 24 00 
kernel:   Vendor: OLYMPUS   Model: C750UZ            Rev: 1.00
kernel:   Type:   Direct-Access                      ANSI SCSI revision: 02
kernel: scsi <1:0:0:0> send                  Test Unit Ready 00 00 00 00 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Test Unit Ready 00 00 00 00 00 
kernel: scsi <1:0:0:0> send                  Read Capacity 00 00 00 00 00 00 00 00 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Read Capacity 00 00 00 00 00 00 00 00 00 
kernel: SCSI device sda: 512000 512-byte hdwr sectors (262 MB)
kernel: sda: assuming Write Enabled
kernel: sda: assuming drive cache: write through
kernel: scsi <1:0:0:0> send                  Test Unit Ready 00 00 00 00 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Test Unit Ready 00 00 00 00 00 
kernel: scsi <1:0:0:0> send                  Prevent/Allow Medium Removal 00 00 00 01 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Prevent/Allow Medium Removal 00 00 00 01 00 
kernel:  sda:<6>scsi <1:0:0:0> send                  Read (10) 00 00 00 00 00 00 00 08 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Read (10) 00 00 00 00 00 00 00 08 00 
kernel:  sda1
kernel: scsi <1:0:0:0> send                  Prevent/Allow Medium Removal 00 00 00 00 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Prevent/Allow Medium Removal 00 00 00 00 00 

Command logs using 192 (0xc0) byte MODE SENSE - tested via addition of a devinfo entry using:

	echo "OLYMPUS:C750UZ:0x1a000" > /proc/scsi/device_info

kernel: scsi <1:0:0:0> send                  Inquiry 00 00 00 24 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Inquiry 00 00 00 24 00 
kernel:   Vendor: OLYMPUS   Model: C750UZ            Rev: 1.00
kernel:   Type:   Direct-Access                      ANSI SCSI revision: 02
kernel: scsi <1:0:0:0> send                  Test Unit Ready 00 00 00 00 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Test Unit Ready 00 00 00 00 00 
kernel: scsi <1:0:0:0> send                  Read Capacity 00 00 00 00 00 00 00 00 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Read Capacity 00 00 00 00 00 00 00 00 00 
kernel: SCSI device sda: 512000 512-byte hdwr sectors (262 MB)
kernel: scsi <1:0:0:0> send                  Mode Sense (10) 00 3f 00 00 00 00 00 c0 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Mode Sense (10) 00 3f 00 00 00 00 00 c0 00 
kernel: sda: Write Protect is off
kernel: sda: assuming drive cache: write through
kernel: scsi <1:0:0:0> send                  Test Unit Ready 00 00 00 00 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Test Unit Ready 00 00 00 00 00 
kernel: scsi <1:0:0:0> send                  Prevent/Allow Medium Removal 00 00 00 01 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Prevent/Allow Medium Removal 00 00 00 01 00 
kernel:  sda:<6>scsi <1:0:0:0> send                  Read (10) 00 00 00 00 00 00 00 08 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Read (10) 00 00 00 00 00 00 00 08 00 
kernel:  sda1
kernel: scsi <1:0:0:0> send                  Prevent/Allow Medium Removal 00 00 00 00 00 
kernel: scsi <1:0:0:0> done SUCCESS        0 Prevent/Allow Medium Removal 00 00 00 00 00 

-- Patrick Mansfield

Replace the scsi_host flags with scsi_device sdev_bflags. Change USB to set sdev_bflags in its new slave_alloc function.

diff -uprN -X /home/patman/dontdiff base-2.6/drivers/scsi/scsi_devinfo.c add_sdev_bflags-2.6/drivers/scsi/scsi_devinfo.c
@@ -329,7 +329,7 @@ int scsi_get_device_flags(struct scsi_de
 	struct scsi_dev_info_list *devinfo;
 	unsigned int bflags;
 
-	bflags = sdev->host->hostt->flags;
+	bflags = sdev->sdev_bflags;
 	if (!bflags)
 		bflags = scsi_default_dev_flags;
 

diff -uprN -X /home/patman/dontdiff base-2.6/drivers/usb/storage/scsiglue.c add_sdev_bflags-2.6/drivers/usb/storage/scsiglue.c
@@ -64,6 +64,17 @@ static const char* host_info(struct Scsi
 	return "SCSI emulation for USB Mass Storage devices";
 }
 
+static int slave_alloc (struct scsi_device *sdev)
+{
+	/*
+	 * Set default bflags. These can be overridden for individual
+	 * models and vendors via the scsi devinfo mechanism.
+	 */
+	sdev->sdev_bflags = (BLIST_MS_SKIP_PAGE_08 | BLIST_MS_SKIP_PAGE_3F |
+			     BLIST_USE_10_BYTE_MS);
+	return 0;
+}
+
 static int slave_configure (struct scsi_device *sdev)
 {
 	/* Scatter-gather buffers (all but the last) must have a length

@@ -365,6 +376,7 @@ struct scsi_host_template usb_stor_host_
 	/* unknown initiator id */
 	.this_id =			-1,
 
+	.slave_alloc =			slave_alloc,
 	.slave_configure =		slave_configure,
 
 	/* lots of sg segments can be handled */

@@ -384,10 +396,6 @@ struct scsi_host_template usb_stor_host_
 
 	/* sysfs device attributes */
 	.sdev_attrs =			sysfs_device_attr_list,
-
-	/* modify scsi_device bits on probe */
-	.flags = (BLIST_MS_SKIP_PAGE_08 | BLIST_MS_SKIP_PAGE_3F |
-		  BLIST_USE_10_BYTE_MS),
 
 	/* module management */
 	.module =			THIS_MODULE

diff -uprN -X /home/patman/dontdiff base-2.6/include/scsi/scsi_device.h add_sdev_bflags-2.6/include/scsi/scsi_device.h
@@ -64,6 +64,10 @@ struct scsi_device {
 	unsigned char current_tag;	/* current tag */
 	struct scsi_target      *sdev_target;   /* used only for single_lun */
 
+	unsigned int	sdev_bflags; /* black/white flags as also found in
+				 * scsi_devinfo.[hc]. For now used only to
+				 * pass settings from slave_alloc to scsi
+				 * core. */
 	unsigned online:1;
 
 	unsigned writeable:1;

diff -uprN -X /home/patman/dontdiff base-2.6/include/scsi/scsi_host.h add_sdev_bflags-2.6/include/scsi/scsi_host.h
@@ -345,12 +345,6 @@ struct scsi_host_template {
 	 * module_init/module_exit.
 	 */
 	struct list_head legacy_hosts;
-
-	/*
-	 * Default flags settings, these modify the setting of scsi_device
-	 * bits.
-	 */
-	unsigned int flags;
 };
 
 /*

Add a BLIST_MS_192_BYTES_FOR_3F flag. If it is set, sends a 192 byte MODE SENSE in sd.c.

diff -uprN -X /home/patman/dontdiff add_sdev_bflags-2.6/drivers/scsi/scsi_scan.c 192_byte_ms-2.6/drivers/scsi/scsi_scan.c
@@ -643,6 +643,9 @@ static int scsi_add_lun(struct scsi_devi
 	if (*bflags & BLIST_USE_10_BYTE_MS)
 		sdev->use_10_for_ms = 1;
 
+	if (*bflags & BLIST_MS_192_BYTES_FOR_3F)
+		sdev->use_192_bytes_for_3f = 1;
+
 	if(sdev->host->hostt->slave_configure)
 		sdev->host->hostt->slave_configure(sdev);
 

diff -uprN -X /home/patman/dontdiff add_sdev_bflags-2.6/include/scsi/scsi_device.h 192_byte_ms-2.6/include/scsi/scsi_device.h
@@ -97,6 +97,7 @@ struct scsi_device {
 	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
 	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */
 	unsigned skip_ms_page_3f:1;	/* do not use MODE SENSE page 0x3f */
+	unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
 	unsigned no_start_on_add:1;	/* do not issue start on add */
 	unsigned allow_restart:1; /* issue START_UNIT in error handler */
 

diff -uprN -X /home/patman/dontdiff add_sdev_bflags-2.6/include/scsi/scsi_devinfo.h 192_byte_ms-2.6/include/scsi/scsi_devinfo.h
@@ -19,4 +19,5 @@
 #define BLIST_MS_SKIP_PAGE_08	0x2000	/* do not send ms page 0x08 */
 #define BLIST_MS_SKIP_PAGE_3F	0x4000	/* do not send ms page 0x3f */
 #define BLIST_USE_10_BYTE_MS	0x8000	/* use 10 byte ms before 6 byte ms */
+#define BLIST_MS_192_BYTES_FOR_3F	0x10000	/*  192 byte ms page 0x3f request */
 #endif

Great work! Thanks, Patrick.

Patch below. It's not ideal because these things depend on the device type, which isn't known until after slave_alloc(). Hence the scsi devinfo mechanism can't be used for overrides. It probably won't matter though. We might want to keep using MODE SENSE(10) always even though Windows doesn't. Nobody has complained about it so far.

There are devices that like MS(10) and dislike MS(6). I haven't seen the reverse, but maybe Matt has.

What we really need is testing by the few people whose devices couldn't handle page 3f at all, before.

Alan Stern

P.S.: A separate question has come up recently. There are error conditions in which the USB Mass Storage protocols require a device reset. Currently usb-storage has been doing its own resetting, but maybe it would be better to let the SCSI error handler take care of things. Is there a result code the driver can return to indicate that error recovery should commence immediately (no command retries)? Better yet, is there a code to indicate that the initial TEST UNIT READY should be skipped and the error handler should go straight into a device reset?

@@ -48,6 +48,7 @@
 #include "usb.h"
 #include "debug.h"
 #include "transport.h"
+#include "protocol.h"
 
 #include <linux/slab.h>
 #include <linux/module.h>

@@ -70,8 +71,15 @@
 	 * Set default bflags. These can be overridden for individual
 	 * models and vendors via the scsi devinfo mechanism.
 	 */
-	sdev->sdev_bflags = (BLIST_MS_SKIP_PAGE_08 | BLIST_MS_SKIP_PAGE_3F |
-			     BLIST_USE_10_BYTE_MS);
+
+	/* By default, USB mass storage devices use MODE SENSE(10).
+	 * Use 192 byte transfers for MODE SENSE page x3f.
+	 * We don't know yet whether MODE SENSE page x08 is reliable. */
+	sdev->sdev_bflags = (BLIST_USE_10_BYTE_MS |
+			BLIST_MS_192_BYTES_FOR_3F |
+			BLIST_MS_SKIP_PAGE_08);
+	/* Maybe add an unusual_devs flag for BLIST_MS_SKIP_PAGE_3F */
+
 	return 0;
 }
 

@@ -98,6 +106,23 @@
 	if (us->pusb_dev->descriptor.idVendor == USB_VENDOR_ID_GENESYS &&
 			us->pusb_dev->speed == USB_SPEED_HIGH)
 		blk_queue_max_sectors(sdev->request_queue, 128);
+
+	/* We can't put these settings in slave_alloc() because that gets
+	 * called before the device type is known.  Consequently these
+	 * settings can't be overridden via the scsi devinfo mechanism. */
+	if (sdev->type == TYPE_DISK) {
+
+		/* Disk-type devices use MODE SENSE(6) if the protocol
+		 * (SubClass) is Transparent SCSI */
+		if (us->subclass == US_SC_SCSI)
+			sdev->use_10_for_ms = 0;
+	} else {
+
+		/* Non-disk-type devices don't need to blacklist MS page x08
+		 * or to force 192 byte transfer lengths for MODE SENSE */
+		sdev->skip_ms_page_8 = 0;
+		sdev->use_192_bytes_for_3f = 0;
+	}
 
 	/* this is to satisify the compiler, tho I don't think the 
 	 * return code is ever checked anywhere. */

With my USB CDRW drive, Windows 2000 issued x5a = MODE SENSE(10). It did so even though the class/protocol/subclass are 08/50/06. Furthermore, testing with usb-storage showed that the drive doesn't accept MODE SENSE(6).

Alan Stern

Yes, per spec. That is:

t10.org mmc4r02h.pdf "Table 213" "Commands ... (Opcode order)" shows no definition for the response of a PDT x05 DVD/ CD drive to an assault of PDT x00 SBC ops x1A/ 5A Mode Sense/ Select (6).

t10.org spc2r20.pdf "Table 13" "Commands for all device types" via code "Z" gives this rude freedom to MMC: "Command implementation is" peripheral "device type specific".

Pat LaVarre

Three items:

1. I haven't seen much feedback regarding the suggested patches that
Patrick and I wrote:

http://marc.theaimsgroup.com/?l=linux-scsi&m=107922926816157&w=2
http://marc.theaimsgroup.com/?l=linux-scsi&m=107922905218137&w=2
http://marc.theaimsgroup.com/?l=linux-scsi&m=107945931705407&w=2

Do they look good? Should they be applied? One suggestion for a change:
the flag could be renamed "use_192_bytes_for_ms" since we may end up wanting to use it for both page 0x3f and 0x08.

Also, why does sd.c check write-enable only for removable media? Can't non-removable-media drives also have a write-protect switch?

2. Some users have reported problems with USB digital cameras that
don't support PREVENT-ALLOW MEDIUM REMOVAL and crash when they receive that command, even though they return the RMB flag set in their INQUIRY data. Their medium really _is_ removable, in the sense that you can swap memory cards while the camera is turned on and plugged in to the computer. How should we handle this?

3. When a USB mass storage device enters a state that requires a
device reset, what command completion code should the usb-storage driver return to invoke the SCSI error handler? And is there any way to do this so that the error handler skips the initial TEST UNIT READY step and goes directly to the device reset?

Alan Stern

They already are in the scsi-misc-2.6 repository.

Anything that maps to FAILED in scsi_decide_disposition()

What TEST UNIT READY? The error handler only issues those *after* recovery, not before.

James

Great! Were just the SCSI patches included or were the usb-storage changes put in as well? When can I expect to see them in a distribution kernel?

So for example, (DID_OK << 16) + (ABORT << 8) would do the trick, right? Or (0xff << 16). Is there any equivalent that expresses the intention more clearly?

Hmm... apparently that disappeared while I wasn't looking. I recall reading lots and lots of debugging logs in which the first thing that happened after a command timed out was a TEST UNIT READY. But it doesn't matter.

Alan Stern