David:
Here's a few ideas.
You mentioned before the need for documenting how devices get disconnected
some time after their state is set not NOTATTACHED. Below is a patch to
take care of this. I made some slight changes to your logic in a couple
of spots, mainly because the new hub_port_logical_disconnect() routine
returns void. What do you think of the patch?
Your hub_port_suspend() and hub_port_resume() routines use port numbers
with origin-1, in contrast to all the rest of the hub driver. Is there
some particular reason for this? Would you mind having those two routines
changed to use origin-0? Or if you want to keep them as they are, do you
mind if I rename the variable from "port" to "port1" to emphasize the
difference? BTW, you made a couple of off-by-one mistakes; they are
corrected in the patch below.
On a more philosophical note... What should it mean when a user suspends
a usb_interface instead of a usb_device? What about audio or network
class drivers that claim multiple interfaces; does it make sense to
suspend just one of the interfaces they hold? Should the drivers ignore
suspend calls to any but their "primary" interface, whichever that is?
In a composite device, what sense does it make to suspend one of the
interfaces? Should the driver simply prepare to minimize its own power
usage, recognizing that the device as a while may or may not get
suspended?
What about hubs, which are after all a very special sort of device? You
have structured the code so that children get suspended when the hub's
interface is suspended, even though the hub device may be left active.
Considering that in the driver-model tree, a hub's children actually
descend from the hub's usb_device and not its usb_interface, wouldn't it
be more logical to suspend the children only when the hub device is
suspended?
Alan Stern
diff -Nru a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
--- a/drivers/usb/core/hub.c Wed Aug 18 14:36:23 2004
+++ b/drivers/usb/core/hub.c Wed Aug 18 14:36:23 2004
@@ -1307,7 +1307,6 @@
int ret;
if (hdev->children[port]) {
- /* FIXME need disconnect() for NOTATTACHED device */
usb_set_device_state(hdev->children[port],
USB_STATE_NOTATTACHED);
}
@@ -1319,6 +1318,30 @@
return ret;
}
+/*
+ * Disable a port and mark a logical connnect-change event, so that some
+ * time later khubd will disconnect() any existing usb_device on the port
+ * and will re-enumerate if there actually is a device attached.
+ */
+static void hub_port_logical_disconnect(struct usb_device *hdev, int port)
+{
+ struct usb_hub *hub;
+
+ dev_dbg(hubdev(hdev), "logical disconnect on port %d\n", port + 1);
+ hub_port_disable(hdev, port);
+
+ hub = usb_get_intfdata(hdev->actconfig->interface[0]);
+ set_bit(port, hub->change_bits);
+
+ spin_lock_irq(&hub_event_lock);
+ if (list_empty(&hub->event_list)) {
+ list_add_tail(&hub->event_list, &hub_event_list);
+ wake_up(&khubd_wait);
+ }
+ spin_unlock_irq(&hub_event_lock);
+}
+
+
#ifdef CONFIG_USB_SUSPEND
/*
@@ -1652,7 +1675,7 @@
}
}
if (status < 0)
- status = hub_port_disable(hdev, port);
+ hub_port_logical_disconnect(hdev, port - 1);
return status;
}
@@ -1792,11 +1815,11 @@
status = hub_port_resume(hdev, port + 1);
else {
status = finish_port_resume(udev);
- if (status < 0)
- status = hub_port_disable(hdev, port);
- if (status < 0)
+ if (status < 0) {
dev_dbg(&intf->dev, "resume port %d --> %d\n",
- port, status);
+ port + 1, status);
+ hub_port_logical_disconnect(hdev, port);
+ }
}
up(&udev->serialize);
}
@@ -2415,15 +2438,17 @@
if (portchange & USB_PORT_STAT_C_SUSPEND) {
clear_port_feature(hdev, i + 1,
USB_PORT_FEAT_C_SUSPEND);
- if (hdev->children[i])
+ if (hdev->children[i]) {
ret = remote_wakeup(hdev->children[i]);
- else
+ if (ret < 0)
+ connect_change = 1;
+ } else {
ret = -ENODEV;
+ hub_port_disable(hdev, i);
+ }
dev_dbg (hub_dev,
"resume on port %d, status %d\n",
i + 1, ret);
- if (ret < 0)
- ret = hub_port_disable(hdev, i);
}
if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
@@ -2626,7 +2651,6 @@
struct usb_device *parent = udev->parent;
struct usb_device_descriptor descriptor = udev->descriptor;
int i, ret, port = -1;
- struct usb_hub *hub;
if (udev->state == USB_STATE_NOTATTACHED ||
udev->state == USB_STATE_SUSPENDED) {
@@ -2707,18 +2731,7 @@
return 0;
re_enumerate:
- hub_port_disable(parent, port);
-
- hub = usb_get_intfdata(parent->actconfig->interface[0]);
- set_bit(port, hub->change_bits);
-
- spin_lock_irq(&hub_event_lock);
- if (list_empty(&hub->event_list)) {
- list_add_tail(&hub->event_list, &hub_event_list);
- wake_up(&khubd_wait);
- }
- spin_unlock_irq(&hub_event_lock);
-
+ hub_port_logical_disconnect(parent, port);
return -ENODEV;
}
EXPORT_SYMBOL(__usb_reset_device);
-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
