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

[Pkshih <pkshih@realtek.com>]

On Thu, 2021-04-29 at 21:10 +0000, Brian Norris wrote:
> Hi,
> 
> On Thu, Apr 29, 2021 at 04:01:43PM +0800, Ping-Ke Shih wrote:
> > --- /dev/null
> > +++ b/drivers/net/wireless/realtek/rtw89/rtw8852a.c
> > @@ -0,0 +1,2047 @@
> ...
> > +static void rtw8852a_btc_init_cfg(struct rtw89_dev *rtwdev)
> > +{
> > +	struct rtw89_btc *btc = &rtwdev->btc;
> > +	struct rtw89_btc_module *module = &btc->mdinfo;
> > +	const struct rtw89_chip_info *chip = rtwdev->chip;
> > +	const struct rtw89_mac_ax_coex coex_params = {
> > +		.pta_mode = RTW89_MAC_AX_COEX_RTK_MODE,
> > +		.direction = RTW89_MAC_AX_COEX_INNER,
> > +	};
> > +
> > +	/* PTA init  */
> > +	rtw89_mac_coex_init(rtwdev, &coex_params);
> > +
> > +	/* set WL Tx response = Hi-Pri */
> > +	chip->ops->btc_set_wl_pri(rtwdev, BTC_PRI_MASK_TX_RESP, true);
> > +
> > +	/* set rf gnt debug off */
> > +	rtw89_write_rf(rtwdev, RF_PATH_A, RR_WLSEL, 0xfffff, 0x0);
> 
> I fired this up and quickly ran into this:
> 
> [ 1746.061015] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:281
> [ 1746.061029] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 838, name: wpa_supplicant
> [ 1746.061037] CPU: 2 PID: 838 Comm: wpa_supplicant Tainted: G        W         5.10.27 #3
> ...
> [ 1746.061047] Call Trace:
> [ 1746.061061]  dump_stack+0x9e/0xe9
> [ 1746.061080]  ___might_sleep+0x154/0x16a
> [ 1746.061093]  mutex_lock+0x20/0x3c
> [ 1746.061106]  rtw8852a_btc_init_cfg+0x60/0x177 [rtw89_core]
> [ 1746.061127]  rtw89_btc_ntfy_radio_state+0xb8/0x115 [rtw89_core]
> [ 1746.061153]  __iterate_interfaces+0xa9/0x109 [mac80211]
> [ 1746.061165]  ? rtw89_leave_lps+0x44/0x44 [rtw89_core]
> [ 1746.061175]  ? rtw89_leave_lps+0x44/0x44 [rtw89_core]
> [ 1746.061194]  ieee80211_iterate_active_interfaces_atomic+0x33/0x40 [mac80211]
> [ 1746.061205]  rtw89_ops_sw_scan_start+0x2e/0x48 [rtw89_core]
> [ 1746.061234]  drv_sw_scan_start+0x97/0xf0 [mac80211]
> [ 1746.061261]  __ieee80211_start_scan+0x3c7/0x4ae [mac80211]
> [ 1746.061284]  ieee80211_request_scan+0x33/0x4f [mac80211]
> [ 1746.061307]  rdev_scan+0x72/0xd1 [cfg80211]
> [ 1746.061335]  nl80211_trigger_scan+0x610/0x669 [cfg80211]
> [ 1746.061349]  genl_rcv_msg+0x3b0/0x3e0
> [ 1746.061372]  ? nl80211_update_mesh_config+0x1b7/0x1b7 [cfg80211]
> [ 1746.061382]  ? genl_rcv+0x36/0x36
> [ 1746.061387]  netlink_rcv_skb+0x89/0xfb
> [ 1746.061394]  genl_rcv+0x28/0x36
> [ 1746.061400]  netlink_unicast+0x169/0x23b
> [ 1746.061408]  netlink_sendmsg+0x379/0x3f1
> [ 1746.061416]  sock_sendmsg+0x72/0x76
> [ 1746.061423]  ____sys_sendmsg+0x171/0x1ea
> [ 1746.061429]  ? copy_msghdr_from_user+0x82/0xaa
> [ 1746.061436]  ___sys_sendmsg+0xa0/0xdc
> [ 1746.061445]  ? _copy_from_user+0x70/0x9c
> [ 1746.061451]  __sys_sendmsg+0x8c/0xc6
> [ 1746.061460]  do_syscall_64+0x43/0x55
> [ 1746.061467]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> 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.

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

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.

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.

--
Ping-Ke