On Mon, Dec 03, 2012 at 08:07:06PM +0100, Sebastian Andrzej Siewior wrote:
> The USB 2.0 specification says that bMaxPower is the maximum power
> consumption expressed in 2 mA units and the USB 3.0 specification says
> that it is expressed in 8 mA units.
Thanks for catching this, Sebastian! However, I think this patch needs
to be extended in several ways to work properly.
First, it looks like you need to update hcd->bus_mA. The USB 3.0 spec
increases the bus power from 500mA to 900mA. See section 9.2.5.1:
"The amount of current draw for SuperSpeed devices are increased to 150
mA for low-power devices and 900 mA for high-power"
This code in hcd.c:usb_add_hcd needs to change:
/* starting here, usbcore will pay attention to this root hub */
rhdev->bus_mA = min(500u, hcd->power_budget);
This code in hub.c:hub_configure also needs to change:
if (hdev == hdev->bus->root_hub) {
if (hdev->bus_mA == 0 || hdev->bus_mA >= 500)
hub->mA_per_port = 500;
else {
hub->mA_per_port = hdev->bus_mA;
hub->limited_power = 1;
}
It's capping the mA_per_port to 500mA, which shouldn't be done for USB
3.0 roothubs. Instead of having hard coded values, we probably either
need to add a per-hub bus power, or have some #defines and set the
mA_per_port based on roothub speed.
The else statement after the above code also needs to change:
} else if ((hubstatus & (1 << USB_DEVICE_SELF_POWERED)) == 0) {
dev_dbg(hub_dev, "hub controller current requirement: %dmA\n",
hub->descriptor->bHubContrCurrent);
hub->limited_power = 1;
if (hdev->maxchild > 0) {
int remaining = hdev->bus_mA -
hub->descriptor->bHubContrCurrent;
if (remaining < hdev->maxchild * 100)
dev_warn(hub_dev,
"insufficient power available "
"to use all downstream ports\n");
hub->mA_per_port = 100; /* 7.2.1.1 */
I think this is trying to set the mA_per_port to the unconfigured device
power, which 7.2.1.1 says is one unit load. USB 2.0 uses 100 mA as the
unit load. However, the USB 3.0 spec says, "When operating in
SuperSpeed mode, 150 mA equals one unit load" and increases the
unconfigured power consumption limit to 150 mA. So this code needs to
set mA_per_port to 150 for SuperSpeed hubs.
The 500 mA and 100 mA limits seem to be hard coded all over the place.
For example, I think this code in hub.c:hub_port_connect_change() is
looking for whether the above else statement has run:
if (udev->descriptor.bDeviceClass == USB_CLASS_HUB
&& udev->bus_mA <= 100) {
Of course, that code won't work properly when you change the bus_mA to
be 150 mA for SuperSpeed hubs. Perhaps we need a new function to get
the unconfigured bus power, that takes a usb_hub, and returns either 100
or 150 based on device speed?
And this code has to change as well:
} else { /* Self-powered external hub */
/* FIXME: What about battery-powered external hubs that
* provide less current per port? */
hub->mA_per_port = 500;
}
if (hub->mA_per_port < 500)
dev_dbg(hub_dev, "%umA bus power budget for each child\n",
hub->mA_per_port);
You might want to break out that code into its own function, in a
separate patch.
And I think all this code will break once the USB Power Delivery spec
gets implemented, and the USB host controllers can put out something
like 12V to devices that ask for it. But that's a different issue.
http://www.usb.org/developers/powerdelivery/
> This patch adds a helper function usb_get_max_power() which computes the
> value based on config & usb_device's speed value. The sysfs interface &
> the device descriptor dump compute the value on their own.
>
> Cc: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
> Sarah, I leave it up to you to decide if this is stable material and/or
> other testing. My set of devices is very limited.
>
> drivers/usb/core/devices.c | 13 ++++++++++---
> drivers/usb/core/generic.c | 15 ++++++++++++++-
> drivers/usb/core/hub.c | 2 +-
> drivers/usb/core/message.c | 2 +-
> drivers/usb/core/sysfs.c | 20 ++++++++++++++------
> drivers/usb/core/usb.h | 1 +
> 6 files changed, 41 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
> index cbacea9..e33224e 100644
> --- a/drivers/usb/core/devices.c
> +++ b/drivers/usb/core/devices.c
> @@ -316,17 +316,23 @@ static char *usb_dump_iad_descriptor(char *start, char *end,
> */
> static char *usb_dump_config_descriptor(char *start, char *end,
> const struct usb_config_descriptor *desc,
> - int active)
> + int active, int speed)
> {
> + int mul;
> +
> if (start > end)
> return start;
> + if (speed == USB_SPEED_SUPER)
> + mul = 8;
> + else
> + mul = 2;
> start += sprintf(start, format_config,
> /* mark active/actual/current cfg. */
> active ? '*' : ' ',
> desc->bNumInterfaces,
> desc->bConfigurationValue,
> desc->bmAttributes,
> - desc->bMaxPower * 2);
> + desc->bMaxPower * mul);
> return start;
> }
>
> @@ -342,7 +348,8 @@ static char *usb_dump_config(int speed, char *start, char *end,
> if (!config)
> /* getting these some in 2.3.7; none in 2.3.6 */
> return start + sprintf(start, "(null Cfg. desc.)\n");
> - start = usb_dump_config_descriptor(start, end, &config->desc, active);
> + start = usb_dump_config_descriptor(start, end, &config->desc, active,
> + speed);
> for (i = 0; i < USB_MAXIADS; i++) {
> if (config->intf_assoc[i] == NULL)
> break;
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index eff2010..68ed80b 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -40,6 +40,19 @@ static int is_activesync(struct usb_interface_descriptor *desc)
> && desc->bInterfaceProtocol == 1;
> }
>
> +unsigned usb_get_max_power(struct usb_device *udev, struct usb_host_config *c)
> +{
> + unsigned val = c->desc.bMaxPower;
> +
> + switch (udev->speed) {
> + case USB_SPEED_SUPER:
> + return val * 8;
> + break;
> + default:
> + return val * 2;
> + }
> +}
> +
> int usb_choose_configuration(struct usb_device *udev)
> {
> int i;
> @@ -100,7 +113,7 @@ int usb_choose_configuration(struct usb_device *udev)
> */
>
> /* Rule out configs that draw too much bus current */
> - if (c->desc.bMaxPower * 2 > udev->bus_mA) {
> + if (usb_get_max_power(udev, c) > udev->bus_mA) {
> insufficient_power++;
> continue;
> }
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index a815fd2..4735425 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4152,7 +4152,7 @@ hub_power_remaining (struct usb_hub *hub)
> /* Unconfigured devices may not use more than 100mA,
> * or 8mA for OTG ports */
> if (udev->actconfig)
> - delta = udev->actconfig->desc.bMaxPower * 2;
> + delta = usb_get_max_power(udev, udev->actconfig);
> else if (port1 != udev->bus->otg_port || hdev->parent)
> delta = 100;
> else
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 131f736..444d30e 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1751,7 +1751,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
> }
> }
>
> - i = dev->bus_mA - cp->desc.bMaxPower * 2;
> + i = dev->bus_mA - usb_get_max_power(dev, cp);
> if (i < 0)
> dev_warn(&dev->dev, "new config #%d exceeds power "
> "limit by %dmA\n",
> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> index 818e4a0..d31d1c8 100644
> --- a/drivers/usb/core/sysfs.c
> +++ b/drivers/usb/core/sysfs.c
> @@ -17,14 +17,22 @@
> #include "usb.h"
>
> /* Active configuration fields */
> -#define usb_actconfig_show(field, multiplier, format_string) \
> +#define usb_actconfig_show(field, ss_mult, format_string) \
> static ssize_t show_##field(struct device *dev, \
> struct device_attribute *attr, char *buf) \
> { \
> struct usb_device *udev; \
> struct usb_host_config *actconfig; \
> + unsigned multiplier; \
> \
> + multiplier = 1; \
> udev = to_usb_device(dev); \
> + if (ss_mult) { \
> + if (udev->speed == USB_SPEED_SUPER) \
> + multiplier = 8; \
> + else \
> + multiplier = 2; \
> + } \
> actconfig = udev->actconfig; \
> if (actconfig) \
> return sprintf(buf, format_string, \
> @@ -33,13 +41,13 @@ static ssize_t show_##field(struct device *dev, \
> return 0; \
> } \
>
> -#define usb_actconfig_attr(field, multiplier, format_string) \
> -usb_actconfig_show(field, multiplier, format_string) \
> +#define usb_actconfig_attr(field, ss_mult, format_string) \
> +usb_actconfig_show(field, ss_mult, format_string) \
> static DEVICE_ATTR(field, S_IRUGO, show_##field, NULL);
>
> -usb_actconfig_attr(bNumInterfaces, 1, "%2d\n")
> -usb_actconfig_attr(bmAttributes, 1, "%2x\n")
> -usb_actconfig_attr(bMaxPower, 2, "%3dmA\n")
> +usb_actconfig_attr(bNumInterfaces, 0, "%2d\n")
> +usb_actconfig_attr(bmAttributes, 0, "%2x\n")
> +usb_actconfig_attr(bMaxPower, 1, "%3dmA\n")
>
> static ssize_t show_configuration_string(struct device *dev,
> struct device_attribute *attr, char *buf)
> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
> index 1c528c1..ddac825 100644
> --- a/drivers/usb/core/usb.h
> +++ b/drivers/usb/core/usb.h
> @@ -37,6 +37,7 @@ extern void usb_release_bos_descriptor(struct usb_device *dev);
> extern char *usb_cache_string(struct usb_device *udev, int index);
> extern int usb_set_configuration(struct usb_device *dev, int configuration);
> extern int usb_choose_configuration(struct usb_device *udev);
> +unsigned usb_get_max_power(struct usb_device *udev, struct usb_host_config *c);
>
> extern void usb_kick_khubd(struct usb_device *dev);
> extern int usb_match_one_id_intf(struct usb_device *dev,
> --
> 1.7.10.4
>
--
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
