On Sat, 2014-01-18 at 08:59 -0500, Alan Stern wrote:
> On Fri, 17 Jan 2014, Dan Williams wrote:
>
> > > 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
>
> This is the open question mentioned above, if you add in the
> requirement that we also want to prevent a reset from starting while
> khubd is running (except when khubd performs the reset itself).
>
> > 2/ prevent khubd from seeing an intermediate device state during
> > usb_port_resume
>
> khubd doesn't pay all that much attention to device states; mostly it
> is concerned with port statuses. We don't want khubd to see an
> intermediate port status during usb_port_resume. Basically that means
> we don't want khubd to run while usb_port_resume is in progress, and we
> don't want usb_port_resume to start while khubd is running unless khubd
> performs the resume itself.
Yes, it does not pay all that much attention to device states, but it is
critical that it not read ->state while usb_port_resume() is changing
it. Specifically this hole in hub_port_connect_change():
> if ((portstatus & USB_PORT_STAT_CONNECTION) && udev &&
> udev->state != USB_STATE_NOTATTACHED) {
> if (portstatus & USB_PORT_STAT_ENABLE) {
> status = 0; /* Nothing to do */
>
> #ifdef CONFIG_PM_RUNTIME
...usb_port_resume() can change the state at this point and cause a
disconnect.
> } else if (udev->state == USB_STATE_SUSPENDED &&
> udev->persist_enabled) {
> /* For a suspended device, treat this as a
> * remote wakeup event.
> */
> status = usb_remote_wakeup(udev);
> #endif
> } else {
> /* Don't resuscitate */;
> }
> }
> > 3/ prevent khubd from running in the interval between completion of
> > ubs_port_runtime_resume() completing and usb_port_resume() running.
>
> Hmmm. I don't remember what usb_port_runtime_resume does about a
> connected USB-2 child device. It can't simply do a runtime resume of
> the child. Maybe it should tell khubd to resume the child? Then khubd
> would never see the intermediate state; the next time it looked at the
> port, it would issue the runtime resume. (Provided that
> usb_port_runtime_resume didn't complete after khubd was already
> running.)
Yes, that's what I had in the patch, more straightforward than the work
queue.
> > All these scenarios to point to problems with ->device_state collisions
> > not necessarily the port state.
>
> I don't think so, for the reason mentioned above.
I'll rephrase, checking USB_STATE_SUSPENDED needs usb_port_resume()
synchronization.
> And it is starting
> to seem less likely that we can rely on runtime PM synchronization to
> do everything we want. For example, if something is suspended then
> there is no way to prevent a runtime resume from occurring at any
> moment.
I propose we use usb_port pm runtime synchronization to block khubd when
we know the portstatus is not valid (powered off), and we need a new
lock to synchronize with usb_device pm operations before checking the
device state.
> > How about a new lock that synchronizes
> > device state changes with state checks? This replaces patch 9, only
> > compile tested:
>
> When considering overall strategies for solving a problem, I find that
> posting patches is almost never helpful. A patch focuses the mind on
> low-level coding details, many of which are irrelevant to the problem
> at hand, preventing you from concentrating on the overall picture --
> which is the most important thing at this stage of planning.
> Pseudo-code can be better in this regard, and I sometimes use it when a
> degree of precision is necessary in the discussion. However, I don't
> think we have reached that point yet.
Getting closer...
> Therefore I would greatly prefer if you could describe in prose what
> you want to do.
Ok.
I think we agree that khubd needs to not look at the portstatus while
the port is down. pm runtime synchronization takes care of that and
prevents khubd from starting while the port is inactive. With that in
place we can now make requests in usb_port_runtime_resume() that are
later serviced in khubd when the port is marked active (pm runtime
enforces that ->runtime_resume() return 0 before the port is marked
active). This mechanism can be used for forcing at least one child
resume for each port resume, and rate-limiting port power requests.
For the next case, fixing khubd vs device_state changes, we can't use
pm_runtime synchronization because khubd is expected to be able to
resume a suspended usb_device. So we need to prevent khubd from
evaluating the device_state (USB_STATE_SUSPENDED) while usb_device pm
operations are in flight. In the case of dpm_{suspend|resume} khubd is
frozen so there's no collision, in the case of rpm_{suspend|resume} we
need to take a lock.
Because dpm ops are performed with the device_lock held,
usb_autoresume_device() requires callers to hold the device_lock when
triggering rpm ops. If it was guaranteed that usb_port_resume() is
always called with the device_lock held we could simply use the
device_lock to sync with khubd. However, that assumption can be
trivially violated by resuming a device via sysfs or marking a child
interface active. Even if that was fixed it's not a workable solution
because other locked device core operations (probe, etc) would be
disallowed from triggering rpm ops. We need a new lock.
Let's wrap usb_runtime_{resume|suspend} in a new usb_device->state_lock
and then take that lock in khubd whenever it needs to check ->state or
portstatus values associated with suspend. Specifically:
@@ -1765,10 +1774,14 @@ 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.
+ /* 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;
}
The lock hangs off the usb_device rather than the the usb_port since it
is meant to prevent unintended disconnects. Any portstatus vs khubd
collisions are handled by pm runtime synchronization since there is no
expectation that khubd resumes a usb_port.
Once khubd has made a determination that it wants to resume or reset a
device it will of course need to drop the state lock. Those actions
will implicitly take the state lock.
Here's the patch (to replace patch 9), now tested. There are of course
cleanups that can follow this, but holding off until we have agreement
on closing these races.
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 8d989b1d3dc5..1ae5a7a05685 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,14 @@ 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.
+ /* 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..be5d0d0d7d22 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 */
@@ -4415,19 +4420,20 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
/* For a suspended device, treat this as a
* remote wakeup event.
*/
+ usb_unlock_device_state(udev);
status = usb_remote_wakeup(udev);
+ usb_lock_device_state(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 +4442,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,24 +4630,30 @@ 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);
+ usb_unlock_device_state(udev);
ret = usb_remote_wakeup(udev);
- usb_unlock_device(udev);
+ usb_lock_device_state(udev);
if (ret < 0)
connect_change = 1;
} else {
@@ -4651,6 +4662,10 @@ static int hub_handle_remote_wakeup(struct usb_hub *hub, unsigned int port,
}
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 +4826,20 @@ static void hub_events(void)
/* deal with port status changes */
for_each_pm_active_port(i, port_dev, hub) {
+ struct usb_device *udev = ACCESS_ONCE(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);
+ pm_runtime_put_sync(&port_dev->dev);
+ }
+
if (test_bit(i, hub->busy_bits))
continue;
connect_change = test_bit(i, hub->change_bits);
@@ -4883,8 +4912,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 +4925,14 @@ static void hub_events(void)
if (status < 0)
hub_port_disable(hub, i, 1);
} else {
- usb_lock_device(udev);
+ usb_unlock_device_state(udev);
status = usb_reset_device(udev);
- usb_unlock_device(udev);
+ usb_lock_device_state(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..dfe00819fbb6 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -102,6 +102,13 @@ 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);
+
+ /* keep this port awake until we have had a chance to recover
+ * the child
+ */
+ pm_runtime_get_noresume(&port_dev->dev);
+ port_dev->resume_child = 1;
+ usb_kick_khubd(hdev);
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/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 9992fbfec85f..2333ec573594 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1004,9 +1004,16 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
xhci_disable_port(hcd, xhci, wIndex,
port_array[wIndex], temp);
break;
- case USB_PORT_FEAT_POWER:
- writel(temp & ~PORT_POWER, port_array[wIndex]);
+ case USB_PORT_FEAT_POWER: {
+ struct xhci_bus_state *bus_state;
+ bus_state = &xhci->bus_state[hcd_index(hcd)];
+ writel(temp & ~PORT_POWER, port_array[wIndex]);
+ if (test_and_clear_bit(wIndex, &bus_state->resuming_ports)) {
+ bus_state->resume_done[wIndex] = 0;
+ xhci_dbg(xhci, "port%d: resume cancelled\n",
+ wIndex);
+ }
spin_unlock_irqrestore(&xhci->lock, flags);
temp = usb_acpi_power_manageable(hcd->self.root_hub,
wIndex);
@@ -1015,6 +1022,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
wIndex, false);
spin_lock_irqsave(&xhci->lock, flags);
break;
+ }
default:
goto error;
}
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
