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
