PERFORCE change 161961 for review

[Hans Petter Selasky]

On Tuesday 12 May 2009, Andrew Thompson wrote:
> On Tue, May 12, 2009 at 06:39:53AM +0000, Hans Petter Selasky wrote:
> > http://perforce.freebsd.org/chv.cgi?CH=161961
> >
> > Change 161961 by hselasky at hselasky_laptop001 on 2009/05/12 06:38:57
> >
> >
> > 	USB CORE:
> > 	 - Cancelled error code must be returned in special
> > 	 case, else device drivers might end up in infinite loop.
> > 	 - The special case code was introduced by a recent commit.
> >
> > 	Bug report by: Andrey <andrey.kosachenko at gmail.com>
> >
> > Affected files ...
> >
> > .. //depot/projects/usb/src/sys/dev/usb/usb_transfer.c#144 edit
> >
> > Differences ...
> >
> > ==== //depot/projects/usb/src/sys/dev/usb/usb_transfer.c#144 (text+ko)
> > ====
> >
> > @@ -1439,7 +1439,11 @@
> >  	/* Check if the device is still alive */
> >  	if (info->udev->state < USB_STATE_POWERED) {
> >  		USB_BUS_LOCK(bus);
> > -		usb2_transfer_done(xfer, USB_ERR_NOT_CONFIGURED);
> > +		/*
> > +		 * Must return cancelled error code else
> > +		 * device drivers can hang.
> > +		 */
> > +		usb2_transfer_done(xfer, USB_ERR_CANCELLED);
> >  		USB_BUS_UNLOCK(bus);
> >  		return;
> >  	}
>
> I rather think this is a bug in the drivers, they blindly resubmit the
> transfer without knowing if the error is restartable.

No, this is no bug.

And they do not blindly re-submit the transfer.

When a transfer goes through a failure, then it will do a clear stall. After 
the clear stall is finished the transfer is re-started. The clear stall is 
niced at 50ms intervals. Because you added that check, the driver will spin 
100% CPU instead of every 50ms interval at detach. All drivers check for 
CANCELLED. Adding another check for a similar error code is just waste in my 
opinion.

Nicing a transfer is a feature, but the timeout is started after the not 
configured check, so that is where the bug is.

--HPS