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.
> 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?
> 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".
> @@ -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?
> */
> int usb_hcd_check_bandwidth(struct usb_device *udev,
Since this does more than just check, it should have a different name.
> --- 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?
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.
> --- 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.
Otherwise this seems generally correct.
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
