Hi,
when plugging and unplugging an ure(4) adapter continuously, I came
across the following error with URE_DEBUG set:
ure0 at uhub0 port 21 configuration 1 interface 0 "Realtek USB 10/100/1000
LAN" rev 3.00/30.00 addr 4
ure0: RTL8153 (0x5c10)ure_ctl: error 15
ure_ctl: error 15
ure_ctl: error 15
It waits for a timeout over and over again. The problem is that the
device has been unplugged, but the attach handler still issues USB
requests via ure_ctl(). While there is an early abort check with
usbd_is_dying() in that function, it does not work during attach. The
reason is that the attach handler runs from the USB exploration task.
But that task also handles device detach. So while the hardware port
change event has been noticed, it is up to the USB exploration task to
actually detach (and deactivate) the device. But it is still running
the ure(4) attach handler, which now happily waits for each USB request
to time out. And the ure(4) attach handler has a bunch of loops calling
ure_ctl(), which takes a really long time to complete altogether when
it waits for a timeout for each of them. During that time no other USB
device will get attached/detached either.
The following diff helps with debugging. Plug a ure(4) device and
unplug it within 10 seconds.
--- 8< ---
diff --git a/sys/dev/usb/if_ure.c b/sys/dev/usb/if_ure.c
index ea73db00954..93b76b421b3 100644
--- a/sys/dev/usb/if_ure.c
+++ b/sys/dev/usb/if_ure.c
@@ -1702,6 +1704,8 @@ ure_attach(struct device *parent, struct device *self,
void *aux)
break;
}
+ usbd_delay_ms(sc->ure_udev, 10000);
+
if (sc->ure_flags & URE_FLAG_8152)
ure_rtl8152_init(sc);
else if (sc->ure_flags & (URE_FLAG_8153B | URE_FLAG_8156))
--- 8< ---
The problem is that most of the callers of ure_ctl() do not check the
error return code, especially during attach. One solution could be to
propagate the error and handle it during attach. OTOH other USB device
drivers have the same problem. So one might argue that the USB stack is
at fault then. If we wanted to fix it in the USB stack, we would need
to decouple how port change events are handled. Maybe we could get away
with sneaking it into usbd_do_request(). But all of that looks rather
fragile and is well beyond me.
The easiest solution is to deactivate the device proactively in
ure_ctl() when we see that error there. Linux's rtl8152 does something
similar and checks if the device is unplugged [1][2].
[1]
https://elixir.bootlin.com/linux/v5.14-rc5/source/drivers/net/usb/r8152.c#L1249
[2]
https://elixir.bootlin.com/linux/v5.14-rc5/source/drivers/net/usb/r8152.c#L1293
Thanks to kevlo@ for helping me debug this.
So long,
- Christian
---
sys/dev/usb/if_ure.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletions(-)
diff --git a/sys/dev/usb/if_ure.c b/sys/dev/usb/if_ure.c
index ef2bc75a7c0..f5e0039648d 100644
--- a/sys/dev/usb/if_ure.c
+++ b/sys/dev/usb/if_ure.c
@@ -223,7 +223,7 @@ ure_ctl(struct ure_softc *sc, uint8_t rw, uint16_t val,
uint16_t index,
usbd_status err;
if (usbd_is_dying(sc->ure_udev))
- return 0;
+ return -1;
if (rw == URE_CTL_WRITE)
req.bmRequestType = UT_WRITE_VENDOR_DEVICE;
@@ -239,6 +239,8 @@ ure_ctl(struct ure_softc *sc, uint8_t rw, uint16_t val,
uint16_t index,
err = usbd_do_request(sc->ure_udev, &req, buf);
if (err) {
DPRINTF(("ure_ctl: error %d\n", err));
+ if (err == USBD_CANCELLED || err == USBD_TIMEOUT)
+ usbd_deactivate(sc->ure_udev);
return -1;
}
