On Mon, 4 Jan 2010, Sarah Sharp wrote:
> > But we'll need to find a real solution. The problem is that we mustn't
> > change intf->cur_altsetting here, because usb_set_interface() needs to
> > know exactly what endpoint devices already exist for this interface.
>
> Let's see if I can repeat the issue. :) usb_reset_and_verify_device()
> is attempting to reset the device, and then re-instate the old
> configuration and alternate settings. It takes a short cut by not
> calling usb_reset_configuration(), since it doesn't want to rebind the
> drivers if necessary.
By not calling usb_set_configuration(). usb_reset_configuration() does
something else.
> Thus, the sysfs files from the original configuration and a non-default
> alternate setting are still there when it calls usb_set_interface(), but
> the device (and xHC host controller) think the default alternate
> settings for the configuration are installed.
Yes. For non-xHCI controllers this doesn't matter, because we need to
use only ep0 to install the alternate settings.
> Before, the hub driver hid the reset from usb_set_interface() by leaving
> intf->cur_altsetting set to the non-default alternate setting. Now that
> usb_set_interface() needs to know which alternate settings are
> *actually* installed to allocated bandwidth, we can't rely on that.
> Does that sound correct?
Sort of. It doesn't really need to know anything to allocate the new
bandwidth, but it does need to know in order to release the old
bandwidth. Or rather, usb_set_interface() doesn't need to know -- the
bandwidth-manipulation routines need to.
And "*actually* installed" is ambiguous. There are three different
notions here: what usbcore thinks, what the HC hardware/driver thinks,
and what the device thinks. They are all "actual", but they don't
necessarily agree.
Also you must bear in mind that with non-xHCI controllers, the
bandwidth settings for the old altsetting will still be in force. Only
xHCI realizes that the reset will have changed things.
> > Sarah, any ideas? Add an extra flag to struct usb_interface to
> > indicate that the hardware thinks the interface is in altsetting 0
> > because of a reset?
>
> I can't think of any other way to do it. usb_set_interface() needs to
> know the true device/xHCI state to allocate bandwidth, so we can't just
> remove the lines in hub.c as you suggested for the temporary fix.
Actually usb_set_interface() doesn't need to know the old state. Only
the bandwidth routines in xhci-hcd do.
> So you're suggesting that the extra flag be checked in this if statement
> in message.c?
>
> /* prevent submissions using previous endpoint settings */
> if (iface->cur_altsetting != alt) {
> remove_intf_ep_devs(iface);
> usb_remove_sysfs_intf_files(iface);
> }
> usb_disable_interface(dev, iface, true);
>
> Something like:
>
> if (!iface->resetting && iface->cur_altsetting != alt) {
>
> and then have hub.c set and then clear that flag around the
> usb_set_interface() call in usb_reset_and_verify_device()? It seems
> a bit silly not just to add the flag as an argument to
> usb_set_interface(), but that would mean a lot of drivers would have to
> change for an internal USB core flag.
No, I'm suggesting that the extra flag be used in
xhci_check_bandwidth() and/or xhci_reset_bandwidth(). Other HCs can
sharee bandwidth-allocation facilities to be provided by usbcore, which
won't need to be aware that the device's state isn't what we think.
Ideally, they would skip all the bandwidth stuff when the flag was set.
The idea here is that the hardware must go through this suspend - reset
- re-enumerate - set config - set altsetting sequence, but the whole
thing should be hidden as far as possible from the upper layers. The
entire transition should be transparent. Whether the HC can be counted
among those "upper layers" depends on the technology; xHCI can't but
the others can.
This whole thing is becoming a little unwieldy, which suggests that the
underlying concepts don't fit the situation well. I'm not at all sure
that the idea of disabling endpoints while continuing to reserve their
bandwidth is sensible. A large part of the difficulty is that
bandwidth is difficult to handle and somewhat unreliable (if you
release some bandwidth and then try to allocate it again, there's no
guarantee you'll succeed -- even if nobody else has claimed any in the
meantime).
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
