| Message ID | 20220108005533.947787-1-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rtw88: prepare locking for SDIO support | expand |
Hi, > -----Original Message----- > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Sent: Saturday, January 8, 2022 8:55 AM > To: linux-wireless@vger.kernel.org > Cc: tony0620emma@gmail.com; kvalo@codeaurora.org; johannes@sipsolutions.net; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org; Neo Jou <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; > Pkshih <pkshih@realtek.com>; Ed Swierk <eswierk@gh.st>; Martin Blumenstingl > <martin.blumenstingl@googlemail.com> > Subject: [PATCH v3 0/8] rtw88: prepare locking for SDIO support > [...] I do stressed test of connection and suspend, and it get stuck after about 4 hours but no useful messages. I will re-build my kernel and turn on lockdep debug to see if it can tell me what is wrong. -- Ping-Ke
> -----Original Message----- > From: Pkshih > Sent: Wednesday, January 19, 2022 5:38 PM > To: 'Martin Blumenstingl' <martin.blumenstingl@googlemail.com>; linux-wireless@vger.kernel.org > Cc: tony0620emma@gmail.com; kvalo@codeaurora.org; johannes@sipsolutions.net; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org; Neo Jou <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; Ed > Swierk <eswierk@gh.st> > Subject: RE: [PATCH v3 0/8] rtw88: prepare locking for SDIO support > > Hi, > > > -----Original Message----- > > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > Sent: Saturday, January 8, 2022 8:55 AM > > To: linux-wireless@vger.kernel.org > > Cc: tony0620emma@gmail.com; kvalo@codeaurora.org; johannes@sipsolutions.net; netdev@vger.kernel.org; > > linux-kernel@vger.kernel.org; Neo Jou <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; > > Pkshih <pkshih@realtek.com>; Ed Swierk <eswierk@gh.st>; Martin Blumenstingl > > <martin.blumenstingl@googlemail.com> > > Subject: [PATCH v3 0/8] rtw88: prepare locking for SDIO support > > > > [...] > > I do stressed test of connection and suspend, and it get stuck after about > 4 hours but no useful messages. I will re-build my kernel and turn on lockdep debug > to see if it can tell me what is wrong. > I found some deadlock: [ 4891.169653] CPU0 CPU1 [ 4891.169732] ---- ---- [ 4891.169799] lock(&rtwdev->mutex); [ 4891.169874] lock(&local->sta_mtx); [ 4891.169948] lock(&rtwdev->mutex); [ 4891.170050] lock(&local->sta_mtx); [ 4919.598630] CPU0 CPU1 [ 4919.598715] ---- ---- [ 4919.598779] lock(&local->iflist_mtx); [ 4919.598900] lock(&rtwdev->mutex); [ 4919.598995] lock(&local->iflist_mtx); [ 4919.599092] lock(&rtwdev->mutex); So, I add wrappers to iterate rtw_iterate_stas() and rtw_iterate_vifs() that use _atomic version to collect sta and vif, and use list_for_each() to iterate. Reference code is attached, and I'm still thinking if we can have better method. -- Ping-Ke
Hi Ping-Ke, On Fri, Jan 21, 2022 at 9:10 AM Pkshih <pkshih@realtek.com> wrote: [...] > > > > I do stressed test of connection and suspend, and it get stuck after about > > 4 hours but no useful messages. I will re-build my kernel and turn on lockdep debug > > to see if it can tell me what is wrong. First of all: thank you so much for testing this and investigating the deadlock! > I found some deadlock: > > [ 4891.169653] CPU0 CPU1 > [ 4891.169732] ---- ---- > [ 4891.169799] lock(&rtwdev->mutex); > [ 4891.169874] lock(&local->sta_mtx); > [ 4891.169948] lock(&rtwdev->mutex); > [ 4891.170050] lock(&local->sta_mtx); > > > [ 4919.598630] CPU0 CPU1 > [ 4919.598715] ---- ---- > [ 4919.598779] lock(&local->iflist_mtx); > [ 4919.598900] lock(&rtwdev->mutex); > [ 4919.598995] lock(&local->iflist_mtx); > [ 4919.599092] lock(&rtwdev->mutex); This looks similar to the problem fixed by 5b0efb4d670c8b ("rtw88: avoid circular locking between local->iflist_mtx and rtwdev->mutex") which you have pointed out earlier. It seems to me that we should avoid using the mutex version of ieee80211_iterate_*() because it can lead to more of these issues. So from my point of view the general idea of the code from your attached patch looks good. That said, I'm still very new to mac80211/cfg80211 so I'm also interested in other's opinions. > So, I add wrappers to iterate rtw_iterate_stas() and rtw_iterate_vifs() that > use _atomic version to collect sta and vif, and use list_for_each() to iterate. > Reference code is attached, and I'm still thinking if we can have better method. With "better method" do you mean something like in patch #2 from this series (using unsigned int num_si and struct rtw_sta_info *si[RTW_MAX_MAC_ID_NUM] inside the iter_data) are you thinking of a better way in general? Best regards, Martin
Hi, > -----Original Message----- > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Sent: Monday, January 24, 2022 3:04 AM > To: Pkshih <pkshih@realtek.com> > Cc: linux-wireless@vger.kernel.org; tony0620emma@gmail.com; kvalo@codeaurora.org; > johannes@sipsolutions.net; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Neo Jou > <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; Ed Swierk <eswierk@gh.st> > Subject: Re: [PATCH v3 0/8] rtw88: prepare locking for SDIO support > > Hi Ping-Ke, > > On Fri, Jan 21, 2022 at 9:10 AM Pkshih <pkshih@realtek.com> wrote: > [...] > > > > > > I do stressed test of connection and suspend, and it get stuck after about > > > 4 hours but no useful messages. I will re-build my kernel and turn on lockdep debug > > > to see if it can tell me what is wrong. > First of all: thank you so much for testing this and investigating the deadlock! > > > I found some deadlock: > > > > [ 4891.169653] CPU0 CPU1 > > [ 4891.169732] ---- ---- > > [ 4891.169799] lock(&rtwdev->mutex); > > [ 4891.169874] lock(&local->sta_mtx); > > [ 4891.169948] lock(&rtwdev->mutex); > > [ 4891.170050] lock(&local->sta_mtx); > > > > > > [ 4919.598630] CPU0 CPU1 > > [ 4919.598715] ---- ---- > > [ 4919.598779] lock(&local->iflist_mtx); > > [ 4919.598900] lock(&rtwdev->mutex); > > [ 4919.598995] lock(&local->iflist_mtx); > > [ 4919.599092] lock(&rtwdev->mutex); > This looks similar to the problem fixed by 5b0efb4d670c8b ("rtw88: > avoid circular locking between local->iflist_mtx and rtwdev->mutex") > which you have pointed out earlier. > It seems to me that we should avoid using the mutex version of > ieee80211_iterate_*() because it can lead to more of these issues. So > from my point of view the general idea of the code from your attached > patch looks good. That said, I'm still very new to mac80211/cfg80211 > so I'm also interested in other's opinions. > The attached patch can work "mostly", because both callers of iterate() and ::remove_interface hold rtwdev->mutex. Theoretically, the exception is a caller forks another work to iterate() between leaving ::remove_interface and mac80211 doesn't yet free the vif, but the work executes after mac80211 free the vif. This will lead use-after-free, but I'm not sure if this scenario will happen. I need time to dig this, or you can help to do this. To avoid this, we can add a flag to struct rtw_vif, and set this flag when ::remove_interface. Then, only collect vif without this flag into list when we use iterate_actiom(). As well as ieee80211_sta can do similar fix. > > So, I add wrappers to iterate rtw_iterate_stas() and rtw_iterate_vifs() that > > use _atomic version to collect sta and vif, and use list_for_each() to iterate. > > Reference code is attached, and I'm still thinking if we can have better method. > With "better method" do you mean something like in patch #2 from this > series (using unsigned int num_si and struct rtw_sta_info > *si[RTW_MAX_MAC_ID_NUM] inside the iter_data) are you thinking of a > better way in general? > I would like a straight method, for example, we can have another version of ieee80211_iterate_xxx() and do things in iterator, like original, so we just need to change the code slightly. Initially, I have an idea we can hold driver lock, like rtwdev->mutex, in both places where we use ieee80211_iterate_() and remove sta or vif. Hopefully, this can ensure it's safe to run iterator without other locks. Then, we can define another ieee80211_iterate_() version with a drv_lock argument, like #define ieee80211_iterate_active_interfaces_drv_lock(hw, iter_flags, iterator, data, drv_lock) \ while (0) { \ lockdep_assert_wiphy(drv_lock); \ ieee80211_iterate_active_interfaces_no_lock(hw, iter_flags, iterator, data); \ } The driv_lock argument can avoid user forgetting to hold a lock, and we need a helper of no_lock version: void ieee80211_iterate_active_interfaces_no_lock( struct ieee80211_hw *hw, u32 iter_flags, void (*iterator)(void *data, u8 *mac, struct ieee80211_vif *vif), void *data) { struct ieee80211_local *local = hw_to_local(hw); __iterate_interfaces(local, iter_flags | IEEE80211_IFACE_ITER_ACTIVE, iterator, data); } However, as I mentioned theoretically it is not safe entirely. So, I think the easiest way is to maintains the vif/sta lists in driver when ::{add,remove }_interface/::sta_{add,remove}, and hold rtwdev->mutex lock to access these lists. But, Johannes pointed out this is not a good idea [1]. [1] https://lore.kernel.org/linux-wireless/d61f3947cddec660cbb2a59e2424d2bd8c01346a.camel@sipsolutions.net/ -- Ping-Ke
Hi Ping-Ke, On Mon, Jan 24, 2022 at 3:59 AM Pkshih <pkshih@realtek.com> wrote: [...] > > It seems to me that we should avoid using the mutex version of > > ieee80211_iterate_*() because it can lead to more of these issues. So > > from my point of view the general idea of the code from your attached > > patch looks good. That said, I'm still very new to mac80211/cfg80211 > > so I'm also interested in other's opinions. > > > > The attached patch can work "mostly", because both callers of iterate() and > ::remove_interface hold rtwdev->mutex. Theoretically, the exception is a caller > forks another work to iterate() between leaving ::remove_interface and mac80211 > doesn't yet free the vif, but the work executes after mac80211 free the vif. > This will lead use-after-free, but I'm not sure if this scenario will happen. > I need time to dig this, or you can help to do this. > > To avoid this, we can add a flag to struct rtw_vif, and set this flag > when ::remove_interface. Then, only collect vif without this flag into list > when we use iterate_actiom(). > > As well as ieee80211_sta can do similar fix. > > > > So, I add wrappers to iterate rtw_iterate_stas() and rtw_iterate_vifs() that > > > use _atomic version to collect sta and vif, and use list_for_each() to iterate. > > > Reference code is attached, and I'm still thinking if we can have better method. > > With "better method" do you mean something like in patch #2 from this > > series (using unsigned int num_si and struct rtw_sta_info > > *si[RTW_MAX_MAC_ID_NUM] inside the iter_data) are you thinking of a > > better way in general? > > > > I would like a straight method, for example, we can have another version of > ieee80211_iterate_xxx() and do things in iterator, like original, so we just > need to change the code slightly. > > Initially, I have an idea we can hold driver lock, like rtwdev->mutex, in both > places where we use ieee80211_iterate_() and remove sta or vif. Hopefully, > this can ensure it's safe to run iterator without other locks. Then, we can > define another ieee80211_iterate_() version with a drv_lock argument, like > > #define ieee80211_iterate_active_interfaces_drv_lock(hw, iter_flags, iterator, data, drv_lock) \ > while (0) { \ > lockdep_assert_wiphy(drv_lock); \ > ieee80211_iterate_active_interfaces_no_lock(hw, iter_flags, iterator, data); \ > } > > The driv_lock argument can avoid user forgetting to hold a lock, and we need > a helper of no_lock version: > > void ieee80211_iterate_active_interfaces_no_lock( > struct ieee80211_hw *hw, u32 iter_flags, > void (*iterator)(void *data, u8 *mac, > struct ieee80211_vif *vif), > void *data) > { > struct ieee80211_local *local = hw_to_local(hw); > > __iterate_interfaces(local, iter_flags | IEEE80211_IFACE_ITER_ACTIVE, > iterator, data); > } > > However, as I mentioned theoretically it is not safe entirely. > > So, I think the easiest way is to maintains the vif/sta lists in driver when > ::{add,remove }_interface/::sta_{add,remove}, and hold rtwdev->mutex lock to > access these lists. But, Johannes pointed out this is not a good idea [1]. Thank you for this detailed explanation! I appreciate that you took the time to clearly explain this. For the sta use-case I thought about adding a dedicated rwlock (include/linux/rwlock.h) for rtw_dev->mac_id_map. rtw_sta_{add,remove} would take a write-lock. rtw_iterate_stas() takes the read-lock (the lock would be acquired before calling into ieee80211_iterate_...). Additionally rtw_iterate_stas() needs to check if the station is still valid according to mac_id_map - if not: skip/ignore it for that iteration. This could be combined with your 0001-rtw88-use-atomic-to-collect-stas-and-does-iterators.patch. For the interface use-case it's not clear to me how this works at all. rtw_ops_add_interface() has (in a simplified view): u8 port = 0; // the port variable is never changed rtwvif->port = port; rtwvif->conf = &rtw_vif_port[port]; rtw_info(rtwdev, "start vif %pM on port %d\n", vif->addr, rtwvif->port); How do multiple interfaces (vifs) work in rtw88 if the port is always zero? Is some kind of tracking of the used ports missing (similar to how we track the used station IDs - also called mac_id - in rtw_dev->mac_id_map)? Thank you again and best regards, Martin
Hi, > -----Original Message----- > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Sent: Friday, January 28, 2022 5:53 AM > To: Pkshih <pkshih@realtek.com> > Cc: linux-wireless@vger.kernel.org; tony0620emma@gmail.com; kvalo@codeaurora.org; > johannes@sipsolutions.net; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Neo Jou > <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; Ed Swierk <eswierk@gh.st> > Subject: Re: [PATCH v3 0/8] rtw88: prepare locking for SDIO support > > Hi Ping-Ke, > > On Mon, Jan 24, 2022 at 3:59 AM Pkshih <pkshih@realtek.com> wrote: [...] > > > > To avoid this, we can add a flag to struct rtw_vif, and set this flag > > when ::remove_interface. Then, only collect vif without this flag into list > > when we use iterate_actiom(). > > > > As well as ieee80211_sta can do similar fix. > > I would prefer my method that adds a 'bool disabled' flag to struct rtw_vif/rtw_sta and set it when ::remove_interface/::sta_remove. Then rtw_iterate_stas() can check this flag to decide whether does thing or not. [...] > > For the sta use-case I thought about adding a dedicated rwlock > (include/linux/rwlock.h) for rtw_dev->mac_id_map. > rtw_sta_{add,remove} would take a write-lock. > rtw_iterate_stas() takes the read-lock (the lock would be acquired > before calling into ieee80211_iterate_...). Additionally > rtw_iterate_stas() needs to check if the station is still valid > according to mac_id_map - if not: skip/ignore it for that iteration. > This could be combined with your > 0001-rtw88-use-atomic-to-collect-stas-and-does-iterators.patch. Using a 'disabled' flag within rtw_vif/rtw_sta will be intuitive and better than bitmap of mac_id_map. Please reference my mention above. > > For the interface use-case it's not clear to me how this works at all. > rtw_ops_add_interface() has (in a simplified view): > u8 port = 0; > // the port variable is never changed > rtwvif->port = port; > rtwvif->conf = &rtw_vif_port[port]; > rtw_info(rtwdev, "start vif %pM on port %d\n", vif->addr, rtwvif->port); > How do multiple interfaces (vifs) work in rtw88 if the port is always > zero? Is some kind of tracking of the used ports missing (similar to > how we track the used station IDs - also called mac_id - in > rtw_dev->mac_id_map)? The port should be allocated dynamically if we support two or more vifs. We have internal tree that is going to support p2p by second vif. Ping-Ke
Hi Ping-Ke, On Fri, Jan 28, 2022 at 1:51 AM Pkshih <pkshih@realtek.com> wrote: [...] > > > > > > > To avoid this, we can add a flag to struct rtw_vif, and set this flag > > > when ::remove_interface. Then, only collect vif without this flag into list > > > when we use iterate_actiom(). > > > > > > As well as ieee80211_sta can do similar fix. > > > > > I would prefer my method that adds a 'bool disabled' flag to struct rtw_vif/rtw_sta > and set it when ::remove_interface/::sta_remove. Then rtw_iterate_stas() can > check this flag to decide whether does thing or not. That would indeed be a very straight forward approach and easy to read. In net/mac80211/iface.c there's some cases where after drv_remove_interface() (which internally calls our .remove_interface op) will kfree the vif (sdata). Doesn't that then result in a use-after-free if we rely on a boolean within rtw_vif? [...] > > For the interface use-case it's not clear to me how this works at all. > > rtw_ops_add_interface() has (in a simplified view): > > u8 port = 0; > > // the port variable is never changed > > rtwvif->port = port; > > rtwvif->conf = &rtw_vif_port[port]; > > rtw_info(rtwdev, "start vif %pM on port %d\n", vif->addr, rtwvif->port); > > How do multiple interfaces (vifs) work in rtw88 if the port is always > > zero? Is some kind of tracking of the used ports missing (similar to > > how we track the used station IDs - also called mac_id - in > > rtw_dev->mac_id_map)? > > The port should be allocated dynamically if we support two or more vifs. > We have internal tree that is going to support p2p by second vif. I see, thanks for clarifying this! Best regards, Martin
Hi, On Sun, 2022-01-30 at 22:40 +0100, Martin Blumenstingl wrote: > > On Fri, Jan 28, 2022 at 1:51 AM Pkshih <pkshih@realtek.com> wrote: > [...] > > > > To avoid this, we can add a flag to struct rtw_vif, and set this flag > > > > when ::remove_interface. Then, only collect vif without this flag into list > > > > when we use iterate_actiom(). > > > > > > > > As well as ieee80211_sta can do similar fix. > > > > > > > > I would prefer my method that adds a 'bool disabled' flag to struct rtw_vif/rtw_sta > > and set it when ::remove_interface/::sta_remove. Then rtw_iterate_stas() can > > check this flag to decide whether does thing or not. > That would indeed be a very straight forward approach and easy to read. > In net/mac80211/iface.c there's some cases where after > drv_remove_interface() (which internally calls our .remove_interface > op) will kfree the vif (sdata). Doesn't that then result in a > use-after-free if we rely on a boolean within rtw_vif? The rtw_vif is drv_priv of ieee80211_vif, and they will be freed at the same time. We must set 'bool disabled' after holding rtwdev->mutex lock, and check this flag in iterator of ieee80211_iterate_active_interfaces_atomic() to contruct a list of vif. That means we never access this flag out of rtwdev->mutx or iterator. Does it make sense? -- Ping-Ke
Hi, Sorry, I kind of saw this fly by, read over it and wasn't sure I should take a closer look, and then promptly forgot about it ... > > So, I think the easiest way is to maintains the vif/sta lists in driver when > > ::{add,remove }_interface/::sta_{add,remove}, and hold rtwdev->mutex lock to > > access these lists. But, Johannes pointed out this is not a good idea [1]. > Thank you for this detailed explanation! I appreciate that you took > the time to clearly explain this. > > For the sta use-case I thought about adding a dedicated rwlock > (include/linux/rwlock.h) for rtw_dev->mac_id_map. You can't sleep under an rwlock either though? Wasn't that the point? > rtw_sta_{add,remove} would take a write-lock. > rtw_iterate_stas() takes the read-lock (the lock would be acquired > before calling into ieee80211_iterate_...). Additionally > rtw_iterate_stas() needs to check if the station is still valid > according to mac_id_map - if not: skip/ignore it for that iteration. All of that "still valid" seems fragile though, IMHO. Hard to reason that it's not racy or prone to use-after-free situations. IIUC, you're trying to iterate interfaces and stations in a sleepable context, but you're worried about deadlocks with locking in mac80211, if ieee80211_iterate_interfaces() or ieee80211_iterate_stations() take locks that we already hold due to coming into the code from mac80211? Or about ABBA deadlocks arising from this? IMHO you should still do it, and just be careful about the locking. I'd be happy to add APIs for e.g. ieee80211_iterate_stations_locked() when you know you already hold local->sta_mtx, though whether or not that can even happen today in any callbacks I don't know, haven't audited it, but it shouldn't be that hard to audit the path(s) you want to create? Unless you need this in some very frequently called function ... Now all of this is potentially also (and just as) error prone as doing your own iteration machinery, however, my longer-term plan is to unify the locking more. Now that we're no longer relying on the RTNL so much, I'm planning to see if we couldn't get rid of most locks in mac80211. The thing is, that most of the time we're doing all this fine-grained locking ... under the wdev->mtx, so it's entirely pointless, and in fact just a bunch of extra work. So now that from cfg80211 perspective we have everything running under wdev lock, I think we could in mac80211 just remove all the locks and take the wdev lock in all the async workers. We already do in many I guess, or we take local->mtx which is kind of equivalent anyway. The question is then if we keep wdev->mtx at all, and I'm not really sure about that yet. There are arguments both ways, and maybe that means we'd move some things under there rather than under the overall mutex. One of the strongest arguments for this is probably that mac80211 doesn't really do anything expensive (there aren't really any expensive calculations in it), so most time spent is in drivers ... and guess what, drivers mostly just have their own global lock they take for everything, which makes sense since they need to talk to the device or firmware. However we answer that question though, and I'm trending towards removing the wdev/sdata locks, I (now) think all the mutexes that we have at the 'local' level in mac80211 are fairly much pointless. In essence then, with some work in the stack, we could basically have all calls (apart from TX) into the drivers done with the wiphy->mtx held, at which point drivers don't even really need their own lock (they can acquire wiphy_lock() too in workers). Then this whole thing really all collapses down to being able to do the iteration, just having to ensure you wiphy_lock() your calls that aren't coming from mac80211 in the first place. I really think that's the medium-term strategy, and any help would be welcome. I'm not sure I can dedicate time to this soon, and the RTNL rework took I think over a year after I started thinking about it, so I guess we'll see... We can do this incrementally btw, like remove local->mtx now and use wiphy_lock() in its place, then remove local->sta_mtx, local->iface_mtx, one by one. The RTNL rework was one of the major stepping stones to this I think, because it ensured a consistent handling of the wiphy_lock() in cfg80211. I realize this doesn't help you in the short term, but if you do a lot of complicated things now, it'll also be hard to get rid of. If you use the normal iteration functions now it'll be harder to reason about now, but will sort of automatically become easier once the lock redux work I outlined above starts. And like I said, any help welcome :) johannes
Hello rtw88 and mac80211 maintainers/contributors, there is an ongoing effort where Jernej and I are working on adding SDIO support to the rtw88 driver [0]. The hardware we use at the moment is RTL8822BS and RTL8822CS. We are at a point where scanning, assoc etc. works. Performance is better in the latest version: - RX throughput is between 20Mbit/s and 25Mbit/s on RTL8822CS - TX throughput varies a lot, typically it's between 25Mbit/s and 50Mbit/s This series contains some preparation work for adding SDIO support. While testing our changes we found that there are some "scheduling while atomic" errors in the kernel log. These are due to the fact that the SDIO accessors (sdio_readb, sdio_writeb and friends) may sleep internally. Some background on why SDIO access (for example: sdio_writeb) cannot be done with a spinlock held (this is a copy from my previous mail, see [1]): - when using for example sdio_writeb the MMC subsystem in Linux prepares a so-called MMC request - this request is submitted to the MMC host controller hardware - the host controller hardware forwards the MMC request to the card - the card signals when it's done processing the request - the MMC subsystem in Linux waits for the card to signal that it's done processing the request in mmc_wait_for_req_done() -> this uses wait_for_completion() internally, which might sleep (which is not allowed while a spinlock is held) Based on Ping-Ke's suggestion I came up with the code in this series. The goal is to use non-atomic locking for all register access in the rtw88 driver. One patch adds a new function to mac80211 which did not have a "non-atomic" version of it's "atomic" counterpart yet. As mentioned before I don't have any rtw88 PCIe device so I am unable to test on that hardware. I am sending this as an RFC series since I am new to the mac80211 subsystem as well as the rtw88 driver. So any kind of feedback is very welcome! The actual changes for adding SDIO support will be sent separately in the future. Changes since v2 at [3]: - patch #1: dropped "mac80211: Add stations iterator where the iterator function may sleep" which was already applied to mac80211-next by Johannes (thanks!) - patch #2 (previously #3): move locking to the (only) caller of rtw_ra_mask_info_update() to make the locking consistent with other functions. Thank you Ping-Ke for this suggestion! - cover-letter: update link to the current SDIO work-in-progress code at [0] Changes since v1 at [2] (which I sent back in summer): - patch #1: fixed kernel doc copy & paste (remove _atomic) as suggested by Ping-Ke and Johannes - patch #1: added paragraph about driver authors having to be careful where they use this new function as suggested by Johannes - patch #2 (new): keep rtw_iterate_vifs_atomic() to not undo the fix from commit 5b0efb4d670c8 ("rtw88: avoid circular locking between local->iflist_mtx and rtwdev->mutex") and instead call rtw_bf_cfg_csi_rate() from rtw_watch_dog_work() (outside the atomic section) as suggested by Ping-Ke. - patch #3 (new): keep rtw_iterate_vifs_atomic() to prevent deadlocks as Johannes suggested. Keep track of all relevant stations inside rtw_ra_mask_info_update_iter() and the iter-data and then call rtw_update_sta_info() while held under rtwdev->mutex instead - patch #7: shrink the critical section as suggested by Ping-Ke [0] https://github.com/xdarklight/linux/tree/rtw88-test-20220107 [1] https://lore.kernel.org/linux-wireless/CAFBinCDMPPJ7qW7xTkep1Trg+zP0B9Jxei6sgjqmF4NDA1JAhQ@mail.gmail.com/ [2] https://lore.kernel.org/netdev/2170471a1c144adb882d06e08f3c9d1a@realtek.com/T/ [3] https://lore.kernel.org/netdev/20211228211501.468981-1-martin.blumenstingl@googlemail.com/ Martin Blumenstingl (8): rtw88: Move rtw_chip_cfg_csi_rate() out of rtw_vif_watch_dog_iter() rtw88: Move rtw_update_sta_info() out of rtw_ra_mask_info_update_iter() rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers rtw88: Use rtw_iterate_stas where the iterator reads or writes registers rtw88: Replace usage of rtw_iterate_keys_rcu() with rtw_iterate_keys() rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock rtw88: hci: Convert rf_lock from a spinlock to a mutex rtw88: fw: Convert h2c.lock from a spinlock to a mutex drivers/net/wireless/realtek/rtw88/bf.c | 13 ++--- drivers/net/wireless/realtek/rtw88/fw.c | 14 +++--- drivers/net/wireless/realtek/rtw88/hci.h | 11 ++--- drivers/net/wireless/realtek/rtw88/mac80211.c | 12 ++++- drivers/net/wireless/realtek/rtw88/main.c | 47 +++++++++---------- drivers/net/wireless/realtek/rtw88/main.h | 4 +- drivers/net/wireless/realtek/rtw88/phy.c | 4 +- drivers/net/wireless/realtek/rtw88/ps.c | 2 +- drivers/net/wireless/realtek/rtw88/util.h | 4 +- drivers/net/wireless/realtek/rtw88/wow.c | 2 +- 10 files changed, 58 insertions(+), 55 deletions(-)