Re: [PATCH] JMicron JM20337 USB-SATA data corruption bugfix - device 152d:2338

[Robert Hancock <hancockr@shaw.ca>]

(adding CCs)

Tomas Styblo wrote:
> 
> Hello,
> 
> this message includes a patch that provides a workaround for
> a silent data corruption bug caused by incorrect error handling in
> the JMicron JM20337 Hi-Speed USB to SATA & PATA Combo Bridge chipset,
> USB device id 152d:2338.
> 
> 
> - the problem occurs quite rarely, approx. once for 
>   every 20 GB of transfered data during heavy load
> 
> - it seems that only read operations are affected
> 
> - the problem is accompanied by these messages in syslog each
>   time it occurs:
> 
> May 17 15:06:56 kernel: sd 6:0:0:0: [sdb] Sense Key : 0x0 [current] 
> May 17 15:06:56 kernel: sd 6:0:0:0: [sdb] ASC=0x0 ASCQ=0x0
> 
> - the bug is not detected as an error and incorrect data is returned, 
>   causing insidious data corruption
> 
> - tested with 3 external disk enclosures (Akasa Integral AK-ENP2SATA-BL) 
>   with different disks on different computers, with kernel 2.6.24 and 2.6.25
> 
> - the patch provides a crude workaround by detecting the error condition
>   and retrying the faulty transfer
> 
> 
> The fix needs a review as I don't know much about USB and SCSI.  
> It's possible that this approach is wrong and that the problem should
> be fixed somewhere else.
> 
> There are other problems with this chipset that make it necessary 
> to disconnect and power off the enclosure from time to time, but at least
> there's no data corruption anymore.

I'm not sure this is a good approach. More that this code right above in 
usb_stor_invoke_transport, which your code undoes the effect of for this 
device, doesn't seem right:

	/* If things are really okay, then let's show that.  Zero
	 * out the sense buffer so the higher layers won't realize
	 * we did an unsolicited auto-sense. */
	if (result == USB_STOR_TRANSPORT_GOOD &&
		/* Filemark 0, ignore EOM, ILI 0, no sense */
			(srb->sense_buffer[2] & 0xaf) == 0 &&
		/* No ASC or ASCQ */
			srb->sense_buffer[12] == 0 &&
			srb->sense_buffer[13] == 0) {
		srb->result = SAM_STAT_GOOD;
		srb->sense_buffer[0] = 0x0;
	}

So if the transport initially gets a failure, but then request sense 
doesn't show any error, we just go "hmm, guess it was ok after all". 
That seems kind of dangerous, I shouldn't think we should assume a 
successful transfer occurred if we got any kind of error.

If you just delete that code above, does the corruption go away?

Original attached patch was (likely whitespace damaged now):

--- linux-2.6.25.9/drivers/usb/storage/transport.c.orig	2008-06-24 
23:09:06.000000000 +0200
+++ linux-2.6.25.9/drivers/usb/storage/transport.c	2008-07-20 
05:14:32.000000000 +0200
@@ -661,6 +661,21 @@ void usb_stor_invoke_transport(struct sc
  			srb->result = SAM_STAT_GOOD;
  			srb->sense_buffer[0] = 0x0;
  		}
+
+        /* JMicron JM20337 chipset bug workaround - BEGIN */
+		if (us->pusb_dev->descriptor.idVendor == 0x152d &&
+            us->pusb_dev->descriptor.idProduct == 0x2338 &&
+            result == USB_STOR_TRANSPORT_FAILED &&
+			/* Filemark 0, ignore EOM, ILI 0, no sense */
+				(srb->sense_buffer[2] & 0xaf) == 0 &&
+			/* No ASC or ASCQ */
+				srb->sense_buffer[12] == 0 &&
+				srb->sense_buffer[13] == 0) {
+            printk(KERN_WARNING "USB Storage - Working around the 
JMicron JM20337 chipset bug (idVendor=%04x, idProduct=%04x, NO_SENSE, 
ASC=0, ASCQ=0) - retrying the read operation\n", 
us->pusb_dev->descriptor.idVendor, us->pusb_dev->descriptor.idProduct);
+		    srb->result = (DID_ERROR << 16) | (SUGGEST_RETRY << 24);
+            return;
+        }
+        /* JMicron JM20337 chipset bug workaround - END */
  	}

  	/* Did we transfer less than the minimum amount required? */