Bug 11125 - iRiver T10 is incorrectly handled because of commit bdb2b8ca
iRiver T10 is incorrectly handled because of commit bdb2b8ca
Status: CLOSED CODE_FIX
Product: IO/Storage
Classification: Unclassified
Component: SCSI
All Linux
: P1 normal
Assigned To: Alan Stern
:
: 11183 (view as bug list)
Depends on:
Blocks: 10492
  Show dependency treegraph
 
Reported: 2008-07-19 12:39 UTC by Andrey Rahmatullin
Modified: 2008-09-21 15:56 UTC (History)
6 users (show)

See Also:
Kernel Version: 2.6.26
Tree: Mainline
Regression: Yes


Attachments
debug log without the commit (2.66 KB, application/bzip2)
2008-07-19 15:02 UTC, Andrey Rahmatullin
Details
unusual_devs entry for iRiver T10 (653 bytes, patch)
2008-07-19 19:34 UTC, Alan Stern
Details | Diff
debug log with Apacer USB SD card reader (19.40 KB, text/plain)
2008-07-28 04:51 UTC, Ondrej Zary
Details
Residue heuristic using INQUIRY and READ CAPACITY (1.00 KB, patch)
2008-07-28 08:46 UTC, Alan Stern
Details | Diff
Residue heuristic using INQUIRY and READ CAPACITY (corrected) (1.01 KB, patch)
2008-07-28 08:50 UTC, Alan Stern
Details | Diff

Description Andrey Rahmatullin 2008-07-19 12:39:30 UTC
iRiver T10 mp3 player, which is basically an USB mass storage, isn't working anymore:

usb 5-1: new high speed USB device using ehci_hcd and address 5
usb 5-1: configuration #1 chosen from 1 choice
usb 5-1: New USB device found, idVendor=4102, idProduct=1020
usb 5-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
usb 5-1: Product: iriver MP3 T10
usb 5-1: Manufacturer: iriver Limited
usbcore: registered new interface driver libusual
Initializing USB Mass Storage driver...
scsi4 : SCSI emulation for USB Mass Storage devices
usbcore: registered new interface driver usb-storage
USB Mass Storage support registered.
usb-storage: device found at 5
usb-storage: waiting for device to settle before scanning
scsi scan: INQUIRY result too short (5), using 36
scsi 4:0:0:0: Direct-Access                                    PQ: 0 ANSI: 0
sd 4:0:0:0: [sdc] Sector size 0 reported, assuming 512.
sd 4:0:0:0: [sdc] 1 512-byte hardware sectors (0 MB)
sd 4:0:0:0: [sdc] Write Protect is off
sd 4:0:0:0: [sdc] Mode Sense: 03 00 00 00
sd 4:0:0:0: [sdc] Assuming drive cache: write through
sd 4:0:0:0: [sdc] Sector size 0 reported, assuming 512.
sd 4:0:0:0: [sdc] 1 512-byte hardware sectors (0 MB)
sd 4:0:0:0: [sdc] Write Protect is off
sd 4:0:0:0: [sdc] Mode Sense: 03 00 00 00
sd 4:0:0:0: [sdc] Assuming drive cache: write through
 sdc:
sd 4:0:0:0: [sdc] Attached SCSI disk
sd 4:0:0:0: Attached scsi generic sg4 type 0
usb-storage: device scan complete


If I revert bdb2b8ca, it starts to work again:

usb 5-1: new high speed USB device using ehci_hcd and address 6
usb 5-1: configuration #1 chosen from 1 choice
scsi5 : SCSI emulation for USB Mass Storage devices
usb 5-1: New USB device found, idVendor=4102, idProduct=1020
usb 5-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
usb 5-1: Product: iriver MP3 T10
usb 5-1: Manufacturer: iriver Limited
usb-storage: device found at 6
usb-storage: waiting for device to settle before scanning
scsi 5:0:0:0: Direct-Access     iriver   MP3 T10E         0.83 PQ: 0 ANSI: 2
sd 5:0:0:0: [sdc] 1929216 512-byte hardware sectors (988 MB)
sd 5:0:0:0: [sdc] Write Protect is off
sd 5:0:0:0: [sdc] Mode Sense: 03 00 00 00
sd 5:0:0:0: [sdc] Assuming drive cache: write through
sd 5:0:0:0: [sdc] 1929216 512-byte hardware sectors (988 MB)
sd 5:0:0:0: [sdc] Write Protect is off
sd 5:0:0:0: [sdc] Mode Sense: 03 00 00 00
sd 5:0:0:0: [sdc] Assuming drive cache: write through
 sdc:
sd 5:0:0:0: [sdc] Attached SCSI removable disk
sd 5:0:0:0: Attached scsi generic sg4 type 0
usb-storage: device scan complete
Comment 1 Alan Stern 2008-07-19 13:40:10 UTC
Can you build a kernel with CONFIG_USB_STORAGE_DEBUG enabled and attach the dmesg log for the time when you plug in the iRiver T10?  Probably it needs an unusual_devs entry with the IGNORE_RESIDUE flag set; the debugging log will tell us for sure.
Comment 2 Andrey Rahmatullin 2008-07-19 15:02:10 UTC
Created attachment 16894 [details]
debug log without the commit

This is the log with commit reverted. If you need the log with commit active, I can do it tomorrow.
Comment 3 Alan Stern 2008-07-19 19:34:08 UTC
Created attachment 16899 [details]
unusual_devs entry for iRiver T10

Yes, it's a bad Residue value, as I thought.  Interestingly, the value is correct for READ(10) and presumably also WRITE(10) commands -- but it's wrong for INQUIRY, READ CAPACITY, and MODE SENSE.  One has to wonder what the engineers were smoking when they designed this device.

Anyway, this patch should fix the problem.
Comment 4 Andrey Rahmatullin 2008-07-20 00:22:44 UTC
Yes, it does.
Comment 5 Andrew Morton 2008-07-20 18:43:29 UTC
Alan, can you please cc me on the final patch so I can ensure that
it gets into 2.6.26.x?

Thanks.
Comment 6 Anonymous Emailer 2008-07-21 08:15:31 UTC
Reply-To: dougg@torque.net

bugme-daemon@bugzilla.kernel.org wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=11125
> 
>            Summary: iRiver T10 is incorrectly handled because of commit
>                     bdb2b8ca
>            Product: IO/Storage
>            Version: 2.5
>      KernelVersion: 2.6.26
>           Platform: All
>         OS/Version: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: normal
>           Priority: P1
>          Component: SCSI
>         AssignedTo: linux-scsi@vger.kernel.org
>         ReportedBy: wrar@altlinux.org
> 
> 
> iRiver T10 mp3 player, which is basically an USB mass storage, isn't working
> anymore:
> 
> usb 5-1: new high speed USB device using ehci_hcd and address 5
> usb 5-1: configuration #1 chosen from 1 choice
> usb 5-1: New USB device found, idVendor=4102, idProduct=1020
> usb 5-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> usb 5-1: Product: iriver MP3 T10
> usb 5-1: Manufacturer: iriver Limited
> usbcore: registered new interface driver libusual
> Initializing USB Mass Storage driver...
> scsi4 : SCSI emulation for USB Mass Storage devices
> usbcore: registered new interface driver usb-storage
> USB Mass Storage support registered.
> usb-storage: device found at 5
> usb-storage: waiting for device to settle before scanning
> scsi scan: INQUIRY result too short (5), using 36
> scsi 4:0:0:0: Direct-Access                                    PQ: 0 ANSI: 0
> sd 4:0:0:0: [sdc] Sector size 0 reported, assuming 512.
> sd 4:0:0:0: [sdc] 1 512-byte hardware sectors (0 MB)
> sd 4:0:0:0: [sdc] Write Protect is off
> sd 4:0:0:0: [sdc] Mode Sense: 03 00 00 00
> sd 4:0:0:0: [sdc] Assuming drive cache: write through
> sd 4:0:0:0: [sdc] Sector size 0 reported, assuming 512.
> sd 4:0:0:0: [sdc] 1 512-byte hardware sectors (0 MB)
> sd 4:0:0:0: [sdc] Write Protect is off
> sd 4:0:0:0: [sdc] Mode Sense: 03 00 00 00
> sd 4:0:0:0: [sdc] Assuming drive cache: write through
>  sdc:
> sd 4:0:0:0: [sdc] Attached SCSI disk
> sd 4:0:0:0: Attached scsi generic sg4 type 0
> usb-storage: device scan complete
> 
> 
> If I revert bdb2b8ca, it starts to work again:
> 
> usb 5-1: new high speed USB device using ehci_hcd and address 6
> usb 5-1: configuration #1 chosen from 1 choice
> scsi5 : SCSI emulation for USB Mass Storage devices
> usb 5-1: New USB device found, idVendor=4102, idProduct=1020
> usb 5-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> usb 5-1: Product: iriver MP3 T10
> usb 5-1: Manufacturer: iriver Limited
> usb-storage: device found at 6
> usb-storage: waiting for device to settle before scanning
> scsi 5:0:0:0: Direct-Access     iriver   MP3 T10E         0.83 PQ: 0 ANSI: 2
> sd 5:0:0:0: [sdc] 1929216 512-byte hardware sectors (988 MB)
> sd 5:0:0:0: [sdc] Write Protect is off
> sd 5:0:0:0: [sdc] Mode Sense: 03 00 00 00
> sd 5:0:0:0: [sdc] Assuming drive cache: write through
> sd 5:0:0:0: [sdc] 1929216 512-byte hardware sectors (988 MB)
> sd 5:0:0:0: [sdc] Write Protect is off
> sd 5:0:0:0: [sdc] Mode Sense: 03 00 00 00
> sd 5:0:0:0: [sdc] Assuming drive cache: write through
>  sdc:
> sd 5:0:0:0: [sdc] Attached SCSI removable disk
> sd 5:0:0:0: Attached scsi generic sg4 type 0
> usb-storage: device scan complete
> 

I'm seeing exactly the same failure with a JetFlash 2GB
USB stick. Works fine with production Ubuntu 8.04 and
Fedora 9 (latest updates) but fails as shown above in
lk 2.6.26 . With my tools (sg3_utils) it just yields an
empty response to an INQUIRY command, MODE SENSE fails but
READ CAPACITY half works. When I use my tools on XP with
the same device there is no problem.

Looks like resid is wrong.


# sg_inq -vvvv /dev/sg2
open /dev/sg2 with flags=0x800
     inquiry cdb: 12 00 00 00 24 00
       duration=4 ms
     inquiry: requested 36 bytes but got 0 bytes
inquiry: got too few bytes (0)
     inquiry: failed requesting 36 byte response: res=97


 From /var/log/messages:
usb-storage: Command INQUIRY (6 bytes)
usb-storage:  12 00 00 00 24 00
usb-storage: Bulk Command S 0x43425355 T 0xe L 36 F 128 Trg 0 LUN 0 CL 6
usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
usb-storage: Status code 0; transferred 31/31
usb-storage: -- transfer complete
usb-storage: Bulk command transfer result=0
usb-storage: usb_stor_bulk_transfer_sglist: xfer 36 bytes, 1 entries
usb-storage: Status code 0; transferred 36/36
usb-storage: -- transfer complete
usb-storage: Bulk data transfer result 0x0
usb-storage: Attempting to get CSW...
usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
usb-storage: Status code 0; transferred 13/13
usb-storage: -- transfer complete
usb-storage: Bulk status result = 0
usb-storage: Bulk Status S 0x53425355 T 0xe R 36 Stat 0x0
usb-storage: scsi cmd done, result=0x0
usb-storage: *** thread sleeping.


# sg_readcap -vvvv /dev/sg2
open /dev/sg2 with flags=0x800
     read capacity (10) cdb: 25 00 00 00 00 00 00 00 00 00
       duration=0 ms
     read capacity (10): requested 8 bytes but got 0 bytes
Read Capacity results:
    Last logical block address=3969023 (0x3c8fff), Number of blocks=3969024
    Logical block length=512 bytes
Hence:
    Device size: 2032140288 bytes, 1938.0 MiB, 2.03 GB

dmesg:
usb-storage: Command READ_CAPACITY (10 bytes)
usb-storage:  25 00 00 00 00 00 00 00 00 00
usb-storage: Bulk Command S 0x43425355 T 0xf L 8 F 128 Trg 0 LUN 0 CL 10
usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes
usb-storage: Status code 0; transferred 31/31
usb-storage: -- transfer complete
usb-storage: Bulk command transfer result=0
usb-storage: usb_stor_bulk_transfer_sglist: xfer 8 bytes, 1 entries
usb-storage: Status code 0; transferred 8/8
usb-storage: -- transfer complete
usb-storage: Bulk data transfer result 0x0
usb-storage: Attempting to get CSW...
usb-storage: usb_stor_bulk_transfer_buf: xfer 13 bytes
usb-storage: Status code 0; transferred 13/13
usb-storage: -- transfer complete
usb-storage: Bulk status result = 0
usb-storage: Bulk Status S 0x53425355 T 0xf R 8 Stat 0x0
usb-storage: -- unexpectedly short transfer
usb-storage: scsi cmd done, result=0x0
usb-storage: *** thread sleeping



Doug Gilbert

Comment 7 Dmitry Milinevsky 2008-07-21 22:50:20 UTC
The same behavior with iRiver T20/T30.
Here is the patch:

diff -Nupr linux-2.6.26-gentoo-old/drivers/usb/storage/unusual_devs.h linux-2.6.26-gentoo/drivers/usb/storage/unusual_devs.h
--- linux-2.6.26-gentoo-old/drivers/usb/storage/unusual_devs.h	2008-07-22 08:47:36.000000000 +0300
+++ linux-2.6.26-gentoo/drivers/usb/storage/unusual_devs.h	2008-07-22 08:38:14.000000000 +0300
@@ -1758,6 +1758,16 @@ UNUSUAL_DEV(  0x2770, 0x915d, 0x0010, 0x
 		US_SC_DEVICE, US_PR_DEVICE, NULL,
 		US_FL_FIX_CAPACITY ),
 
+/* Reported by Milinevsky Dmitry <niam.niam@gmail.com> */
+UNUSUAL_DEV(  0x4102, 0x1019, 0x0100, 0x0100,
+		"iRiver, Ltd.",
+		"iriver T30",
+		US_SC_DEVICE, US_PR_DEVICE, NULL, US_FL_IGNORE_RESIDUE ),
+UNUSUAL_DEV(  0x4102, 0x1014, 0x0100, 0x0100,
+		"iRiver, Ltd.",
+		"iriver T20",
+		US_SC_DEVICE, US_PR_DEVICE, NULL, US_FL_IGNORE_RESIDUE ),
+
 /*
  * David Härdeman <david@2gen.com>
  * The key makes the SCSI stack print confusing (but harmless) messages
Comment 8 Ondrej Zary 2008-07-23 03:43:12 UTC
I have the same problem with Apacer USB SmartCard reader. Please don't add new quirks and revert the offending patch instead as it obviously breaks many devices.
Comment 9 Alan Stern 2008-07-23 06:14:57 UTC
On Wed, 23 Jul 2008 bugme-daemon@bugzilla.kernel.org wrote:

> http://bugzilla.kernel.org/show_bug.cgi?id=11125

> I have the same problem with Apacer USB SmartCard reader. Please don't add new
> quirks and revert the offending patch instead as it obviously breaks many
> devices.

Sorry, the offending patch cannot be removed.  Not only does it make 
the kernel properly obey the standard, it also is vitally necessary to 
fix the original problem it was created for.

The patch does not break many devices; these devices were already 
broken.  The fact that they worked at all was more or less an accident.

If you would like to submit a patch adding the appropriate quirk for 
your device to the usb-storage mailing list, it will be accepted.

Alan Stern

Comment 10 Ondrej Zary 2008-07-23 07:27:16 UTC
I wonder what device or problem does the patch fix (don't know where and how to get the actual patch from the ID "bdb2b8ca")?

Obeying the standard is not a good reason to break devices. I know that there is a lot of broken USB storage devices everywhere - but I don't think it's a good idea to list them all in a blacklist file. You'll always find another one that's broken and not listed, thus not working. Why can't we do something to workaround the bugs? 1-sector USB storage device is obviously wrong, so why don't we try the old detection method?
Comment 11 Andrey Rahmatullin 2008-07-23 10:15:58 UTC
(In reply to comment #10)
> I wonder what device or problem does the patch fix (don't know where and how to
> get the actual patch from the ID "bdb2b8ca")?

https://lists.one-eyed-alien.net/pipermail/usb-storage/2008-June/003795.html
Comment 12 Alan Stern 2008-07-23 15:26:10 UTC
Concerning http://bugzilla.kernel.org/show_bug.cgi?id=11125

Commit bdb2b8ca fixed a real problem, but it is generating problems of
its own.  Rather than adding a bunch of new unusual_devs entries, one
at a time, I think we may be able to solve most of these new problems
in a single stroke with the following patch.  Could everyone please try
it out, with their respective new unusual_devs changes removed?

For the sake of those not familiar with the original problem, here's a 
capsule summary.  The USB Mass-Storage Bulk-only Transport allows 
devices to send invalid data back to the host.  The device indicates 
the data is invalid by putting a positive Residue value in the CSW.  It 
turns out that some devices actually do this, but the kernel was trying 
to _use_ the invalid data instead of ignoring it.

The commit mentioned above fixed that problem by zeroing out the last
Residue bytes of the device's reply.  But now it seems that some
devices erroneously set the Residue to a positive value even when the
data is entirely correct.  Commit bdb2b8ca causes the kernel to ignore
the valid data too; to fix this we have to tell usb-storage to
disregard the bogus Residue value.

Since the bad Residue shows up in the very first command, an INQUIRY, 
we ought to be able to detect these problematic devices automatically.  
That's what this patch does.

(Note: I haven't been able even to compile-test it.  So don't be too 
surprised if it doesn't work right.)

Alan Stern



Index: usb-2.6/drivers/usb/storage/transport.c
===================================================================
--- usb-2.6.orig/drivers/usb/storage/transport.c
+++ usb-2.6/drivers/usb/storage/transport.c
@@ -1033,7 +1033,15 @@ int usb_stor_Bulk_transport(struct scsi_
 	/* try to compute the actual residue, based on how much data
 	 * was really transferred and what the device tells us */
 	if (residue) {
-		if (!(us->fflags & US_FL_IGNORE_RESIDUE)) {
+
+		/* Heuristically detect devices that generate bogus residues
+		 * by seeing what happens with INQUIRY commands
+		 */
+		if (srb->cmnd[0] == INQUIRY && transfer_length == 36 &&
+				scsi_get_resid(srb) == 0) {
+			us->fflags |= US_FL_IGNORE_RESIDUE;
+
+		} else if (!(us->fflags & US_FL_IGNORE_RESIDUE)) {
 			residue = min(residue, transfer_length);
 			scsi_set_resid(srb, max(scsi_get_resid(srb),
 			                                       (int) residue));

Comment 13 Phil Dibowitz 2008-07-23 15:32:46 UTC
Alan Stern wrote:
> Since the bad Residue shows up in the very first command, an INQUIRY, 
> we ought to be able to detect these problematic devices automatically.  
> That's what this patch does.

Neat one, Alan, I really like that.

-- 
Phil Dibowitz                             phil@ipom.com
Open Source software and tech docs        Insanity Palace of Metallica
http://www.phildev.net/                   http://www.ipom.com/

"Never write it in C if you can do it in 'awk';
 Never do it in 'awk' if 'sed' can handle it;
 Never use 'sed' when 'tr' can do the job;
 Never invoke 'tr' when 'cat' is sufficient;
 Avoid using 'cat' whenever possible" -- Taylor's Laws of Programming


Comment 14 Anonymous Emailer 2008-07-23 15:46:01 UTC
Reply-To: zaitcev@redhat.com

On Wed, 23 Jul 2008 18:26:03 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> wrote:

>  	if (residue) {
> -		if (!(us->fflags & US_FL_IGNORE_RESIDUE)) {
> +
> +		/* Heuristically detect devices that generate bogus residues
> +		 * by seeing what happens with INQUIRY commands
> +		 */
> +		if (srb->cmnd[0] == INQUIRY && transfer_length == 36 &&
> +				scsi_get_resid(srb) == 0) {
> +			us->fflags |= US_FL_IGNORE_RESIDUE;
> +
> +		} else if (!(us->fflags & US_FL_IGNORE_RESIDUE)) {
>  			residue = min(residue, transfer_length);

Why do you have to link this to INQUIRY? Doing the same for
any command would work, I expect.

-- Pete

Comment 15 Alan Stern 2008-07-23 20:36:26 UTC
On Wed, 23 Jul 2008, Pete Zaitcev wrote:

> > +		/* Heuristically detect devices that generate bogus residues
> > +		 * by seeing what happens with INQUIRY commands
> > +		 */

> Why do you have to link this to INQUIRY? Doing the same for
> any command would work, I expect.

For most commands it probably would.  (Although sometimes the non-zero
Residue value _isn't_ bogus.)  But if we don't do it at the very first
command, that command will fail.  The patch implicitly assumes that a
device would never send a legitimate non-zero Residue in response to a
36-byte INQUIRY.

Alan Stern

Comment 16 Ondrej Zary 2008-07-24 04:01:53 UTC
I tested the patch (had to edit fflags to flags) and it does not fix my device (maybe because it sets the IGNORE_RESIDUE flag but the IDENTIFY was already done?):
usb-storage: device found at 4
usb-storage: waiting for device to settle before scanning
scsi 2:0:0:0: Direct-Access     Generic                   6000 PQ: 0 ANSI: 0 CCS
sd 2:0:0:0: [sdb] Sector size 0 reported, assuming 512.
sd 2:0:0:0: [sdb] 1 512-byte hardware sectors (0 MB)
sd 2:0:0:0: [sdb] Write Protect is off
sd 2:0:0:0: [sdb] Mode Sense: 4b 00 00 08
sd 2:0:0:0: [sdb] Assuming drive cache: write through
sd 2:0:0:0: [sdb] Sector size 0 reported, assuming 512.
sd 2:0:0:0: [sdb] 1 512-byte hardware sectors (0 MB)
sd 2:0:0:0: [sdb] Write Protect is off
sd 2:0:0:0: [sdb] Mode Sense: 4b 00 00 08
sd 2:0:0:0: [sdb] Assuming drive cache: write through
 sdb: sdb1
 sdb: p1 exceeds device capacity
sd 2:0:0:0: [sdb] Attached SCSI removable disk
sd 2:0:0:0: Attached scsi generic sg3 type 0
usb-storage: device scan complete
attempt to access beyond end of device
sdb: rw=0, want=40, limit=1
Buffer I/O error on device sdb1, logical block 0
attempt to access beyond end of device
sdb: rw=0, want=48, limit=1
Buffer I/O error on device sdb1, logical block 1
attempt to access beyond end of device
sdb: rw=0, want=56, limit=1
Buffer I/O error on device sdb1, logical block 2
attempt to access beyond end of device
sdb: rw=0, want=64, limit=1
Buffer I/O error on device sdb1, logical block 3
attempt to access beyond end of device
sdb: rw=0, want=40, limit=1
Buffer I/O error on device sdb1, logical block 0
attempt to access beyond end of device
sdb: rw=0, want=40, limit=1
Buffer I/O error on device sdb1, logical block 0
attempt to access beyond end of device
sdb: rw=0, want=48, limit=1
Buffer I/O error on device sdb1, logical block 1
attempt to access beyond end of device
sdb: rw=0, want=56, limit=1
Buffer I/O error on device sdb1, logical block 2
attempt to access beyond end of device
sdb: rw=0, want=64, limit=1
Buffer I/O error on device sdb1, logical block 3
attempt to access beyond end of device
sdb: rw=0, want=40, limit=1
Buffer I/O error on device sdb1, logical block 0
Comment 17 Alan Stern 2008-07-24 06:25:52 UTC
Can you do the same thing with CONFIG_USB_STORAGE_DEBUG enabled, and attach the corresponding dmesg log to this bug report?
Comment 18 Andrey Rahmatullin 2008-07-24 12:09:28 UTC
(In reply to comment #12)
> Index: usb-2.6/drivers/usb/storage/transport.c
Seems to work here.

(In reply to comment #16)
> I tested the patch (had to edit fflags to flags)
Why did you do that?
Comment 19 Ondrej Zary 2008-07-24 13:36:26 UTC
> (In reply to comment #16)
> > I tested the patch (had to edit fflags to flags)
> Why did you do that?
> 

Because the patch didn't apply without that. Maybe I'm doing something wrong (applying the patch to 2.6.26 kernel).
Comment 20 Ondrej Zary 2008-07-28 04:51:07 UTC
Created attachment 17011 [details]
debug log with Apacer USB SD card reader
Comment 21 Alan Stern 2008-07-28 08:46:54 UTC
Created attachment 17012 [details]
Residue heuristic using INQUIRY and READ CAPACITY

It turns out that in your case the bad Residue is attached to a READ CAPACITY command and not to the initial INQUIRY command.  This is an updated version of the patch which should detect either error.  (You'll still have to edit it to change fflags to flags; this patch is based on a development tree instead of vanilla 2.6.26.)
Comment 22 Alan Stern 2008-07-28 08:50:30 UTC
Created attachment 17013 [details]
Residue heuristic using INQUIRY and READ CAPACITY (corrected)

Sorry -- this patch now has proper parentheses and no syntax errors.
Comment 23 Anonymous Emailer 2008-07-28 09:03:38 UTC
Reply-To: dougg@torque.net

Alan Stern wrote:
> Concerning http://bugzilla.kernel.org/show_bug.cgi?id=11125
> 
> Commit bdb2b8ca fixed a real problem, but it is generating problems of
> its own.  Rather than adding a bunch of new unusual_devs entries, one
> at a time, I think we may be able to solve most of these new problems
> in a single stroke with the following patch.  Could everyone please try
> it out, with their respective new unusual_devs changes removed?
> 
> For the sake of those not familiar with the original problem, here's a 
> capsule summary.  The USB Mass-Storage Bulk-only Transport allows 
> devices to send invalid data back to the host.  The device indicates 
> the data is invalid by putting a positive Residue value in the CSW.  It 
> turns out that some devices actually do this, but the kernel was trying 
> to _use_ the invalid data instead of ignoring it.
> 
> The commit mentioned above fixed that problem by zeroing out the last
> Residue bytes of the device's reply.  But now it seems that some
> devices erroneously set the Residue to a positive value even when the
> data is entirely correct.  Commit bdb2b8ca causes the kernel to ignore
> the valid data too; to fix this we have to tell usb-storage to
> disregard the bogus Residue value.
> 
> Since the bad Residue shows up in the very first command, an INQUIRY, 
> we ought to be able to detect these problematic devices automatically.  
> That's what this patch does.
> 
> (Note: I haven't been able even to compile-test it.  So don't be too 
> surprised if it doesn't work right.)
> 
> Alan Stern
> 
> 
> 
> Index: usb-2.6/drivers/usb/storage/transport.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/storage/transport.c
> +++ usb-2.6/drivers/usb/storage/transport.c
> @@ -1033,7 +1033,15 @@ int usb_stor_Bulk_transport(struct scsi_
>  	/* try to compute the actual residue, based on how much data
>  	 * was really transferred and what the device tells us */
>  	if (residue) {
> -		if (!(us->fflags & US_FL_IGNORE_RESIDUE)) {
> +
> +		/* Heuristically detect devices that generate bogus residues
> +		 * by seeing what happens with INQUIRY commands
> +		 */
> +		if (srb->cmnd[0] == INQUIRY && transfer_length == 36 &&
> +				scsi_get_resid(srb) == 0) {
> +			us->fflags |= US_FL_IGNORE_RESIDUE;
> +
> +		} else if (!(us->fflags & US_FL_IGNORE_RESIDUE)) {
>  			residue = min(residue, transfer_length);
>  			scsi_set_resid(srb, max(scsi_get_resid(srb),
>  			                                       (int) residue));
> 

Alan,
Thanks for fixing this.

I would like to point out that the response to a SCSI
INQUIRY command should not be complete "crap" or a
null response. Obviously the transport layer and lower
can fail but that should be handled in different ways.

Taking the latest SPC-4 draft:
http://www.t10.org/ftp/t10/drafts/spc4/spc4r15.pdf
section 6.4.1 on page 240 there is a paragraph that
states:
"The INQUIRY data should be returned even though the device server
is not ready for other commands. The standard INQUIRY data should
be available without incurring any media access delays. If the
device server does store some of the standard INQUIRY data or VPD
data on the media, it may return ASCII spaces (20h) in ASCII
fields and zeros in other fields until data is available from the
media."

The INQUIRY response is special in this respect.

My interpretation of the above is that the USB mass storage
driver needs to look at what it is sending "up" as an INQUIRY
response and sanitize it if necessary. If the USB mass storage
device is defective then either work round the defect or report
to the SCSI subsystem that there is no (SCSI) device there.

Doug Gilbert

Comment 24 Alan Stern 2008-07-28 09:27:12 UTC
On Mon, 28 Jul 2008, Douglas Gilbert wrote:

> My interpretation of the above is that the USB mass storage
> driver needs to look at what it is sending "up" as an INQUIRY
> response and sanitize it if necessary. If the USB mass storage
> device is defective then either work round the defect or report
> to the SCSI subsystem that there is no (SCSI) device there.

In this case the INQUIRY data are entirely correct.  The only problem 
is that the Residue value is nonzero when it should really be 0.  This 
is a common sort of problem; devices that can't respond to INQUIRY 
won't work with Windows, but Windows ignores the Residue.

The patch I posted does exactly what you suggest -- it works around the 
defect by ignoring the device's Residue and returning a Residue of 0.

Alan Stern

Comment 25 Ondrej Zary 2008-07-29 01:32:01 UTC
Thank you very much for the work. My Apacer SD card reader works fine with the patch.
Comment 26 Alan Stern 2008-07-29 09:02:54 UTC
The patch has been submitted for 2.6.27.  When it has been accepted I will also submit it for 2.6.26-stable.
Comment 27 Adrian Bunk 2008-07-31 03:20:44 UTC
*** Bug 11183 has been marked as a duplicate of this bug. ***
Comment 28 Alan Stern 2008-08-18 07:19:28 UTC
Accepted into mainline: commit 59f4ff2ecff4cef36378928cec891785b402e80c

Note You need to log in before you can comment on or make changes to this bug.