Re: [RFC] Patch to allow violation of USB specs (low-speed BULK to start with)

On Mon, 1 Oct 2007, Brad Campbell wrote:

> G'day all,
> 
> This has taken a while as I've gone and built some devices to characterise the uhci stack under
> extreme duress caused by low-speed bulk devices.
> 
> In short, it handles it very well. No bus starvation noticable. Throughput to full-speed devices is
> pretty much unworried, latency is not an issue. The uhci stack just throttles the BULK tokens to the
> low-speed device back until it's not having any adverse effect to the other devices.

That doesn't sound right.  uhci-hcd doesn't do any throttling.  If
things work okay, it's because the low-speed device doesn't try to
abuse the extra privileges you've just given it.

> Interestingly enough.. this test hardware turned up some serious bugs in combination with Windows 
> and old VIA uhci hardware. Linux handles these bugs without comment and just keeps on keeping on.
> 
> Very simply.. a single parameter to usbcore to tell it we _do_ want to do nasty things with the
> specification. I'm not sure of the best way to do this, so I thought I'd try it first and deal
> with the comments as they arise.
> 
> diff -ur kernel-clean/drivers/usb/core/config.c kernel-modified/drivers/usb/core/config.c
> --- kernel-clean/drivers/usb/core/config.c	2007-09-29 21:00:15.000000000 +0400
> +++ kernel-modified/drivers/usb/core/config.c	2007-10-01 15:12:16.000000000 +0400
> @@ -13,6 +13,11 @@
> 
>   #define USB_MAXCONFIG			8	/* Arbitrary limit */
> 
> +/* Allow the violation of the USB Specifications */

How about "Allow some violations..."?  Saying "the violation" makes 
it sound like you have a particular violation in mind, without letting 
the reader know which one it is.

> +static int usb_violate_specs = 0;

Zero initializer isn't needed (or recommended) for static data.

> +module_param (usb_violate_specs, bool, S_IRUGO);

Extraneous space preceding the '('.

> +MODULE_PARM_DESC (usb_violate_specs, "true to allow the blatant violation of the specifications");

80-column rule and extraneous space.

> +EXPORT_SYMBOL(usb_violate_specs);

Should be EXPORT_SYMBOL_GPL.

> 
>   static inline const char *plural(int n)
>   {
> @@ -135,7 +140,8 @@
>   	 * them usable, we will try treating them as Interrupt endpoints.
>   	 */
>   	if (to_usb_device(ddev)->speed == USB_SPEED_LOW &&
> -			usb_endpoint_xfer_bulk(d)) {
> +			usb_endpoint_xfer_bulk(d) &&
> +			!usb_violate_specs) {
>   		dev_warn(ddev, "config %d interface %d altsetting %d "
>   		    "endpoint 0x%X is Bulk; changing to Interrupt\n",
>   		    cfgno, inum, asnum, d->bEndpointAddress);

That part looks okay.  You should add a section to 
Documentation/kernel-parameters.txt explaining the meaning of your new 
parameter.

> diff -ur kernel-clean/drivers/usb/host/uhci-q.c kernel-modified/drivers/usb/host/uhci-q.c
> --- kernel-clean/drivers/usb/host/uhci-q.c	2007-09-29 21:00:15.000000000 +0400
> +++ kernel-modified/drivers/usb/host/uhci-q.c	2007-10-01 15:12:28.000000000 +0400
> @@ -16,6 +16,7 @@
>    * (C) Copyright 2004-2007 Alan Stern, [email protected]
>    */
> 
> +extern int usb_violate_specs;

This belongs in a .h file somewhere.  After all, there may be other 
spec violations we want to allow in the future.

>   /*
>    * Technically, updating td->status here is a race, but it's not really a
> @@ -1043,7 +1044,7 @@
>   	int ret;
> 
>   	/* Can't have low-speed bulk transfers */
> -	if (urb->dev->speed == USB_SPEED_LOW)
> +	if (urb->dev->speed == USB_SPEED_LOW && !usb_violate_specs)
>   		return -EINVAL;
> 
>   	if (qh->state != QH_STATE_ACTIVE)

This isn't quite right.  You need to make some additional changes, like 
this:

 	if (qh->state != QH_STATE_ACTIVE)
-		qh->skel = SKEL_BULK;
+		qh->skel = (urb->dev->speed == USB_SPEED_LOW ?
+				SKEL_LS_CONTROL : SKEL_BULK);
	ret = uhci_submit_common(uhci, urb, qh);
-	if (ret == 0)
+	if (ret == 0 && urb->dev->speed != USB_SPEED_LOW)
		uhci_add_fsbr(uhci, urb);
	return ret;
}

(Starting to look a little awkward, isn't it?  It might be cleaner to 
precompute the skel value and use that for the uhci_add_fsbr() test.)

Otherwise the controller will try to enable Full Speed Bandwidth 
Reclamation and send more than one transaction per frame to these 
low-speed "bulk" endpoints.

Alan Stern


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel