[PATCH v3 0/3] wifi: rtw88: Three locking fixes for existing code

[PATCH v3 0/3] wifi: rtw88: Three locking fixes for existing code

[Martin Blumenstingl]

This series consists of three patches which are fixing existing
behavior (meaning: it either affects PCIe or USB or both) in the rtw88
driver.
We previously had discussed patches for these locking issues while
working on SDIO support, but the problem never ocurred while testing
USB cards. It turns out that these are still needed and I think that
they also fix the same problems for USB users (it's not clear how often
it happens there though) - and possibly even PCIe card users.

The issue fixed by the second and third patches have been spotted by a
user who tested rtw88 SDIO support. Everything is working fine for him
but there are warnings [1] and [2] in the kernel log stating "Voluntary
context switch within RCU read-side critical section!".

The solution in the third and fourth patch was actually suggested by
Ping-Ke in [3]. Thanks again!

These fixes are indepdent of my other series adding SDIO support to the
rtw88 driver, meaning they can be added to the wireless driver tree on
top of Linux 6.2-rc1 or linux-next.


Changes since v1 at [4]:
- Keep the u8 bitfields in patch 1 but split the res2 field into res2_1
  and res2_2 as suggested by Ping-Ke
- Added Ping-Ke's reviewed-by to patches 2-4 - thank you!
- Added a paragraph in the cover-letter to avoid confusion whether
  these patches depend on the rtw88 SDIO support series

Changes since v2 at [5]:
- Added Ping-Ke's Reviewed-by and Sascha's Tested-by (thanks to both of
  you!)
- Dropped patch 1/4 "rtw88: Add packed attribute to the eFuse structs"
  This requires more discussion. I'll send a separate patch for this.
- Updated cover letter title so it's clear that this is independent of
  SDIO support code


[0] https://lore.kernel.org/linux-wireless/695c976e02ed44a2b2345a3ceb226fc4@realtek.com/
[1] https://github.com/LibreELEC/LibreELEC.tv/pull/7301#issuecomment-1366421445
[2] https://github.com/LibreELEC/LibreELEC.tv/pull/7301#issuecomment-1366610249
[3] https://lore.kernel.org/lkml/e0aa1ba4336ab130712e1fcb425e6fd0adca4145.camel@realtek.com/
[4] https://lore.kernel.org/linux-wireless/20221228133547.633797-1-martin.blumenstingl@googlemail.com/
[5] https://lore.kernel.org/linux-wireless/20221229124845.1155429-1-martin.blumenstingl@googlemail.com/


Martin Blumenstingl (3):
  wifi: rtw88: Move register access from rtw_bf_assoc() outside the RCU
  wifi: rtw88: Use rtw_iterate_vifs() for rtw_vif_watch_dog_iter()
  wifi: rtw88: Use non-atomic sta iterator in rtw_ra_mask_info_update()

 drivers/net/wireless/realtek/rtw88/bf.c       | 13 +++++++------
 drivers/net/wireless/realtek/rtw88/mac80211.c |  4 +++-
 drivers/net/wireless/realtek/rtw88/main.c     |  6 ++++--
 3 files changed, 14 insertions(+), 9 deletions(-)

-- 
2.39.0

[PATCH v3 1/3] wifi: rtw88: Move register access from rtw_bf_assoc() outside the RCU

[Martin Blumenstingl]

USB and (upcoming) SDIO support may sleep in the read/write handlers.
Shrink the RCU critical section so it only cover the call to
ieee80211_find_sta() and finding the ic_vht_cap/vht_cap based on the
found station. This moves the chip's BFEE configuration outside the
rcu_read_lock section and thus prevent "scheduling while atomic" or
"Voluntary context switch within RCU read-side critical section!"
warnings when accessing the registers using an SDIO card (which is
where this issue has been spotted in the real world - but it also
affects USB cards).

Reviewed-by: Ping-Ke Shih <pkshih@realtek.com>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
v1 -> v2:
- Added Ping-Ke's Reviewed-by (thank you!)

v2 -> v3:
- Added Sascha's Tested-by (thank you!)
- added "wifi" prefix to the subject and reworded the title accordingly


 drivers/net/wireless/realtek/rtw88/bf.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/bf.c b/drivers/net/wireless/realtek/rtw88/bf.c
index 038a30b170ef..c827c4a2814b 100644
--- a/drivers/net/wireless/realtek/rtw88/bf.c
+++ b/drivers/net/wireless/realtek/rtw88/bf.c
@@ -49,19 +49,23 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
 
 	sta = ieee80211_find_sta(vif, bssid);
 	if (!sta) {
+		rcu_read_unlock();
+
 		rtw_warn(rtwdev, "failed to find station entry for bss %pM\n",
 			 bssid);
-		goto out_unlock;
+		return;
 	}
 
 	ic_vht_cap = &hw->wiphy->bands[NL80211_BAND_5GHZ]->vht_cap;
 	vht_cap = &sta->deflink.vht_cap;
 
+	rcu_read_unlock();
+
 	if ((ic_vht_cap->cap & IEEE80211_VHT_CAP_MU_BEAMFORMEE_CAPABLE) &&
 	    (vht_cap->cap & IEEE80211_VHT_CAP_MU_BEAMFORMER_CAPABLE)) {
 		if (bfinfo->bfer_mu_cnt >= chip->bfer_mu_max_num) {
 			rtw_dbg(rtwdev, RTW_DBG_BF, "mu bfer number over limit\n");
-			goto out_unlock;
+			return;
 		}
 
 		ether_addr_copy(bfee->mac_addr, bssid);
@@ -75,7 +79,7 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
 		   (vht_cap->cap & IEEE80211_VHT_CAP_SU_BEAMFORMER_CAPABLE)) {
 		if (bfinfo->bfer_su_cnt >= chip->bfer_su_max_num) {
 			rtw_dbg(rtwdev, RTW_DBG_BF, "su bfer number over limit\n");
-			goto out_unlock;
+			return;
 		}
 
 		sound_dim = vht_cap->cap &
@@ -98,9 +102,6 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
 
 		rtw_chip_config_bfee(rtwdev, rtwvif, bfee, true);
 	}
-
-out_unlock:
-	rcu_read_unlock();
 }
 
 void rtw_bf_init_bfer_entry_mu(struct rtw_dev *rtwdev,
-- 
2.39.0

[PATCH v3 2/3] wifi: rtw88: Use rtw_iterate_vifs() for rtw_vif_watch_dog_iter()

[Martin Blumenstingl]

USB and (upcoming) SDIO support may sleep in the read/write handlers.
Make rtw_watch_dog_work() use rtw_iterate_vifs() to prevent "scheduling
while atomic" or "Voluntary context switch within RCU read-side
critical section!" warnings when accessing the registers using an SDIO
card (which is where this issue has been spotted in the real world but
it also affects USB cards).

Fixes: 78d5bf925f30 ("wifi: rtw88: iterate over vif/sta list non-atomically")
Suggested-by: Ping-Ke Shih <pkshih@realtek.com>
Reviewed-by: Ping-Ke Shih <pkshih@realtek.com>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
v1 -> v2:
- no change

v2 -> v3:
- Added Ping-Ke's Reviewed-by (thank you!)
- Added Sascha's Tested-by (thank you!)
- added "wifi" prefix to the subject and reworded the title accordingly


 drivers/net/wireless/realtek/rtw88/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index 888427cf3bdf..b2e78737bd5d 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -241,8 +241,10 @@ static void rtw_watch_dog_work(struct work_struct *work)
 	rtw_phy_dynamic_mechanism(rtwdev);
 
 	data.rtwdev = rtwdev;
-	/* use atomic version to avoid taking local->iflist_mtx mutex */
-	rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data);
+	/* rtw_iterate_vifs internally uses an atomic iterator which is needed
+	 * to avoid taking local->iflist_mtx mutex
+	 */
+	rtw_iterate_vifs(rtwdev, rtw_vif_watch_dog_iter, &data);
 
 	/* fw supports only one station associated to enter lps, if there are
 	 * more than two stations associated to the AP, then we can not enter
-- 
2.39.0

[PATCH v3 3/3] wifi: rtw88: Use non-atomic sta iterator in rtw_ra_mask_info_update()

[Martin Blumenstingl]

USB and (upcoming) SDIO support may sleep in the read/write handlers.
Use non-atomic rtw_iterate_stas() in rtw_ra_mask_info_update() because
the iterator function rtw_ra_mask_info_update_iter() needs to read and
write registers from within rtw_update_sta_info(). Using the non-atomic
iterator ensures that we can sleep during USB and SDIO register reads
and writes. This fixes "scheduling while atomic" or "Voluntary context
switch within RCU read-side critical section!" warnings as seen by SDIO
card users (but it also affects USB cards).

Fixes: 78d5bf925f30 ("wifi: rtw88: iterate over vif/sta list non-atomically")
Suggested-by: Ping-Ke Shih <pkshih@realtek.com>
Reviewed-by: Ping-Ke Shih <pkshih@realtek.com>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
v1 -> v2:
- Added Ping-Ke's Reviewed-by (thank you!)

v2 -> v3:
- Added Sascha's Tested-by (thank you!)
- added "wifi" prefix to the subject and reworded the title accordingly


 drivers/net/wireless/realtek/rtw88/mac80211.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c b/drivers/net/wireless/realtek/rtw88/mac80211.c
index 776a9a9884b5..3b92ac611d3f 100644
--- a/drivers/net/wireless/realtek/rtw88/mac80211.c
+++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
@@ -737,7 +737,7 @@ static void rtw_ra_mask_info_update(struct rtw_dev *rtwdev,
 	br_data.rtwdev = rtwdev;
 	br_data.vif = vif;
 	br_data.mask = mask;
-	rtw_iterate_stas_atomic(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
+	rtw_iterate_stas(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
 }
 
 static int rtw_ops_set_bitrate_mask(struct ieee80211_hw *hw,
@@ -746,7 +746,9 @@ static int rtw_ops_set_bitrate_mask(struct ieee80211_hw *hw,
 {
 	struct rtw_dev *rtwdev = hw->priv;
 
+	mutex_lock(&rtwdev->mutex);
 	rtw_ra_mask_info_update(rtwdev, vif, mask);
+	mutex_unlock(&rtwdev->mutex);
 
 	return 0;
 }
-- 
2.39.0

Re: [PATCH v3 0/3] wifi: rtw88: Three locking fixes for existing code

[Martin Blumenstingl]

Hi Kalle,

On Sun, Jan 8, 2023 at 10:13 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> This series consists of three patches which are fixing existing
> behavior (meaning: it either affects PCIe or USB or both) in the rtw88
> driver.
In reply to an earlier version of this series you wrote [0]:
> BTW wireless-next or wireless-testing are the preferred baselines for
> wireless patches. Of course you can use other trees if you really want,
> but please try to make sure they apply to wireless-next. Conflicts are
> always extra churn I would prefer to avoid.
Noted.
Additionally I just tested it and can confirm that these patches apply
fine (without any fuzz) on top of the wireless tree.


Best regards,
Martin


[0] https://lore.kernel.org/linux-wireless/87mt6qfvb1.fsf@kernel.org/

Re: [PATCH v3 1/3] wifi: rtw88: Move register access from rtw_bf_assoc() outside the RCU

[Kalle Valo]

Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> USB and (upcoming) SDIO support may sleep in the read/write handlers.
> Shrink the RCU critical section so it only cover the call to
> ieee80211_find_sta() and finding the ic_vht_cap/vht_cap based on the
> found station. This moves the chip's BFEE configuration outside the
> rcu_read_lock section and thus prevent "scheduling while atomic" or
> "Voluntary context switch within RCU read-side critical section!"
> warnings when accessing the registers using an SDIO card (which is
> where this issue has been spotted in the real world - but it also
> affects USB cards).
> 
> Reviewed-by: Ping-Ke Shih <pkshih@realtek.com>
> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

3 patches applied to wireless-next.git, thanks.

8a1e2fd8e2da wifi: rtw88: Move register access from rtw_bf_assoc() outside the RCU
313f6dc7c5ed wifi: rtw88: Use rtw_iterate_vifs() for rtw_vif_watch_dog_iter()
2931978cd74f wifi: rtw88: Use non-atomic sta iterator in rtw_ra_mask_info_update()

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230108211324.442823-2-martin.blumenstingl@googlemail.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v3 1/3] wifi: rtw88: Move register access from rtw_bf_assoc() outside the RCU

[Sascha Hauer]

On Sun, Jan 08, 2023 at 10:13:22PM +0100, Martin Blumenstingl wrote:
> USB and (upcoming) SDIO support may sleep in the read/write handlers.
> Shrink the RCU critical section so it only cover the call to
> ieee80211_find_sta() and finding the ic_vht_cap/vht_cap based on the
> found station. This moves the chip's BFEE configuration outside the
> rcu_read_lock section and thus prevent "scheduling while atomic" or
> "Voluntary context switch within RCU read-side critical section!"
> warnings when accessing the registers using an SDIO card (which is
> where this issue has been spotted in the real world - but it also
> affects USB cards).

Unfortunately this introduces a regression on my RTW8821CU chip. With
this it constantly looses connection to the AP and reconnects shortly
after:

[  199.771143] wlan0: authenticate with b0:be:76:5e:7b:34
[  201.447301] wlan0: send auth to b0:be:76:5e:7b:34 (try 1/3)
[  201.456789] wlan0: authenticated
[  201.462356] wlan0: associate with b0:be:76:5e:7b:34 (try 1/3)
[  201.477263] wlan0: RX AssocResp from b0:be:76:5e:7b:34 (capab=0x431 status=0 aid=2)
[  201.512995] wlan0: associated
[  213.790399] wlan0: authenticate with b0:be:76:5e:7b:34
[  215.467302] wlan0: send auth to b0:be:76:5e:7b:34 (try 1/3)
[  215.470532] wlan0: authenticated
[  215.490355] wlan0: associate with b0:be:76:5e:7b:34 (try 1/3)
[  215.503777] wlan0: RX AssocResp from b0:be:76:5e:7b:34 (capab=0x431 status=0 aid=2)
[  215.539608] wlan0: associated
[  227.770596] wlan0: authenticate with b0:be:76:5e:7b:34
[  229.443302] wlan0: send auth to b0:be:76:5e:7b:34 (try 1/3)
[  229.451209] wlan0: authenticated
[  229.462487] wlan0: associate with b0:be:76:5e:7b:34 (try 1/3)
[  229.476077] wlan0: RX AssocResp from b0:be:76:5e:7b:34 (capab=0x431 status=0 aid=2)
[  229.513499] wlan0: associated
[  241.738494] wlan0: authenticate with b0:be:76:5e:7b:34
[  243.407301] wlan0: send auth to b0:be:76:5e:7b:34 (try 1/3)
[  243.411207] wlan0: authenticated
[  243.423213] wlan0: associate with b0:be:76:5e:7b:34 (try 1/3)
[  243.439822] wlan0: RX AssocResp from b0:be:76:5e:7b:34 (capab=0x431 status=0 aid=2)
[  243.476731] wlan0: associated

I haven't got any further information yet, I just realized this when I
rebased my own RTW88 bugfix series from v6.2.2 to v6.3-rc4 before
sending it.

RTW8723D and RTW8822CU seem unaffected though.

Sascha

> 
> Reviewed-by: Ping-Ke Shih <pkshih@realtek.com>
> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> v1 -> v2:
> - Added Ping-Ke's Reviewed-by (thank you!)
> 
> v2 -> v3:
> - Added Sascha's Tested-by (thank you!)
> - added "wifi" prefix to the subject and reworded the title accordingly
> 
> 
>  drivers/net/wireless/realtek/rtw88/bf.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/bf.c b/drivers/net/wireless/realtek/rtw88/bf.c
> index 038a30b170ef..c827c4a2814b 100644
> --- a/drivers/net/wireless/realtek/rtw88/bf.c
> +++ b/drivers/net/wireless/realtek/rtw88/bf.c
> @@ -49,19 +49,23 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
>  
>  	sta = ieee80211_find_sta(vif, bssid);
>  	if (!sta) {
> +		rcu_read_unlock();
> +
>  		rtw_warn(rtwdev, "failed to find station entry for bss %pM\n",
>  			 bssid);
> -		goto out_unlock;
> +		return;
>  	}
>  
>  	ic_vht_cap = &hw->wiphy->bands[NL80211_BAND_5GHZ]->vht_cap;
>  	vht_cap = &sta->deflink.vht_cap;
>  
> +	rcu_read_unlock();
> +
>  	if ((ic_vht_cap->cap & IEEE80211_VHT_CAP_MU_BEAMFORMEE_CAPABLE) &&
>  	    (vht_cap->cap & IEEE80211_VHT_CAP_MU_BEAMFORMER_CAPABLE)) {
>  		if (bfinfo->bfer_mu_cnt >= chip->bfer_mu_max_num) {
>  			rtw_dbg(rtwdev, RTW_DBG_BF, "mu bfer number over limit\n");
> -			goto out_unlock;
> +			return;
>  		}
>  
>  		ether_addr_copy(bfee->mac_addr, bssid);
> @@ -75,7 +79,7 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
>  		   (vht_cap->cap & IEEE80211_VHT_CAP_SU_BEAMFORMER_CAPABLE)) {
>  		if (bfinfo->bfer_su_cnt >= chip->bfer_su_max_num) {
>  			rtw_dbg(rtwdev, RTW_DBG_BF, "su bfer number over limit\n");
> -			goto out_unlock;
> +			return;
>  		}
>  
>  		sound_dim = vht_cap->cap &
> @@ -98,9 +102,6 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
>  
>  		rtw_chip_config_bfee(rtwdev, rtwvif, bfee, true);
>  	}
> -
> -out_unlock:
> -	rcu_read_unlock();
>  }
>  
>  void rtw_bf_init_bfer_entry_mu(struct rtw_dev *rtwdev,
> -- 
> 2.39.0
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 1/3] wifi: rtw88: Move register access from rtw_bf_assoc() outside the RCU

[Martin Blumenstingl]

Hi Sascha,

On Fri, Mar 31, 2023 at 2:59 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Sun, Jan 08, 2023 at 10:13:22PM +0100, Martin Blumenstingl wrote:
> > USB and (upcoming) SDIO support may sleep in the read/write handlers.
> > Shrink the RCU critical section so it only cover the call to
> > ieee80211_find_sta() and finding the ic_vht_cap/vht_cap based on the
> > found station. This moves the chip's BFEE configuration outside the
> > rcu_read_lock section and thus prevent "scheduling while atomic" or
> > "Voluntary context switch within RCU read-side critical section!"
> > warnings when accessing the registers using an SDIO card (which is
> > where this issue has been spotted in the real world - but it also
> > affects USB cards).
>
> Unfortunately this introduces a regression on my RTW8821CU chip. With
> this it constantly looses connection to the AP and reconnects shortly
> after:
Sorry to hear this! This is odd and unfortunately I don't understand
the reason for this.
rtw_bf_assoc() is only called from
drivers/net/wireless/realtek/rtw88/mac80211.c with rtwdev->mutex held.
So I don't think that it's a race condition.

There's a module parameter which lets you enable/disable BF support:
$ git grep rtw_bf_support drivers/net/wireless/realtek/rtw88/ | grep param
drivers/net/wireless/realtek/rtw88/main.c:module_param_named(support_bf,
rtw_bf_support, bool, 0644);

Have you tried disabling BF support?
Also +Cc Jernej in case he has an idea.


Best regards,
Martin

Re: [PATCH v3 1/3] wifi: rtw88: Move register access from rtw_bf_assoc() outside the RCU

[Linux regression tracking #adding (Thorsten Leemhuis)]

[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

On 31.03.23 14:59, Sascha Hauer wrote:
> On Sun, Jan 08, 2023 at 10:13:22PM +0100, Martin Blumenstingl wrote:
>> USB and (upcoming) SDIO support may sleep in the read/write handlers.
>> Shrink the RCU critical section so it only cover the call to
>> ieee80211_find_sta() and finding the ic_vht_cap/vht_cap based on the
>> found station. This moves the chip's BFEE configuration outside the
>> rcu_read_lock section and thus prevent "scheduling while atomic" or
>> "Voluntary context switch within RCU read-side critical section!"
>> warnings when accessing the registers using an SDIO card (which is
>> where this issue has been spotted in the real world - but it also
>> affects USB cards).
> 
> Unfortunately this introduces a regression on my RTW8821CU chip. With
> this it constantly looses connection to the AP and reconnects shortly
> after:

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced c7eca79def44
#regzbot title net: wifi: rtw88: RTW8821CU constantly looses connection
to the AP and reconnects shortly after
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: [PATCH v3 1/3] wifi: rtw88: Move register access from rtw_bf_assoc() outside the RCU

[Sascha Hauer]

Hi Martin,

On Sat, Apr 01, 2023 at 11:30:40PM +0200, Martin Blumenstingl wrote:
> Hi Sascha,
> 
> On Fri, Mar 31, 2023 at 2:59 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > On Sun, Jan 08, 2023 at 10:13:22PM +0100, Martin Blumenstingl wrote:
> > > USB and (upcoming) SDIO support may sleep in the read/write handlers.
> > > Shrink the RCU critical section so it only cover the call to
> > > ieee80211_find_sta() and finding the ic_vht_cap/vht_cap based on the
> > > found station. This moves the chip's BFEE configuration outside the
> > > rcu_read_lock section and thus prevent "scheduling while atomic" or
> > > "Voluntary context switch within RCU read-side critical section!"
> > > warnings when accessing the registers using an SDIO card (which is
> > > where this issue has been spotted in the real world - but it also
> > > affects USB cards).
> >
> > Unfortunately this introduces a regression on my RTW8821CU chip. With
> > this it constantly looses connection to the AP and reconnects shortly
> > after:
> Sorry to hear this! This is odd and unfortunately I don't understand
> the reason for this.
> rtw_bf_assoc() is only called from
> drivers/net/wireless/realtek/rtw88/mac80211.c with rtwdev->mutex held.
> So I don't think that it's a race condition.
> 
> There's a module parameter which lets you enable/disable BF support:
> $ git grep rtw_bf_support drivers/net/wireless/realtek/rtw88/ | grep param
> drivers/net/wireless/realtek/rtw88/main.c:module_param_named(support_bf,
> rtw_bf_support, bool, 0644);

I was a bit too fast reporting this. Yes, there seems to be a problem
with the RTW8821CU, but it doesn't seem to be related to this patch.

Sorry for the noise.

The chipset seems to have problems with one access point that I have and
I can see these problems with or without the patch. Maybe NetworkManager
decided to connect to another accesspoint without me noticing it, making
it look to me as if this patch was guilty.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 1/3] wifi: rtw88: Move register access from rtw_bf_assoc() outside the RCU

[Martin Blumenstingl]

Hi Sascha,

On Mon, Apr 3, 2023 at 12:00 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
[...]
> > There's a module parameter which lets you enable/disable BF support:
> > $ git grep rtw_bf_support drivers/net/wireless/realtek/rtw88/ | grep param
> > drivers/net/wireless/realtek/rtw88/main.c:module_param_named(support_bf,
> > rtw_bf_support, bool, 0644);
>
> I was a bit too fast reporting this. Yes, there seems to be a problem
> with the RTW8821CU, but it doesn't seem to be related to this patch.
>
> Sorry for the noise.
Thanks for investigating further and confirming that this is not the cause!
And don't worry: we're all human and with complex drivers that can be
impacted by so many things (other APs, phones, antennas, ...) it's
easy to miss a tiny detail (I've been there before).


Best regards,
Martin

Re: [PATCH v3 1/3] wifi: rtw88: Move register access from rtw_bf_assoc() outside the RCU

[Linux regression tracking #update (Thorsten Leemhuis)]

On 02.04.23 13:30, Linux regression tracking #adding (Thorsten Leemhuis)
wrote:
> On 31.03.23 14:59, Sascha Hauer wrote:
>> On Sun, Jan 08, 2023 at 10:13:22PM +0100, Martin Blumenstingl wrote:
>>> USB and (upcoming) SDIO support may sleep in the read/write handlers.
>>> Shrink the RCU critical section so it only cover the call to
>>> ieee80211_find_sta() and finding the ic_vht_cap/vht_cap based on the
>>> found station. This moves the chip's BFEE configuration outside the
>>> rcu_read_lock section and thus prevent "scheduling while atomic" or
>>> "Voluntary context switch within RCU read-side critical section!"
>>> warnings when accessing the registers using an SDIO card (which is
>>> where this issue has been spotted in the real world - but it also
>>> affects USB cards).
>>
>> Unfortunately this introduces a regression on my RTW8821CU chip. With
>> this it constantly looses connection to the AP and reconnects shortly
>> after:
> 
> #regzbot ^introduced c7eca79def44
> #regzbot title net: wifi: rtw88: RTW8821CU constantly looses connection
> to the AP and reconnects shortly after
> #regzbot ignore-activity

Forgot to resolve this in regzbot:

#regzbot resolve: turn's out this wasn't a regression, see
https://lore.kernel.org/lkml/20230403100043.GT19113@pengutronix.de/
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.