Re: [PATCH v4 13/19] rtw89: 8852a: add 8852a specific files

[Brian Norris <briannorris@chromium.org>]

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