-----Original Message-----
From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com]
Sent: Monday, July 26, 2021 5:51 AM
To: Johannes Berg; Pkshih
Cc: linux-wireless@vger.kernel.org; tony0620emma@gmail.com; kvalo@codeaurora.org;
netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Neo Jou; Jernej Skrabec
Subject: Re: [PATCH RFC v1 3/7] rtw88: Use rtw_iterate_stas where the iterator reads or writes registers

Hi Johannes, Hi Ping-Ke,

On Mon, Jul 19, 2021 at 8:36 AM Johannes Berg [off-list ref] wrote:

quoted

On Sat, 2021-07-17 at 22:40 +0200, Martin Blumenstingl wrote:

quoted

--- a/drivers/net/wireless/realtek/rtw88/mac80211.c
+++ b/drivers/net/wireless/realtek/rtw88/mac80211.c

@@ -721,7 +721,7 @@ static void rtw_ra_mask_info_update(struct rtw_dev *rtwdev,
      br_data.rtwdev = rtwdev;
      br_data.vif = vif;
      br_data.mask = mask;
-     rtw_iterate_stas_atomic(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
+     rtw_iterate_stas(rtwdev, rtw_ra_mask_info_update_iter, &br_data);

And then you pretty much immediately break that invariant here, namely
that you're calling this within the set_bitrate_mask() method called by
mac80211.

you are right, I was not aware of this

quoted

That's not actually fundamentally broken today, but it does *severely*
restrict what we can do in mac80211 wrt. locking, and I really don't
want to keep the dozen or so locks forever, this needs simplification
because clearly we don't even know what should be under what lock.

To me it's also not clear what the goal of the whole locking is.
The lock in ieee80211_iterate_stations_atomic is obviously for the
mac80211-internal state-machine
But I *believe* that there's a second purpose (rtw88 specific) -
here's my understanding of that part:
- rtw_sta_info contains a "mac_id" which is an identifier for a
specific station used by the rtw88 driver and is shared with the
firmware
- rtw_ops_sta_{add,remove} uses rtwdev->mutex to protect the rtw88
side of this "mac_id" identifier
- (for some reason rtw_update_sta_info doesn't use rtwdev->mutex)

I am thinking rtw88 needs to maintain sta and vif lists itself, and
these lists are also protected by rtwdev->mutex. When rtw88 wants to
iterate all sta/vif, it holds rtwdev->mutex to do list_for_each_entry.
No need to hold mac80211 locks.

So now I am wondering if the ieee80211_iterate_stations_atomic lock is
also used to protect any modifications to rtw_sta_info.
Ping-Ke, I am wondering if the attached patch (untested - to better
demonstrate what I want to say) would:
- allow us to move the register write outside of
ieee80211_iterate_stations_atomic
- mean we can keep ieee80211_iterate_stations_atomic (instead of the
non-atomic variant)
- protect the code managing the "mac_id" with rtwdev->mutex consistently

I think your attached patch can work well.

quoted

The other cases look OK, it's being called from outside contexts
(wowlan, etc.)

Thanks for reviewing this Johannes!

--
Ping-Ke