usb/110988: [patch] Handling of quirk IGNORE_RESIDUE is umass.c is broken

[grem]

PR number changed... see below

Hans Petter Selasky wrote:
> On Thursday 29 March 2007 02:17, Michael Gmelin wrote:
>>> Number:         110989
>>> Category:       usb
>>> Synopsis:       [patch] Handling of quirk IGNORE_RESIDUE is umass.c is
>>> broken Confidential:   no
>>> Severity:       serious
>>> Priority:       medium
>>> Responsible:    freebsd-usb
>>> State:          open
>>> Quarter:
>>> Keywords:
>>> Date-Required:
>>> Class:          sw-bug
>>> Submitter-Id:   current-users
>>> Arrival-Date:   Thu Mar 29 00:30:09 GMT 2007
>>> Closed-Date:
>>> Last-Modified:
>>> Originator:     Michael Gmelin
>>> Release:        FreeBSD 6.2-RELEASE-p3 i386
>>> Organization:
>> /bin/done digital solutions GmbH
>>
>>> Environment:
>> FreeBSD bombat.bindone.de 6.2-RELEASE-p3 FreeBSD 6.2-RELEASE-p3 #21: Wed
>> Mar 28 04:08:44 CEST 2007    
>> root at bombat.bindone.de:/usr/src/sys/i386/compile/bombat  i386
>>
>>> Description:
>> I had to add a new device to usbdevs/umass.c which requires the
>> IGNORE_RESIDUE quirk to be set (extra PR will follow). It didn't work,
>> because IGNORE_RESIDUE isn't handled properly in umass.c (it isn't really
>> handled at all, since Residue is set in lines are 1668-1672 in umass.c in
>> the following was:
>>
>> int Residue;
>> Residue = UGETDW(sc->csw.dCSWDataResidue);
>> if (Residue == 0 &&
>> 	sc->transfer_datalen - sc->transfer_actlen != 0)
>> 		Residue = sc->transfer_datalen - sc->transfer_actlen;
>>
>> The patch below fixes this issue (tested and proven to work).
>>
>>> How-To-Repeat:
>> Use a really broken USB device which returns "random" values for
>> sc->csw.dCSWDataResidue (like devices that use the SuperTop IDEDEVICE USB
>> controller, e.g. the ICY BOX IB-220U-Wh). Every attempt to use the device
>> will lead to error messages, like:
>>
>> dd if=/dev/zero of=/dev/da0 count=10
>> da0: end of device
>>
>> or
>>
>> disklabel da0
>> read: Unknown error
>> etc.
>>
>>> Fix:
>> Apply the attached patch, which forces  residue to be calculated if
>> IGNORE_RESIDUE is set.
>>
>>
>>
>> Patch attached with submission follows:
>>
>> --- umass.c.orig	Thu Mar 29 02:07:04 2007
>> +++ umass.c	Thu Mar 29 02:08:06 2007
>> @@ -1666,7 +1666,10 @@
>>  		}
>>
>>  		int Residue;
>> -		Residue = UGETDW(sc->csw.dCSWDataResidue);
>> +		if (sc->quirks & IGNORE_RESIDUE)
>> +			Residue = 0;
>> +		else
>> +			Residue = UGETDW(sc->csw.dCSWDataResidue);
>>  		if (Residue == 0 &&
>>  		    sc->transfer_datalen - sc->transfer_actlen != 0)
>>  			Residue = sc->transfer_datalen - sc->transfer_actlen;
>>
>>> Release-Note:
>>> Audit-Trail:
>>> Unformatted:
> 
Hmmm, what I've seen in HEAD looked eactly the same, but anyway...

Do you think it's possible to 100% reliably detect bad residue this way? If not (what I
assume, imagine a device returning 0 < residue < sc->transfer_datalen -
sc->transfer_actlen) I think it's better to stick with an unusual devices list unless you
have all devices that require it available for testing.

Do you think there are any chances to get this patch into one of the next releases?
Personally I'm not really interested, how things will look into the next version of the
USB stack, since I have devices that are attached to production machines that have to run
RELEASE versions of FreeBSD...

> I think that we should auto-detect bad residue. If the residue is some fixed 
> constant way off the actual length, then maybe something like this is better:
> 
> Residue = UGETDW(sc->csw.dCSWDataResidue);
> 
> if (Residue > sc->transfer_datalen) {
>     Residue = sc->transfer_datalen - sc->transfer_actlen;
> }
> 
> --HPS