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? > > 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. Brian
