From riastradh@netbsd.org Thu Aug 8 16:57:12 2024 To: Phil Nelson , Martin Husemann Subject: Re: ieee80211_swbmiss In-reply-to: <20240808133828.8293A84D5D@mail.netbsd.org> (riastradh@NetBSD.org) Date: Thu, 8 Aug 2024 16:57:06 +0000 From: Taylor R Campbell MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-Id: <20240808165711.005E584D7D@mail.netbsd.org> [resending to martin@ who got dropped from cc somewhere upthread] Some notes from a thread in August 2022 which for some reason you weren't cc'd on (and maybe should have been done on tech-net): 1. IEEE80211_LOCK is forbidden in interrupt/softint/callout. Should add KASSERT(!cpu_intr_p()); KASSERT(!cpu_softintr_p()) to it. 2. Need a new lock to serialize access to ic_vaps, say ic_vapslock, for use in ieee80211_start/stop/suspend/resume_all in order to avoid IFNET_LOCK vs IEEE80211_LOCK reversal. (E.g., ieee80211_suspend_all currently holds IEEE80211_LOCK while it calls ieee80211_stop_locked which reads ifp->if_flags which requires IFNET_LOCK, but ieee80211_init holds IFNET_LOCK while it takes IEEE80211_LOCK. Instead, ieee80211_suspend_all should take ic_vapslock to iterate, then take IFNET_LOCK for each interface, then take IEEE80211_LOCK, then do ieee80211_stop_locked.) If it's inconvenient to change all users of ic_vaps, the access rule can be: (a) Read (TAILQ_FOREACH) requires ic_vapslock _or_ IEEE80211_LOCK. (b) Write (TAILQ_INSERT/REMOVE) requires ic_vapslock _and_ IEEE80211_LOCK. 3. Anywhere in paths during packet processing like tx, rx, or mii tick that needs access to state currently serialized by IEEE82011_LOCK needs a separate lock, like the usbnet unp_txlock/rxlock/miilock -- and if necessary to avoid unlocked access to things like ifp->if_flags, a cache of the state like unp_rxstopped/txstopped. Any state machine transitions requiring IEEE80211_LOCK need to be deferred to thread context. 4. Lock order is: ic_vapslock -> IFNET_LOCK -> IEEE80211_LOCK -> {txlock, rxlock, miilock, ...}. And, generally, if you are tempted to work around lock recursion by unlock/subroutine/lock, don't -- either fix the context or split the subroutine into a subroutine_locked variant like we did with ieee80211_new_state_locked. From riastradh@netbsd.org Thu Aug 8 19:34:32 2024 To: Phil Nelson CC: Martin Husemann In-reply-to: <20240808181813.C33E684E7B@mail.netbsd.org> (riastradh@NetBSD.org) Subject: Re: ieee80211_swbmiss Date: Thu, 8 Aug 2024 19:34:28 +0000 From: Taylor R Campbell MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-Id: <20240808193431.B4F9484EA8@mail.netbsd.org> Something else from the thread in August 2022 that might be helpful is this sketch of the normal flow of things in interface bringup, usage, and teardown: 1. (IFNET_LOCK) if_init: (a) resets hardware (b) programs multicast filter (c) starts Tx/Rx and tick for mii/watchdog (d) sets IFF_RUNNING 2. concurrently: - (mii lock) periodic mii ticks or (with IFNET_LOCK too) ifmedia ioctls - (tx lock) Tx submissions - (rx lock) Rx notifications - (multicast filter lock) SIOCADDMULTI/SIOCDELMULTI - (ic lock) 802.11 state machine notifications - (IFNET_LOCK) maybe other ioctls (some of which return ENETRESET to cause an if_stop/init cycle in thread context) 3. (IFNET_LOCK) if_stop: (a) clears IFF_RUNNING (b) halts Tx/Rx/tick and waits for completion (c) disables concurrent multicast updates (d) calls mii_down (e) resets hardware There's a similar flow for usbnet(9) sketched in the man page: . usbwifi(9) will presumably need something like usbnet(9)'s usage flow written down too. Of course, net80211 has an extra layer, the 802.11 device and all of its network interfaces, so there may be a slightly more complicated state diagram. But the same general ideas hold: - IFNET_LOCK serializes bringup/teardown or other ifconfig-like configuration changes to each interface independently, which may involve asking the tx/rx/miitick/... paths to stop and waiting for them to complete; - IEEE82011_LOCK serializes bringup/teardown or other ifconfig-like configuration changes or state machine transitions in the 802.11 device, which may involve asking the tx/rx/miitick/... paths to stop and waiting for them to complete; and - the finer-grained locks (or pserialize or psref or other types of access paths) are for when the interface is running and processing data in the tx/rx/miitick/... paths. From riastradh@netbsd.org Fri Aug 9 12:22:39 2024 To: Martin Husemann CC: Phil Nelson In-reply-to: <20240809063437.GC29452@mail.duskware.de> (martin@duskware.de) Subject: Re: Quick wifi update .... Date: Fri, 9 Aug 2024 12:22:32 +0000 From: Taylor R Campbell MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-Id: <20240809122237.5988884D74@mail.netbsd.org> > Date: Fri, 9 Aug 2024 08:34:37 +0200 > From: Martin Husemann > > On Thu, Aug 08, 2024 at 11:04:16PM -0700, Phil Nelson wrote: > > The above is on an rpi4. (as you can see) I got urtwn0 coming up > > reliably on the amd64 now. I have a much better idea of the locking > > issues for urtwn. In the init phase, I've caused all init code to > > do busy waiting rather than a sleep/wakeup kind of delay. That > > allows it to start reliably. > > nat@ had a simillar change and I'd like to understand why it helps better. > > I have two urtwn devices and one is coming up reliably without any init > hacks, while the other randomly fails but no hacks so far changed anything > about that besides the propability. This is the same on the older (non > wifi branch) code. > > Is this just the "can't hold IC lock across kpause()" point that Taylor > braught up? The lock should not be needed during init (or easily avoided). Sort of, but you have the rule wrong: the rule is that the ic lock (IEEE80211_LOCK) is forbidden in softint/callout context _because_ it (correctly) is used across things like kpause and USB requests to configure the 802.11 device, just like IFNET_LOCK for a network interface. So, as I noted in a previous message: 3. Anywhere in paths during packet processing like tx, rx, or mii tick that needs access to state currently serialized by IEEE82011_LOCK needs a separate lock, like the usbnet unp_txlock/rxlock/miilock -- and if necessary to avoid unlocked access to things like ifp->if_flags, a cache of the state like unp_rxstopped/txstopped. All the interesting state machine transitions are performed in thread context, not in softint/callout.