On Thu, 1 Sep 2005, David Brownell wrote:
> > Okay, I get it (I think). You're saying that if the device status
> > indicates there's no external power source then Linux shouldn't choose a
> > self-powered config, right?
>
> That was the notion. I think it's what the specs say about how
> devices advertise their state, but I've not made time to verify.
It is indeed what the spec says. However it's not what the devices do.
See below.
> Details of the power circuitry in the peripheral are clearly visible
> to the peripheral, which has many options ... and only minimally
> visible to the host: how much VBUS current does it use? Is that
> augmented with some other power supply? Not: how many rails, what
> voltages and currents, what battery type(s), how many cells, etc.
And yet such issues as whether there is a battery and whether it is being
recharged, although not visible to the host, are arguably important for
the host to know about when choosing configurations. :-(
Okay, here's a patch to handle power configuration more thoroughly. You
should find that everything in it makes sense. (Perhaps the algorithm in
choose_configuration could use a little improvement -- at least it's
easier to understand than the current code. And the check for consecutive
bus-powered hubs may not be fully correct.) However I don't think people
will want to use it...
The problem is that (based on a very limited sample) a high proportion of
devices have mistakes in their descriptors. I tried out the patch with
two devices that were lying around, and look what happened:
The first device was a four-port bus-powered USB 1.1 hub (made by Genesys,
which is not a very good sign). It advertises both the
USB_CONFIG_ATT_SELFPOWER bit in its configuration _and_ the
USB_DEVICE_SELF_POWERED bit in its device status! (To be absolutely clear
about this, the hub has no power cable at all, and if it has a battery
then it's very well hidden with no means of replacement.) It also
advertises bMaxPower = 100 mA in its config. Either there's a concealed
battery capable of supplying 400 mA or else the device info is totally
bogus. Note that, given the absence of any information to the contrary,
my patched code believes the hub is capable of supplying 500 mA to each
port!
The second device was a bus-powered keyboard, with an internal hub having
one fixed connection (to the keyboard) and two removable connections
exposed on the case. Both the internal hub and keyboard device have
USB_CONFIG_ATT_SELFPOWER set in their configurations, but neither reports
USB_DEVICE_SELF_POWERED. As a result, with my patch installed, usbcore
does not configure either the hub or the keyboard. (Of course, I was able
to configure them manually through sysfs.)
These results might be exceptional, but it's a good bet that plenty of
other devices out there are just as badly misdesigned.
The patch is based on 2.6.13 plus gregkh-all-2.6.13.patch.
Alan Stern
Index: usb-2.6/drivers/usb/core/hub.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/hub.c
+++ usb-2.6/drivers/usb/core/hub.c
@@ -676,20 +676,38 @@ static int hub_configure(struct usb_hub
}
le16_to_cpus(&hubstatus);
if (hdev == hdev->bus->root_hub) {
- struct usb_hcd *hcd =
- container_of(hdev->bus, struct usb_hcd, self);
-
- hub->power_budget = min(500u, hcd->power_budget) / 2;
+ if (hdev->bus_mA == 0)
+ hub->mA_per_port = 500;
+ else {
+ hub->mA_per_port = hdev->bus_mA;
+ hub->limited_power = 1;
+ }
} else if ((hubstatus & (1 << USB_DEVICE_SELF_POWERED)) == 0) {
dev_dbg(hub_dev, "hub controller current requirement: %dmA\n",
hub->descriptor->bHubContrCurrent);
- hub->power_budget = (501 - hub->descriptor->bHubContrCurrent)
- / 2;
- }
- if (hub->power_budget)
- dev_dbg(hub_dev, "%dmA bus power budget for children\n",
- hub->power_budget * 2);
-
+ hub->limited_power = 1;
+ if (hdev->maxchild > 0) {
+ int remaining = hdev->bus_mA -
+ hub->descriptor->bHubContrCurrent;
+
+ if (remaining > 0)
+ hub->mA_per_port = min(100,
+ remaining / hdev->maxchild);
+ if (hub->mA_per_port < 100) {
+ dev_warn(hub_dev,
+ "insufficient power available "
+ "to use all downstream ports\n");
+ hub->mA_per_port = 100; /* 7.2.1.1 */
+ }
+ }
+ } 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);
ret = hub_hub_status(hub, &hubstatus, &hubchange);
if (ret < 0) {
@@ -1136,43 +1154,78 @@ void usb_disconnect(struct usb_device **
static int choose_configuration(struct usb_device *udev)
{
- int c, i;
+ int i;
+ u16 devstatus;
+ int bus_powered;
+ struct usb_host_config *c, *best;
+
+ /* If this fails, assume the device is bus-powered */
+ devstatus = 0;
+ usb_get_status(udev, USB_RECIP_DEVICE, 0, &devstatus);
+ bus_powered = !(devstatus & (1 << USB_DEVICE_SELF_POWERED));
+ dev_dbg(&udev->dev, "device is %s-powered\n",
+ bus_powered ? "bus" : "self");
+
+ best = NULL;
+ c = udev->config;
+ for (i = 0; i < udev->descriptor.bNumConfigurations; (i++, c++)) {
+ struct usb_interface_descriptor *desc =
+ &c->intf_cache[0]->altsetting->desc;
+
+ /* Rule out self-powered configs for a bus-powered device */
+ if (bus_powered && (c->desc.bmAttributes &
+ USB_CONFIG_ATT_SELFPOWER))
+ continue;
- /* NOTE: this should interact with hub power budgeting */
+ /* Rule out configs that draw too much bus current */
+ if (c->desc.bMaxPower * 2 > udev->bus_mA)
+ continue;
- c = udev->config[0].desc.bConfigurationValue;
- if (udev->descriptor.bNumConfigurations != 1) {
- for (i = 0; i < udev->descriptor.bNumConfigurations; i++) {
- struct usb_interface_descriptor *desc;
+ /* If the first config's first interface is COMM/2/0xff
+ * (MSFT RNDIS), rule it out unless Linux has host-side
+ * RNDIS support. */
+ if (i == 0 && desc->bInterfaceClass == USB_CLASS_COMM
+ && desc->bInterfaceSubClass == 2
+ && desc->bInterfaceProtocol == 0xff) {
+#ifndef CONFIG_USB_NET_RNDIS
+ continue;
+#else
+ best = c;
+#endif
+ }
- /* heuristic: Linux is more likely to have class
- * drivers, so avoid vendor-specific interfaces.
- */
- desc = &udev->config[i].intf_cache[0]
- ->altsetting->desc;
- if (desc->bInterfaceClass == USB_CLASS_VENDOR_SPEC)
- continue;
- /* COMM/2/all is CDC ACM, except 0xff is MSFT RNDIS.
- * MSFT needs this to be the first config; never use
- * it as the default unless Linux has host-side RNDIS.
- * A second config would ideally be CDC-Ethernet, but
- * may instead be the "vendor specific" CDC subset
- * long used by ARM Linux for sa1100 or pxa255.
- */
- if (desc->bInterfaceClass == USB_CLASS_COMM
- && desc->bInterfaceSubClass == 2
- && desc->bInterfaceProtocol == 0xff) {
- c = udev->config[1].desc.bConfigurationValue;
- continue;
- }
- c = udev->config[i].desc.bConfigurationValue;
+ /* From the remaining configs, choose the first one whose
+ * first interface is for a non-vendor-specific class.
+ * Reason: Linux is more likely to have a class driver
+ * than a vendor-specific driver. */
+ else if (udev->descriptor.bDeviceClass !=
+ USB_CLASS_VENDOR_SPEC &&
+ desc->bInterfaceClass !=
+ USB_CLASS_VENDOR_SPEC) {
+ best = c;
break;
}
+
+ /* If all the remaining configs are vendor-specific,
+ * choose the first one. */
+ else {
+ if (!best)
+ best = c;
+ }
+ }
+
+ if (best) {
+ i = best->desc.bConfigurationValue;
dev_info(&udev->dev,
"configuration #%d chosen from %d choices\n",
- c, udev->descriptor.bNumConfigurations);
+ i, udev->descriptor.bNumConfigurations);
+ } else {
+ i = -1;
+ dev_warn(&udev->dev,
+ "no configuration chosen from %d choices\n",
+ udev->descriptor.bNumConfigurations);
}
- return c;
+ return i;
}
#ifdef DEBUG
@@ -1340,17 +1393,13 @@ int usb_new_device(struct usb_device *ud
* with the driver core, and lets usb device drivers bind to them.
*/
c = choose_configuration(udev);
- if (c < 0)
- dev_warn(&udev->dev,
- "can't choose an initial configuration\n");
- else {
+ if (c >= 0) {
err = usb_set_configuration(udev, c);
if (err) {
dev_err(&udev->dev, "can't set config #%d, error %d\n",
c, err);
- usb_remove_sysfs_dev_files(udev);
- device_del(&udev->dev);
- goto fail;
+ /* This need not be fatal. The user can try to
+ * set other configurations. */
}
}
@@ -2415,39 +2464,37 @@ hub_power_remaining (struct usb_hub *hub
{
struct usb_device *hdev = hub->hdev;
int remaining;
- unsigned i;
+ int port1;
- remaining = hub->power_budget;
- if (!remaining) /* self-powered */
+ if (!hub->limited_power)
return 0;
- for (i = 0; i < hdev->maxchild; i++) {
- struct usb_device *udev = hdev->children[i];
- int delta, ceiling;
+ remaining = hdev->bus_mA;
+ for (port1 = 1; port1 <= hdev->maxchild; ++port1) {
+ struct usb_device *udev = hdev->children[port1 - 1];
+ int delta;
if (!udev)
continue;
- /* 100mA per-port ceiling, or 8mA for OTG ports */
- if (i != (udev->bus->otg_port - 1) || hdev->parent)
- ceiling = 50;
- else
- ceiling = 4;
-
+ /* Unconfigured devices may not use more than 100mA,
+ * or 8mA for OTG ports */
if (udev->actconfig)
- delta = udev->actconfig->desc.bMaxPower;
+ delta = udev->actconfig->desc.bMaxPower * 2;
+ else if (port1 != udev->bus->otg_port || hdev->parent)
+ delta = 100;
else
- delta = ceiling;
- // dev_dbg(&udev->dev, "budgeted %dmA\n", 2 * delta);
- if (delta > ceiling)
- dev_warn(&udev->dev, "%dmA over %dmA budget!\n",
- 2 * (delta - ceiling), 2 * ceiling);
+ delta = 8;
+ if (delta > hub->mA_per_port)
+ dev_warn(&udev->dev, "%dmA over %umA budget "
+ "for port %d!\n",
+ delta, hub->mA_per_port, port1);
remaining -= delta;
}
if (remaining < 0) {
dev_warn(hub->intfdev,
"%dmA over power budget!\n",
- -2 * remaining);
+ - remaining);
remaining = 0;
}
return remaining;
@@ -2542,7 +2589,8 @@ static void hub_port_connect_change(stru
usb_set_device_state(udev, USB_STATE_POWERED);
udev->speed = USB_SPEED_UNKNOWN;
-
+ udev->bus_mA = hub->mA_per_port;
+
/* set the address */
choose_address(udev);
if (udev->devnum <= 0) {
@@ -2562,7 +2610,7 @@ static void hub_port_connect_change(stru
* on the parent.
*/
if (udev->descriptor.bDeviceClass == USB_CLASS_HUB
- && hub->power_budget) {
+ && udev->bus_mA <= 100) {
u16 devstat;
status = usb_get_status(udev, USB_RECIP_DEVICE, 0,
@@ -2626,9 +2674,7 @@ static void hub_port_connect_change(stru
status = hub_power_remaining(hub);
if (status)
- dev_dbg(hub_dev,
- "%dmA power budget left\n",
- 2 * status);
+ dev_dbg(hub_dev, "%dmA power budget left\n", status);
return;
@@ -2833,6 +2879,11 @@ static void hub_events(void)
if (hubchange & HUB_CHANGE_LOCAL_POWER) {
dev_dbg (hub_dev, "power change\n");
clear_hub_feature(hdev, C_HUB_LOCAL_POWER);
+ if (hubstatus & HUB_STATUS_LOCAL_POWER)
+ /* FIXME: Is this always true? */
+ hub->limited_power = 0;
+ else
+ hub->limited_power = 1;
}
if (hubchange & HUB_CHANGE_OVERCURRENT) {
dev_dbg (hub_dev, "overcurrent change\n");
Index: usb-2.6/drivers/usb/core/hub.h
===================================================================
--- usb-2.6.orig/drivers/usb/core/hub.h
+++ usb-2.6/drivers/usb/core/hub.h
@@ -220,8 +220,9 @@ struct usb_hub {
struct usb_hub_descriptor *descriptor; /* class descriptor */
struct usb_tt tt; /* Transaction Translator */
- u8 power_budget; /* in 2mA units; or zero */
+ unsigned mA_per_port; /* current for each child */
+ unsigned limited_power:1;
unsigned quiescing:1;
unsigned activating:1;
unsigned resume_root_hub:1;
Index: usb-2.6/drivers/usb/core/hcd.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/hcd.c
+++ usb-2.6/drivers/usb/core/hcd.c
@@ -1774,8 +1774,6 @@ int usb_add_hcd(struct usb_hcd *hcd,
retval = -ENOMEM;
goto err_allocate_root_hub;
}
- rhdev->speed = (hcd->driver->flags & HCD_USB2) ? USB_SPEED_HIGH :
- USB_SPEED_FULL;
/* Although in principle hcd->driver->start() might need to use rhdev,
* none of the current drivers do.
@@ -1793,6 +1791,9 @@ int usb_add_hcd(struct usb_hcd *hcd,
dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
hcd->remote_wakeup = hcd->can_wakeup;
+ rhdev->speed = (hcd->driver->flags & HCD_USB2) ? USB_SPEED_HIGH :
+ USB_SPEED_FULL;
+ rhdev->bus_mA = min(500u, hcd->power_budget);
if ((retval = register_root_hub(rhdev, hcd)) != 0)
goto err_register_root_hub;
Index: usb-2.6/include/linux/usb.h
===================================================================
--- usb-2.6.orig/include/linux/usb.h
+++ usb-2.6/include/linux/usb.h
@@ -343,6 +343,8 @@ struct usb_device {
char **rawdescriptors; /* Raw descriptors for each config */
+ unsigned short bus_mA; /* Current available from the bus */
+
int have_langid; /* whether string_langid is valid yet */
int string_langid; /* language ID for strings */
Index: usb-2.6/drivers/usb/core/message.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/message.c
+++ usb-2.6/drivers/usb/core/message.c
@@ -1349,6 +1349,12 @@ free_interfaces:
if (dev->state != USB_STATE_ADDRESS)
usb_disable_device (dev, 1); // Skip ep0
+ n = dev->bus_mA - cp->desc.bMaxPower * 2;
+ if (n < 0)
+ dev_warn(&dev->dev, "new config #%d exceeds power "
+ "limit by %dmA\n",
+ configuration, -n);
+
if ((ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
USB_REQ_SET_CONFIGURATION, 0, configuration, 0,
NULL, 0, USB_CTRL_SET_TIMEOUT)) < 0)
-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
