Hi Johannes, Hi Ping-Ke,

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

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

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)

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

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

Thanks for reviewing this Johannes!


Best regards,
Martin