> -----Original Message----- > From: Martin Blumenstingl [mailto:martin.blumenstingl@xxxxxxxxxxxxxx] > Sent: Monday, July 26, 2021 5:51 AM > To: Johannes Berg; Pkshih > Cc: linux-wireless@xxxxxxxxxxxxxxx; tony0620emma@xxxxxxxxx; kvalo@xxxxxxxxxxxxxx; > netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 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 <johannes@xxxxxxxxxxxxxxxx> wrote: > > > > On Sat, 2021-07-17 at 22:40 +0200, Martin Blumenstingl wrote: > > > > > > --- 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) 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. > > > The other cases look OK, it's being called from outside contexts > > (wowlan, etc.) > Thanks for reviewing this Johannes! > -- Ping-Ke
