Re: [PATCH] USB and Driver Core patches for 2.6.10

From: Greg KH
Date: Sat Jan 08 2005 - 01:37:05 EST

ChangeSet 1.1938.446.47, 2004/12/20 15:00:38-08:00, stern@xxxxxxxxxxxxxxxxxxx

[PATCH] USB: Hub driver: several bug fixes and simplifications [11/13]

This patch adds several relatively minor bug fixes and code
simplifications for the hub driver.

Perhaps most significantly, a test is added to usbfs to prevent
users from unbinding the hub driver from a hub with children.
That could cause some bad headaches, and it's best to avoid the
situation.

Don't set unused bits in the change_bits and event_bits fields
(this aids in debugging, nothing more).

Use the correct spinlock in the hub ioctl routine!

Add an argument to hub_port_disable, indicating whether the
device behind the disabled port should be marked NOTATTACHED.
Normally it should, but for retries during usb_reset_device
it should not.

Pass the child device as an argument to hub_port_suspend and
hub_port_resume rather than making them recalculate it.

Call usb_set_device_state in the suspend/resume code rather
than setting udev->state directly.

Set udev->power.power_state in the suspend/resume code so that
the correct values are visible in sysfs.

Recognize during hub_port_resume that the child device udev
might be NULL (this will happen when a new device is plugged
into a suspended port).

Don't allow hub_port_init to drop the usb_address0_sem lock
while a device on the bus is still using address 0!

Make khubd acquire a reference to the hub interface rather than
the hub device. This simplifies disconnect/unbind testing.

Don't print a debugging message about hub events while not
holding some lock to protect the hub.

Remember to drop the reference acquired above if we fail to
lock the hub.

Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <greg@xxxxxxxxx>


drivers/usb/core/devio.c | 11 +++++
drivers/usb/core/hub.c | 101 +++++++++++++++++++++++++----------------------
2 files changed, 66 insertions(+), 46 deletions(-)


diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
--- a/drivers/usb/core/devio.c 2005-01-07 15:42:59 -08:00
+++ b/drivers/usb/core/devio.c 2005-01-07 15:42:59 -08:00
@@ -1122,6 +1122,7 @@
int retval = 0;
struct usb_interface *intf = NULL;
struct usb_driver *driver = NULL;
+ int i;

/* get input parameters and alloc buffer */
if (copy_from_user(&ctrl, arg, sizeof (ctrl)))
@@ -1153,6 +1154,16 @@

/* disconnect kernel driver from interface */
case USBDEVFS_DISCONNECT:
+
+ /* don't allow the user to unbind the hub driver from
+ * a hub with children to manage */
+ for (i = 0; i < ps->dev->maxchild; ++i) {
+ if (ps->dev->children[i])
+ retval = -EBUSY;
+ }
+ if (retval)
+ break;
+
down_write(&usb_bus_type.subsys.rwsem);
if (intf->dev.driver) {
driver = to_usb_driver(intf->dev.driver);
diff -Nru a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
--- a/drivers/usb/core/hub.c 2005-01-07 15:42:59 -08:00
+++ b/drivers/usb/core/hub.c 2005-01-07 15:42:59 -08:00
@@ -465,7 +465,7 @@
schedule_delayed_work(&hub->leds, LED_CYCLE_PERIOD);

/* scan all ports ASAP */
- hub->event_bits[0] = ~0;
+ hub->event_bits[0] = (1UL << (hub->descriptor->bNbrPorts + 1)) - 1;
kick_khubd(hub);
}

@@ -688,7 +688,7 @@
hub->indicator [0] = INDICATOR_CYCLE;

hub_power_on(hub);
- hub->change_bits[0] = ~0;
+ hub->change_bits[0] = (1UL << (hub->descriptor->bNbrPorts)) - 1;
hub_activate(hub);
return 0;

@@ -816,10 +816,9 @@
switch (code) {
case USBDEVFS_HUB_PORTINFO: {
struct usbdevfs_hub_portinfo *info = user_data;
- unsigned long flags;
int i;

- spin_lock_irqsave(&hub_event_lock, flags);
+ spin_lock_irq(&device_state_lock);
if (hdev->devnum <= 0)
info->nports = 0;
else {
@@ -832,7 +831,7 @@
hdev->children[i]->devnum;
}
}
- spin_unlock_irqrestore(&hub_event_lock, flags);
+ spin_unlock_irq(&device_state_lock);

return info->nports + 1;
}
@@ -1413,11 +1412,11 @@
return status;
}

-static int hub_port_disable(struct usb_device *hdev, int port)
+static int hub_port_disable(struct usb_device *hdev, int port, int set_state)
{
int ret;

- if (hdev->children[port]) {
+ if (hdev->children[port] && set_state) {
usb_set_device_state(hdev->children[port],
USB_STATE_NOTATTACHED);
}
@@ -1439,7 +1438,7 @@
struct usb_hub *hub;

dev_dbg(hubdev(hdev), "logical disconnect on port %d\n", port + 1);
- hub_port_disable(hdev, port);
+ hub_port_disable(hdev, port, 1);

/* FIXME let caller ask to power down the port:
* - some devices won't enumerate without a VBUS power cycle
@@ -1468,12 +1467,11 @@
* tree above them to deliver data, such as a keypress or packet. In
* some cases, this wakes the USB host.
*/
-static int hub_port_suspend(struct usb_device *hdev, int port)
+static int hub_port_suspend(struct usb_device *hdev, int port,
+ struct usb_device *udev)
{
- int status;
- struct usb_device *udev;
+ int status;

- udev = hdev->children[port];
// dev_dbg(hubdev(hdev), "suspend port %d\n", port + 1);

/* enable remote wakeup when appropriate; this lets the device
@@ -1513,7 +1511,7 @@
} else {
/* device has up to 10 msec to fully suspend */
dev_dbg(&udev->dev, "usb suspend\n");
- udev->state = USB_STATE_SUSPENDED;
+ usb_set_device_state(udev, USB_STATE_SUSPENDED);
msleep(10);
}
return status;
@@ -1620,8 +1618,10 @@
} else
status = -EOPNOTSUPP;
} else
- status = hub_port_suspend(udev->parent, port);
+ status = hub_port_suspend(udev->parent, port, udev);

+ if (status == 0)
+ udev->dev.power.power_state = state;
return status;
}
EXPORT_SYMBOL(__usb_suspend_device);
@@ -1675,9 +1675,10 @@
* first two on the host side; they'd be inside hub_port_init()
* during many timeouts, but khubd can't suspend until later.
*/
- udev->state = udev->actconfig
- ? USB_STATE_CONFIGURED
- : USB_STATE_ADDRESS;
+ usb_set_device_state(udev, udev->actconfig
+ ? USB_STATE_CONFIGURED
+ : USB_STATE_ADDRESS);
+ udev->dev.power.power_state = PM_SUSPEND_ON;

/* 10.5.4.5 says be sure devices in the tree are still there.
* For now let's assume the device didn't go crazy on resume,
@@ -1745,12 +1746,10 @@
}

static int
-hub_port_resume(struct usb_device *hdev, int port)
+hub_port_resume(struct usb_device *hdev, int port, struct usb_device *udev)
{
- int status;
- struct usb_device *udev;
+ int status;

- udev = hdev->children[port];
// dev_dbg(hubdev(hdev), "resume port %d\n", port + 1);

/* see 7.1.7.7; affects power usage, but not budgeting */
@@ -1764,7 +1763,8 @@
u16 portchange;

/* drive resume for at least 20 msec */
- dev_dbg(&udev->dev, "RESUME\n");
+ if (udev)
+ dev_dbg(&udev->dev, "RESUME\n");
msleep(25);

#define LIVE_FLAGS ( USB_PORT_STAT_POWER \
@@ -1788,7 +1788,8 @@
} else {
/* TRSMRCY = 10 msec */
msleep(10);
- status = finish_port_resume(udev);
+ if (udev)
+ status = finish_port_resume(udev);
}
}
if (status < 0)
@@ -1838,7 +1839,7 @@
}
} else if (udev->state == USB_STATE_SUSPENDED) {
// NOTE this fails if parent is also suspended...
- status = hub_port_resume(udev->parent, port);
+ status = hub_port_resume(udev->parent, port, udev);
} else {
status = 0;
}
@@ -1937,7 +1938,7 @@
continue;
down (&udev->serialize);
if (portstat & USB_PORT_STAT_SUSPEND)
- status = hub_port_resume(hdev, port);
+ status = hub_port_resume(hdev, port, udev);
else {
status = finish_port_resume(udev);
if (status < 0) {
@@ -2292,6 +2293,8 @@
retval = 0;

fail:
+ if (retval)
+ hub_port_disable(hdev, port, 0);
up(&usb_address0_sem);
return retval;
}
@@ -2424,11 +2427,15 @@
}

#ifdef CONFIG_USB_SUSPEND
- /* If something is connected, but the port is suspended, wake it up.. */
+ /* If something is connected, but the port is suspended, wake it up. */
if (portstatus & USB_PORT_STAT_SUSPEND) {
- status = hub_port_resume(hdev, port);
- if (status < 0)
- dev_dbg(hub_dev, "can't clear suspend on port %d; %d\n", port+1, status);
+ status = hub_port_resume(hdev, port, NULL);
+ if (status < 0) {
+ dev_dbg(hub_dev,
+ "can't clear suspend on port %d; %d\n",
+ port+1, status);
+ goto done;
+ }
}
#endif

@@ -2474,7 +2481,7 @@
&devstat);
if (status < 0) {
dev_dbg(&udev->dev, "get status %d ?\n", status);
- goto loop;
+ goto loop_disable;
}
cpu_to_le16s(&devstat);
if ((devstat & (1 << USB_DEVICE_SELF_POWERED)) == 0) {
@@ -2487,7 +2494,7 @@
schedule_work (&hub->leds);
}
status = -ENOTCONN; /* Don't retry */
- goto loop;
+ goto loop_disable;
}
}

@@ -2527,7 +2534,7 @@

up (&udev->serialize);
if (status)
- goto loop;
+ goto loop_disable;

status = hub_power_remaining(hub);
if (status)
@@ -2537,8 +2544,9 @@

return;

+loop_disable:
+ hub_port_disable(hdev, port, 1);
loop:
- hub_port_disable(hdev, port);
ep0_reinit(udev);
release_address(udev);
usb_put_dev(udev);
@@ -2547,13 +2555,14 @@
}

done:
- hub_port_disable(hdev, port);
+ hub_port_disable(hdev, port, 1);
}

static void hub_events(void)
{
struct list_head *tmp;
struct usb_device *hdev;
+ struct usb_interface *intf;
struct usb_hub *hub;
struct device *hub_dev;
u16 hubstatus;
@@ -2583,10 +2592,8 @@

hub = list_entry(tmp, struct usb_hub, event_list);
hdev = hub->hdev;
- hub_dev = &hub->intf->dev;
-
- usb_get_dev(hdev);
- spin_unlock_irq(&hub_event_lock);
+ intf = hub->intf;
+ hub_dev = &intf->dev;

dev_dbg(hub_dev, "state %d ports %d chg %04x evt %04x\n",
hdev->state, hub->descriptor
@@ -2596,14 +2603,16 @@
(u16) hub->change_bits[0],
(u16) hub->event_bits[0]);

+ usb_get_intf(intf);
+ spin_unlock_irq(&hub_event_lock);
+
/* Lock the device, then check to see if we were
* disconnected while waiting for the lock to succeed. */
- if (locktree(hdev) < 0)
- break;
- if (hdev->state != USB_STATE_CONFIGURED ||
- !hdev->actconfig ||
- hub != usb_get_intfdata(
- hdev->actconfig->interface[0]))
+ if (locktree(hdev) < 0) {
+ usb_put_intf(intf);
+ continue;
+ }
+ if (hub != usb_get_intfdata(intf) || hub->quiescing)
goto loop;

if (hub->error) {
@@ -2675,7 +2684,7 @@
connect_change = 1;
} else {
ret = -ENODEV;
- hub_port_disable(hdev, i);
+ hub_port_disable(hdev, i, 1);
}
dev_dbg (hub_dev,
"resume on port %d, status %d\n",
@@ -2724,7 +2733,7 @@

loop:
usb_unlock_device(hdev);
- usb_put_dev(hdev);
+ usb_put_intf(intf);

} /* end while (1) */
}

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/