Re: USB suspend/resume race
On 19/01/10(Tue) 01:07, Jean-Philippe Ouellet wrote: On 26/05/14(Mon) 13:46, Martin Pieuchot wrote: I'd appreciate if people having troubles with suspend/resume could try this diff an report back. Fixes it for me! :D Many thanks. You're welcome. Previous diff was lacking the header chunk, please use this one instead. Was the corresponding commit missing the ehci.c changes from this diff on purpose? Yes, it was just a debugging chunk I'm thinking about a proper change for all the *hci drivers.
Re: USB suspend/resume race
On 26/05/14(Mon) 13:46, Martin Pieuchot wrote: I'd appreciate if people having troubles with suspend/resume could try this diff an report back. Fixes it for me! :D Many thanks. Previous diff was lacking the header chunk, please use this one instead. Was the corresponding commit missing the ehci.c changes from this diff on purpose?
USB suspend/resume race
It is currently possible to trigger a race between the thread doing
DVACT_QUIESCE and the USB thread exploring the buses.
This race is really easy to reproduce if you have a lot of controllers
and you try to suspend just after resuming. In the best case, it blows
your kernel during suspend, in the worst case it blows it at resume :)
So here's a diff to avoid such race by deferring the action of detaching
the root hub to the USB thread doing exploration.
I'd appreciate if people having troubles with suspend/resume could try
this diff an report back.
Martin
Index: usb.c
===
RCS file: /home/ncvs/src/sys/dev/usb/usb.c,v
retrieving revision 1.96
diff -u -p -r1.96 usb.c
--- usb.c 11 May 2014 16:33:21 - 1.96
+++ usb.c 26 May 2014 11:19:31 -
@@ -260,11 +260,15 @@ usb_attach_roothub(struct usb_softc *sc)
void
usb_detach_roothub(struct usb_softc *sc)
{
- /* Make all devices disconnect. */
- if (sc-sc_port.device != NULL)
- usb_disconnect_port(sc-sc_port, (struct device *)sc);
+ /*
+* To avoid races with the usb task thread, mark the root hub
+* as disconnecting and schedule an exploration task to detach
+* it.
+*/
+ sc-sc_bus-flags |= USB_BUS_DISCONNECTING;
+ usb_needs_explore(sc-sc_bus-root_hub, 0);
- usb_rem_wait_task(sc-sc_bus-root_hub, sc-sc_explore_task);
+ usb_wait_task(sc-sc_bus-root_hub, sc-sc_explore_task);
sc-sc_bus-root_hub = NULL;
}
@@ -840,7 +844,18 @@ usb_explore(void *v)
usb_delay_ms(sc-sc_bus, pwrdly - waited_ms);
}
- sc-sc_bus-root_hub-hub-explore(sc-sc_bus-root_hub);
+ if (sc-sc_bus-flags USB_BUS_DISCONNECTING) {
+ /* Prevent new tasks from being scheduled. */
+ sc-sc_bus-dying = 1;
+
+ /* Make all devices disconnect. */
+ if (sc-sc_port.device != NULL)
+ usb_disconnect_port(sc-sc_port, (struct device *)sc);
+
+ sc-sc_bus-flags = ~USB_BUS_DISCONNECTING;
+ } else {
+ sc-sc_bus-root_hub-hub-explore(sc-sc_bus-root_hub);
+ }
if (sc-sc_bus-flags USB_BUS_CONFIG_PENDING) {
DPRINTF((%s: %s: first explore done\n, __func__,
@@ -896,7 +911,6 @@ usb_activate(struct device *self, int ac
switch (act) {
case DVACT_QUIESCE:
- sc-sc_bus-dying = 1;
if (sc-sc_bus-root_hub != NULL)
usb_detach_roothub(sc);
break;
@@ -914,10 +928,6 @@ usb_activate(struct device *self, int ac
usb_needs_explore(sc-sc_bus-root_hub, 0);
sc-sc_bus-use_polling--;
break;
- case DVACT_DEACTIVATE:
- rv = config_activate_children(self, act);
- sc-sc_bus-dying = 1;
- break;
default:
rv = config_activate_children(self, act);
break;
@@ -929,10 +939,6 @@ int
usb_detach(struct device *self, int flags)
{
struct usb_softc *sc = (struct usb_softc *)self;
-
- DPRINTF((usb_detach: start\n));
-
- sc-sc_bus-dying = 1;
if (sc-sc_bus-root_hub != NULL) {
usb_detach_roothub(sc);
Index: uhub.c
===
RCS file: /home/ncvs/src/sys/dev/usb/uhub.c,v
retrieving revision 1.66
diff -u -p -r1.66 uhub.c
--- uhub.c 11 Mar 2014 10:24:42 - 1.66
+++ uhub.c 26 May 2014 11:19:31 -
@@ -546,6 +546,9 @@ uhub_intr(struct usbd_xfer *xfer, void *
{
struct uhub_softc *sc = addr;
+ if (usbd_is_dying(sc-sc_hub))
+ return;
+
DPRINTF(%s: intr status=%d\n, sc-sc_dev.dv_xname, status);
if (status == USBD_STALLED)
usbd_clear_endpoint_stall_async(sc-sc_ipipe);
Index: ehci.c
===
RCS file: /home/ncvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.155
diff -u -p -r1.155 ehci.c
--- ehci.c 16 May 2014 19:00:18 - 1.155
+++ ehci.c 26 May 2014 11:19:31 -
@@ -1075,6 +1075,18 @@ ehci_activate(struct device *self, int a
ehci_reset(sc);
sc-sc_bus.use_polling--;
+
+ if (!TAILQ_EMPTY(sc-sc_intrhead)) {
+ printf(%s: intr head not empty\n,
+ sc-sc_bus.bdev.dv_xname);
+ return (EINVAL);
+ }
+ if (sc-sc_intrxfer != NULL) {
+ printf(%s: root intr not null\n,
+ sc-sc_bus.bdev.dv_xname);
+ return (EINVAL);
+ }
+
break;
case DVACT_RESUME:
sc-sc_bus.use_polling++;
Re: USB suspend/resume race
On 26/05/14(Mon) 13:46, Martin Pieuchot wrote:
It is currently possible to trigger a race between the thread doing
DVACT_QUIESCE and the USB thread exploring the buses.
This race is really easy to reproduce if you have a lot of controllers
and you try to suspend just after resuming. In the best case, it blows
your kernel during suspend, in the worst case it blows it at resume :)
So here's a diff to avoid such race by deferring the action of detaching
the root hub to the USB thread doing exploration.
I'd appreciate if people having troubles with suspend/resume could try
this diff an report back.
Previous diff was lacking the header chunk, please use this one instead.
Martin
Index: usb.c
===
RCS file: /home/ncvs/src/sys/dev/usb/usb.c,v
retrieving revision 1.96
diff -u -p -r1.96 usb.c
--- usb.c 11 May 2014 16:33:21 - 1.96
+++ usb.c 26 May 2014 11:19:31 -
@@ -260,11 +260,15 @@ usb_attach_roothub(struct usb_softc *sc)
void
usb_detach_roothub(struct usb_softc *sc)
{
- /* Make all devices disconnect. */
- if (sc-sc_port.device != NULL)
- usb_disconnect_port(sc-sc_port, (struct device *)sc);
+ /*
+* To avoid races with the usb task thread, mark the root hub
+* as disconnecting and schedule an exploration task to detach
+* it.
+*/
+ sc-sc_bus-flags |= USB_BUS_DISCONNECTING;
+ usb_needs_explore(sc-sc_bus-root_hub, 0);
- usb_rem_wait_task(sc-sc_bus-root_hub, sc-sc_explore_task);
+ usb_wait_task(sc-sc_bus-root_hub, sc-sc_explore_task);
sc-sc_bus-root_hub = NULL;
}
@@ -840,7 +844,18 @@ usb_explore(void *v)
usb_delay_ms(sc-sc_bus, pwrdly - waited_ms);
}
- sc-sc_bus-root_hub-hub-explore(sc-sc_bus-root_hub);
+ if (sc-sc_bus-flags USB_BUS_DISCONNECTING) {
+ /* Prevent new tasks from being scheduled. */
+ sc-sc_bus-dying = 1;
+
+ /* Make all devices disconnect. */
+ if (sc-sc_port.device != NULL)
+ usb_disconnect_port(sc-sc_port, (struct device *)sc);
+
+ sc-sc_bus-flags = ~USB_BUS_DISCONNECTING;
+ } else {
+ sc-sc_bus-root_hub-hub-explore(sc-sc_bus-root_hub);
+ }
if (sc-sc_bus-flags USB_BUS_CONFIG_PENDING) {
DPRINTF((%s: %s: first explore done\n, __func__,
@@ -896,7 +911,6 @@ usb_activate(struct device *self, int ac
switch (act) {
case DVACT_QUIESCE:
- sc-sc_bus-dying = 1;
if (sc-sc_bus-root_hub != NULL)
usb_detach_roothub(sc);
break;
@@ -914,10 +928,6 @@ usb_activate(struct device *self, int ac
usb_needs_explore(sc-sc_bus-root_hub, 0);
sc-sc_bus-use_polling--;
break;
- case DVACT_DEACTIVATE:
- rv = config_activate_children(self, act);
- sc-sc_bus-dying = 1;
- break;
default:
rv = config_activate_children(self, act);
break;
@@ -929,10 +939,6 @@ int
usb_detach(struct device *self, int flags)
{
struct usb_softc *sc = (struct usb_softc *)self;
-
- DPRINTF((usb_detach: start\n));
-
- sc-sc_bus-dying = 1;
if (sc-sc_bus-root_hub != NULL) {
usb_detach_roothub(sc);
Index: uhub.c
===
RCS file: /home/ncvs/src/sys/dev/usb/uhub.c,v
retrieving revision 1.66
diff -u -p -r1.66 uhub.c
--- uhub.c 11 Mar 2014 10:24:42 - 1.66
+++ uhub.c 26 May 2014 11:19:31 -
@@ -546,6 +546,9 @@ uhub_intr(struct usbd_xfer *xfer, void *
{
struct uhub_softc *sc = addr;
+ if (usbd_is_dying(sc-sc_hub))
+ return;
+
DPRINTF(%s: intr status=%d\n, sc-sc_dev.dv_xname, status);
if (status == USBD_STALLED)
usbd_clear_endpoint_stall_async(sc-sc_ipipe);
Index: ehci.c
===
RCS file: /home/ncvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.155
diff -u -p -r1.155 ehci.c
--- ehci.c 16 May 2014 19:00:18 - 1.155
+++ ehci.c 26 May 2014 11:19:31 -
@@ -1075,6 +1075,18 @@ ehci_activate(struct device *self, int a
ehci_reset(sc);
sc-sc_bus.use_polling--;
+
+ if (!TAILQ_EMPTY(sc-sc_intrhead)) {
+ printf(%s: intr head not empty\n,
+ sc-sc_bus.bdev.dv_xname);
+ return (EINVAL);
+ }
+ if (sc-sc_intrxfer != NULL) {
+ printf(%s: root intr not null\n,
+ sc-sc_bus.bdev.dv_xname);
+ return (EINVAL);
+ }
+
break;
case DVACT_RESUME:
Re: USB suspend/resume race
On Mon, May 26, 2014 at 1:51 PM, Martin Pieuchot mpieuc...@nolizard.orgwrote: On 26/05/14(Mon) 13:46, Martin Pieuchot wrote: It is currently possible to trigger a race between the thread doing DVACT_QUIESCE and the USB thread exploring the buses. This race is really easy to reproduce if you have a lot of controllers and you try to suspend just after resuming. In the best case, it blows your kernel during suspend, in the worst case it blows it at resume :) So here's a diff to avoid such race by deferring the action of detaching the root hub to the USB thread doing exploration. I'd appreciate if people having troubles with suspend/resume could try this diff an report back. Previous diff was lacking the header chunk, please use this one instead. Hi Martin, This seems to fix the freezes I encounter since some time when suspending my laptop. Thanks
