On Mon, 6 Jun 2011, Sarah Sharp wrote:
> On Mon, Jun 06, 2011 at 11:10:23AM -0400, Alan Stern wrote:
> > How about just unconditionally calling usb_hcd_alloc_bandwidth() before
> > looping through the endpoint arrays?
>
> I don't think that's a good idea, because there could be URBs pending
> before the loop calling usb_disable_endpoint() completes. The bandwidth
> allocation functions assume that all URB have been canceled or completed
> before an endpoint is dropped. Otherwise the command to remove the
> endpoint rings can race with transfer completion.
And xhci-hcd isn't prepared to handle those races ... ?
> > Alternatively, how about making
> > xhci-hcd's endpoint_disable method responsible for releasing the
> > endpoint's bandwidth reservation?
>
> The problem is that the USB core calls the endpoint_disable method too
> often, for all endpoints, without checking whether the endpoints were
> actually active.
It does? The only place I see it getting called is from within
usb_disable_endpoint() when the endpoint array entry exists and
reset_hardware is True.
> The xHCI driver doesn't keep track of which endpoints
> are added; it expects the USB core to do so. If the USB core
> unconditionally drops all endpoints, the command to drop them will be
> rejected by the host controller.
>
> Besides, I'm not too keen on re-writing the whole bandwidth checking
> code just for this special case. :) Any other thoughts? Is there
> anything technically wrong with the patch, or it just looks ugly?
Well, it does look ugly. At least you could change it to look like
this:
dev_dbg(&dev->dev, "%s nuking %s URBs\n", __func__,
skip_ep0 ? "non-ep0" : "all");
+
+ if (hcd->driver->check_bandwidth) {
+ /* First pass: Cancel URBs, leave endpoint pointers intact. */
+ for (i = skip_ep0; i < 16; ++i) {
+ usb_disable_endpoint(dev, i, false);
+ usb_disable_endpoint(dev, i + USB_DIR_IN, false);
+ }
+ /* Remove endpoints from the host controller internal state */
+ usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL);
+ }
+
for (i = skip_ep0; i < 16; ++i) {
usb_disable_endpoint(dev, i, true);
usb_disable_endpoint(dev, i + USB_DIR_IN, true);
You could even remove the "if" wrapper on the theory that always doing
everything twice won't hurt.
Another alternative is to change usb_hcd_alloc_bandwidth(). It starts
with two copies of the code that drops the endpoints in the current
config, using the ep_in and ep_out arrays. You could replace that with
a single copy that loops through the interface and endpoint descriptors
in the current config instead. Then it wouldn't matter if the arrays
are zeroed before the bandwidth computation.
In the end, it would be really nice to make all this stuff totally
rational. Right now it's kind of an ad-hoc mess. We should go through
a unified procedure whenever updating an altsetting or config:
Disable and flush the current endpoints;
Remove the endpoints' hardware resources (including bandwidth);
Allocate hardware resources for the new endpoints;
Enable the new endpoints.
We should also try to come up with a good strategy for handling errors.
It's too bad that xHCI doesn't provide any way to check a new bandwidth
setting without disabling the current endpoints.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
