On Fri, 2021-04-30 at 01:22 +0000, Brian Norris wrote: > On Thu, Apr 29, 2021 at 11:43:12PM +0000, Pkshih wrote: > > On Thu, 2021-04-29 at 21:10 +0000, Brian Norris wrote: > > > rtw89_write_rf() is holding a mutex (rf_mutex). Judging by its trivial > > > usage (it's only protecting register reads/writes), it probably could be > > > a spinlock instead -- although I do note some magic udelay()s in there. > > > > > > > The udelay() is needed to ensure the indirect-write correct. > > OK. Maybe deserves a comment for the future. Is this a > hardware-specified timing (measured in number of cycles or similar, on > the WiFi chip side), or something you're just guessing at? > This is estimated by designer. > > > Alternatively, it looks like you'd be safe moving to the non-atomic > > > ieee80211_iterate_active_interfaces() in rtw89_leave_lps(). > > > > > > > For most cases of rtw89_leave_lps(), we can use ieee80211_iterate_active_interfaces(), > > which hold iflist_mtx lock, except to > > > > ieee80211_recalc_ps(local); // held iflist_mtx lock > > ... > > ieee80211_hw_config > > ... > > rtw89_leave_lps() > > ... > > ieee80211_iterate_active_interfaces() > > > > That will leads deadlock. > > Good point. > > > Another variant ieee80211_iterate_active_interfaces_mtx() that doesn't > > hold a lock may be a solution. The the comment says "This version can > > only be used while holding the RTNL.", and the code within the function > > says "lockdep_assert_wiphy(hw->wiphy);". I'm not sure if I can use it > > to prevent locking iflist_mtx twice. > > This doesn't quite feel like the right thing. You're in the midst of > many other callback layers, and I don't think this is the right place to > be grabbing those locks. But I haven't researched this very closely yet. > > > If I can use it, I can add a argument 'mtx', like rtw89_leave_lps(rtwdev, bool mtx), > > to judge using ieee80211_iterate_active_interfaces() or ieee80211_iterate_active_interfaces_mtx(). > > > > I'm also thinking that we can still use ieee80211_iterate_active_interfaces_atomic() > > to merely collect rtwvif->mac_id list, and use a loop to do leave_lps > > out of the atomic iterate. > > That's probably safe, because we're already holding rtwdev->mutex, so > there's no chance of our mac_id going away. If that solution isn't too > complex, it makes sense to me. > I drop the rtw89_iterate_vifs_atomic() that cannot sleep, but it's hard to avoid calling sleep functions. To make other programmers easier to choose iterate functions, I re-design them according to running context, and listed below context iflist_mtx iterate vifs functions ------- ---------- ---------------------- BH N/A rtw89_iterate_vifs_bh() thread not held rtw89_iterate_vifs(,,,false); thread held rtw89_iterate_vifs(,,,true); It's clear that iterators of rtw89_iterate_vifs_bh() can't call sleep functions. Iterators of rtw89_iterate_vifs() can always call sleep functions, but additional forth argument must be set correctly. Next submission will include this change. -- Ping-Ke
