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

[Pkshih <pkshih@realtek.com>]

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