In general we do not want khubd to act on port status changes that are
the result of in progress resets or port pm runtime operations.
Specifically port power control testing has been able to trigger an
unintended disconnect in hub_port_connect_change(), paraphrasing:
if ((portstatus & USB_PORT_STAT_CONNECTION) && udev &&
udev->state != USB_STATE_NOTATTACHED) {
if (portstatus & USB_PORT_STAT_ENABLE) {
/* Nothing to do */
} else if (udev->state == USB_STATE_SUSPENDED &&
udev->persist_enabled) {
...
} else {
/* Don't resuscitate */;
}
}
...by falling to the "Don't resuscitate" path or missing
USB_PORT_STAT_CONNECTION becuase usb_port_resume() was in the middle of
modifying the port status.
The lock ordering rules are now usb_lock_device() => usb_lock_port().
This is mandated by the device core which may hold the device_lock on
the usb_device before invoking usb_port_{suspend|resume} which in turn
take the status_lock on the usb_port. We attempt to hold the
status_lock for the duration of a port_event() run, and drop/re-aquire
it when needing to take the device_lock.
Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
---
drivers/usb/core/hub.c | 93 +++++++++++++++++++++++++++++++++++------------
drivers/usb/core/hub.h | 2 +
drivers/usb/core/port.c | 1 +
3 files changed, 72 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 0217299a2402..a310028e210d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2905,6 +2905,16 @@ static unsigned wakeup_enabled_descendants(struct usb_device *udev)
(hub ? hub->wakeup_enabled_descendants : 0);
}
+static void usb_lock_port(struct usb_port *port_dev)
+{
+ mutex_lock(&port_dev->status_lock);
+}
+
+static void usb_unlock_port(struct usb_port *port_dev)
+{
+ mutex_unlock(&port_dev->status_lock);
+}
+
/*
* usb_port_suspend - suspend a usb device's upstream port
* @udev: device that's no longer in active use, not a root hub
@@ -2961,6 +2971,8 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
int status;
bool really_suspend = true;
+ usb_lock_port(port_dev);
+
/* enable remote wakeup when appropriate; this lets the device
* wake up the upstream hub (including maybe the root hub).
*
@@ -3056,6 +3068,8 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
}
usb_mark_last_busy(hub->hdev);
+
+ usb_unlock_port(port_dev);
return status;
}
@@ -3195,6 +3209,8 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
}
}
+ usb_lock_port(port_dev);
+
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, &portstatus, &portchange);
if (status == 0 && !port_is_suspended(hub, portstatus))
@@ -3262,6 +3278,8 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
usb_unlocked_enable_lpm(udev);
}
+ usb_unlock_port(port_dev);
+
return status;
}
@@ -4371,17 +4389,21 @@ hub_power_remaining (struct usb_hub *hub)
* usb_reset_and_verify_device() encounters changed descriptors (as from
* a firmware download)
* caller already locked the hub
+ *
+ * It assumes the port is locked on entry and arranges for it to be
+ * always unlocked on exit
*/
-static void hub_port_connect_change(struct usb_hub *hub, int port1,
- u16 portstatus, u16 portchange)
+static void hub_port_connect_change_unlock(struct usb_hub *hub, int port1,
+ u16 portstatus, u16 portchange)
{
struct usb_device *hdev = hub->hdev;
struct device *hub_dev = hub->intfdev;
struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
unsigned wHubCharacteristics =
le16_to_cpu(hub->descriptor->wHubCharacteristics);
+ struct usb_port *port_dev = hub->ports[port1 - 1];
struct usb_device *udev;
- int status, i;
+ int status = -ENODEV, i;
unsigned unit_load;
dev_dbg (hub_dev,
@@ -4401,10 +4423,10 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
#endif
/* Try to resuscitate an existing device */
- udev = hub->ports[port1 - 1]->child;
+ udev = port_dev->child;
+
if ((portstatus & USB_PORT_STAT_CONNECTION) && udev &&
udev->state != USB_STATE_NOTATTACHED) {
- usb_lock_device(udev);
if (portstatus & USB_PORT_STAT_ENABLE) {
status = 0; /* Nothing to do */
@@ -4412,30 +4434,39 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
} else if (udev->state == USB_STATE_SUSPENDED &&
udev->persist_enabled) {
/* For a suspended device, treat this as a
- * remote wakeup event.
+ * remote wakeup event. Promote to the device
+ * lock since we have already evaluated the
+ * portstatus at this point and now want to take
+ * action on the child device.
*/
+ usb_unlock_port(port_dev);
+ usb_lock_device(udev);
status = usb_remote_wakeup(udev);
+ usb_unlock_device(udev);
+ usb_lock_port(port_dev);
#endif
-
} else {
- status = -ENODEV; /* Don't resuscitate */
- }
- usb_unlock_device(udev);
-
- if (status == 0) {
- clear_bit(port1, hub->change_bits);
- return;
+ /* Don't resuscitate */;
}
}
+ /* from this point forward we no longer need synchronization
+ * with usb_port_{suspend|resume} because we are about to
+ * disconnect / reconnect
+ */
+ usb_unlock_port(port_dev);
+
+ clear_bit(port1, hub->change_bits);
+ if (status == 0)
+ return;
+
/* Disconnect any existing devices under this port */
if (udev) {
if (hcd->phy && !hdev->parent &&
!(portstatus & USB_PORT_STAT_CONNECTION))
usb_phy_notify_disconnect(hcd->phy, udev->speed);
- usb_disconnect(&hub->ports[port1 - 1]->child);
+ usb_disconnect(&port_dev->child);
}
- clear_bit(port1, hub->change_bits);
/* We can forget about a "removed" device when there's a physical
* disconnect or the connect status changes.
@@ -4567,7 +4598,7 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
if (hdev->state == USB_STATE_NOTATTACHED)
status = -ENOTCONN;
else
- hub->ports[port1 - 1]->child = udev;
+ port_dev->child = udev;
spin_unlock_irq(&device_state_lock);
/* Run it through the hoops (find a driver, etc) */
@@ -4575,7 +4606,7 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
status = usb_new_device(udev);
if (status) {
spin_lock_irq(&device_state_lock);
- hub->ports[port1 - 1]->child = NULL;
+ port_dev->child = NULL;
spin_unlock_irq(&device_state_lock);
}
}
@@ -4617,13 +4648,14 @@ done:
static int hub_handle_remote_wakeup(struct usb_hub *hub, unsigned int port,
u16 portstatus, u16 portchange)
{
+ struct usb_port *port_dev = hub->ports[port - 1];
struct usb_device *hdev;
struct usb_device *udev;
int connect_change = 0;
int ret;
hdev = hub->hdev;
- udev = hub->ports[port - 1]->child;
+ udev = port_dev->child;
if (!hub_is_superspeed(hdev)) {
if (!(portchange & USB_PORT_STAT_C_SUSPEND))
return 0;
@@ -4639,9 +4671,11 @@ static int hub_handle_remote_wakeup(struct usb_hub *hub, unsigned int port,
/* TRSMRCY = 10 msec */
msleep(10);
+ usb_unlock_port(port_dev);
usb_lock_device(udev);
ret = usb_remote_wakeup(udev);
usb_unlock_device(udev);
+ usb_lock_port(port_dev);
if (ret < 0)
connect_change = 1;
} else {
@@ -4731,7 +4765,13 @@ static void port_event(struct usb_hub *hub, int port1)
/* take port actions that require the port to be powered on */
pm_runtime_get_noresume(&port_dev->dev);
pm_runtime_barrier(&port_dev->dev);
- if (pm_runtime_active(&port_dev->dev)) {
+ usb_lock_port(port_dev);
+ do if (pm_runtime_active(&port_dev->dev)) {
+ /* re-read portstatus now that we are in-sync with
+ * usb_port_{suspend|resume}
+ */
+ if (hub_port_status(hub, port1, &portstatus, &portchange) < 0)
+ break;
if (hub_handle_remote_wakeup(hub, port1,
portstatus, portchange))
connect_change = 1;
@@ -4751,17 +4791,22 @@ static void port_event(struct usb_hub *hub, int port1)
if (status < 0)
hub_port_disable(hub, port1, 1);
} else {
+ usb_unlock_port(port_dev);
usb_lock_device(udev);
status = usb_reset_device(udev);
usb_unlock_device(udev);
+ usb_lock_port(port_dev);
connect_change = 0;
}
}
- if (connect_change)
- hub_port_connect_change(hub, port1,
- portstatus, portchange);
- }
+ if (connect_change) {
+ hub_port_connect_change_unlock(hub, port1,
+ portstatus, portchange);
+ usb_lock_port(port_dev);
+ }
+ } while (0);
+ usb_unlock_port(port_dev);
pm_runtime_put_sync(&port_dev->dev);
}
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 5ba1bc16d4b7..e965474f2575 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -88,6 +88,7 @@ struct usb_port_location {
* @peer: related usb2 and usb3 ports (share the same connector)
* @connect_type: port's connect type
* @location: opaque representation of platform connector location
+ * @status_lock: synchronize port_event() vs usb_port_{suspend|resume}
* @portnum: port index num based one
* @power_is_on: port's power state
* @did_runtime_put: port has done pm_runtime_put().
@@ -99,6 +100,7 @@ struct usb_port {
struct usb_port *peer;
enum usb_port_connect_type connect_type;
struct usb_port_location location;
+ struct mutex status_lock;
u8 portnum;
unsigned power_is_on:1;
unsigned did_runtime_put:1;
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 6225ae4772d9..be9c4486816a 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -346,6 +346,7 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
port_dev->dev.type = &usb_port_device_type;
dev_set_name(&port_dev->dev, "port%d", port1);
device_initialize(&port_dev->dev);
+ mutex_init(&port_dev->status_lock);
peer = find_peer_port(hub, port1);
dev_dbg(&hub->hdev->dev, "port%d peer = %s\n",
--
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
