From e43f45d412de8a90a71596980d0d63838ece51af Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 22:10:56 +0000 Subject: [PATCH 01/24] usbnet: Simplify usbnet_isdying. usbnet_detach (or its caller) stops all users before it returns. If un->un_pri is null at this point, there's a bug -- something didn't wait for everything to finish before calling usbnet_detach. --- sys/dev/usb/usbnet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 7a2732f07e2a..8f6e955e9e89 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1318,7 +1318,7 @@ usbnet_havelink(struct usbnet *un) bool usbnet_isdying(struct usbnet *un) { - return un->un_pri == NULL || un->un_pri->unp_dying; + return un->un_pri->unp_dying; } From f497d908756e0858eb4982543bac32fb1da5b92a Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 22:56:01 +0000 Subject: [PATCH 02/24] usbnet: Set and clear IFF_RUNNING slightly earlier and later. - Set IFF_RUNNING before any calls to usbnet_rxeof are possible. - Don't clear IFF_RUNNING until all transfers have been aborted. --- sys/dev/usb/usbnet.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 8f6e955e9e89..8598a9357a40 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -850,9 +850,6 @@ usbnet_init_rx_tx(struct usbnet * const un) goto out; } - /* Start up the receive pipe(s). */ - usbnet_rx_start_pipes(un); - /* Indicate we are up and running. */ #if 0 /* XXX if_mcast_op() can call this without ifnet locked */ @@ -860,6 +857,9 @@ usbnet_init_rx_tx(struct usbnet * const un) #endif ifp->if_flags |= IFF_RUNNING; + /* Start up the receive pipe(s). */ + usbnet_rx_start_pipes(un); + callout_schedule(&unp->unp_stat_ch, hz); out: @@ -1122,14 +1122,6 @@ usbnet_stop(struct usbnet *un, struct ifnet *ifp, int disable) uno_stop(un, ifp, disable); - /* - * XXXSMP Would like to - * KASSERT(IFNET_LOCKED(ifp)) - * here but the locking order is: - * ifnet -> core_lock -> rxlock -> txlock - * and core_lock is already held. - */ - ifp->if_flags &= ~IFF_RUNNING; unp->unp_timer = 0; callout_halt(&unp->unp_stat_ch, &unp->unp_core_lock); @@ -1146,6 +1138,15 @@ usbnet_stop(struct usbnet *un, struct ifnet *ifp, int disable) /* Close pipes. */ usbnet_ep_close_pipes(un); + /* + * XXXSMP Would like to + * KASSERT(IFNET_LOCKED(ifp)) + * here but the locking order is: + * ifnet -> core_lock -> rxlock -> txlock + * and core_lock is already held. + */ + ifp->if_flags &= ~IFF_RUNNING; + usbnet_unbusy(un); } From 5f4767ce32b012973932ea24733031363e6e364d Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 22:59:10 +0000 Subject: [PATCH 03/24] usbnet: Take IFNET_LOCK around access to if_flags in usbnet_detach. This is not stable without IFNET_LOCK. Extraneous calls to usbnet_stop arising from this race might be harmless, but let's render it unnecessary to even think about that. --- sys/dev/usb/usbnet.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 8598a9357a40..97f7459afa25 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1550,11 +1550,11 @@ usbnet_detach(device_t self, int flags) unp->unp_dying = true; mutex_exit(&unp->unp_core_lock); + IFNET_LOCK(ifp); if (ifp->if_flags & IFF_RUNNING) { - IFNET_LOCK(ifp); usbnet_if_stop(ifp, 1); - IFNET_UNLOCK(ifp); } + IFNET_UNLOCK(ifp); callout_halt(&unp->unp_stat_ch, NULL); usb_rem_task_wait(un->un_udev, &unp->unp_ticktask, USB_TASKQ_DRIVER, From d2bb5b1dc9de643f3e857a07d66db83adca2ed14 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 23:01:27 +0000 Subject: [PATCH 04/24] usbnet: Ensure ifp->if_softc is initialized _before_ publishing ifp. Otherwise other parts of the system might start using ifp the moment we if_register it, and trip over null if_softc. --- sys/dev/usb/usbnet.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 97f7459afa25..46b14c07ce30 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -80,6 +80,7 @@ struct usbnet_private { bool unp_dying; bool unp_stopping; bool unp_attached; + bool unp_ifp_attached; bool unp_link; int unp_refcnt; @@ -1488,7 +1489,9 @@ usbnet_attach_ifp(struct usbnet *un, struct ifnet * const ifp = usbnet_ifp(un); KASSERT(unp->unp_attached); + KASSERT(!unp->unp_ifp_attached); + ifp->if_softc = un; strlcpy(ifp->if_xname, device_xname(un->un_dev), IFNAMSIZ); ifp->if_flags = if_flags; ifp->if_extflags = IFEF_MPSAFE | if_extflags; @@ -1507,6 +1510,7 @@ usbnet_attach_ifp(struct usbnet *un, if (ifp->_if_input == NULL) ifp->if_percpuq = if_percpuq_create(ifp); if_register(ifp); + unp->unp_ifp_attached = true; /* * If ethernet address is all zero, skip ether_ifattach() and @@ -1524,7 +1528,6 @@ usbnet_attach_ifp(struct usbnet *un, /* Now ready, and attached. */ IFQ_SET_READY(&ifp->if_snd); - ifp->if_softc = un; usbd_add_drv_event(USB_EVENT_DRIVER_ATTACH, un->un_udev, un->un_dev); @@ -1580,7 +1583,7 @@ usbnet_detach(device_t self, int flags) mii_detach(mii, MII_PHY_ANY, MII_OFFSET_ANY); ifmedia_fini(&mii->mii_media); } - if (ifp->if_softc) { + if (unp->unp_ifp_attached) { if (!usbnet_empty_eaddr(un)) ether_ifdetach(ifp); else From 059efad454ac7ee238a5fc222401c8e976b43627 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 23:08:03 +0000 Subject: [PATCH 05/24] usbnet: Ensure access to unp_timer is protected by unp_txlock. --- sys/dev/usb/usbnet.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 46b14c07ce30..9951b76e0692 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1123,7 +1123,9 @@ usbnet_stop(struct usbnet *un, struct ifnet *ifp, int disable) uno_stop(un, ifp, disable); + mutex_enter(&unp->unp_txlock); unp->unp_timer = 0; + mutex_exit(&unp->unp_txlock); callout_halt(&unp->unp_stat_ch, &unp->unp_core_lock); usb_rem_task_wait(un->un_udev, &unp->unp_ticktask, USB_TASKQ_DRIVER, @@ -1235,7 +1237,10 @@ usbnet_tick_task(void *arg) usbnet_busy(un); mutex_exit(&unp->unp_core_lock); - if (unp->unp_timer != 0 && --unp->unp_timer == 0) + mutex_enter(&unp->unp_txlock); + const bool timeout = unp->unp_timer != 0 && --unp->unp_timer == 0; + mutex_exit(&unp->unp_txlock); + if (timeout) usbnet_watchdog(ifp); DPRINTFN(8, "mii %#jx ifp %#jx", (uintptr_t)mii, (uintptr_t)ifp, 0, 0); From 5fc1d0b2ab5f128ab83d6212b7635fe35eba5bf0 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 23:34:52 +0000 Subject: [PATCH 06/24] usbnet: Assert IFNET_LOCKED on if_flags change callbacks. - if_init - if_stop - ethersubr(9) ifflags_cb --- sys/dev/usb/usbnet.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 9951b76e0692..fe28e7796895 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1006,6 +1006,8 @@ usbnet_ifflags_cb(struct ethercom *ec) struct usbnet_private * const unp = un->un_pri; int rv = 0; + KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname); + mutex_enter(&unp->unp_core_lock); const u_short changed = ifp->if_flags ^ unp->unp_if_flags; @@ -1159,6 +1161,8 @@ usbnet_if_stop(struct ifnet *ifp, int disable) struct usbnet * const un = ifp->if_softc; struct usbnet_private * const unp = un->un_pri; + KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname); + mutex_enter(&unp->unp_core_lock); usbnet_stop(un, ifp, disable); mutex_exit(&unp->unp_core_lock); @@ -1268,6 +1272,8 @@ usbnet_if_init(struct ifnet *ifp) USBNETHIST_FUNC(); USBNETHIST_CALLED(); struct usbnet * const un = ifp->if_softc; + KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname); + return uno_init(un, ifp); } From 5a9ef2381aad519ea39ee092e20d6e57aac51cf3 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 23:52:41 +0000 Subject: [PATCH 07/24] usbnet: Assert IFNET_LOCKED in usbnet_init_rx_tx, usbnet_stop. Exception: urndis(4) abuses this API to start this logic before the ifp is actually initialized. So for the sake of urndis(4), until sense can be beaten into it, allow the !unp_ifp_attached case to run without IFNET_LOCK. --- sys/dev/usb/usbnet.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index fe28e7796895..d5ad6e277c16 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -151,6 +151,8 @@ fail: static void uno_stop(struct usbnet *un, struct ifnet *ifp, int disable) { + KASSERTMSG(!un->un_pri->unp_ifp_attached || IFNET_LOCKED(ifp), + "%s", ifp->if_xname); usbnet_isowned_core(un); if (un->un_ops->uno_stop) (*un->un_ops->uno_stop)(ifp, disable); @@ -820,6 +822,9 @@ usbnet_init_rx_tx(struct usbnet * const un) usbd_status err; int error = 0; + KASSERTMSG(!unp->unp_ifp_attached || IFNET_LOCKED(ifp), + "%s", ifp->if_xname); + usbnet_isowned_core(un); if (unp->unp_dying) { @@ -1113,6 +1118,8 @@ usbnet_stop(struct usbnet *un, struct ifnet *ifp, int disable) USBNETHIST_FUNC(); USBNETHIST_CALLED(); + KASSERTMSG(!unp->unp_ifp_attached || IFNET_LOCKED(ifp), + "%s", ifp->if_xname); usbnet_isowned_core(un); usbnet_busy(un); @@ -1143,13 +1150,9 @@ usbnet_stop(struct usbnet *un, struct ifnet *ifp, int disable) /* Close pipes. */ usbnet_ep_close_pipes(un); - /* - * XXXSMP Would like to - * KASSERT(IFNET_LOCKED(ifp)) - * here but the locking order is: - * ifnet -> core_lock -> rxlock -> txlock - * and core_lock is already held. - */ + /* Everything is quesced now. */ + KASSERTMSG(!unp->unp_ifp_attached || IFNET_LOCKED(ifp), + "%s", ifp->if_xname); ifp->if_flags &= ~IFF_RUNNING; usbnet_unbusy(un); From 581648fc8c01e608d45d23f37567d608b49d6261 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 22:50:37 +0000 Subject: [PATCH 08/24] usbnet: Don't check if_flags for IFF_RUNNING in usbnet_rxeof. This can only run after we start the pipes in usbnet_init_rx_tx, and before we abort the pipes in usbnet_stop, during which time if_flags & IFF_RUNNING is stably set. --- sys/dev/usb/usbnet.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index d5ad6e277c16..6021cec5cf18 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -337,7 +337,6 @@ usbnet_rxeof(struct usbd_xfer *xfer, void *priv, usbd_status status) struct usbnet_chain * const c = priv; struct usbnet * const un = c->unc_un; struct usbnet_private * const unp = un->un_pri; - struct ifnet * const ifp = usbnet_ifp(un); uint32_t total_len; USBNETHIST_CALLARGSN(5, "%jd: enter: status %#jx xfer %#jx", @@ -347,7 +346,7 @@ usbnet_rxeof(struct usbd_xfer *xfer, void *priv, usbd_status status) if (unp->unp_dying || unp->unp_stopping || status == USBD_INVAL || status == USBD_NOT_STARTED || - status == USBD_CANCELLED || !(ifp->if_flags & IFF_RUNNING)) + status == USBD_CANCELLED) goto out; if (status != USBD_NORMAL_COMPLETION) { From 6908daf40fc3a87df943c764c0175cbad025b6a4 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 22:52:49 +0000 Subject: [PATCH 09/24] usbnet: Don't check if_flags for IFF_RUNNING in usbnet_pipe_intr. The one user of this interface in tree, aue(4), doesn't care -- if_statinc is safe whether IFF_RUNNING or not. --- sys/dev/usb/usbnet.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 6021cec5cf18..25e3ec460cf5 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -443,11 +443,10 @@ usbnet_pipe_intr(struct usbd_xfer *xfer, void *priv, usbd_status status) struct usbnet * const un = priv; struct usbnet_private * const unp = un->un_pri; struct usbnet_intr * const uni = un->un_intr; - struct ifnet * const ifp = usbnet_ifp(un); if (uni == NULL || unp->unp_dying || unp->unp_stopping || status == USBD_INVAL || status == USBD_NOT_STARTED || - status == USBD_CANCELLED || !(ifp->if_flags & IFF_RUNNING)) { + status == USBD_CANCELLED) { USBNETHIST_CALLARGS("%jd: uni %#jx d/s %#jx status %#jx", unp->unp_number, (uintptr_t)uni, (unp->unp_dying << 8) | unp->unp_stopping, status); From 87cc8266f5428b8b71fafa7ccfc1e9ac25d91b3e Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sat, 25 Dec 2021 12:52:21 +0000 Subject: [PATCH 10/24] usbnet: Omit needless unp == NULL test in usbnet_tick_task. The task is never scheduled until after un->un_pri is initialized, and un->un_pri isn't nulled until after the callout and task are quiescent. --- sys/dev/usb/usbnet.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 25e3ec460cf5..23b0cc538848 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1223,9 +1223,6 @@ usbnet_tick_task(void *arg) struct usbnet * const un = arg; struct usbnet_private * const unp = un->un_pri; - if (unp == NULL) - return; - USBNETHIST_CALLARGSN(8, "%jd: enter", unp->unp_number, 0, 0, 0); mutex_enter(&unp->unp_core_lock); From acad622f0a46b7f03be9c1a3fb6133095568fd5a Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sat, 25 Dec 2021 20:31:02 +0000 Subject: [PATCH 11/24] axen(4), mue(4), smsc(4): Omit irrelevant cases in ioctl. SIOCSIFFLAGS and SIOCSETHERCAP always end up in ether_ioctl_reinit, which triggers the same logic to reprogram the multicast filters anyway. --- sys/dev/usb/if_axen.c | 2 -- sys/dev/usb/if_mue.c | 2 -- sys/dev/usb/if_smsc.c | 2 -- 3 files changed, 6 deletions(-) diff --git a/sys/dev/usb/if_axen.c b/sys/dev/usb/if_axen.c index bf91d56b356d..fe433722d185 100644 --- a/sys/dev/usb/if_axen.c +++ b/sys/dev/usb/if_axen.c @@ -559,8 +559,6 @@ axen_uno_ioctl(struct ifnet *ifp, u_long cmd, void *data) usbnet_busy(un); switch (cmd) { - case SIOCSIFFLAGS: - case SIOCSETHERCAP: case SIOCADDMULTI: case SIOCDELMULTI: axen_setiff_locked(un); diff --git a/sys/dev/usb/if_mue.c b/sys/dev/usb/if_mue.c index 559b9eff2569..16a52e4c6ed4 100644 --- a/sys/dev/usb/if_mue.c +++ b/sys/dev/usb/if_mue.c @@ -1273,8 +1273,6 @@ mue_uno_ioctl(struct ifnet *ifp, u_long cmd, void *data) usbnet_busy(un); switch (cmd) { - case SIOCSIFFLAGS: - case SIOCSETHERCAP: case SIOCADDMULTI: case SIOCDELMULTI: mue_setiff_locked(un); diff --git a/sys/dev/usb/if_smsc.c b/sys/dev/usb/if_smsc.c index 6cf37e507d1c..12eaf3fa3cc6 100644 --- a/sys/dev/usb/if_smsc.c +++ b/sys/dev/usb/if_smsc.c @@ -757,8 +757,6 @@ smsc_uno_ioctl(struct ifnet *ifp, u_long cmd, void *data) usbnet_busy(un); switch (cmd) { - case SIOCSIFFLAGS: - case SIOCSETHERCAP: case SIOCADDMULTI: case SIOCDELMULTI: smsc_setiff_locked(un); From cc0bb441770427c2ff329c8b541ad2a0e223d425 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 00:52:09 +0000 Subject: [PATCH 12/24] usbnet: Remove usbnet_set_dying. Not necessary for the one caller that did it (url(4)): usbnet_detach handles failed attach just fine without it. --- sys/dev/usb/if_url.c | 8 +------- sys/dev/usb/usbnet.c | 6 ------ sys/dev/usb/usbnet.h | 1 - 3 files changed, 1 insertion(+), 14 deletions(-) diff --git a/sys/dev/usb/if_url.c b/sys/dev/usb/if_url.c index 764b9c654682..dbb57bf66ca7 100644 --- a/sys/dev/usb/if_url.c +++ b/sys/dev/usb/if_url.c @@ -256,18 +256,12 @@ url_attach(device_t parent, device_t self, void *aux) usbnet_unlock_core(un); if (err) { aprint_error_dev(self, "read MAC address failed\n"); - goto bad; + return; } /* initialize interface information */ usbnet_attach_ifp(un, IFF_SIMPLEX | IFF_BROADCAST | IFF_MULTICAST, 0, &unm); - - return; - - bad: - usbnet_set_dying(un, true); - return; } /* read/write memory */ diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 23b0cc538848..f8448f1be6f7 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1284,12 +1284,6 @@ usbnet_set_link(struct usbnet *un, bool link) un->un_pri->unp_link = link; } -void -usbnet_set_dying(struct usbnet *un, bool link) -{ - un->un_pri->unp_dying = link; -} - struct ifnet * usbnet_ifp(struct usbnet *un) { diff --git a/sys/dev/usb/usbnet.h b/sys/dev/usb/usbnet.h index 15e9dfc7b351..1faa822887c5 100644 --- a/sys/dev/usb/usbnet.h +++ b/sys/dev/usb/usbnet.h @@ -284,7 +284,6 @@ struct usbnet { /* Various accessors. */ void usbnet_set_link(struct usbnet *, bool); -void usbnet_set_dying(struct usbnet *, bool); struct ifnet *usbnet_ifp(struct usbnet *); struct ethercom *usbnet_ec(struct usbnet *); From b61f683824fa9f1cde7c7e29123983a64d43f745 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 01:17:06 +0000 Subject: [PATCH 13/24] usbnet: Fix ordering of actions in usbnet_stop. Make sure all software activity is quiescent (callouts and tasks, including ifmedia and mii callbacks -- anything that might trigger register access) before asking the driver to stop the hardware. This way, the driver uno_stop routine is guaranteed exclusive access to the registers. This will also enable us to simplify the callouts and tasks so they don't have to check the software state -- to be done in a separate commit. --- sys/dev/usb/usbnet.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index f8448f1be6f7..ae30f752dff3 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1128,16 +1128,29 @@ usbnet_stop(struct usbnet *un, struct ifnet *ifp, int disable) mutex_exit(&unp->unp_txlock); mutex_exit(&unp->unp_rxlock); + /* + * Stop the timer first, then the task -- if the timer was + * already firing, we stop the task or wait for it complete + * only after if last fired. Setting unp_stopping prevents the + * timer task from being scheduled again. + */ + callout_halt(&unp->unp_stat_ch, &unp->unp_core_lock); + usb_rem_task_wait(un->un_udev, &unp->unp_ticktask, USB_TASKQ_DRIVER, + &unp->unp_core_lock); + + /* + * Now that the software is quiescent, ask the driver to stop + * the hardware. The driver's uno_stop routine now has + * exclusive access to any registers that might previously have + * been used by to ifmedia, mii, or ioctl callbacks. + */ uno_stop(un, ifp, disable); + /* Clear the watchdog timer. */ mutex_enter(&unp->unp_txlock); unp->unp_timer = 0; mutex_exit(&unp->unp_txlock); - callout_halt(&unp->unp_stat_ch, &unp->unp_core_lock); - usb_rem_task_wait(un->un_udev, &unp->unp_ticktask, USB_TASKQ_DRIVER, - &unp->unp_core_lock); - /* Stop transfers. */ usbnet_ep_stop_pipes(un); From 346d0cdaae09a5ebc1b3925f36d0800de4fa4a18 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 01:22:46 +0000 Subject: [PATCH 14/24] usbnet: Assert IFNET_LOCKED in usbnet_media_upd. This ensures, if the device is being initialized or stopped, usbnet_media_upd will not run until it's done, so the reset sequence has exclusive access to the device registers used by mii. --- sys/dev/usb/usbnet.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index ae30f752dff3..1bab776c734f 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -983,6 +983,9 @@ usbnet_media_upd(struct ifnet *ifp) /* ifmedia layer ensures core_lock is held. */ usbnet_isowned_core(un); + /* ifmedia changes only with IFNET_LOCK held. */ + KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname); + if (unp->unp_dying) return EIO; From 882ad027c9ec3cf9b428849330b4fc1cc6574503 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 01:29:16 +0000 Subject: [PATCH 15/24] usbnet: Set unp_dying under IFNET_LOCK. Refuse to bring the interface back up, which happens under IFNET_LOCK, if it's dying. This ensures new activity on the interface can't happen by the time we stop existing activity and wait for it to complete. --- sys/dev/usb/usbnet.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 1bab776c734f..883339fb998e 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1288,6 +1288,13 @@ usbnet_if_init(struct ifnet *ifp) KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname); + /* + * Prevent anyone from bringing the interface back up once + * we're detaching. + */ + if (un->un_pri->unp_dying) + return EIO; + return uno_init(un, ifp); } @@ -1568,11 +1575,15 @@ usbnet_detach(device_t self, int flags) struct ifnet * const ifp = usbnet_ifp(un); struct mii_data * const mii = usbnet_mii(un); + /* + * Prevent new activity. If we're still running on the + * network, stop and wait for all asynchronous activity to + * finish. + */ + IFNET_LOCK(ifp); mutex_enter(&unp->unp_core_lock); unp->unp_dying = true; mutex_exit(&unp->unp_core_lock); - - IFNET_LOCK(ifp); if (ifp->if_flags & IFF_RUNNING) { usbnet_if_stop(ifp, 1); } From 299364d9e87300e342160ffed1eb247498e34587 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 01:32:42 +0000 Subject: [PATCH 16/24] usbnet: Omit needless callout_halt and usb_rem_task_wait. The callout and tasks cannot be pending at this point -- it is a bug if usbnet_if_stop failed to quiesce everything, so turn these into KASSERTs. --- sys/dev/usb/usbnet.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 883339fb998e..2ce48a68c857 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1589,9 +1589,13 @@ usbnet_detach(device_t self, int flags) } IFNET_UNLOCK(ifp); - callout_halt(&unp->unp_stat_ch, NULL); - usb_rem_task_wait(un->un_udev, &unp->unp_ticktask, USB_TASKQ_DRIVER, - NULL); + /* + * The callout and tick task can't be scheduled anew at this + * point, and usbnet_if_stop has waited for them to complete. + */ + KASSERT(!callout_pending(&unp->unp_stat_ch)); + KASSERT(!usb_task_pending(un->un_udev, &unp->unp_ticktask)); + usb_rem_task_wait(un->un_udev, &unp->unp_mcasttask, USB_TASKQ_DRIVER, NULL); From fb4046f26fd2d15bf5160c6c259b520a25d87f9f Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 01:47:10 +0000 Subject: [PATCH 17/24] usbnet: Detach interface and mii before waiting for refcnt to drain. All outstanding software activity under usbnet's control -- which is all that participates in the refcnting -- should be quiesced by stopping and detaching everything. --- sys/dev/usb/usbnet.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 2ce48a68c857..79176c7ee205 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1599,20 +1599,6 @@ usbnet_detach(device_t self, int flags) usb_rem_task_wait(un->un_udev, &unp->unp_mcasttask, USB_TASKQ_DRIVER, NULL); - mutex_enter(&unp->unp_core_lock); - unp->unp_refcnt--; - while (unp->unp_refcnt >= 0) { - /* Wait for processes to go away */ - cv_wait(&unp->unp_detachcv, &unp->unp_core_lock); - } - mutex_exit(&unp->unp_core_lock); - - usbnet_rx_list_free(un); - usbnet_tx_list_free(un); - - callout_destroy(&unp->unp_stat_ch); - rnd_detach_source(&unp->unp_rndsrc); - if (mii) { mii_detach(mii, MII_PHY_ANY, MII_OFFSET_ANY); ifmedia_fini(&mii->mii_media); @@ -1646,6 +1632,20 @@ usbnet_detach(device_t self, int flags) usb_rem_task_wait(un->un_udev, &unp->unp_mcasttask, USB_TASKQ_DRIVER, NULL); + mutex_enter(&unp->unp_core_lock); + unp->unp_refcnt--; + while (unp->unp_refcnt >= 0) { + /* Wait for processes to go away */ + cv_wait(&unp->unp_detachcv, &unp->unp_core_lock); + } + mutex_exit(&unp->unp_core_lock); + + usbnet_rx_list_free(un); + usbnet_tx_list_free(un); + + callout_destroy(&unp->unp_stat_ch); + rnd_detach_source(&unp->unp_rndsrc); + cv_destroy(&unp->unp_detachcv); mutex_destroy(&unp->unp_core_lock); mutex_destroy(&unp->unp_rxlock); From 44d9fdb3c7fe840904971b7c2d4a705d818293e4 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 01:49:56 +0000 Subject: [PATCH 18/24] usbnet: Make detach order reverse attach order, for unp_stat_ch. No functional change intended. --- sys/dev/usb/usbnet.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 79176c7ee205..4bc2a7d1b8c2 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1643,7 +1643,6 @@ usbnet_detach(device_t self, int flags) usbnet_rx_list_free(un); usbnet_tx_list_free(un); - callout_destroy(&unp->unp_stat_ch); rnd_detach_source(&unp->unp_rndsrc); cv_destroy(&unp->unp_detachcv); @@ -1651,6 +1650,8 @@ usbnet_detach(device_t self, int flags) mutex_destroy(&unp->unp_rxlock); mutex_destroy(&unp->unp_txlock); + callout_destroy(&unp->unp_stat_ch); + pmf_device_deregister(un->un_dev); usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, un->un_udev, un->un_dev); From efc80ae6b9f345442b9b7f6f884f1dcd63508384 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 01:50:32 +0000 Subject: [PATCH 19/24] usbnet: Don't issue a detach event if we never issued an attach one. --- sys/dev/usb/usbnet.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 4bc2a7d1b8c2..ce5605b78872 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1654,7 +1654,14 @@ usbnet_detach(device_t self, int flags) pmf_device_deregister(un->un_dev); - usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, un->un_udev, un->un_dev); + /* + * Notify userland that we're going away, if we arrived in the + * first place. + */ + if (unp->unp_ifp_attached) { + usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, un->un_udev, + un->un_dev); + } kmem_free(unp, sizeof(*unp)); un->un_pri = NULL; From f9db9d5c3d7f3397c67bce1a74aadcd2723e08c2 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 01:50:59 +0000 Subject: [PATCH 20/24] usbnet: Uncomment and fix assertion for ifp->if_flags |= IFF_RUNNING. We always hold IFNET_LOCK for ioctls that end up here -- the ones that don't hold it are only SIOCADDMULTI/SIOCDELMULTI, which don't end up here. However, urndis(4) throws a spanner in the works by doing weird device initialization. --- sys/dev/usb/usbnet.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index ce5605b78872..030c5ce3cc2d 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -855,10 +855,9 @@ usbnet_init_rx_tx(struct usbnet * const un) } /* Indicate we are up and running. */ -#if 0 - /* XXX if_mcast_op() can call this without ifnet locked */ - KASSERT(ifp->if_softc == NULL || IFNET_LOCKED(ifp)); -#endif + /* XXX urndis calls usbnet_init_rx_tx before usbnet_attach_ifp. */ + KASSERTMSG(!unp->unp_ifp_attached || IFNET_LOCKED(ifp), + "%s", ifp->if_xname); ifp->if_flags |= IFF_RUNNING; /* Start up the receive pipe(s). */ From f326e8e03f773a4ce9f1040aad1e88ffdc7bad7d Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 22:07:39 +0000 Subject: [PATCH 21/24] usbnet: Omit needless tests in usbnet_tick. It's harmless for us to schedule the tick task even if unp_dying or unp_stopping is set by now, because usbnet_stop will just wait for it to finish anyway, and the callout can't be scheduled again until the interface is done stopping and is brought back up again. No need for unp == NULL test -- un->un_pri is initialized well before this callout can be scheduled, and is nulled out only at the end of usbnet_detach, at which point we have already halted this callout. --- sys/dev/usb/usbnet.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 030c5ce3cc2d..250c660125f3 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1199,10 +1199,8 @@ usbnet_tick(void *arg) USBNETHIST_CALLARGSN(10, "%jd: enter", unp->unp_number, 0, 0, 0); - if (unp != NULL && !unp->unp_stopping && !unp->unp_dying) { - /* Perform periodic stuff in process context */ - usb_add_task(un->un_udev, &unp->unp_ticktask, USB_TASKQ_DRIVER); - } + /* Perform periodic stuff in process context */ + usb_add_task(un->un_udev, &unp->unp_ticktask, USB_TASKQ_DRIVER); } static void From 69b303c08ef68f1b4f05040e68a11e7859473e1b Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 01:54:38 +0000 Subject: [PATCH 22/24] usbnet: Omit needless locking/busying/testing in usbnet_tick_task. usbnet_stop waits for the task to complete before resetting the hardware, and usbnet_detach waits for usbnet_stop to complete before destroying anything, so there's no need for any of this. --- sys/dev/usb/usbnet.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 250c660125f3..587750cbc136 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1235,22 +1235,10 @@ usbnet_tick_task(void *arg) USBNETHIST_FUNC(); struct usbnet * const un = arg; struct usbnet_private * const unp = un->un_pri; - - USBNETHIST_CALLARGSN(8, "%jd: enter", unp->unp_number, 0, 0, 0); - - mutex_enter(&unp->unp_core_lock); - if (unp->unp_stopping || unp->unp_dying) { - mutex_exit(&unp->unp_core_lock); - return; - } - struct ifnet * const ifp = usbnet_ifp(un); struct mii_data * const mii = usbnet_mii(un); - KASSERT(ifp != NULL); /* embedded member */ - - usbnet_busy(un); - mutex_exit(&unp->unp_core_lock); + USBNETHIST_CALLARGSN(8, "%jd: enter", unp->unp_number, 0, 0, 0); mutex_enter(&unp->unp_txlock); const bool timeout = unp->unp_timer != 0 && --unp->unp_timer == 0; @@ -1271,7 +1259,6 @@ usbnet_tick_task(void *arg) uno_tick(un); mutex_enter(&unp->unp_core_lock); - usbnet_unbusy(un); if (!unp->unp_stopping && !unp->unp_dying) callout_schedule(&unp->unp_stat_ch, hz); mutex_exit(&unp->unp_core_lock); From 68aacaaccb7c8a01758d807e9e2cc86317876e41 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 10:06:50 +0000 Subject: [PATCH 23/24] usbnet: Clear watchdog timer before stopping hardware. No need to take the lock again -- which might not be necessary because the callout and task have completed, but let's obviate the need to think about that. --- sys/dev/usb/usbnet.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 587750cbc136..0f251e3dac78 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1124,9 +1124,14 @@ usbnet_stop(struct usbnet *un, struct ifnet *ifp, int disable) usbnet_busy(un); + /* + * Prevent new activity (rescheduling ticks, xfers, &c.) and + * clear the watchdog timer. + */ mutex_enter(&unp->unp_rxlock); mutex_enter(&unp->unp_txlock); unp->unp_stopping = true; + unp->unp_timer = 0; mutex_exit(&unp->unp_txlock); mutex_exit(&unp->unp_rxlock); @@ -1148,11 +1153,6 @@ usbnet_stop(struct usbnet *un, struct ifnet *ifp, int disable) */ uno_stop(un, ifp, disable); - /* Clear the watchdog timer. */ - mutex_enter(&unp->unp_txlock); - unp->unp_timer = 0; - mutex_exit(&unp->unp_txlock); - /* Stop transfers. */ usbnet_ep_stop_pipes(un); From e1a4844c91a88ff563210f415b41024281e0c209 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 01:56:30 +0000 Subject: [PATCH 24/24] WIP: usbnet: Print diagnostic about refcnt stragglers. I don't think there can be any, but this message, if printed, would falsify my hypothesis! --- sys/dev/usb/usbnet.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c index 0f251e3dac78..9bc85759bd18 100644 --- a/sys/dev/usb/usbnet.c +++ b/sys/dev/usb/usbnet.c @@ -1618,6 +1618,10 @@ usbnet_detach(device_t self, int flags) mutex_enter(&unp->unp_core_lock); unp->unp_refcnt--; + if (unp->unp_refcnt >= 0) { + aprint_error_dev(un->un_dev, "%d stragglers\n", + unp->unp_refcnt + 1); + } while (unp->unp_refcnt >= 0) { /* Wait for processes to go away */ cv_wait(&unp->unp_detachcv, &unp->unp_core_lock);