On Mon, 6 Jun 2011, Sarah Sharp wrote:
> I have always assumed that when a driver requests a device reset, a
> change of configuration or alternate interface setting, that it has
> already either canceled active URBs or completed all the URBs for the
> endpoints that are going away. Otherwise the device could see a packet
> bound for an endpoint that isn't part of the current interface. If
> those race conditions do exist, then we need to revisit the
> implementation, but it seems like only a broken driver would leave URBs
> pending.
Surprisingly, usbcore does not enforce the proper order of operations
(disable and flush endpoints, then issue Set-Config or Set-Interface)
requests in usb_set_interface(). This really should be fixed. I guess
the original motivation was to make error recovery simpler -- if the
device doesn't accept the request then none of the data structures need
to be changed.
> I think the issue was that usb_disable_endpoint() and
> usb_enable_endpoint() are only called after the control transfer to the
> device to change the configuration or alternate interface setting
> succeeds. The bandwidth checking needs to be done before the USB core
> issues that control transfer.
Is this requirement in the xHCI spec? I don't recall seeing it (but
then I haven't read the whole thing).
> I also I think I didn't want to use those functions because I wanted to
> batch all the changes to the endpoints associated with an alternate
> interface setting change or a configuration change into one command to
> issue to the xHCI host.
Yes, that's understandable.
> The code to drop old endpoints and add new ones really needs to be done
> atomically in the functions that attempt to set up the new configuration
> or alternate interface settings. The code in those functions might look
> ad-hock, but it's because I'm trying to satisfy the atomicity
> requirements and the requirement to change the xHCI bandwidth structures
> before the control transfer is issued.
That's why we have hcd->bandwidth_mutex. You don't have to remove the
old endpoints and set up the new ones in the same command, do you?
That is, you could have two commands: one to drop the old endpoints and
one to add the new endpoints.
> > 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);
>
> Yes, that does look a little better.
>
> > You could even remove the "if" wrapper on the theory that always doing
> > everything twice won't hurt.
>
> I was just trying to minimize the changes that EHCI/UHCI/OHCI would see,
> but if you think calling usb_disable_endpoint twice is harmless, I can
> remove the if wrapper.
I guess it should be left in... not that it will make much difference.
> > 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.
>
> I think there was some issue that forced me to use the endpoint arrays.
> Maybe it was something like the data structures that are accessible from
> usb_hcd_alloc_bandwidth() don't allow me to access the
> usb_host_endpoint? I do remember struggling with that code.
I don't remember the issues either... maybe they're mentioned in the
email archives (if you can find the right messages!). Accessing the
usb_host_endpoint structures shouldn't be an issue, since that same
routine uses them when changing altsettings.
> > 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.
>
> Mmm, but the removing of old endpoint bandwidth resources has to happen
> atomically with adding new endpoint bandwidth resources.
Your meaning isn't clear. Do you mean the overall bandwidth change has
to be handled as a single transaction, including both removals and
additions? Or do you mean that bandwidth changes for two devices must
not overlap?
Regardless, this is how we should do it. At the same time, we could
include a feature allowing drivers to decrease the de facto maxpacket
values below the values given in the endpoint descriptors, for improved
scheduling performance.
> > 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.
>
> Yeah, the driver could just attempt to add the new endpoints, but if any
> of the endpoint addresses overlap, the host controller is going to
> reject them, as Tanya has found out. So we really need to drop the old
> endpoints before adding new ones.
That's not what I meant. Suppose an attempt to change a config or
altsetting fails -- this could be because of limitations on the host
side or on the device side. What state should we end up in?
In the Set-Interface case, the kernel should be able to return to its
original state (but goodness knows what state the device will be in).
In the Set-Config case, the kernel won't be able to do much of anything
-- it will have to behave as though the device is unconfigured, even
though it probably isn't.
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
