On Fri, 2014-01-17 at 10:53 -0500, Alan Stern wrote:
> On Thu, 16 Jan 2014, Dan Williams wrote:
>
> > Maybe I need to consider it a bit longer, but introducing a per-port
> > mutex adds at least 2 new lock ordering constraints. A replacement of
> > hub->busy_bits with port_dev->lock now introduces a constraint with not
> > only the child lock, but also synchronous invocations of rpm_{suspend|
> > resume} for the port.
>
> That second constraint is very simple: Since the resume operation will
> acquire the port lock, you have to make sure you don't hold the port
> lock when performing a resume.
>
> > Patch 7 would look something like this:
> >
> > ---
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index d86548edcc36..da12d07da127 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -4740,6 +4740,13 @@ static void hub_events(void)
> >
> > /* deal with port status changes */
> > for (i = 1; i <= hdev->maxchild; i++) {
> > + mutex_lock(&port_dev->lock);
> > + if (port_dev->power_is_on)
> > + do_hub_event(...);
> > + else
> > + hub_clear_misc_change(..);
> > + mutex_unlock(&port_dev->lock);
> > +
> > [..]
>
> I wouldn't split out hub_clear_misc_change in quite that way, but never
> mind.
>
> Also, you left out the places in hub_events where the port lock needs
> to be dropped: around the calls to usb_reset_device and
> hub_port_connect_change. And you left out the places in the resume and
> reset routines where the port lock needs to be acquired.
>
> > diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> > index 97e4939fee1a..a7f32f200d90 100644
> > --- a/drivers/usb/core/port.c
> > +++ b/drivers/usb/core/port.c
> > @@ -88,7 +88,7 @@ static int usb_port_runtime_resume(struct device *dev)
> > pm_runtime_get_sync(&peer->dev);
> >
> > usb_autopm_get_interface(intf);
> > - set_bit(port1, hub->busy_bits);
> > + mutex_lock(&port_dev->lock);
> >
> > retval = usb_hub_set_port_power(hdev, hub, port1, true);
> > if (port_dev->child && !retval) {
> > @@ -105,7 +105,7 @@ static int usb_port_runtime_resume(struct device *dev)
> > retval = 0;
> > }
> >
> > - clear_bit(port1, hub->busy_bits);
> > + mutex_unlock(&port_dev->lock);
> > usb_autopm_put_interface(intf);
> >
> > if (!hub_is_superspeed(hdev) && peer)
> >
> > @@ -138,12 +138,12 @@ static int usb_port_runtime_suspend(struct device
> > *dev)
> > return -EBUSY;
> >
> > usb_autopm_get_interface(intf);
> > - set_bit(port1, hub->busy_bits);
> > + mutex_lock(&port_dev->lock);
> > retval = usb_hub_set_port_power(hdev, hub, port1, false);
> > usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_CONNECTION);
> > if (!hub_is_superspeed(hdev))
> > usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE);
> > - clear_bit(port1, hub->busy_bits);
> > + mutex_unlock(&port_dev->lock);
> > usb_autopm_put_interface(intf);
> >
> > /* bounce our peer now that we are down */
> > ---
>
> Yes, something like that.
>
> > ...not too bad, although I don't really like ->power_is_on compared to
> > just reading the pm runtime state. If we use pm_runtime_active() then
> > this begins to look a lot like patch 7 again.
> >
> > However, patch 9 wants khubd held off until both the port and any child
> > devices have had a chance to resume. It would expand the scope of the
> > lock from protecting concurrent port access to ordering port vs child
> > device resume. Patch 9 as coded does so without adding a locking
> > constraint (beyond flushing resume work). We certainly can't wrap a
> > port mutex around usb_autoresume_device(port_dev->child) given the child
> > synchronously resumes the port. What is the alternative I am missing?
>
> For one thing, like I mentioned above, usb_port_resume needs to hold
> the port lock. (And so does usb_reset_device, after it calls
> usb_autoresume_device.)
>
> But maybe I'm not getting your point.
>
> > Some nice benefits fall out from forcing a port+child resume cycle on
> > port resume:
> >
> > 1/ prevents usb_port_runtime_resume() from growing recovery logic that
> > usb_port_resume() implements, like reset and disconnect.
> >
> > 2/ similarly, if usb_port_resume() grows warm reset recovery logic (as
> > it seems to need) that is applicable to port power recovery as well.
> >
> > Leaning on pm_runtime synchronization to implement new hub_events()
> > requirements of "port pm active" and "flushes pending usb_device
> > recovery" is a tighter implementation. Specifically, it is tighter than
> > adding a new lock and its ordering constraints between suspend, resume,
> > child and potentially peer port events.
>
> If you mean that we should avoid duplicating code between
> usb_port_resume/finish_port_resume and the resume routines in port.c, I
> agree.
>
> The basic idea of using runtime PM synchronization to prevent khubd and
> port power operations from interfering sounds good, and it is simpler
> than adding a new mutex. And I think we can also use it to prevent
> port power operations and port resets from interfering. But that
> leaves open the question of how to prevent usb_device_reset from
> interfering with khubd.
>
Yes, patch 9 narrowly addresses a problem that needs a wider solution.
We want to:
1/ prevent khubd from running while reset is in progress
2/ prevent khubd from seeing an intermediate device state during
usb_port_resume
3/ prevent khubd from running in the interval between completion of
ubs_port_runtime_resume() completing and usb_port_resume() running.
All these scenarios to point to problems with ->device_state collisions
not necessarily the port state. How about a new lock that synchronizes
device state changes with state checks? This replaces patch 9, only
compile tested:
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 8d989b1d3dc5..edf30888e282 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1477,6 +1477,9 @@ void usb_autosuspend_device(struct usb_device *udev)
{
int status;
+ if (!udev)
+ return;
+
usb_mark_last_busy(udev);
status = pm_runtime_put_sync_autosuspend(&udev->dev);
dev_vdbg(&udev->dev, "%s: cnt %d -> %d\n",
@@ -1508,6 +1511,9 @@ int usb_autoresume_device(struct usb_device *udev)
{
int status;
+ if (!udev)
+ return 0;
+
status = pm_runtime_get_sync(&udev->dev);
if (status < 0)
pm_runtime_put_sync(&udev->dev);
@@ -1746,7 +1752,10 @@ int usb_runtime_suspend(struct device *dev)
if (autosuspend_check(udev) != 0)
return -EAGAIN;
+ /* lock the device to synchronize its state with khubd */
+ mutex_lock(&udev->state_lock);
status = usb_suspend_both(udev, PMSG_AUTO_SUSPEND);
+ mutex_unlock(&udev->state_lock);
/* Allow a retry if autosuspend failed temporarily */
if (status == -EAGAIN || status == -EBUSY)
@@ -1765,10 +1774,15 @@ int usb_runtime_resume(struct device *dev)
struct usb_device *udev = to_usb_device(dev);
int status;
- /* Runtime resume for a USB device means resuming both the device
- * and all its interfaces.
+ WARN_ON_ONCE(!mutex_is_locked(&dev->mutex));
+ /* Runtime resume for a USB device means resuming both the
+ * device and all its interfaces. We lock to synchronize the
+ * device state with khubd
*/
+ mutex_lock(&udev->state_lock);
status = usb_resume_both(udev, PMSG_AUTO_RESUME);
+ mutex_unlock(&udev->state_lock);
+
return status;
}
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 80661e20de9e..63d789558b1d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4382,7 +4382,7 @@ static void hub_port_connect_change(struct usb_hub *hub,
int port1,
unsigned wHubCharacteristics =
le16_to_cpu(hub->descriptor->wHubCharacteristics);
struct usb_device *udev;
- int status, i;
+ int status = -ENODEV, i;
unsigned unit_load;
dev_dbg (hub_dev,
@@ -4403,9 +4403,14 @@ static void hub_port_connect_change(struct usb_hub *hub,
int port1,
/* Try to resuscitate an existing device */
udev = hub->ports[port1 - 1]->child;
+
+ /* lock the device is case we need to wakeup, otherwise flush
+ * any pending state changes
+ */
+ usb_lock_device(udev);
+ usb_lock_device_state(udev);
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 */
@@ -4417,17 +4422,16 @@ static void hub_port_connect_change(struct usb_hub
*hub, int port1,
*/
status = usb_remote_wakeup(udev);
#endif
-
} else {
- status = -ENODEV; /* Don't resuscitate */
- }
- usb_unlock_device(udev);
-
- if (status == 0) {
- clear_bit(port1, hub->change_bits);
- return;
+ /* Don't resuscitate */;
}
}
+ usb_unlock_device_state(udev);
+ usb_unlock_device(udev);
+
+ clear_bit(port1, hub->change_bits);
+ if (status == 0)
+ return;
/* Disconnect any existing devices under this port */
if (udev) {
@@ -4436,7 +4440,6 @@ static void hub_port_connect_change(struct usb_hub *hub,
int port1,
usb_phy_notify_disconnect(hcd->phy, udev->speed);
usb_disconnect(&hub->ports[port1 - 1]->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.
@@ -4625,32 +4628,41 @@ static int hub_handle_remote_wakeup(struct usb_hub
*hub, unsigned int port,
hdev = hub->hdev;
udev = hub->ports[port - 1]->child;
+
+ /* lock the device since we may issue resume here, lock the
+ * device_state to synchronize with an in-flight pm operation
+ */
+ usb_lock_device(udev);
+ usb_lock_device_state(udev);
if (!hub_is_superspeed(hdev)) {
if (!(portchange & USB_PORT_STAT_C_SUSPEND))
- return 0;
+ goto out;
usb_clear_port_feature(hdev, port, USB_PORT_FEAT_C_SUSPEND);
} else {
if (!udev || udev->state != USB_STATE_SUSPENDED ||
(portstatus & USB_PORT_STAT_LINK_STATE) !=
USB_SS_PORT_LS_U0)
- return 0;
+ goto out;
}
if (udev) {
/* TRSMRCY = 10 msec */
msleep(10);
- usb_lock_device(udev);
ret = usb_remote_wakeup(udev);
- usb_unlock_device(udev);
if (ret < 0)
connect_change = 1;
} else {
ret = -ENODEV;
hub_port_disable(hub, port, 1);
}
+
dev_dbg(hub->intfdev, "resume on port %d, status %d\n",
port, ret);
+ out:
+ usb_unlock_device_state(udev);
+ usb_unlock_device(udev);
+
return connect_change;
}
@@ -4811,6 +4823,19 @@ static void hub_events(void)
/* deal with port status changes */
for_each_pm_active_port(i, port_dev, hub) {
+ struct usb_device *udev = port_dev->child;
+
+ /* usb_port_runtime_resume may have requested resume
make
+ * sure it has completed
+ */
+ if (port_dev->resume_child) {
+ port_dev->resume_child = 0;
+ usb_lock_device(udev);
+ usb_autoresume_device(udev);
+ usb_autosuspend_device(udev);
+ usb_unlock_device(udev);
+ }
+
if (test_bit(i, hub->busy_bits))
continue;
connect_change = test_bit(i, hub->change_bits);
@@ -4883,8 +4908,9 @@ static void hub_events(void)
*/
if (hub_port_warm_reset_required(hub, portstatus)) {
int status;
- struct usb_device *udev = port_dev->child;
+ usb_lock_device(udev);
+ usb_lock_device_state(udev);
dev_dbg(hub_dev, "warm reset port %d\n", i);
if (!udev ||
!(portstatus & USB_PORT_STAT_CONNECTION) ||
@@ -4895,11 +4921,11 @@ static void hub_events(void)
if (status < 0)
hub_port_disable(hub, i, 1);
} else {
- usb_lock_device(udev);
status = usb_reset_device(udev);
- usb_unlock_device(udev);
connect_change = 0;
}
+ usb_unlock_device_state(udev);
+ usb_unlock_device(udev);
}
if (connect_change)
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 2ba10798c943..3513548a0f06 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -90,6 +90,7 @@ struct usb_port_location {
* @location: opaque representation of platform connector location
* @portnum: port index num based one
* @power_is_on: port's power state
+ * @resume_child: set at resume to sync khubd with child recovery
* @did_runtime_put: port has done pm_runtime_put().
*/
struct usb_port {
@@ -101,6 +102,7 @@ struct usb_port {
struct usb_port_location location;
u8 portnum;
unsigned power_is_on:1;
+ unsigned resume_child:1;
unsigned did_runtime_put:1;
};
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 97e4939fee1a..602653015980 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -102,6 +102,7 @@ static int usb_port_runtime_resume(struct device *dev)
if (retval < 0)
dev_dbg(&port_dev->dev, "can't get reconnection after
setting port power on, status %d\n",
retval);
+ port_dev->resume_child = 1;
retval = 0;
}
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 4d1144990d4c..7e8e5591e852 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -426,6 +426,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
}
device_initialize(&dev->dev);
+ mutex_init(&dev->state_lock);
dev->dev.bus = &usb_bus_type;
dev->dev.type = &usb_device_type;
dev->dev.groups = usb_device_groups;
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 512ab162832c..d4e3b9b92085 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -445,6 +445,7 @@ struct usb3_lpm_parameters {
* @route: tree topology hex string for use with xHCI
* @state: device state: configured, not attached, etc.
* @speed: device speed: high/full/low (or error)
+ * @state_lock: sync khubd vs reset and resume
* @tt: Transaction Translator info; used with low/full speed dev, highspeed
hub
* @ttport: device port on that tt hub
* @toggle: one bit for each endpoint, with ([0] = IN, [1] = OUT) endpoints
@@ -513,6 +514,7 @@ struct usb_device {
u32 route;
enum usb_device_state state;
enum usb_device_speed speed;
+ struct mutex state_lock;
struct usb_tt *tt;
int ttport;
@@ -607,9 +609,37 @@ extern struct usb_device *usb_hub_find_child(struct
usb_device *hdev,
if (!child) continue; else
/* USB device locking */
-#define usb_lock_device(udev) device_lock(&(udev)->dev)
-#define usb_unlock_device(udev) device_unlock(&(udev)->dev)
-#define usb_trylock_device(udev) device_trylock(&(udev)->dev)
+static inline void usb_lock_device(struct usb_device *udev)
+{
+ if (udev)
+ device_lock(&udev->dev);
+}
+
+static inline void usb_unlock_device(struct usb_device *udev)
+{
+ if (udev)
+ device_unlock(&udev->dev);
+}
+
+static inline int usb_trylock_device(struct usb_device *udev)
+{
+ if (udev)
+ return device_trylock(&udev->dev);
+ return 0;
+}
+
+static inline void usb_lock_device_state(struct usb_device *udev)
+{
+ if (udev)
+ mutex_lock(&udev->state_lock);
+}
+
+static inline void usb_unlock_device_state(struct usb_device *udev)
+{
+ if (udev)
+ mutex_unlock(&udev->state_lock);
+}
+
extern int usb_lock_device_for_reset(struct usb_device *udev,
const struct usb_interface *iface);
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
