Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
       [not found] <19395543.564391255279074897.JavaMail.root@mail.pc-doctor.com>
@ 2009-10-11 16:38 ` Ben Efros
  2009-10-11 20:51   ` Benjamin Herrenschmidt
  0 siblings, 1 reply; 15+ messages in thread
From: Ben Efros @ 2009-10-11 16:38 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Alan Stern, fangxiaozhi, Greg KH, Kernel development list,
	USB list, Hugh Blemings, Josua Dietze

----- "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote:
> On Sat, 2009-10-10 at 17:21 -0700, Ben Efros wrote:
> > 
> > Ben Herrenschmidt's patch[1] to retry without SANE_SENSE might be
> > combined be able to be used to detect this 'INSANE_SENSE' scenario,
> > but not in its current form.
> > 
> > Devices lying about the "additional sense length" doesn't seem all
> > that common, so it might be better to just flag the device as
> insane
> > and not worry about reworking Ben Herrenschmidt's patch.
> 
> I don't like flagging devices ... though we already somewhat do it
> for
> the mode switch so it would be possible to stick the flag there.
> 
> Another option is to set the insane flag from a retry path similar
> to what I posted so that it doesn't ping pong.

The more i think of it, I don't think removal of the SANE_SENSE flag is a good idea after we've made reasonable effort to detect it.  The problem is the buggy devices that trigger SANE_SENSE to be set by error in the first place because they lie about the additional sense length.

Since these Huawei devices are already listed in the unusual devices, we could add the INSANE_SENSE flag to them as I did below...  Ben Herrenschmidt, can you confirm that this works for the regression as well?


Add support for INSANE_SENSE flag to disable SANE_SENSE
detection on known buggy usb mass storage devices.

 Signed-off-by: Ben Efros <ben@pc-doctor.com>

--- linux-2.6.31.3/drivers/usb/storage/transport.c	2009-09-24 08:45:25.000000000 -0700
+++ linux-2.6.31.3/drivers/usb/storage/transport.c	2009-10-11 08:06:26.000000000 -0700
@@ -668,7 +668,7 @@ void usb_stor_invoke_transport(struct sc
 	 */
 	if ((srb->cmnd[0] == ATA_16 || srb->cmnd[0] == ATA_12) &&
 	    result == USB_STOR_TRANSPORT_GOOD &&
-	    !(us->fflags & US_FL_SANE_SENSE) &&
+	    !(us->fflags & (US_FL_SANE_SENSE|US_FL_INSANE_SENSE)) &&
 	    !(srb->cmnd[2] & 0x20)) {
 		US_DEBUGP("-- SAT supported, increasing auto-sense\n");
 		us->fflags |= US_FL_SANE_SENSE;
@@ -738,8 +744,9 @@ void usb_stor_invoke_transport(struct sc
 		 * The response code must be 70h through 73h inclusive.
 		 */
 		if (srb->sense_buffer[7] > (US_SENSE_SIZE - 8) &&
-		    !(us->fflags & US_FL_SANE_SENSE) &&
+		    !(us->fflags & (US_FL_SANE_SENSE|US_FL_INSANE_SENSE)) &&
 		    (srb->sense_buffer[0] & 0x7C) == 0x70) {
+
 			US_DEBUGP("-- SANE_SENSE support enabled\n");
 			us->fflags |= US_FL_SANE_SENSE;
 
--- linux-2.6.31.3/drivers/usb/storage/scsiglue.c	2009-09-24 08:45:25.000000000 -0700
+++ linux-2.6.31.3/drivers/usb/storage/scsiglue.c	2009-10-11 07:52:22.000000000 -0700
@@ -209,8 +209,11 @@ static int slave_configure(struct scsi_d
 		if (us->fflags & US_FL_CAPACITY_HEURISTICS)
 			sdev->guess_capacity = 1;
 
-		/* assume SPC3 or latter devices support sense size > 18 */
-		if (sdev->scsi_level > SCSI_SPC_2)
+		/* assume SPC3 or latter devices support sense size > 18
+		 * if they haven't been marked to have insane sense data
+		 */
+		if (sdev->scsi_level > SCSI_SPC_2 &&
+		    !(us->fflags & US_FL_INSANE_SENSE))
 			us->fflags |= US_FL_SANE_SENSE;
 
 		/* Some devices report a SCSI revision level above 2 but are
--- linux-2.6.31.3/include/linux/usb_usual.h	2009-09-24 08:45:25.000000000 -0700
+++ linux-2.6.31.3/include/linux/usb_usual.h	2009-10-11 07:54:06.000000000 -0700
@@ -56,7 +56,9 @@
 	US_FLAG(SANE_SENSE,     0x00008000)			\
 		/* Sane Sense (> 18 bytes) */			\
 	US_FLAG(CAPACITY_OK,	0x00010000)			\
-		/* READ CAPACITY response is correct */
+		/* READ CAPACITY response is correct */         \
+	US_FLAG(INSANE_SENSE,   0x00020000)                     \
+		/* Device does not support SANE_SENSE (> 18 bytes) */
 
 #define US_FLAG(name, value)	US_FL_##name = value ,
 enum { US_DO_ALL_FLAGS };
--- linux-2.6.31.3/drivers/usb/storage/unusual_devs.h	2009-10-11 08:57:08.000000000 -0700
+++ linux-2.6.31.3/drivers/usb/storage/unusual_devs.h	2009-10-11 08:59:34.000000000 -0700
@@ -1422,332 +1422,332 @@ UNUSUAL_DEV(  0x12d1, 0x1001, 0x0000, 0x
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1003, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1004, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1401, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1402, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1403, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1404, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1405, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1406, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1407, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1408, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1409, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x140A, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x140B, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x140C, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x140D, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x140E, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x140F, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1410, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1411, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1412, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1413, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1414, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1415, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1416, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1417, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1418, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1419, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x141A, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x141B, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x141C, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x141D, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x141E, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x141F, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1420, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1421, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1422, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1423, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1424, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1425, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1426, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1427, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1428, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1429, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x142A, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x142B, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x142C, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x142D, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x142E, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x142F, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1430, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1431, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1432, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1433, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1434, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1435, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1436, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1437, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1438, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x1439, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x143A, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x143B, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x143C, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x143D, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x143E, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 UNUSUAL_DEV(  0x12d1, 0x143F, 0x0000, 0x0000,
 		"HUAWEI MOBILE",
 		"Mass Storage",
 		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
-		0),
+		US_FL_INSANE_SENSE),
 
 /* Reported by Vilius Bilinkevicius <vilisas AT xxx DOT lt) */
 UNUSUAL_DEV(  0x132b, 0x000b, 0x0001, 0x0001,

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-11 16:38 ` USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH] Ben Efros
@ 2009-10-11 20:51   ` Benjamin Herrenschmidt
  2009-10-12  1:54     ` Alan Stern
  0 siblings, 1 reply; 15+ messages in thread
From: Benjamin Herrenschmidt @ 2009-10-11 20:51 UTC (permalink / raw)
  To: Ben Efros
  Cc: Alan Stern, fangxiaozhi, Greg KH, Kernel development list,
	USB list, Hugh Blemings, Josua Dietze


> The more i think of it, I don't think removal of the SANE_SENSE flag is a good idea after we've made reasonable effort to detect it.  The problem is the buggy devices that trigger SANE_SENSE to be set by error in the first place because they lie about the additional sense length.
> 
> Since these Huawei devices are already listed in the unusual devices, we could add the INSANE_SENSE flag to them as I did below...  Ben Herrenschmidt, can you confirm that this works for the regression as well?
> 
> 
> Add support for INSANE_SENSE flag to disable SANE_SENSE
> detection on known buggy usb mass storage devices.

Don't those devices support SANE_SENSE fine on LUN 0 according to
Alan parsing of my traces ? Heh, they may even work if I actually
populate the SD-CARD slot, I'll try later.

I still think it's simpler and potentially more future-proof to just
have a back-off scenario.

Now, whether we ping pong the US_FL_SANE_SENSE flag or not is minor,
I tend to think that your approach that doesn't touch the flag might
indeed be better.

I'll test the patch later today.

Cheers,
Ben.

>  Signed-off-by: Ben Efros <ben@pc-doctor.com>
> 
> --- linux-2.6.31.3/drivers/usb/storage/transport.c	2009-09-24 08:45:25.000000000 -0700
> +++ linux-2.6.31.3/drivers/usb/storage/transport.c	2009-10-11 08:06:26.000000000 -0700
> @@ -668,7 +668,7 @@ void usb_stor_invoke_transport(struct sc
>  	 */
>  	if ((srb->cmnd[0] == ATA_16 || srb->cmnd[0] == ATA_12) &&
>  	    result == USB_STOR_TRANSPORT_GOOD &&
> -	    !(us->fflags & US_FL_SANE_SENSE) &&
> +	    !(us->fflags & (US_FL_SANE_SENSE|US_FL_INSANE_SENSE)) &&
>  	    !(srb->cmnd[2] & 0x20)) {
>  		US_DEBUGP("-- SAT supported, increasing auto-sense\n");
>  		us->fflags |= US_FL_SANE_SENSE;
> @@ -738,8 +744,9 @@ void usb_stor_invoke_transport(struct sc
>  		 * The response code must be 70h through 73h inclusive.
>  		 */
>  		if (srb->sense_buffer[7] > (US_SENSE_SIZE - 8) &&
> -		    !(us->fflags & US_FL_SANE_SENSE) &&
> +		    !(us->fflags & (US_FL_SANE_SENSE|US_FL_INSANE_SENSE)) &&
>  		    (srb->sense_buffer[0] & 0x7C) == 0x70) {
> +
>  			US_DEBUGP("-- SANE_SENSE support enabled\n");
>  			us->fflags |= US_FL_SANE_SENSE;
>  
> --- linux-2.6.31.3/drivers/usb/storage/scsiglue.c	2009-09-24 08:45:25.000000000 -0700
> +++ linux-2.6.31.3/drivers/usb/storage/scsiglue.c	2009-10-11 07:52:22.000000000 -0700
> @@ -209,8 +209,11 @@ static int slave_configure(struct scsi_d
>  		if (us->fflags & US_FL_CAPACITY_HEURISTICS)
>  			sdev->guess_capacity = 1;
>  
> -		/* assume SPC3 or latter devices support sense size > 18 */
> -		if (sdev->scsi_level > SCSI_SPC_2)
> +		/* assume SPC3 or latter devices support sense size > 18
> +		 * if they haven't been marked to have insane sense data
> +		 */
> +		if (sdev->scsi_level > SCSI_SPC_2 &&
> +		    !(us->fflags & US_FL_INSANE_SENSE))
>  			us->fflags |= US_FL_SANE_SENSE;
>  
>  		/* Some devices report a SCSI revision level above 2 but are
> --- linux-2.6.31.3/include/linux/usb_usual.h	2009-09-24 08:45:25.000000000 -0700
> +++ linux-2.6.31.3/include/linux/usb_usual.h	2009-10-11 07:54:06.000000000 -0700
> @@ -56,7 +56,9 @@
>  	US_FLAG(SANE_SENSE,     0x00008000)			\
>  		/* Sane Sense (> 18 bytes) */			\
>  	US_FLAG(CAPACITY_OK,	0x00010000)			\
> -		/* READ CAPACITY response is correct */
> +		/* READ CAPACITY response is correct */         \
> +	US_FLAG(INSANE_SENSE,   0x00020000)                     \
> +		/* Device does not support SANE_SENSE (> 18 bytes) */
>  
>  #define US_FLAG(name, value)	US_FL_##name = value ,
>  enum { US_DO_ALL_FLAGS };
> --- linux-2.6.31.3/drivers/usb/storage/unusual_devs.h	2009-10-11 08:57:08.000000000 -0700
> +++ linux-2.6.31.3/drivers/usb/storage/unusual_devs.h	2009-10-11 08:59:34.000000000 -0700
> @@ -1422,332 +1422,332 @@ UNUSUAL_DEV(  0x12d1, 0x1001, 0x0000, 0x
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1003, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1004, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1401, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1402, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1403, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1404, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1405, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1406, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1407, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1408, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1409, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x140A, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x140B, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x140C, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x140D, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x140E, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x140F, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1410, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1411, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1412, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1413, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1414, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1415, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1416, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1417, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1418, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1419, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x141A, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x141B, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x141C, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x141D, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x141E, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x141F, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1420, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1421, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1422, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1423, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1424, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1425, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1426, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1427, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1428, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1429, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x142A, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x142B, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x142C, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x142D, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x142E, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x142F, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1430, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1431, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1432, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1433, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1434, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1435, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1436, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1437, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1438, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x1439, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x143A, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x143B, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x143C, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x143D, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x143E, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  UNUSUAL_DEV(  0x12d1, 0x143F, 0x0000, 0x0000,
>  		"HUAWEI MOBILE",
>  		"Mass Storage",
>  		US_SC_DEVICE, US_PR_DEVICE, usb_stor_huawei_e220_init,
> -		0),
> +		US_FL_INSANE_SENSE),
>  
>  /* Reported by Vilius Bilinkevicius <vilisas AT xxx DOT lt) */
>  UNUSUAL_DEV(  0x132b, 0x000b, 0x0001, 0x0001,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-11 20:51   ` Benjamin Herrenschmidt
@ 2009-10-12  1:54     ` Alan Stern
  2009-10-12  3:28       ` Benjamin Herrenschmidt
  0 siblings, 1 reply; 15+ messages in thread
From: Alan Stern @ 2009-10-12  1:54 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Ben Efros, fangxiaozhi, Greg KH, Kernel development list,
	USB list, Hugh Blemings, Josua Dietze

On Mon, 12 Oct 2009, Benjamin Herrenschmidt wrote:

> Don't those devices support SANE_SENSE fine on LUN 0 according to
> Alan parsing of my traces ?

Not really; the device returned only the usual 18 bytes of sense data.  
But at least it did so correctly instead of failing.

>  Heh, they may even work if I actually
> populate the SD-CARD slot, I'll try later.

Possibly, but I sure wouldn't bet on it.   :-)

> I still think it's simpler and potentially more future-proof to just
> have a back-off scenario.
> 
> Now, whether we ping pong the US_FL_SANE_SENSE flag or not is minor,
> I tend to think that your approach that doesn't touch the flag might
> indeed be better.
> 
> I'll test the patch later today.

And of course there remains the question of why the serial driver
wasn't loaded and probed.  Would it make a difference if that driver
was already loaded when you plugged in the modem?

Alan Stern


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-12  1:54     ` Alan Stern
@ 2009-10-12  3:28       ` Benjamin Herrenschmidt
  2009-10-12 14:27         ` Alan Stern
  0 siblings, 1 reply; 15+ messages in thread
From: Benjamin Herrenschmidt @ 2009-10-12  3:28 UTC (permalink / raw)
  To: Alan Stern
  Cc: Ben Efros, fangxiaozhi, Greg KH, Kernel development list,
	USB list, Hugh Blemings, Josua Dietze

On Sun, 2009-10-11 at 21:54 -0400, Alan Stern wrote:
> Possibly, but I sure wouldn't bet on it.   :-)
> 
> > I still think it's simpler and potentially more future-proof to just
> > have a back-off scenario.
> > 
> > Now, whether we ping pong the US_FL_SANE_SENSE flag or not is minor,
> > I tend to think that your approach that doesn't touch the flag might
> > indeed be better.
> > 
> > I'll test the patch later today.
> 
> And of course there remains the question of why the serial driver
> wasn't loaded and probed.  Would it make a difference if that driver
> was already loaded when you plugged in the modem? 

Without my hack, the serial driver -was- loaded an probed ... but the
device would go away as soon as the reset happened triggered by
usb-storage.

BTW. I noticed that USB storage is supposed to avoid doing the reset if
the device is multifunction. I suppose that flag gets set prior to the
"mode switch" and so the reset happens regardless... maybe we should set
that MF flag from the quirk that sends the mode switch ?

With my hack, I don't know what happened the first time around, it
worked fine afterward. I'll let you know if it happens again. Could be a
userspace glitch (I'm running Ubuntu Karmic, it's still a bit wet behind
the ears :-)

Cheers,
Ben.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-12  3:28       ` Benjamin Herrenschmidt
@ 2009-10-12 14:27         ` Alan Stern
  0 siblings, 0 replies; 15+ messages in thread
From: Alan Stern @ 2009-10-12 14:27 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Matthew Dharm
  Cc: Ben Efros, fangxiaozhi, Greg KH, Kernel development list,
	USB list, Hugh Blemings, Josua Dietze

On Mon, 12 Oct 2009, Benjamin Herrenschmidt wrote:

> BTW. I noticed that USB storage is supposed to avoid doing the reset if
> the device is multifunction.

Not "multifunction" but "multitarget" -- which means that the device is
attached to a real SCSI bus which may have more than one target.  In
such a situation it's best to avoid bus resets when possible, since
they affect all the targets.

>  I suppose that flag gets set prior to the
> "mode switch" and so the reset happens regardless... maybe we should set
> that MF flag from the quirk that sends the mode switch ?

You mean, avoid doing a reset after an auto-sense failure if the device
has other interfaces?  Maybe...  I'm not really sure.  To tell the
truth, that comment about "failure of an auto-sense is perfectly valid"  
doesn't make much sense to me.  In principle, the SCSI core would have
to issue its own REQUEST SENSE command and the end result would be the
same.

(In your case that wouldn't happen, because the device actually did
send valid sense data before sending the failure code.  The SCSI core
would see that data sitting in the buffer and believe it.  So things
would work out okay, but only by coincidence, not by design -- unless 
we wipe the buffer before returning.)

I don't know.  Maybe it really would be best not to reset after an 
auto-sense failure, ever.  I can't recall the issue coming up in any 
bug reports before.  But then what should we do after an auto-sense 
error?  It probably should be treated the same as a normal error, which 
means doing a reset.

Alan Stern

^ permalink raw reply	[flat|nested] 15+ messages in thread

[parent not found: <26767470.564301255277612694.JavaMail.root@mail.pc-doctor.com>]

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
       [not found] <26767470.564301255277612694.JavaMail.root@mail.pc-doctor.com>
@ 2009-10-11 16:16 ` Ben Efros
  2009-10-12  5:44   ` Benjamin Herrenschmidt
  0 siblings, 1 reply; 15+ messages in thread
From: Ben Efros @ 2009-10-11 16:16 UTC (permalink / raw)
  To: Alan Stern
  Cc: Josua Dietze, fangxiaozhi, Greg KH, Kernel development list,
	USB list, Hugh Blemings, Benjamin Herrenschmidt


----- "Alan Stern" <stern@rowland.harvard.edu> wrote:
> On Sun, 11 Oct 2009, Benjamin Herrenschmidt wrote:
> 
> > 
> > > Further REQUEST SENSE commands therefore requested 96 bytes of
> data
> > > instead of the standard 18 bytes.  With LUN 0 this worked okay. 
> But 
> > > with LUN 1 it didn't; the device reported a failure of the REQUEST
> 
> > > SENSE.  This is what caused usb-storage to issue the device
> reset.
> > > 
> > > After the reset usb-storage continued to ask for 96 bytes of
> sense
> > > data, and LUN 1 continued to fail the commands.  Hence the
> repeated
> > > resets.
> > 
> > Maybe a better approach would be to go back to 18 bytes when it
> fails,
> > what do you think ?
> 
> We certainly could do that.  But should we turn off the SANE_SENSE
> flag 
> at the same time?

No I don't think its a good idea to turn off SANE_SENSE in this situation.  Here is a patch similar to Ben Herrenschmidt's but will not turn off SANE_SENSE just because a transport failure.


Retry with short sense when SANE_SENSE fails.

 Signed-off-by: Ben Efros <ben@pc-doctor.com>

--- linux-2.6.31.1/drivers/usb/storage/transport.c        2009-09-24 08:45:25.000000000 -0700
+++ linux-2.6.31.1.new/drivers/usb/storage/transport.c        2009-10-11 08:06:26.000000000 -0700
@@ -696,7 +696,7 @@ void usb_stor_invoke_transport(struct sc
                 /* device supports and needs bigger sense buffer */
                 if (us->fflags & US_FL_SANE_SENSE)
                         sense_size = ~0;
-
+Retry_Sense:
                 US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
 
                 scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size);
@@ -723,6 +723,12 @@ void usb_stor_invoke_transport(struct sc
                 if (temp_result != USB_STOR_TRANSPORT_GOOD) {
                         US_DEBUGP("-- auto-sense failure\n");
 
+                        if ((us->fflags & US_FL_SANE_SENSE) &&
+                            sense_size != US_SENSE_SIZE) {
+                                sense_size = US_SENSE_SIZE;
+                                US_DEBUGP("-- retry without SANE_SENSE\n");
+                                goto Retry_Sense;
+                        }
                         /* we skip the reset if this happens to be a
                          * multi-target device, since failure of an
                          * auto-sense is perfectly valid



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-11 16:16 ` Ben Efros
@ 2009-10-12  5:44   ` Benjamin Herrenschmidt
  2009-10-12 14:58     ` Alan Stern
  0 siblings, 1 reply; 15+ messages in thread
From: Benjamin Herrenschmidt @ 2009-10-12  5:44 UTC (permalink / raw)
  To: Ben Efros
  Cc: Alan Stern, Josua Dietze, fangxiaozhi, Greg KH,
	Kernel development list, USB list, Hugh Blemings

On Sun, 2009-10-11 at 09:16 -0700, Ben Efros wrote:

> No I don't think its a good idea to turn off SANE_SENSE in this situation.
> Here is a patch similar to Ben Herrenschmidt's but will not turn off SANE_SENSE
> just because a transport failure.
>
> Retry with short sense when SANE_SENSE fails.

Works for me too.

Alan, what do you think ? I definitely don't like Ben E's most recent
patch with a quirk for all devices, it's simply a lot more code for
something that will come back and bite again when somebody does the
same mistake again. I'd rather have the request sense code be more
robust. But this patch is fine, as was my previous one.

So it boils down on clearing SANE_SENSE vs. not clearing it. If we
clear it, we probably want to keep it cleared (via an INSANE_SENSE
flag ?). But on the other hand, I don't think that always going
for a retry when a SANE_SENSE fails is going to hurt and sounds
like the robust thing to do, so I don't mind that simple patch
from Ben. So up to you :-)

Cheers,
Ben.

>  Signed-off-by: Ben Efros <ben@pc-doctor.com>

Tested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> --- linux-2.6.31.1/drivers/usb/storage/transport.c        2009-09-24 08:45:25.000000000 -0700
> +++ linux-2.6.31.1.new/drivers/usb/storage/transport.c        2009-10-11 08:06:26.000000000 -0700
> @@ -696,7 +696,7 @@ void usb_stor_invoke_transport(struct sc
>                  /* device supports and needs bigger sense buffer */
>                  if (us->fflags & US_FL_SANE_SENSE)
>                          sense_size = ~0;
> -
> +Retry_Sense:
>                  US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
>  
>                  scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size);
> @@ -723,6 +723,12 @@ void usb_stor_invoke_transport(struct sc
>                  if (temp_result != USB_STOR_TRANSPORT_GOOD) {
>                          US_DEBUGP("-- auto-sense failure\n");
>  
> +                        if ((us->fflags & US_FL_SANE_SENSE) &&
> +                            sense_size != US_SENSE_SIZE) {
> +                                sense_size = US_SENSE_SIZE;
> +                                US_DEBUGP("-- retry without SANE_SENSE\n");
> +                                goto Retry_Sense;
> +                        }
>                          /* we skip the reset if this happens to be a
>                           * multi-target device, since failure of an
>                           * auto-sense is perfectly valid
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-12  5:44   ` Benjamin Herrenschmidt
@ 2009-10-12 14:58     ` Alan Stern
  2009-10-12 15:12       ` Matthew Dharm
                         ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Alan Stern @ 2009-10-12 14:58 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Ben Efros, Josua Dietze, fangxiaozhi, Greg KH,
	Kernel development list, USB list, Hugh Blemings

On Mon, 12 Oct 2009, Benjamin Herrenschmidt wrote:

> On Sun, 2009-10-11 at 09:16 -0700, Ben Efros wrote:
> 
> > No I don't think its a good idea to turn off SANE_SENSE in this situation.
> > Here is a patch similar to Ben Herrenschmidt's but will not turn off SANE_SENSE
> > just because a transport failure.
> >
> > Retry with short sense when SANE_SENSE fails.
> 
> Works for me too.
> 
> Alan, what do you think ? I definitely don't like Ben E's most recent
> patch with a quirk for all devices, it's simply a lot more code for
> something that will come back and bite again when somebody does the
> same mistake again. I'd rather have the request sense code be more
> robust. But this patch is fine, as was my previous one.

I agree that it seems silly to have a flag _for_ SANE_SENSE and another 
flag _against_ SANE_SENSE.  Retrying seems easier and more robust.

> So it boils down on clearing SANE_SENSE vs. not clearing it. If we
> clear it, we probably want to keep it cleared (via an INSANE_SENSE
> flag ?). But on the other hand, I don't think that always going
> for a retry when a SANE_SENSE fails is going to hurt and sounds
> like the robust thing to do, so I don't mind that simple patch
> from Ben. So up to you :-)

I agree; it won't hurt much and only if the device is buggy to begin 
with.

> Cheers,
> Ben.
> 
> >  Signed-off-by: Ben Efros <ben@pc-doctor.com>
> 
> Tested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> > --- linux-2.6.31.1/drivers/usb/storage/transport.c        2009-09-24 08:45:25.000000000 -0700
> > +++ linux-2.6.31.1.new/drivers/usb/storage/transport.c        2009-10-11 08:06:26.000000000 -0700
> > @@ -696,7 +696,7 @@ void usb_stor_invoke_transport(struct sc
> >                  /* device supports and needs bigger sense buffer */
> >                  if (us->fflags & US_FL_SANE_SENSE)
> >                          sense_size = ~0;
> > -
> > +Retry_Sense:
> >                  US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
> >  
> >                  scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size);
> > @@ -723,6 +723,12 @@ void usb_stor_invoke_transport(struct sc
> >                  if (temp_result != USB_STOR_TRANSPORT_GOOD) {
> >                          US_DEBUGP("-- auto-sense failure\n");
> >  
> > +                        if ((us->fflags & US_FL_SANE_SENSE) &&
> > +                            sense_size != US_SENSE_SIZE) {
> > +                                sense_size = US_SENSE_SIZE;
> > +                                US_DEBUGP("-- retry without SANE_SENSE\n");
> > +                                goto Retry_Sense;
> > +                        }

Except that this is wrong.  We can retry if temp_result == 
USB_STOR_TRANSPORT_FAILURE, but we should not retry if temp_result ==
USB_STOR_TRANSPORT_ERROR.

Alan Stern

P.S.: In case you don't already know...  TRANSPORT_FAILURE means we
were able to communicate with the device, and it told us that it
couldn't carry out the command.  TRANSPORT_ERROR means we weren't able
to communicate with the device.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-12 14:58     ` Alan Stern
@ 2009-10-12 15:12       ` Matthew Dharm
  2009-10-12 15:18       ` Ben Efros
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 15+ messages in thread
From: Matthew Dharm @ 2009-10-12 15:12 UTC (permalink / raw)
  To: Alan Stern
  Cc: Benjamin Herrenschmidt, Ben Efros, Josua Dietze, fangxiaozhi,
	Greg KH, Kernel development list, USB list, Hugh Blemings

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

On Mon, Oct 12, 2009 at 10:58:40AM -0400, Alan Stern wrote:
> On Mon, 12 Oct 2009, Benjamin Herrenschmidt wrote:
> > 
> > Alan, what do you think ? I definitely don't like Ben E's most recent
> > patch with a quirk for all devices, it's simply a lot more code for
> > something that will come back and bite again when somebody does the
> > same mistake again. I'd rather have the request sense code be more
> > robust. But this patch is fine, as was my previous one.
> 
> I agree that it seems silly to have a flag _for_ SANE_SENSE and another 
> flag _against_ SANE_SENSE.  Retrying seems easier and more robust.

Dualing flags, where one is auto-set and the other quirked, is almost
guaranteed to get us into a maintance nightmare.

> > So it boils down on clearing SANE_SENSE vs. not clearing it. If we
> > clear it, we probably want to keep it cleared (via an INSANE_SENSE
> > flag ?). But on the other hand, I don't think that always going
> > for a retry when a SANE_SENSE fails is going to hurt and sounds
> > like the robust thing to do, so I don't mind that simple patch
> > from Ben. So up to you :-)
> 
> I agree; it won't hurt much and only if the device is buggy to begin 
> with.

I agree; the extra retry is more robust, more straightforward, and more
maintainable long-term.

Matt

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

My mother not mind to die for stoppink Windows NT!  She is rememberink 
Stalin!
					-- Pitr
User Friendly, 9/6/1998

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-12 14:58     ` Alan Stern
  2009-10-12 15:12       ` Matthew Dharm
@ 2009-10-12 15:18       ` Ben Efros
  2009-10-12 21:19       ` Benjamin Herrenschmidt
  2009-10-13  4:53       ` Benjamin Herrenschmidt
  3 siblings, 0 replies; 15+ messages in thread
From: Ben Efros @ 2009-10-12 15:18 UTC (permalink / raw)
  To: Alan Stern
  Cc: Josua Dietze, fangxiaozhi, Greg KH, Kernel development list,
	USB list, Hugh Blemings, Benjamin Herrenschmidt


----- "Alan Stern" <stern@rowland.harvard.edu> wrote:

> > >  Signed-off-by: Ben Efros <ben@pc-doctor.com>
> > 
> > Tested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > 
> > > --- linux-2.6.31.1/drivers/usb/storage/transport.c       
> 2009-09-24 08:45:25.000000000 -0700
> > > +++ linux-2.6.31.1.new/drivers/usb/storage/transport.c       
> 2009-10-11 08:06:26.000000000 -0700
> > > @@ -696,7 +696,7 @@ void usb_stor_invoke_transport(struct sc
> > >                  /* device supports and needs bigger sense buffer
> */
> > >                  if (us->fflags & US_FL_SANE_SENSE)
> > >                          sense_size = ~0;
> > > -
> > > +Retry_Sense:
> > >                  US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
> > >  
> > >                  scsi_eh_prep_cmnd(srb, &ses, NULL, 0,
> sense_size);
> > > @@ -723,6 +723,12 @@ void usb_stor_invoke_transport(struct sc
> > >                  if (temp_result != USB_STOR_TRANSPORT_GOOD) {
> > >                          US_DEBUGP("-- auto-sense failure\n");
> > >  
> > > +                        if ((us->fflags & US_FL_SANE_SENSE) &&
> > > +                            sense_size != US_SENSE_SIZE) {
> > > +                                sense_size = US_SENSE_SIZE;
> > > +                                US_DEBUGP("-- retry without
> SANE_SENSE\n");
> > > +                                goto Retry_Sense;
> > > +                        }
> 
> Except that this is wrong.  We can retry if temp_result == 
> USB_STOR_TRANSPORT_FAILURE, but we should not retry if temp_result ==
> USB_STOR_TRANSPORT_ERROR.
> 

Agreed.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-12 14:58     ` Alan Stern
  2009-10-12 15:12       ` Matthew Dharm
  2009-10-12 15:18       ` Ben Efros
@ 2009-10-12 21:19       ` Benjamin Herrenschmidt
  2009-10-13  4:53       ` Benjamin Herrenschmidt
  3 siblings, 0 replies; 15+ messages in thread
From: Benjamin Herrenschmidt @ 2009-10-12 21:19 UTC (permalink / raw)
  To: Alan Stern
  Cc: Ben Efros, Josua Dietze, fangxiaozhi, Greg KH,
	Kernel development list, USB list, Hugh Blemings

On Mon, 2009-10-12 at 10:58 -0400, Alan Stern wrote:
> Except that this is wrong.  We can retry if temp_result == 
> USB_STOR_TRANSPORT_FAILURE, but we should not retry if temp_result ==
> USB_STOR_TRANSPORT_ERROR.
> 
> Alan Stern
> 
> P.S.: In case you don't already know...  TRANSPORT_FAILURE means we
> were able to communicate with the device, and it told us that it
> couldn't carry out the command.  TRANSPORT_ERROR means we weren't able
> to communicate with the device.

Ah good, I was wondering about precisely that (discriminating the errors
more intelligently). No point retrying if the device was yanked or went
totally dead.

Cheers,
Ben.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-12 14:58     ` Alan Stern
                         ` (2 preceding siblings ...)
  2009-10-12 21:19       ` Benjamin Herrenschmidt
@ 2009-10-13  4:53       ` Benjamin Herrenschmidt
  2009-10-13 14:03         ` Alan Stern
  3 siblings, 1 reply; 15+ messages in thread
From: Benjamin Herrenschmidt @ 2009-10-13  4:53 UTC (permalink / raw)
  To: Alan Stern
  Cc: Ben Efros, Josua Dietze, fangxiaozhi, Greg KH,
	Kernel development list, USB list, Hugh Blemings

usb-storage: Workaround devices with bogus sense size

Some devices, such as Huawei E169, advertise more than the standard
amount of sense data, causing us to set US_FL_SANE_SENSE, assuming
they support it. However, they subsequently fail the request sense
with that size.

This works around it generically. When a sense request fails due to
a device returning an error, US_FL_SANE_SENSE was set, and that sense
request used a larger sense size, we retry with a smaller size before
giving up.

Based on an original patch by Ben Efros <ben@pc-doctor.com>

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

This should also be applied to 2.6.31 stable

diff -urN linux-source-2.6.31-11-benh/drivers/usb/storage/transport.c linux-source-2.6.31-12-benh/drivers/usb/storage/transport.c
--- linux-source-2.6.31-11-benh/drivers/usb/storage/transport.c	2009-09-10 08:13:59.000000000 +1000
+++ linux-source-2.6.31-12-benh/drivers/usb/storage/transport.c	2009-10-13 15:44:57.117722653 +1100
@@ -696,7 +696,7 @@
 		/* device supports and needs bigger sense buffer */
 		if (us->fflags & US_FL_SANE_SENSE)
 			sense_size = ~0;
-
+Retry_Sense:
 		US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
 
 		scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size);
@@ -720,6 +720,21 @@
 			srb->result = DID_ABORT << 16;
 			goto Handle_Errors;
 		}
+
+		/* Some devices claim to support larger sense but fail when
+		 * trying to request it. When a transport failure happens
+		 * using US_FS_SANE_SENSE, we always retry with a standard
+		 * (small) sense request. This fixes some USB GSM modems
+		 */
+		if (temp_result == USB_STOR_TRANSPORT_FAILED &&
+		    (us->fflags & US_FL_SANE_SENSE) &&
+		    sense_size != US_SENSE_SIZE) {
+			US_DEBUGP("-- auto-sense failure, retry small sense\n");
+			sense_size = US_SENSE_SIZE;
+			goto Retry_Sense;
+		}
+
+		/* Other failures */
 		if (temp_result != USB_STOR_TRANSPORT_GOOD) {
 			US_DEBUGP("-- auto-sense failure\n");
 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-13  4:53       ` Benjamin Herrenschmidt
@ 2009-10-13 14:03         ` Alan Stern
  2009-10-13 23:30           ` Benjamin Herrenschmidt
  0 siblings, 1 reply; 15+ messages in thread
From: Alan Stern @ 2009-10-13 14:03 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Ben Efros, Josua Dietze, fangxiaozhi, Greg KH,
	Kernel development list, USB list, Hugh Blemings

On Tue, 13 Oct 2009, Benjamin Herrenschmidt wrote:

> usb-storage: Workaround devices with bogus sense size
> 
> Some devices, such as Huawei E169, advertise more than the standard
> amount of sense data, causing us to set US_FL_SANE_SENSE, assuming
> they support it. However, they subsequently fail the request sense
> with that size.
> 
> This works around it generically. When a sense request fails due to
> a device returning an error, US_FL_SANE_SENSE was set, and that sense
> request used a larger sense size, we retry with a smaller size before
> giving up.
> 
> Based on an original patch by Ben Efros <ben@pc-doctor.com>
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Acked-by: Alan Stern <stern@rowland.harvard.edu>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-13 14:03         ` Alan Stern
@ 2009-10-13 23:30           ` Benjamin Herrenschmidt
  2009-10-14  2:29             ` Greg KH
  0 siblings, 1 reply; 15+ messages in thread
From: Benjamin Herrenschmidt @ 2009-10-13 23:30 UTC (permalink / raw)
  To: Alan Stern
  Cc: Ben Efros, Josua Dietze, fangxiaozhi, Greg KH,
	Kernel development list, USB list, Hugh Blemings

On Tue, 2009-10-13 at 10:03 -0400, Alan Stern wrote:
> On Tue, 13 Oct 2009, Benjamin Herrenschmidt wrote:
> 
> > usb-storage: Workaround devices with bogus sense size
> > 
> > Some devices, such as Huawei E169, advertise more than the standard
> > amount of sense data, causing us to set US_FL_SANE_SENSE, assuming
> > they support it. However, they subsequently fail the request sense
> > with that size.
> > 
> > This works around it generically. When a sense request fails due to
> > a device returning an error, US_FL_SANE_SENSE was set, and that sense
> > request used a larger sense size, we retry with a smaller size before
> > giving up.
> > 
> > Based on an original patch by Ben Efros <ben@pc-doctor.com>
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>

Cool. Who's going to commit it ? Greg ? Or should I send it to Linus ?

Cheers,
Ben


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH]
  2009-10-13 23:30           ` Benjamin Herrenschmidt
@ 2009-10-14  2:29             ` Greg KH
  0 siblings, 0 replies; 15+ messages in thread
From: Greg KH @ 2009-10-14  2:29 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Alan Stern, Ben Efros, Josua Dietze, fangxiaozhi,
	Kernel development list, USB list, Hugh Blemings

On Wed, Oct 14, 2009 at 10:30:06AM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2009-10-13 at 10:03 -0400, Alan Stern wrote:
> > On Tue, 13 Oct 2009, Benjamin Herrenschmidt wrote:
> > 
> > > usb-storage: Workaround devices with bogus sense size
> > > 
> > > Some devices, such as Huawei E169, advertise more than the standard
> > > amount of sense data, causing us to set US_FL_SANE_SENSE, assuming
> > > they support it. However, they subsequently fail the request sense
> > > with that size.
> > > 
> > > This works around it generically. When a sense request fails due to
> > > a device returning an error, US_FL_SANE_SENSE was set, and that sense
> > > request used a larger sense size, we retry with a smaller size before
> > > giving up.
> > > 
> > > Based on an original patch by Ben Efros <ben@pc-doctor.com>
> > > 
> > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > 
> > Acked-by: Alan Stern <stern@rowland.harvard.edu>
> 
> Cool. Who's going to commit it ? Greg ? Or should I send it to Linus ?

I will, I'll send it off tomorrow.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2009-10-14  2:34 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <19395543.564391255279074897.JavaMail.root@mail.pc-doctor.com>
2009-10-11 16:38 ` USB serial regression 2.6.31.1 -> 2.6.31.2 [PATCH] Ben Efros
2009-10-11 20:51   ` Benjamin Herrenschmidt
2009-10-12  1:54     ` Alan Stern
2009-10-12  3:28       ` Benjamin Herrenschmidt
2009-10-12 14:27         ` Alan Stern
     [not found] <26767470.564301255277612694.JavaMail.root@mail.pc-doctor.com>
2009-10-11 16:16 ` Ben Efros
2009-10-12  5:44   ` Benjamin Herrenschmidt
2009-10-12 14:58     ` Alan Stern
2009-10-12 15:12       ` Matthew Dharm
2009-10-12 15:18       ` Ben Efros
2009-10-12 21:19       ` Benjamin Herrenschmidt
2009-10-13  4:53       ` Benjamin Herrenschmidt
2009-10-13 14:03         ` Alan Stern
2009-10-13 23:30           ` Benjamin Herrenschmidt
2009-10-14  2:29             ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox