On Wed, Nov 11, 2009 at 04:22:42PM -0500, Alan Stern wrote:
> On Wed, 11 Nov 2009, Sarah Sharp wrote:
>
> > Make the USB core check the bandwidth when switching from one
> > interface alternate setting to another. Also check the bandwidth
> > when resetting a configuration (so that alt setting 0 is used). If
> > this check fails, the device's state is unchanged. If the device
> > refuses the new alt setting, re-instate the old alt setting in the
> > host controller hardware.
>
> That's right. Our stack should have been designed this way from the
> beginning, with periodic bandwidth being checked and allocated as soon
> as an altsetting is installed. Unfortunately, going back and changing
> all the HCDs now would be a huge job.
You could have a generic USB core function that does the checking for
OHCI, UHCI, and EHCI. The bandwidth requirements are in the bus
specification, and there shouldn't be any host controller specific
limitations, correct?
> > Add a mutex per root hub to protect bandwidth operations:
> > adding/reseting/changing configurations, and changing alternate interface
> > settings. We want to ensure that the xHCI host controller and the USB
> > device are set up for the same configurations and alternate settings.
> > There are two (possibly three) steps to do this:
> >
> > 1. The host controller needs to check that bandwidth is available for a
> > different setting, by issuing and waiting for a configure endpoint
> > command.
>
> That does more than check, doesn't it? It effectively allocates the
> bandwidth too. Or have I got this wrong?
Yes, it calls down to the xHCI hardware to allocate the bandwidth, and
the hardware will refuse the transition if it takes up too much
bandwidth.
> > 2. Once that returns successfully, a control message is sent to the
> > device.
> > 3. If that fails, the host controller must be notified through another
> > configure endpoint command.
> >
> > The mutex is used to make these three operations seem atomic, to prevent
> > another driver from using more bandwidth for a different device while
> > we're in the middle of these operations.
>
> Yes, a mutex is the right way to do it.
>
> > index 34de475..ff53854 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
>
> > @@ -868,6 +869,7 @@ static void usb_bus_init (struct usb_bus *bus)
> > bus->bandwidth_allocated = 0;
> > bus->bandwidth_int_reqs = 0;
> > bus->bandwidth_isoc_reqs = 0;
> > + mutex_init(&bus->bandwidth_lock);
>
> I'd prefer to put the new mutex into struct usb_hcd rather than
> usb_bus. The usb_bus structure is a holdover from long ago.
>
> Also, since it is a mutex and not a spinlock, perhaps you should name
> it something other than "bandwidth_lock".
bandwidth_mutex? Fine. :)
> > @@ -1592,12 +1594,13 @@ rescan:
> > /* Check whether a new configuration or alt setting for an interface
> > * will exceed the bandwidth for the bus (or the host controller resources).
> > * Only pass in a non-NULL config or interface, not both!
> > - * Passing NULL for both new_config and new_intf means the device will be
> > + * Passing NULL for both new_config and old_alt means the device will be
> > * de-configured by issuing a set configuration 0 command.
>
> These comments could be a little clearer. How about listing what
> values to pass for an altsetting change and what values to pass for a
> config change?
Sure.
> > */
> > int usb_hcd_check_bandwidth(struct usb_device *udev,
>
> Since this does more than just check, it should have a different name.
usb_hcd_allocate_bandwidth()?
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -3534,6 +3534,7 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
> > {
> > struct usb_device *parent_hdev = udev->parent;
> > struct usb_hub *parent_hub;
> > + struct usb_bus *bus = udev->bus;
> > struct usb_device_descriptor descriptor = udev->descriptor;
> > int i, ret = 0;
> > int port1 = udev->portnum;
> > @@ -3577,6 +3578,14 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
> > /* Restore the device's previous configuration */
> > if (!udev->actconfig)
> > goto done;
> > +
> > + mutex_lock(&bus->bandwidth_lock);
> > + ret = usb_hcd_check_bandwidth(udev, udev->actconfig, NULL, NULL);
> > + if (ret < 0) {
> > + dev_info(&udev->dev, "Not enough bandwidth for old config.\n");
> > + mutex_unlock(&bus->bandwidth_lock);
> > + goto re_enumerate;
> > + }
>
> This doesn't seem right. How could there not be enough bandwidth for
> the old config?
Yes, devices are supposed to declare alternate interface settings that
require less bandwidth as alternate setting 0 and resetting the
configuration puts everything back at alt setting 0, so technically, the
command to the xHCI hardware to allocate that bandwidth shouldn't fail.
However, the xHCI hardware can refuse a bandwidth configuration if it
doesn't have enough "internal resources" for the new settings. It seems
like only broken hardware would refuse to go to a setup that requires
less bandwidth, but the xHCI specification doesn't prohibit that. Maybe
the info printk should talk about internal resources instead.
> None of these changes should be needed in
> usb_reset_and_verify_device(). Either the old configuration and
> altsettings are retained or else the device is logically disconnected.
> Either way, no bandwidth changes occur here.
I don't disagree with you. Here's the bigger problem I'm trying to
solve. The xHCI specification says you have to issue a reset endpoint
command after a successful device reset. That command will remove all
endpoints except the control endpoint 0 from the internal structures and
schedule. The xHCI hardware also keeps track of the device state, so
the xHCI internal state for that device is reset to default.
The call to issue the reset endpoint command needs to be in
hub_port_init(), where I've crippled configured device reset under xHCI:
if ((hcd->driver->flags & HCD_USB3) && udev->config) {
/* FIXME this will need special handling by the xHCI driver. */
dev_dbg(&udev->dev,
"xHCI reset of configured device "
"not supported yet.\n");
retval = -EINVAL;
goto fail;
} else if (!udev->config && oldspeed == USB_SPEED_SUPER) {
/* Don't reset USB 3.0 devices during an initial setup */
usb_set_device_state(udev, USB_STATE_DEFAULT);
} else {
/* Reset the device; full speed may morph to high speed */
/* FIXME a USB 2.0 device may morph into SuperSpeed on reset. */
retval = hub_port_reset(hub, port1, udev, delay);
if (retval < 0) /* error or disconnect */
goto fail;
/* success, speed is known */
}
Once that call is in place, the code in usb_reset_and_verify_device()
makes more sense.
> > /* Restore the device's previous configuration */
> > if (!udev->actconfig)
> > goto done;
> > +
> > + mutex_lock(&bus->bandwidth_lock);
> > + ret = usb_hcd_check_bandwidth(udev, udev->actconfig, NULL, NULL);
> > + if (ret < 0) {
> > + dev_info(&udev->dev, "Not enough bandwidth for old config.\n");
> > + mutex_unlock(&bus->bandwidth_lock);
> > + goto re_enumerate;
> > + }
Since the reset endpoint command caused all the other endpoints to go
away in the internal xHCI structures, I need to reinstate the old
configuration and alternate interface settings. Reinstating the
configuration with alt setting 0 is this added code. The previous
alternate interface settings are re-instated with the call to
usb_set_interface() later on in usb_reset_and_verify_device().
> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
>
> > @@ -1721,12 +1775,14 @@ free_interfaces:
> > * host controller will not allow submissions to dropped endpoints. If
> > * this call fails, the device state is unchanged.
> > */
> > + mutex_lock(&bus->bandwidth_lock);
> > if (cp)
> > - ret = usb_hcd_check_bandwidth(dev, cp, NULL);
> > + ret = usb_hcd_check_bandwidth(dev, cp, NULL, NULL);
> > else
> > - ret = usb_hcd_check_bandwidth(dev, NULL, NULL);
> > + ret = usb_hcd_check_bandwidth(dev, NULL, NULL, NULL);
>
> I don't really see the point of the "if (cp)" test. The code does the
> same thing whether the test succeeds or fails.
Yes, I'll fix that.
Sarah
--
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
