wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Marcin Ślusarz]

From: Marcin Ślusarz <mslusarz@renau.com>

If I don't connect to any Wifi network, after around 10 minutes, the device
hangs with endless spamming of:
rtw_8821cu 1-1:1.2: rtw_usb_reg_sec: reg 0x4e0, usb write 1 fail, status: -71
killing both Wifi and Bluetooth part of the device.

On arm, just leaving the wifi device unconnected kills it in up to 20 minutes.
If I keep restarting wpa_supplicant I can trigger it within a minute.
Looping "ifconfig wlan0 down; ifconfig wlan0 up" also triggers it within a minute.

On x86_64 system the only way I could trigger this was via ifconfig loop,
but it took 3 hours and 20 minutes to do it.

The only thing that can "fix" the device is replugging it.

I found out that the reason for those hangs is a power-off+on sequence that's
triggered by the above steps.

Disabling power-off for that chip "fixes" the issue. The patches below
implement that, but I'm not seriously proposing them for merging.

Marcin Ślusarz (2):
  wifi: rtw88: use RTW_FLAG_RUNNING for deciding whether to enter/leave
    IPS
  wifi: rtw88: disable power offs for 8821C

 drivers/net/wireless/realtek/rtw88/main.c | 14 ++++++++------
 drivers/net/wireless/realtek/rtw88/ps.c   |  4 ++--
 2 files changed, 10 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH 1/2] wifi: rtw88: use RTW_FLAG_RUNNING for deciding whether to enter/leave IPS

[Marcin Ślusarz]

From: Marcin Ślusarz <mslusarz@renau.com>

Needed by the next patch that disables power off operation for one chip.

Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
Cc: Ping-Ke Shih <pkshih@realtek.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: linux-wireless@vger.kernel.org
---
 drivers/net/wireless/realtek/rtw88/ps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/ps.c b/drivers/net/wireless/realtek/rtw88/ps.c
index add5a20b8432..f9fbc9b3174b 100644
--- a/drivers/net/wireless/realtek/rtw88/ps.c
+++ b/drivers/net/wireless/realtek/rtw88/ps.c
@@ -26,7 +26,7 @@ static int rtw_ips_pwr_up(struct rtw_dev *rtwdev)
 
 int rtw_enter_ips(struct rtw_dev *rtwdev)
 {
-	if (!test_bit(RTW_FLAG_POWERON, rtwdev->flags))
+	if (!test_bit(RTW_FLAG_RUNNING, rtwdev->flags))
 		return 0;
 
 	rtw_coex_ips_notify(rtwdev, COEX_IPS_ENTER);
@@ -50,7 +50,7 @@ int rtw_leave_ips(struct rtw_dev *rtwdev)
 {
 	int ret;
 
-	if (test_bit(RTW_FLAG_POWERON, rtwdev->flags))
+	if (test_bit(RTW_FLAG_RUNNING, rtwdev->flags))
 		return 0;
 
 	rtw_hci_link_ps(rtwdev, false);
-- 
2.25.1

[PATCH 2/2] wifi: rtw88: disable power offs for 8821C

[Marcin Ślusarz]

From: Marcin Ślusarz <mslusarz@renau.com>

This chip fails to reliably wake up from power off.

After some number of power off+on cycles, it stops with endless spamming of:
rtw_8821cu 1-1:1.2: rtw_usb_reg_sec: reg 0x4e0, usb write 1 fail, status: -71
killing both Wifi and Bluetooth part of the device.

On arm, just leaving the wifi device unconfigured kills it in up to 20 minutes.
If I keep restarting wpa_supplicant I can trigger it within a minute.
Looping "ifconfig wlan0 down; ifconfig wlan0 up" also triggers it within a minute.

On x86_64 system the only way I could trigger this was via ifconfig loop,
but it took 3 hours and 20 minutes to do it.

The only thing that "fixes" the device is replugging it.

Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
Cc: Ping-Ke Shih <pkshih@realtek.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: linux-wireless@vger.kernel.org
---
 drivers/net/wireless/realtek/rtw88/main.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index 7ab7a988b123..e8bfa683d7bb 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -1482,11 +1482,11 @@ void rtw_core_scan_complete(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
 
 int rtw_core_start(struct rtw_dev *rtwdev)
 {
-	int ret;
-
-	ret = rtw_power_on(rtwdev);
-	if (ret)
-		return ret;
+	if (!test_bit(RTW_FLAG_POWERON, rtwdev->flags)) {
+		int ret = rtw_power_on(rtwdev);
+		if (ret)
+			return ret;
+	}
 
 	rtw_sec_enable_sec_engine(rtwdev);
 
@@ -1534,7 +1534,9 @@ void rtw_core_stop(struct rtw_dev *rtwdev)
 
 	mutex_lock(&rtwdev->mutex);
 
-	rtw_power_off(rtwdev);
+	/* FIXME: 8821C doesn't wake up from this state from time to time */
+	if (rtwdev->chip->id != RTW_CHIP_TYPE_8821C)
+		rtw_power_off(rtwdev);
 }
 
 static void rtw_init_ht_cap(struct rtw_dev *rtwdev,
-- 
2.25.1

Re: wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Bitterblue Smith]

On 27/05/2024 20:34, Marcin Ślusarz wrote:
> From: Marcin Ślusarz <mslusarz@renau.com>
> 
> If I don't connect to any Wifi network, after around 10 minutes, the device
> hangs with endless spamming of:
> rtw_8821cu 1-1:1.2: rtw_usb_reg_sec: reg 0x4e0, usb write 1 fail, status: -71
> killing both Wifi and Bluetooth part of the device.
> 
> On arm, just leaving the wifi device unconnected kills it in up to 20 minutes.
> If I keep restarting wpa_supplicant I can trigger it within a minute.
> Looping "ifconfig wlan0 down; ifconfig wlan0 up" also triggers it within a minute.
> 
> On x86_64 system the only way I could trigger this was via ifconfig loop,
> but it took 3 hours and 20 minutes to do it.
> 
> The only thing that can "fix" the device is replugging it.
> 
> I found out that the reason for those hangs is a power-off+on sequence that's
> triggered by the above steps.
> 
> Disabling power-off for that chip "fixes" the issue. The patches below
> implement that, but I'm not seriously proposing them for merging.
> 
> Marcin Ślusarz (2):
>   wifi: rtw88: use RTW_FLAG_RUNNING for deciding whether to enter/leave
>     IPS
>   wifi: rtw88: disable power offs for 8821C
> 
>  drivers/net/wireless/realtek/rtw88/main.c | 14 ++++++++------
>  drivers/net/wireless/realtek/rtw88/ps.c   |  4 ++--
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 

The first patch alone doesn't fix it?

RE: wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Ping-Ke Shih]

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> 
> I found out that the reason for those hangs is a power-off+on sequence that's
> triggered by the above steps.

To avoid power-off/on sequence once device becomes idle, I would like to add
a ips_disabled helper. Please revert your changes and apply my attached patch.



[-- Attachment #2: 0001-wifi-rtw88-8821cu-disable-idle-power-save-for-8821CU.patch --]
[-- Type: application/octet-stream, Size: 3624 bytes --]

From e12845f1f6aab639375c5fa5e156d41474572b92 Mon Sep 17 00:00:00 2001
From: Ping-Ke Shih <pkshih@realtek.com>
Date: Tue, 28 May 2024 11:48:13 +0800
Subject: [PATCH] wifi: rtw88: 8821cu: disable idle power save for 8821CU
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This chip fails to reliably wake up from power off.

Change-Id: I295de3c71fe91af37e8cc39b70728a8ba7e94b2f
Reported-by: Marcin Ślusarz <marcin.slusarz@gmail.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 mac80211.c | 2 +-
 main.c     | 4 ++--
 main.h     | 2 ++
 ps.c       | 5 ++++-
 ps.h       | 2 +-
 usb.c      | 3 +++
 wow.c      | 2 +-
 7 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/mac80211.c b/mac80211.c
index 78078d65c88f..5202ba74c20a 100644
--- a/mac80211.c
+++ b/mac80211.c
@@ -99,7 +99,7 @@ static int rtw_ops_config(struct ieee80211_hw *hw, u32 changed)
 	if ((changed & IEEE80211_CONF_CHANGE_IDLE) &&
 	    (hw->conf.flags & IEEE80211_CONF_IDLE) &&
 	    !test_bit(RTW_FLAG_SCANNING, rtwdev->flags))
-		rtw_enter_ips(rtwdev);
+		rtw_enter_ips(rtwdev, false);
 
 out:
 	mutex_unlock(&rtwdev->mutex);
diff --git a/main.c b/main.c
index 567f9d4373c4..9c9ecddb5046 100644
--- a/main.c
+++ b/main.c
@@ -306,7 +306,7 @@ static void rtw_ips_work(struct work_struct *work)
 
 	mutex_lock(&rtwdev->mutex);
 	if (rtwdev->hw->conf.flags & IEEE80211_CONF_IDLE)
-		rtw_enter_ips(rtwdev);
+		rtw_enter_ips(rtwdev, false);
 	mutex_unlock(&rtwdev->mutex);
 }
 
@@ -660,7 +660,7 @@ free:
 	rtw_iterate_stas_atomic(rtwdev, rtw_reset_sta_iter, rtwdev);
 	rtw_iterate_vifs_atomic(rtwdev, rtw_reset_vif_iter, rtwdev);
 	bitmap_zero(rtwdev->hw_port, RTW_PORT_NUM);
-	rtw_enter_ips(rtwdev);
+	rtw_enter_ips(rtwdev, true);
 }
 
 static void rtw_fw_recovery_work(struct work_struct *work)
diff --git a/main.h b/main.h
index d9afb585d423..659bc862d4ca 100644
--- a/main.h
+++ b/main.h
@@ -2283,6 +2283,8 @@ struct rtw_dev {
 	bool beacon_loss;
 	struct completion lps_leave_check;
 
+	bool ips_disabled;
+
 	struct dentry *debugfs;
 
 	u8 sta_cnt;
diff --git a/ps.c b/ps.c
index b171e62d2d57..41de4ecc9bc3 100644
--- a/ps.c
+++ b/ps.c
@@ -24,8 +24,11 @@ static int rtw_ips_pwr_up(struct rtw_dev *rtwdev)
 	return ret;
 }
 
-int rtw_enter_ips(struct rtw_dev *rtwdev)
+int rtw_enter_ips(struct rtw_dev *rtwdev, bool force)
 {
+	if (!force && rtwdev->ips_disabled)
+		return 0;
+
 	if (!test_bit(RTW_FLAG_POWERON, rtwdev->flags))
 		return 0;
 
diff --git a/ps.h b/ps.h
index 5ae83d2526cf..92057d01cbec 100644
--- a/ps.h
+++ b/ps.h
@@ -15,7 +15,7 @@
 #define LEAVE_LPS_TRY_CNT	5
 #define LEAVE_LPS_TIMEOUT	msecs_to_jiffies(100)
 
-int rtw_enter_ips(struct rtw_dev *rtwdev);
+int rtw_enter_ips(struct rtw_dev *rtwdev, bool force);
 int rtw_leave_ips(struct rtw_dev *rtwdev);
 
 void rtw_power_mode_change(struct rtw_dev *rtwdev, bool enter);
diff --git a/usb.c b/usb.c
index a0188511099a..9ca06db6302f 100644
--- a/usb.c
+++ b/usb.c
@@ -854,6 +854,9 @@ int rtw_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	rtwdev->hci.ops = &rtw_usb_ops;
 	rtwdev->hci.type = RTW_HCI_TYPE_USB;
 
+	if (rtwdev->chip->id == RTW_CHIP_TYPE_8821C)
+		rtwdev->ips_disabled = true;
+
 	rtwusb = rtw_get_usb_priv(rtwdev);
 	rtwusb->rtwdev = rtwdev;
 
diff --git a/wow.c b/wow.c
index c86cfc47361a..2163e1dab630 100644
--- a/wow.c
+++ b/wow.c
@@ -677,7 +677,7 @@ static int rtw_wow_restore_ps(struct rtw_dev *rtwdev)
 	int ret = 0;
 
 	if (rtw_wow_no_link(rtwdev) && rtwdev->wow.ips_enabled)
-		ret = rtw_enter_ips(rtwdev);
+		ret = rtw_enter_ips(rtwdev, false);
 
 	return ret;
 }
-- 
2.25.1

RE: [PATCH 1/2] wifi: rtw88: use RTW_FLAG_RUNNING for deciding whether to enter/leave IPS

[Ping-Ke Shih]

Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> From: Marcin Ślusarz <mslusarz@renau.com>
> 
> Needed by the next patch that disables power off operation for one chip.
> 
> Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
> Cc: Ping-Ke Shih <pkshih@realtek.com>
> Cc: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Kalle Valo <kvalo@kernel.org>
> Cc: linux-wireless@vger.kernel.org
> ---
>  drivers/net/wireless/realtek/rtw88/ps.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/ps.c b/drivers/net/wireless/realtek/rtw88/ps.c
> index add5a20b8432..f9fbc9b3174b 100644
> --- a/drivers/net/wireless/realtek/rtw88/ps.c
> +++ b/drivers/net/wireless/realtek/rtw88/ps.c
> @@ -26,7 +26,7 @@ static int rtw_ips_pwr_up(struct rtw_dev *rtwdev)
> 
>  int rtw_enter_ips(struct rtw_dev *rtwdev)
>  {
> -       if (!test_bit(RTW_FLAG_POWERON, rtwdev->flags))
> +       if (!test_bit(RTW_FLAG_RUNNING, rtwdev->flags))

RTW_FLAG_POWERON is to maintain power state (i.e. ips) of WiFi card, and
prevent entering/leaving IPS twice suddenly. Changing this is confused to me. 

Re: wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Marcin Ślusarz]

pon., 27 maj 2024 o 20:43 Bitterblue Smith <rtl8821cerfe2@gmail.com> napisał(a):
>
> On 27/05/2024 20:34, Marcin Ślusarz wrote:
> > From: Marcin Ślusarz <mslusarz@renau.com>
> >
> > If I don't connect to any Wifi network, after around 10 minutes, the device
> > hangs with endless spamming of:
> > rtw_8821cu 1-1:1.2: rtw_usb_reg_sec: reg 0x4e0, usb write 1 fail, status: -71
> > killing both Wifi and Bluetooth part of the device.
> >
> > On arm, just leaving the wifi device unconnected kills it in up to 20 minutes.
> > If I keep restarting wpa_supplicant I can trigger it within a minute.
> > Looping "ifconfig wlan0 down; ifconfig wlan0 up" also triggers it within a minute.
> >
> > On x86_64 system the only way I could trigger this was via ifconfig loop,
> > but it took 3 hours and 20 minutes to do it.
> >
> > The only thing that can "fix" the device is replugging it.
> >
> > I found out that the reason for those hangs is a power-off+on sequence that's
> > triggered by the above steps.
> >
> > Disabling power-off for that chip "fixes" the issue. The patches below
> > implement that, but I'm not seriously proposing them for merging.
> >
> > Marcin Ślusarz (2):
> >   wifi: rtw88: use RTW_FLAG_RUNNING for deciding whether to enter/leave
> >     IPS
> >   wifi: rtw88: disable power offs for 8821C
> >
> >  drivers/net/wireless/realtek/rtw88/main.c | 14 ++++++++------
> >  drivers/net/wireless/realtek/rtw88/ps.c   |  4 ++--
> >  2 files changed, 10 insertions(+), 8 deletions(-)
> >
>
> The first patch alone doesn't fix it?

The first patch exists only to make the second patch work.
Without the first one, rtw_enter_ips will perform all actions except actually
doing the power off, which clears the POWERON flag.
After that, rtw_leave_ips will happily return early success without actually
undoing the stuff that rtw_enter_ips did (like canceling all work_structs).

Re: wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Marcin Ślusarz]

wt., 28 maj 2024 o 05:52 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
>
> Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> >
> > I found out that the reason for those hangs is a power-off+on sequence that's
> > triggered by the above steps.
>
> To avoid power-off/on sequence once device becomes idle, I would like to add
> a ips_disabled helper. Please revert your changes and apply my attached patch.

My first attempt was very similar, and it fixed some cases but not all of them.

This is due to the existence of a second source of power-offs - rtw_ops_stop,
which is called, e.g., on downing the interface (ifconfig wlan0 down).

Re: [PATCH 1/2] wifi: rtw88: use RTW_FLAG_RUNNING for deciding whether to enter/leave IPS

[Marcin Ślusarz]

wt., 28 maj 2024 o 05:56 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
>
> Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > From: Marcin Ślusarz <mslusarz@renau.com>
> >
> > Needed by the next patch that disables power off operation for one chip.
> >
> > Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
> > Cc: Ping-Ke Shih <pkshih@realtek.com>
> > Cc: Larry Finger <Larry.Finger@lwfinger.net>
> > Cc: Kalle Valo <kvalo@kernel.org>
> > Cc: linux-wireless@vger.kernel.org
> > ---
> >  drivers/net/wireless/realtek/rtw88/ps.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/ps.c b/drivers/net/wireless/realtek/rtw88/ps.c
> > index add5a20b8432..f9fbc9b3174b 100644
> > --- a/drivers/net/wireless/realtek/rtw88/ps.c
> > +++ b/drivers/net/wireless/realtek/rtw88/ps.c
> > @@ -26,7 +26,7 @@ static int rtw_ips_pwr_up(struct rtw_dev *rtwdev)
> >
> >  int rtw_enter_ips(struct rtw_dev *rtwdev)
> >  {
> > -       if (!test_bit(RTW_FLAG_POWERON, rtwdev->flags))
> > +       if (!test_bit(RTW_FLAG_RUNNING, rtwdev->flags))
>
> RTW_FLAG_POWERON is to maintain power state (i.e. ips) of WiFi card, and
> prevent entering/leaving IPS twice suddenly. Changing this is confused to me.

I explained it in the other thread as a reply to Bitterblue.

Re: wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Bitterblue Smith]

On 28/05/2024 13:42, Marcin Ślusarz wrote:
> pon., 27 maj 2024 o 20:43 Bitterblue Smith <rtl8821cerfe2@gmail.com> napisał(a):
>>
>> On 27/05/2024 20:34, Marcin Ślusarz wrote:
>>> From: Marcin Ślusarz <mslusarz@renau.com>
>>>
>>> If I don't connect to any Wifi network, after around 10 minutes, the device
>>> hangs with endless spamming of:
>>> rtw_8821cu 1-1:1.2: rtw_usb_reg_sec: reg 0x4e0, usb write 1 fail, status: -71
>>> killing both Wifi and Bluetooth part of the device.
>>>
>>> On arm, just leaving the wifi device unconnected kills it in up to 20 minutes.
>>> If I keep restarting wpa_supplicant I can trigger it within a minute.
>>> Looping "ifconfig wlan0 down; ifconfig wlan0 up" also triggers it within a minute.
>>>
>>> On x86_64 system the only way I could trigger this was via ifconfig loop,
>>> but it took 3 hours and 20 minutes to do it.
>>>
>>> The only thing that can "fix" the device is replugging it.
>>>
>>> I found out that the reason for those hangs is a power-off+on sequence that's
>>> triggered by the above steps.
>>>
>>> Disabling power-off for that chip "fixes" the issue. The patches below
>>> implement that, but I'm not seriously proposing them for merging.
>>>
>>> Marcin Ślusarz (2):
>>>   wifi: rtw88: use RTW_FLAG_RUNNING for deciding whether to enter/leave
>>>     IPS
>>>   wifi: rtw88: disable power offs for 8821C
>>>
>>>  drivers/net/wireless/realtek/rtw88/main.c | 14 ++++++++------
>>>  drivers/net/wireless/realtek/rtw88/ps.c   |  4 ++--
>>>  2 files changed, 10 insertions(+), 8 deletions(-)
>>>
>>
>> The first patch alone doesn't fix it?
> 
> The first patch exists only to make the second patch work.
> Without the first one, rtw_enter_ips will perform all actions except actually
> doing the power off, which clears the POWERON flag.
> After that, rtw_leave_ips will happily return early success without actually
> undoing the stuff that rtw_enter_ips did (like canceling all work_structs).

I see.

I wonder if it's really the chip that has a problem, or rtw88?
Can you try your ifconfig loop with the other driver?
https://github.com/morrownr/8821cu-20210916/

Re: wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Marcin Ślusarz]

wt., 28 maj 2024 o 14:25 Bitterblue Smith <rtl8821cerfe2@gmail.com> napisał(a):
>
> On 28/05/2024 13:42, Marcin Ślusarz wrote:
> > pon., 27 maj 2024 o 20:43 Bitterblue Smith <rtl8821cerfe2@gmail.com> napisał(a):
> >>
> >> On 27/05/2024 20:34, Marcin Ślusarz wrote:
> >>> From: Marcin Ślusarz <mslusarz@renau.com>
> >>>
> >>> If I don't connect to any Wifi network, after around 10 minutes, the device
> >>> hangs with endless spamming of:
> >>> rtw_8821cu 1-1:1.2: rtw_usb_reg_sec: reg 0x4e0, usb write 1 fail, status: -71
> >>> killing both Wifi and Bluetooth part of the device.
> >>>
> >>> On arm, just leaving the wifi device unconnected kills it in up to 20 minutes.
> >>> If I keep restarting wpa_supplicant I can trigger it within a minute.
> >>> Looping "ifconfig wlan0 down; ifconfig wlan0 up" also triggers it within a minute.
> >>>
> >>> On x86_64 system the only way I could trigger this was via ifconfig loop,
> >>> but it took 3 hours and 20 minutes to do it.
> >>>
> >>> The only thing that can "fix" the device is replugging it.
> >>>
> >>> I found out that the reason for those hangs is a power-off+on sequence that's
> >>> triggered by the above steps.
> >>>
> >>> Disabling power-off for that chip "fixes" the issue. The patches below
> >>> implement that, but I'm not seriously proposing them for merging.
> >>>
> >>> Marcin Ślusarz (2):
> >>>   wifi: rtw88: use RTW_FLAG_RUNNING for deciding whether to enter/leave
> >>>     IPS
> >>>   wifi: rtw88: disable power offs for 8821C
> >>>
> >>>  drivers/net/wireless/realtek/rtw88/main.c | 14 ++++++++------
> >>>  drivers/net/wireless/realtek/rtw88/ps.c   |  4 ++--
> >>>  2 files changed, 10 insertions(+), 8 deletions(-)
> >>>
> >>
> >> The first patch alone doesn't fix it?
> >
> > The first patch exists only to make the second patch work.
> > Without the first one, rtw_enter_ips will perform all actions except actually
> > doing the power off, which clears the POWERON flag.
> > After that, rtw_leave_ips will happily return early success without actually
> > undoing the stuff that rtw_enter_ips did (like canceling all work_structs).
>
> I see.
>
> I wonder if it's really the chip that has a problem, or rtw88?
> Can you try your ifconfig loop with the other driver?
> https://github.com/morrownr/8821cu-20210916/

Actually, the issue was found on this driver. I started looking at the issue
more closely (and discovered the exact reproduction steps) after I switched
to rtw88 and discovered it has exactly the same problem.

So yeah, it's either a chip issue or the same bug in both drivers.

RE: wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Ping-Ke Shih]

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> wt., 28 maj 2024 o 05:52 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
> >
> > Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > >
> > > I found out that the reason for those hangs is a power-off+on sequence that's
> > > triggered by the above steps.
> >
> > To avoid power-off/on sequence once device becomes idle, I would like to add
> > a ips_disabled helper. Please revert your changes and apply my attached patch.
> 
> My first attempt was very similar, and it fixed some cases but not all of them.
> 
> This is due to the existence of a second source of power-offs - rtw_ops_stop,
> which is called, e.g., on downing the interface (ifconfig wlan0 down).

Please try attached v2 patch. I would like to have an explicit helper
(i.e. always_power_on in v2) to have this fix, so days later people can be easy
to understand how it works. Not prefer adjusting existing flags to implicitly
have behavior you want. 



[-- Attachment #2: v2-0001-wifi-rtw88-8821cu-keep-power-on-always-for-8821CU.patch --]
[-- Type: application/octet-stream, Size: 4183 bytes --]

From 715a7e758949595646e9134869667dc5a0437c3e Mon Sep 17 00:00:00 2001
From: Ping-Ke Shih <pkshih@realtek.com>
Date: Tue, 28 May 2024 11:48:13 +0800
Subject: [PATCH v2] wifi: rtw88: 8821cu: keep power on always for 8821CU
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This chip fails to reliably wake up from power off.

Change-Id: I295de3c71fe91af37e8cc39b70728a8ba7e94b2f
Reported-by: Marcin Ślusarz <marcin.slusarz@gmail.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 mac80211.c |  2 +-
 main.c     | 10 ++++++++--
 main.h     |  2 ++
 ps.c       |  5 ++++-
 ps.h       |  2 +-
 usb.c      |  3 +++
 wow.c      |  2 +-
 7 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/mac80211.c b/mac80211.c
index 78078d65c88f..5202ba74c20a 100644
--- a/mac80211.c
+++ b/mac80211.c
@@ -99,7 +99,7 @@ static int rtw_ops_config(struct ieee80211_hw *hw, u32 changed)
 	if ((changed & IEEE80211_CONF_CHANGE_IDLE) &&
 	    (hw->conf.flags & IEEE80211_CONF_IDLE) &&
 	    !test_bit(RTW_FLAG_SCANNING, rtwdev->flags))
-		rtw_enter_ips(rtwdev);
+		rtw_enter_ips(rtwdev, false);
 
 out:
 	mutex_unlock(&rtwdev->mutex);
diff --git a/main.c b/main.c
index 567f9d4373c4..90d18b338a12 100644
--- a/main.c
+++ b/main.c
@@ -306,7 +306,7 @@ static void rtw_ips_work(struct work_struct *work)
 
 	mutex_lock(&rtwdev->mutex);
 	if (rtwdev->hw->conf.flags & IEEE80211_CONF_IDLE)
-		rtw_enter_ips(rtwdev);
+		rtw_enter_ips(rtwdev, false);
 	mutex_unlock(&rtwdev->mutex);
 }
 
@@ -660,7 +660,7 @@ free:
 	rtw_iterate_stas_atomic(rtwdev, rtw_reset_sta_iter, rtwdev);
 	rtw_iterate_vifs_atomic(rtwdev, rtw_reset_vif_iter, rtwdev);
 	bitmap_zero(rtwdev->hw_port, RTW_PORT_NUM);
-	rtw_enter_ips(rtwdev);
+	rtw_enter_ips(rtwdev, true);
 }
 
 static void rtw_fw_recovery_work(struct work_struct *work)
@@ -1375,6 +1375,9 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
 	bool wifi_only;
 	int ret;
 
+	if (rtwdev->always_power_on && test_bit(RTW_FLAG_POWERON, rtwdev->flags))
+		return 0;
+
 	ret = rtw_hci_setup(rtwdev);
 	if (ret) {
 		rtw_err(rtwdev, "failed to setup hci\n");
@@ -1525,6 +1528,9 @@ int rtw_core_start(struct rtw_dev *rtwdev)
 
 static void rtw_power_off(struct rtw_dev *rtwdev)
 {
+	if (rtwdev->always_power_on)
+		return;
+
 	rtw_hci_stop(rtwdev);
 	rtw_coex_power_off_setting(rtwdev);
 	rtw_mac_power_off(rtwdev);
diff --git a/main.h b/main.h
index d9afb585d423..e1c316bd7018 100644
--- a/main.h
+++ b/main.h
@@ -2283,6 +2283,8 @@ struct rtw_dev {
 	bool beacon_loss;
 	struct completion lps_leave_check;
 
+	bool always_power_on;
+
 	struct dentry *debugfs;
 
 	u8 sta_cnt;
diff --git a/ps.c b/ps.c
index b171e62d2d57..65223751fe2c 100644
--- a/ps.c
+++ b/ps.c
@@ -24,8 +24,11 @@ static int rtw_ips_pwr_up(struct rtw_dev *rtwdev)
 	return ret;
 }
 
-int rtw_enter_ips(struct rtw_dev *rtwdev)
+int rtw_enter_ips(struct rtw_dev *rtwdev, bool force)
 {
+	if (!force && rtwdev->always_power_on)
+		return 0;
+
 	if (!test_bit(RTW_FLAG_POWERON, rtwdev->flags))
 		return 0;
 
diff --git a/ps.h b/ps.h
index 5ae83d2526cf..92057d01cbec 100644
--- a/ps.h
+++ b/ps.h
@@ -15,7 +15,7 @@
 #define LEAVE_LPS_TRY_CNT	5
 #define LEAVE_LPS_TIMEOUT	msecs_to_jiffies(100)
 
-int rtw_enter_ips(struct rtw_dev *rtwdev);
+int rtw_enter_ips(struct rtw_dev *rtwdev, bool force);
 int rtw_leave_ips(struct rtw_dev *rtwdev);
 
 void rtw_power_mode_change(struct rtw_dev *rtwdev, bool enter);
diff --git a/usb.c b/usb.c
index a0188511099a..22742fe81b9a 100644
--- a/usb.c
+++ b/usb.c
@@ -854,6 +854,9 @@ int rtw_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	rtwdev->hci.ops = &rtw_usb_ops;
 	rtwdev->hci.type = RTW_HCI_TYPE_USB;
 
+	if (rtwdev->chip->id == RTW_CHIP_TYPE_8821C)
+		rtwdev->always_power_on = true;
+
 	rtwusb = rtw_get_usb_priv(rtwdev);
 	rtwusb->rtwdev = rtwdev;
 
diff --git a/wow.c b/wow.c
index c86cfc47361a..2163e1dab630 100644
--- a/wow.c
+++ b/wow.c
@@ -677,7 +677,7 @@ static int rtw_wow_restore_ps(struct rtw_dev *rtwdev)
 	int ret = 0;
 
 	if (rtw_wow_no_link(rtwdev) && rtwdev->wow.ips_enabled)
-		ret = rtw_enter_ips(rtwdev);
+		ret = rtw_enter_ips(rtwdev, false);
 
 	return ret;
 }
-- 
2.25.1

Re: wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Marcin Ślusarz]

śr., 29 maj 2024 o 03:52 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
>
> Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > wt., 28 maj 2024 o 05:52 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
> > >
> > > Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > > >
> > > > I found out that the reason for those hangs is a power-off+on sequence that's
> > > > triggered by the above steps.
> > >
> > > To avoid power-off/on sequence once device becomes idle, I would like to add
> > > a ips_disabled helper. Please revert your changes and apply my attached patch.
> >
> > My first attempt was very similar, and it fixed some cases but not all of them.
> >
> > This is due to the existence of a second source of power-offs - rtw_ops_stop,
> > which is called, e.g., on downing the interface (ifconfig wlan0 down).
>
> Please try attached v2 patch. I would like to have an explicit helper
> (i.e. always_power_on in v2) to have this fix, so days later people can be easy
> to understand how it works. Not prefer adjusting existing flags to implicitly
> have behavior you want.

So, do you think this is a chip issue, not just some driver misconfiguration?

I'm asking because if we are going in this direction, there's something
more to fix... With your v2, very frequently, I hit WARN_ON(!local->started) in
ieee80211_rx_napi (in wireless-next, the code was moved to ieee80211_rx_list).

With my patch, I checked and hit that WARN_ON, too, but very occasionally.

I think the difference is in what happens in rtw_ips_enter - I disabled only
the power_off, but you also disabled everything else, including the cancelation
of work_structs.

The warning itself sounds harmless, but I think users should never see such
warnings, so this needs to be fixed somehow. Probably some additional
work_struct(s) need to be canceled?

Marcin

RE: wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Ping-Ke Shih]

Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> śr., 29 maj 2024 o 03:52 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
> >
> > Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > > wt., 28 maj 2024 o 05:52 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
> > > >
> > > > Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > > > >
> > > > > I found out that the reason for those hangs is a power-off+on sequence that's
> > > > > triggered by the above steps.
> > > >
> > > > To avoid power-off/on sequence once device becomes idle, I would like to add
> > > > a ips_disabled helper. Please revert your changes and apply my attached patch.
> > >
> > > My first attempt was very similar, and it fixed some cases but not all of them.
> > >
> > > This is due to the existence of a second source of power-offs - rtw_ops_stop,
> > > which is called, e.g., on downing the interface (ifconfig wlan0 down).
> >
> > Please try attached v2 patch. I would like to have an explicit helper
> > (i.e. always_power_on in v2) to have this fix, so days later people can be easy
> > to understand how it works. Not prefer adjusting existing flags to implicitly
> > have behavior you want.
> 
> So, do you think this is a chip issue, not just some driver misconfiguration?

I asked internal USB WiFi people who say vendor drivers of USB/SDIO can't
power-on/-off frequently but not very sure if hardware issue or driver issue.

> 
> I'm asking because if we are going in this direction, there's something
> more to fix... With your v2, very frequently, I hit WARN_ON(!local->started) in
> ieee80211_rx_napi (in wireless-next, the code was moved to ieee80211_rx_list).
> 
> With my patch, I checked and hit that WARN_ON, too, but very occasionally.
> 
> I think the difference is in what happens in rtw_ips_enter - I disabled only
> the power_off, but you also disabled everything else, including the cancelation
> of work_structs.
> 
> The warning itself sounds harmless, but I think users should never see such
> warnings, so this needs to be fixed somehow. Probably some additional
> work_struct(s) need to be canceled?
> 

I forgot to say my patch is compiled test only, and I didn't consider flow
too much, just to close the behavior of your patches. You can improve my patch
to be more reliable to avoid WARN_ON().  

Ping-Ke

Re: wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Marcin Ślusarz]

czw., 30 maj 2024 o 05:13 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
>
> Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > śr., 29 maj 2024 o 03:52 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
> > >
> > > Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > > > wt., 28 maj 2024 o 05:52 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
> > > > >
> > > > > Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > > > > >
> > > > > > I found out that the reason for those hangs is a power-off+on sequence that's
> > > > > > triggered by the above steps.
> > > > >
> > > > > To avoid power-off/on sequence once device becomes idle, I would like to add
> > > > > a ips_disabled helper. Please revert your changes and apply my attached patch.
> > > >
> > > > My first attempt was very similar, and it fixed some cases but not all of them.
> > > >
> > > > This is due to the existence of a second source of power-offs - rtw_ops_stop,
> > > > which is called, e.g., on downing the interface (ifconfig wlan0 down).
> > >
> > > Please try attached v2 patch. I would like to have an explicit helper
> > > (i.e. always_power_on in v2) to have this fix, so days later people can be easy
> > > to understand how it works. Not prefer adjusting existing flags to implicitly
> > > have behavior you want.
> >
> > So, do you think this is a chip issue, not just some driver misconfiguration?
>
> I asked internal USB WiFi people who say vendor drivers of USB/SDIO can't
> power-on/-off frequently but not very sure if hardware issue or driver issue.
>
> >
> > I'm asking because if we are going in this direction, there's something
> > more to fix... With your v2, very frequently, I hit WARN_ON(!local->started) in
> > ieee80211_rx_napi (in wireless-next, the code was moved to ieee80211_rx_list).
> >
> > With my patch, I checked and hit that WARN_ON, too, but very occasionally.
> >
> > I think the difference is in what happens in rtw_ips_enter - I disabled only
> > the power_off, but you also disabled everything else, including the cancelation
> > of work_structs.
> >
> > The warning itself sounds harmless, but I think users should never see such
> > warnings, so this needs to be fixed somehow. Probably some additional
> > work_struct(s) need to be canceled?
> >
>
> I forgot to say my patch is compiled test only, and I didn't consider flow
> too much, just to close the behavior of your patches. You can improve my patch
> to be more reliable to avoid WARN_ON().

Two variants of the patch that fix this issue will follow. They are
built on top of yours
v2 and my "wifi: rtw88: schedule rx work after everything is set up"
from the other thread.
Please choose the one you like more :).

[PATCH] wifi: rtw88/usb: stop rx work before potential power off

[Marcin Ślusarz]

From: Marcin Ślusarz <mslusarz@renau.com>

Avoids WARN_ON(!local->started) in ieee80211_rx_list, after
the patch that disables power management of 8821CU.

Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
---
 drivers/net/wireless/realtek/rtw88/hci.h  | 12 +++++++
 drivers/net/wireless/realtek/rtw88/main.c |  7 +++-
 drivers/net/wireless/realtek/rtw88/pci.c  |  6 ++++
 drivers/net/wireless/realtek/rtw88/sdio.c |  6 ++++
 drivers/net/wireless/realtek/rtw88/usb.c  | 40 +++++++++++++++--------
 drivers/net/wireless/realtek/rtw88/usb.h  |  1 +
 6 files changed, 58 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/hci.h b/drivers/net/wireless/realtek/rtw88/hci.h
index 830d7532f2a3..d1b38b34fdd0 100644
--- a/drivers/net/wireless/realtek/rtw88/hci.h
+++ b/drivers/net/wireless/realtek/rtw88/hci.h
@@ -18,6 +18,8 @@ struct rtw_hci_ops {
 	void (*deep_ps)(struct rtw_dev *rtwdev, bool enter);
 	void (*link_ps)(struct rtw_dev *rtwdev, bool enter);
 	void (*interface_cfg)(struct rtw_dev *rtwdev);
+	void (*stop_rx)(struct rtw_dev *rtwdev);
+	void (*start_rx)(struct rtw_dev *rtwdev);
 
 	int (*write_data_rsvd_page)(struct rtw_dev *rtwdev, u8 *buf, u32 size);
 	int (*write_data_h2c)(struct rtw_dev *rtwdev, u8 *buf, u32 size);
@@ -57,6 +59,16 @@ static inline void rtw_hci_stop(struct rtw_dev *rtwdev)
 	rtwdev->hci.ops->stop(rtwdev);
 }
 
+static inline void rtw_hci_start_rx(struct rtw_dev *rtwdev)
+{
+	rtwdev->hci.ops->start_rx(rtwdev);
+}
+
+static inline void rtw_hci_stop_rx(struct rtw_dev *rtwdev)
+{
+	rtwdev->hci.ops->stop_rx(rtwdev);
+}
+
 static inline void rtw_hci_deep_ps(struct rtw_dev *rtwdev, bool enter)
 {
 	rtwdev->hci.ops->deep_ps(rtwdev, enter);
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index a48e919adddb..bb0122d19416 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -1357,7 +1357,7 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
 	int ret;
 
 	if (rtwdev->always_power_on && test_bit(RTW_FLAG_POWERON, rtwdev->flags))
-		return 0;
+		goto success;
 
 	ret = rtw_hci_setup(rtwdev);
 	if (ret) {
@@ -1407,6 +1407,9 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
 	rtw_coex_power_on_setting(rtwdev);
 	rtw_coex_init_hw_config(rtwdev, wifi_only);
 
+success:
+	rtw_hci_start_rx(rtwdev);
+
 	return 0;
 
 err_off:
@@ -1509,6 +1512,8 @@ int rtw_core_start(struct rtw_dev *rtwdev)
 
 static void rtw_power_off(struct rtw_dev *rtwdev)
 {
+	rtw_hci_stop_rx(rtwdev);
+
 	if (rtwdev->always_power_on)
 		return;
 
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index 7a093f3d5f74..0a3ec94f6ab2 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -1590,6 +1590,10 @@ static void rtw_pci_destroy(struct rtw_dev *rtwdev, struct pci_dev *pdev)
 	rtw_pci_io_unmapping(rtwdev, pdev);
 }
 
+static void rtw_pci_nop(struct rtw_dev *rtwdev)
+{
+}
+
 static struct rtw_hci_ops rtw_pci_ops = {
 	.tx_write = rtw_pci_tx_write,
 	.tx_kick_off = rtw_pci_tx_kick_off,
@@ -1600,6 +1604,8 @@ static struct rtw_hci_ops rtw_pci_ops = {
 	.deep_ps = rtw_pci_deep_ps,
 	.link_ps = rtw_pci_link_ps,
 	.interface_cfg = rtw_pci_interface_cfg,
+	.stop_rx = rtw_pci_nop,
+	.start_rx = rtw_pci_nop,
 
 	.read8 = rtw_pci_read8,
 	.read16 = rtw_pci_read16,
diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index 0cae5746f540..4a7923851c81 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -1147,6 +1147,10 @@ static void rtw_sdio_declaim(struct rtw_dev *rtwdev,
 	sdio_release_host(sdio_func);
 }
 
+static void rtw_sdio_nop(struct rtw_dev *rtwdev)
+{
+}
+
 static struct rtw_hci_ops rtw_sdio_ops = {
 	.tx_write = rtw_sdio_tx_write,
 	.tx_kick_off = rtw_sdio_tx_kick_off,
@@ -1156,6 +1160,8 @@ static struct rtw_hci_ops rtw_sdio_ops = {
 	.deep_ps = rtw_sdio_deep_ps,
 	.link_ps = rtw_sdio_link_ps,
 	.interface_cfg = rtw_sdio_interface_cfg,
+	.stop_rx = rtw_sdio_nop,
+	.start_rx = rtw_sdio_nop,
 
 	.read8 = rtw_sdio_read8,
 	.read16 = rtw_sdio_read16,
diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index e1b66f339cca..d5cf3eb51c8a 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -716,6 +716,30 @@ static void rtw_usb_interface_cfg(struct rtw_dev *rtwdev)
 	/* empty function for rtw_hci_ops */
 }
 
+static void rtw_usb_stop_rx(struct rtw_dev *rtwdev)
+{
+	struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
+	rtw_usb_cancel_rx_bufs(rtwusb);
+	rtwusb->rx_enabled = false;
+}
+
+static void rtw_usb_start_rx(struct rtw_dev *rtwdev)
+{
+	struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
+	int i;
+
+	if (rtwusb->rx_enabled)
+		return;
+
+	for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
+		struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
+
+		rtw_usb_rx_resubmit(rtwusb, rxcb);
+	}
+
+	rtwusb->rx_enabled = true;
+}
+
 static struct rtw_hci_ops rtw_usb_ops = {
 	.tx_write = rtw_usb_tx_write,
 	.tx_kick_off = rtw_usb_tx_kick_off,
@@ -725,6 +749,8 @@ static struct rtw_hci_ops rtw_usb_ops = {
 	.deep_ps = rtw_usb_deep_ps,
 	.link_ps = rtw_usb_link_ps,
 	.interface_cfg = rtw_usb_interface_cfg,
+	.stop_rx = rtw_usb_stop_rx,
+	.start_rx = rtw_usb_start_rx,
 
 	.write8  = rtw_usb_write8,
 	.write16 = rtw_usb_write16,
@@ -754,18 +780,6 @@ static int rtw_usb_init_rx(struct rtw_dev *rtwdev)
 	return 0;
 }
 
-static void rtw_usb_setup_rx(struct rtw_dev *rtwdev)
-{
-	struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
-	int i;
-
-	for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
-		struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
-
-		rtw_usb_rx_resubmit(rtwusb, rxcb);
-	}
-}
-
 static void rtw_usb_deinit_rx(struct rtw_dev *rtwdev)
 {
 	struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
@@ -903,7 +917,7 @@ int rtw_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
 		goto err_destroy_rxwq;
 	}
 
-	rtw_usb_setup_rx(rtwdev);
+	rtw_usb_start_rx(rtwdev);
 
 	return 0;
 
diff --git a/drivers/net/wireless/realtek/rtw88/usb.h b/drivers/net/wireless/realtek/rtw88/usb.h
index 86697a5c0103..a6b004d4f74e 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.h
+++ b/drivers/net/wireless/realtek/rtw88/usb.h
@@ -82,6 +82,7 @@ struct rtw_usb {
 	struct rx_usb_ctrl_block rx_cb[RTW_USB_RXCB_NUM];
 	struct sk_buff_head rx_queue;
 	struct work_struct rx_work;
+	bool rx_enabled;
 };
 
 static inline struct rtw_usb_tx_data *rtw_usb_get_tx_data(struct sk_buff *skb)
-- 
2.25.1

[PATCH] wifi: rtw88: usb: drop rx skbs when device is not running

[Marcin Ślusarz]

From: Marcin Ślusarz <mslusarz@renau.com>

Avoids WARN_ON(!local->started) in ieee80211_rx_list, after
the patch that disables power management of 8821CU.

Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
---
 drivers/net/wireless/realtek/rtw88/usb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index e1b66f339cca..c25fd4b193a7 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -570,6 +570,11 @@ static void rtw_usb_rx_handler(struct work_struct *work)
 			continue;
 		}
 
+		if (!test_bit(RTW_FLAG_RUNNING, rtwdev->flags)) {
+			dev_kfree_skb_any(skb);
+			continue;
+		}
+
 		skb_put(skb, pkt_stat.pkt_len);
 		skb_reserve(skb, pkt_offset);
 		memcpy(skb->cb, &rx_status, sizeof(rx_status));
-- 
2.25.1

RE: wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Ping-Ke Shih]

Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > >
> > > I'm asking because if we are going in this direction, there's something
> > > more to fix... With your v2, very frequently, I hit WARN_ON(!local->started) in
> > > ieee80211_rx_napi (in wireless-next, the code was moved to ieee80211_rx_list).
> > >
> > > With my patch, I checked and hit that WARN_ON, too, but very occasionally.
> > >
> > > I think the difference is in what happens in rtw_ips_enter - I disabled only
> > > the power_off, but you also disabled everything else, including the cancelation
> > > of work_structs.
> > >
> > > The warning itself sounds harmless, but I think users should never see such
> > > warnings, so this needs to be fixed somehow. Probably some additional
> > > work_struct(s) need to be canceled?
> > >
> >
> > I forgot to say my patch is compiled test only, and I didn't consider flow
> > too much, just to close the behavior of your patches. You can improve my patch
> > to be more reliable to avoid WARN_ON().
> 
> Two variants of the patch that fix this issue will follow. They are
> built on top of yours
> v2 and my "wifi: rtw88: schedule rx work after everything is set up"
> from the other thread.
> Please choose the one you like more :).

The patch "wifi: rtw88: usb: drop rx skbs when device is not running" is
to drop all skb it receives. USB is still working, so I don't prefer this.

"wifi: rtw88/usb: stop rx work before potential power off" seems to stop
RX. But how rtw_usb_cancel_rx_bufs() can stop RX? Remove RX urb to stop it?
Not sure if this is regular method for USB devices?

By the way, please take and refine my v2 patch into your patchset. That will
be easier to me to merge patches finally.

Ping-Ke

RE: [PATCH] wifi: rtw88/usb: stop rx work before potential power off

[Ping-Ke Shih]

Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> From: Marcin Ślusarz <mslusarz@renau.com>
> 
> Avoids WARN_ON(!local->started) in ieee80211_rx_list, after
> the patch that disables power management of 8821CU.

Please describe how/what you do in this patch. 

> 
> Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
> ---
>  drivers/net/wireless/realtek/rtw88/hci.h  | 12 +++++++
>  drivers/net/wireless/realtek/rtw88/main.c |  7 +++-
>  drivers/net/wireless/realtek/rtw88/pci.c  |  6 ++++
>  drivers/net/wireless/realtek/rtw88/sdio.c |  6 ++++
>  drivers/net/wireless/realtek/rtw88/usb.c  | 40 +++++++++++++++--------
>  drivers/net/wireless/realtek/rtw88/usb.h  |  1 +
>  6 files changed, 58 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/hci.h b/drivers/net/wireless/realtek/rtw88/hci.h
> index 830d7532f2a3..d1b38b34fdd0 100644
> --- a/drivers/net/wireless/realtek/rtw88/hci.h
> +++ b/drivers/net/wireless/realtek/rtw88/hci.h
> @@ -18,6 +18,8 @@ struct rtw_hci_ops {
>         void (*deep_ps)(struct rtw_dev *rtwdev, bool enter);
>         void (*link_ps)(struct rtw_dev *rtwdev, bool enter);
>         void (*interface_cfg)(struct rtw_dev *rtwdev);
> +       void (*stop_rx)(struct rtw_dev *rtwdev);
> +       void (*start_rx)(struct rtw_dev *rtwdev);
> 
>         int (*write_data_rsvd_page)(struct rtw_dev *rtwdev, u8 *buf, u32 size);
>         int (*write_data_h2c)(struct rtw_dev *rtwdev, u8 *buf, u32 size);
> @@ -57,6 +59,16 @@ static inline void rtw_hci_stop(struct rtw_dev *rtwdev)
>         rtwdev->hci.ops->stop(rtwdev);
>  }
> 
> +static inline void rtw_hci_start_rx(struct rtw_dev *rtwdev)
> +{

For PCI/SDIO nop, I would like to give them NULL, so here can be

if (rtwdev->hci.ops->start_rx)
	rtwdev->hci.ops->start_rx(rtwdev);

> +       rtwdev->hci.ops->start_rx(rtwdev);
> +}
> +
> +static inline void rtw_hci_stop_rx(struct rtw_dev *rtwdev)
> +{
> +       rtwdev->hci.ops->stop_rx(rtwdev);
> +}
> +
>  static inline void rtw_hci_deep_ps(struct rtw_dev *rtwdev, bool enter)
>  {
>         rtwdev->hci.ops->deep_ps(rtwdev, enter);
> diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> index a48e919adddb..bb0122d19416 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.c
> +++ b/drivers/net/wireless/realtek/rtw88/main.c
> @@ -1357,7 +1357,7 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
>         int ret;
> 
>         if (rtwdev->always_power_on && test_bit(RTW_FLAG_POWERON, rtwdev->flags))
> -               return 0;
> +               goto success;

rtw_hci_start_rx(rtwdev) is only needed by this case, so 

if (rtwdev->always_power_on && test_bit(RTW_FLAG_POWERON, rtwdev->flags)) {
	rtw_hci_start_rx(rtwdev);
	return 0;
}

> 
>         ret = rtw_hci_setup(rtwdev);
>         if (ret) {
> @@ -1407,6 +1407,9 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
>         rtw_coex_power_on_setting(rtwdev);
>         rtw_coex_init_hw_config(rtwdev, wifi_only);
> 
> +success:
> +       rtw_hci_start_rx(rtwdev);
> +
>         return 0;
> 
>  err_off:
> @@ -1509,6 +1512,8 @@ int rtw_core_start(struct rtw_dev *rtwdev)
> 
>  static void rtw_power_off(struct rtw_dev *rtwdev)
>  {
> +       rtw_hci_stop_rx(rtwdev);
> +

Similarly here can be

if (rtwdev->always_power_on) {
	rtw_hci_stop_rx(rtwdev);
	return;
}


>         if (rtwdev->always_power_on)
>                 return;
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> index 7a093f3d5f74..0a3ec94f6ab2 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -1590,6 +1590,10 @@ static void rtw_pci_destroy(struct rtw_dev *rtwdev, struct pci_dev *pdev)
>         rtw_pci_io_unmapping(rtwdev, pdev);
>  }
> 
> +static void rtw_pci_nop(struct rtw_dev *rtwdev)
> +{
> +}
> +
>  static struct rtw_hci_ops rtw_pci_ops = {
>         .tx_write = rtw_pci_tx_write,
>         .tx_kick_off = rtw_pci_tx_kick_off,
> @@ -1600,6 +1604,8 @@ static struct rtw_hci_ops rtw_pci_ops = {
>         .deep_ps = rtw_pci_deep_ps,
>         .link_ps = rtw_pci_link_ps,
>         .interface_cfg = rtw_pci_interface_cfg,
> +       .stop_rx = rtw_pci_nop,
> +       .start_rx = rtw_pci_nop,
> 
>         .read8 = rtw_pci_read8,
>         .read16 = rtw_pci_read16,
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index 0cae5746f540..4a7923851c81 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -1147,6 +1147,10 @@ static void rtw_sdio_declaim(struct rtw_dev *rtwdev,
>         sdio_release_host(sdio_func);
>  }
> 
> +static void rtw_sdio_nop(struct rtw_dev *rtwdev)
> +{
> +}
> +
>  static struct rtw_hci_ops rtw_sdio_ops = {
>         .tx_write = rtw_sdio_tx_write,
>         .tx_kick_off = rtw_sdio_tx_kick_off,
> @@ -1156,6 +1160,8 @@ static struct rtw_hci_ops rtw_sdio_ops = {
>         .deep_ps = rtw_sdio_deep_ps,
>         .link_ps = rtw_sdio_link_ps,
>         .interface_cfg = rtw_sdio_interface_cfg,
> +       .stop_rx = rtw_sdio_nop,
> +       .start_rx = rtw_sdio_nop,
> 
>         .read8 = rtw_sdio_read8,
>         .read16 = rtw_sdio_read16,
> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> index e1b66f339cca..d5cf3eb51c8a 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -716,6 +716,30 @@ static void rtw_usb_interface_cfg(struct rtw_dev *rtwdev)
>         /* empty function for rtw_hci_ops */
>  }
> 
> +static void rtw_usb_stop_rx(struct rtw_dev *rtwdev)
> +{
> +       struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);

Do we really need a ' rtwusb->rx_enabled' to maintain symmetric calling of 
start/stop_rx? If yes, here should add

	if (!rtwusb->rx_enabled)
		return;

But, I don't like that flag if it isn't strongly required. 

> +       rtw_usb_cancel_rx_bufs(rtwusb);
> +       rtwusb->rx_enabled = false;
> +}
> +
> +static void rtw_usb_start_rx(struct rtw_dev *rtwdev)
> +{
> +       struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> +       int i;
> +
> +       if (rtwusb->rx_enabled)
> +               return;
> +
> +       for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
> +               struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
> +
> +               rtw_usb_rx_resubmit(rtwusb, rxcb);
> +       }
> +
> +       rtwusb->rx_enabled = true;
> +}
> +
>  static struct rtw_hci_ops rtw_usb_ops = {
>         .tx_write = rtw_usb_tx_write,
>         .tx_kick_off = rtw_usb_tx_kick_off,
> @@ -725,6 +749,8 @@ static struct rtw_hci_ops rtw_usb_ops = {
>         .deep_ps = rtw_usb_deep_ps,
>         .link_ps = rtw_usb_link_ps,
>         .interface_cfg = rtw_usb_interface_cfg,
> +       .stop_rx = rtw_usb_stop_rx,
> +       .start_rx = rtw_usb_start_rx,
> 
>         .write8  = rtw_usb_write8,
>         .write16 = rtw_usb_write16,
> @@ -754,18 +780,6 @@ static int rtw_usb_init_rx(struct rtw_dev *rtwdev)
>         return 0;
>  }
> 
> -static void rtw_usb_setup_rx(struct rtw_dev *rtwdev)
> -{
> -       struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> -       int i;
> -
> -       for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
> -               struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
> -
> -               rtw_usb_rx_resubmit(rtwusb, rxcb);
> -       }
> -}
> -
>  static void rtw_usb_deinit_rx(struct rtw_dev *rtwdev)
>  {
>         struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> @@ -903,7 +917,7 @@ int rtw_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
>                 goto err_destroy_rxwq;
>         }
> 
> -       rtw_usb_setup_rx(rtwdev);
> +       rtw_usb_start_rx(rtwdev);
> 
>         return 0;
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/usb.h b/drivers/net/wireless/realtek/rtw88/usb.h
> index 86697a5c0103..a6b004d4f74e 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.h
> +++ b/drivers/net/wireless/realtek/rtw88/usb.h
> @@ -82,6 +82,7 @@ struct rtw_usb {
>         struct rx_usb_ctrl_block rx_cb[RTW_USB_RXCB_NUM];
>         struct sk_buff_head rx_queue;
>         struct work_struct rx_work;
> +       bool rx_enabled;
>  };
> 
>  static inline struct rtw_usb_tx_data *rtw_usb_get_tx_data(struct sk_buff *skb)
> --
> 2.25.1
> 

Re: [PATCH] wifi: rtw88/usb: stop rx work before potential power off

[Marcin Ślusarz]

wt., 4 cze 2024 o 02:57 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
>
> Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > From: Marcin Ślusarz <mslusarz@renau.com>
> >
> > Avoids WARN_ON(!local->started) in ieee80211_rx_list, after
> > the patch that disables power management of 8821CU.
>
> Please describe how/what you do in this patch.
>
> >
> > Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
> > ---
> >  drivers/net/wireless/realtek/rtw88/hci.h  | 12 +++++++
> >  drivers/net/wireless/realtek/rtw88/main.c |  7 +++-
> >  drivers/net/wireless/realtek/rtw88/pci.c  |  6 ++++
> >  drivers/net/wireless/realtek/rtw88/sdio.c |  6 ++++
> >  drivers/net/wireless/realtek/rtw88/usb.c  | 40 +++++++++++++++--------
> >  drivers/net/wireless/realtek/rtw88/usb.h  |  1 +
> >  6 files changed, 58 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/hci.h b/drivers/net/wireless/realtek/rtw88/hci.h
> > index 830d7532f2a3..d1b38b34fdd0 100644
> > --- a/drivers/net/wireless/realtek/rtw88/hci.h
> > +++ b/drivers/net/wireless/realtek/rtw88/hci.h
> > @@ -18,6 +18,8 @@ struct rtw_hci_ops {
> >         void (*deep_ps)(struct rtw_dev *rtwdev, bool enter);
> >         void (*link_ps)(struct rtw_dev *rtwdev, bool enter);
> >         void (*interface_cfg)(struct rtw_dev *rtwdev);
> > +       void (*stop_rx)(struct rtw_dev *rtwdev);
> > +       void (*start_rx)(struct rtw_dev *rtwdev);
> >
> >         int (*write_data_rsvd_page)(struct rtw_dev *rtwdev, u8 *buf, u32 size);
> >         int (*write_data_h2c)(struct rtw_dev *rtwdev, u8 *buf, u32 size);
> > @@ -57,6 +59,16 @@ static inline void rtw_hci_stop(struct rtw_dev *rtwdev)
> >         rtwdev->hci.ops->stop(rtwdev);
> >  }
> >
> > +static inline void rtw_hci_start_rx(struct rtw_dev *rtwdev)
> > +{
>
> For PCI/SDIO nop, I would like to give them NULL, so here can be
>
> if (rtwdev->hci.ops->start_rx)
>         rtwdev->hci.ops->start_rx(rtwdev);

Sure

>
> > +       rtwdev->hci.ops->start_rx(rtwdev);
> > +}
> > +
> > +static inline void rtw_hci_stop_rx(struct rtw_dev *rtwdev)
> > +{
> > +       rtwdev->hci.ops->stop_rx(rtwdev);
> > +}
> > +
> >  static inline void rtw_hci_deep_ps(struct rtw_dev *rtwdev, bool enter)
> >  {
> >         rtwdev->hci.ops->deep_ps(rtwdev, enter);
> > diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> > index a48e919adddb..bb0122d19416 100644
> > --- a/drivers/net/wireless/realtek/rtw88/main.c
> > +++ b/drivers/net/wireless/realtek/rtw88/main.c
> > @@ -1357,7 +1357,7 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
> >         int ret;
> >
> >         if (rtwdev->always_power_on && test_bit(RTW_FLAG_POWERON, rtwdev->flags))
> > -               return 0;
> > +               goto success;
>
> rtw_hci_start_rx(rtwdev) is only needed by this case, so
>
> if (rtwdev->always_power_on && test_bit(RTW_FLAG_POWERON, rtwdev->flags)) {
>         rtw_hci_start_rx(rtwdev);
>         return 0;
> }

Yes, strictly speaking, it's needed only in the always_power_on case,
but doing that in the common code path ensures that it's tested and
still works.

>
> >
> >         ret = rtw_hci_setup(rtwdev);
> >         if (ret) {
> > @@ -1407,6 +1407,9 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
> >         rtw_coex_power_on_setting(rtwdev);
> >         rtw_coex_init_hw_config(rtwdev, wifi_only);
> >
> > +success:
> > +       rtw_hci_start_rx(rtwdev);
> > +
> >         return 0;
> >
> >  err_off:
> > @@ -1509,6 +1512,8 @@ int rtw_core_start(struct rtw_dev *rtwdev)
> >
> >  static void rtw_power_off(struct rtw_dev *rtwdev)
> >  {
> > +       rtw_hci_stop_rx(rtwdev);
> > +
>
> Similarly here can be
>
> if (rtwdev->always_power_on) {
>         rtw_hci_stop_rx(rtwdev);
>         return;
> }

Ditto

>
>
> >         if (rtwdev->always_power_on)
> >                 return;
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> > index 7a093f3d5f74..0a3ec94f6ab2 100644
> > --- a/drivers/net/wireless/realtek/rtw88/pci.c
> > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > @@ -1590,6 +1590,10 @@ static void rtw_pci_destroy(struct rtw_dev *rtwdev, struct pci_dev *pdev)
> >         rtw_pci_io_unmapping(rtwdev, pdev);
> >  }
> >
> > +static void rtw_pci_nop(struct rtw_dev *rtwdev)
> > +{
> > +}
> > +
> >  static struct rtw_hci_ops rtw_pci_ops = {
> >         .tx_write = rtw_pci_tx_write,
> >         .tx_kick_off = rtw_pci_tx_kick_off,
> > @@ -1600,6 +1604,8 @@ static struct rtw_hci_ops rtw_pci_ops = {
> >         .deep_ps = rtw_pci_deep_ps,
> >         .link_ps = rtw_pci_link_ps,
> >         .interface_cfg = rtw_pci_interface_cfg,
> > +       .stop_rx = rtw_pci_nop,
> > +       .start_rx = rtw_pci_nop,
> >
> >         .read8 = rtw_pci_read8,
> >         .read16 = rtw_pci_read16,
> > diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> > index 0cae5746f540..4a7923851c81 100644
> > --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> > +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> > @@ -1147,6 +1147,10 @@ static void rtw_sdio_declaim(struct rtw_dev *rtwdev,
> >         sdio_release_host(sdio_func);
> >  }
> >
> > +static void rtw_sdio_nop(struct rtw_dev *rtwdev)
> > +{
> > +}
> > +
> >  static struct rtw_hci_ops rtw_sdio_ops = {
> >         .tx_write = rtw_sdio_tx_write,
> >         .tx_kick_off = rtw_sdio_tx_kick_off,
> > @@ -1156,6 +1160,8 @@ static struct rtw_hci_ops rtw_sdio_ops = {
> >         .deep_ps = rtw_sdio_deep_ps,
> >         .link_ps = rtw_sdio_link_ps,
> >         .interface_cfg = rtw_sdio_interface_cfg,
> > +       .stop_rx = rtw_sdio_nop,
> > +       .start_rx = rtw_sdio_nop,
> >
> >         .read8 = rtw_sdio_read8,
> >         .read16 = rtw_sdio_read16,
> > diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> > index e1b66f339cca..d5cf3eb51c8a 100644
> > --- a/drivers/net/wireless/realtek/rtw88/usb.c
> > +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> > @@ -716,6 +716,30 @@ static void rtw_usb_interface_cfg(struct rtw_dev *rtwdev)
> >         /* empty function for rtw_hci_ops */
> >  }
> >
> > +static void rtw_usb_stop_rx(struct rtw_dev *rtwdev)
> > +{
> > +       struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
>
> Do we really need a ' rtwusb->rx_enabled' to maintain symmetric calling of
> start/stop_rx? If yes, here should add
>
>         if (!rtwusb->rx_enabled)
>                 return;

Sure

> But, I don't like that flag if it isn't strongly required.

It's required because start_rx is called twice initially - from
rtw_usb_probe and rtw_core_start (via rtw_power_on).

>
> > +       rtw_usb_cancel_rx_bufs(rtwusb);
> > +       rtwusb->rx_enabled = false;
> > +}
> > +
> > +static void rtw_usb_start_rx(struct rtw_dev *rtwdev)
> > +{
> > +       struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> > +       int i;
> > +
> > +       if (rtwusb->rx_enabled)
> > +               return;
> > +
> > +       for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
> > +               struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
> > +
> > +               rtw_usb_rx_resubmit(rtwusb, rxcb);
> > +       }
> > +
> > +       rtwusb->rx_enabled = true;
> > +}
> > +
> >  static struct rtw_hci_ops rtw_usb_ops = {
> >         .tx_write = rtw_usb_tx_write,
> >         .tx_kick_off = rtw_usb_tx_kick_off,
> > @@ -725,6 +749,8 @@ static struct rtw_hci_ops rtw_usb_ops = {
> >         .deep_ps = rtw_usb_deep_ps,
> >         .link_ps = rtw_usb_link_ps,
> >         .interface_cfg = rtw_usb_interface_cfg,
> > +       .stop_rx = rtw_usb_stop_rx,
> > +       .start_rx = rtw_usb_start_rx,
> >
> >         .write8  = rtw_usb_write8,
> >         .write16 = rtw_usb_write16,
> > @@ -754,18 +780,6 @@ static int rtw_usb_init_rx(struct rtw_dev *rtwdev)
> >         return 0;
> >  }
> >
> > -static void rtw_usb_setup_rx(struct rtw_dev *rtwdev)
> > -{
> > -       struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> > -       int i;
> > -
> > -       for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
> > -               struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
> > -
> > -               rtw_usb_rx_resubmit(rtwusb, rxcb);
> > -       }
> > -}
> > -
> >  static void rtw_usb_deinit_rx(struct rtw_dev *rtwdev)
> >  {
> >         struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> > @@ -903,7 +917,7 @@ int rtw_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
> >                 goto err_destroy_rxwq;
> >         }
> >
> > -       rtw_usb_setup_rx(rtwdev);
> > +       rtw_usb_start_rx(rtwdev);
> >
> >         return 0;
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/usb.h b/drivers/net/wireless/realtek/rtw88/usb.h
> > index 86697a5c0103..a6b004d4f74e 100644
> > --- a/drivers/net/wireless/realtek/rtw88/usb.h
> > +++ b/drivers/net/wireless/realtek/rtw88/usb.h
> > @@ -82,6 +82,7 @@ struct rtw_usb {
> >         struct rx_usb_ctrl_block rx_cb[RTW_USB_RXCB_NUM];
> >         struct sk_buff_head rx_queue;
> >         struct work_struct rx_work;
> > +       bool rx_enabled;
> >  };
> >
> >  static inline struct rtw_usb_tx_data *rtw_usb_get_tx_data(struct sk_buff *skb)
> > --
> > 2.25.1
> >
>

Re: wifi: rtw88: 8821CU hangs after some number of power-off/on cycles

[Marcin Ślusarz]

wt., 4 cze 2024 o 02:50 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
>
> Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > > >
> > > > I'm asking because if we are going in this direction, there's something
> > > > more to fix... With your v2, very frequently, I hit WARN_ON(!local->started) in
> > > > ieee80211_rx_napi (in wireless-next, the code was moved to ieee80211_rx_list).
> > > >
> > > > With my patch, I checked and hit that WARN_ON, too, but very occasionally.
> > > >
> > > > I think the difference is in what happens in rtw_ips_enter - I disabled only
> > > > the power_off, but you also disabled everything else, including the cancelation
> > > > of work_structs.
> > > >
> > > > The warning itself sounds harmless, but I think users should never see such
> > > > warnings, so this needs to be fixed somehow. Probably some additional
> > > > work_struct(s) need to be canceled?
> > > >
> > >
> > > I forgot to say my patch is compiled test only, and I didn't consider flow
> > > too much, just to close the behavior of your patches. You can improve my patch
> > > to be more reliable to avoid WARN_ON().
> >
> > Two variants of the patch that fix this issue will follow. They are
> > built on top of yours
> > v2 and my "wifi: rtw88: schedule rx work after everything is set up"
> > from the other thread.
> > Please choose the one you like more :).
>
> The patch "wifi: rtw88: usb: drop rx skbs when device is not running" is
> to drop all skb it receives. USB is still working, so I don't prefer this.
>
> "wifi: rtw88/usb: stop rx work before potential power off" seems to stop
> RX. But how rtw_usb_cancel_rx_bufs() can stop RX? Remove RX urb to stop it?

Yes, URBs are removed, so the buffers will not be filled in, and
completion callbacks will not be called.

> Not sure if this is regular method for USB devices?

I'm not sure, either. Ideally, it should be disabled in hardware, but
AFAIK there's no public documentation for this chip, so no way for me
to figure out how to do it, and we are dealing with a case of power
management failure here.

> By the way, please take and refine my v2 patch into your patchset. That will
> be easier to me to merge patches finally.
>
> Ping-Ke
>

[PATCH v2 1/2] wifi: rtw88: 8821cu: keep power on always for 8821CU

[Marcin Ślusarz]

From: Ping-Ke Shih <pkshih@realtek.com>

This chip fails to reliably wake up from power off.

Change-Id: I295de3c71fe91af37e8cc39b70728a8ba7e94b2f
Reported-by: Marcin Ślusarz <marcin.slusarz@gmail.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
v2: no changes since v1
---
 drivers/net/wireless/realtek/rtw88/mac80211.c |  2 +-
 drivers/net/wireless/realtek/rtw88/main.c     | 10 ++++++++--
 drivers/net/wireless/realtek/rtw88/main.h     |  2 ++
 drivers/net/wireless/realtek/rtw88/ps.c       |  5 ++++-
 drivers/net/wireless/realtek/rtw88/ps.h       |  2 +-
 drivers/net/wireless/realtek/rtw88/usb.c      |  3 +++
 drivers/net/wireless/realtek/rtw88/wow.c      |  2 +-
 7 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c b/drivers/net/wireless/realtek/rtw88/mac80211.c
index 0acebbfa13c4..0302af2ebe5b 100644
--- a/drivers/net/wireless/realtek/rtw88/mac80211.c
+++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
@@ -98,7 +98,7 @@ static int rtw_ops_config(struct ieee80211_hw *hw, u32 changed)
 	if ((changed & IEEE80211_CONF_CHANGE_IDLE) &&
 	    (hw->conf.flags & IEEE80211_CONF_IDLE) &&
 	    !test_bit(RTW_FLAG_SCANNING, rtwdev->flags))
-		rtw_enter_ips(rtwdev);
+		rtw_enter_ips(rtwdev, false);
 
 out:
 	mutex_unlock(&rtwdev->mutex);
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index 7ab7a988b123..a48e919adddb 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -302,7 +302,7 @@ static void rtw_ips_work(struct work_struct *work)
 
 	mutex_lock(&rtwdev->mutex);
 	if (rtwdev->hw->conf.flags & IEEE80211_CONF_IDLE)
-		rtw_enter_ips(rtwdev);
+		rtw_enter_ips(rtwdev, false);
 	mutex_unlock(&rtwdev->mutex);
 }
 
@@ -647,7 +647,7 @@ static void __fw_recovery_work(struct rtw_dev *rtwdev)
 	rtw_iterate_stas_atomic(rtwdev, rtw_reset_sta_iter, rtwdev);
 	rtw_iterate_vifs_atomic(rtwdev, rtw_reset_vif_iter, rtwdev);
 	bitmap_zero(rtwdev->hw_port, RTW_PORT_NUM);
-	rtw_enter_ips(rtwdev);
+	rtw_enter_ips(rtwdev, true);
 }
 
 static void rtw_fw_recovery_work(struct work_struct *work)
@@ -1356,6 +1356,9 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
 	bool wifi_only;
 	int ret;
 
+	if (rtwdev->always_power_on && test_bit(RTW_FLAG_POWERON, rtwdev->flags))
+		return 0;
+
 	ret = rtw_hci_setup(rtwdev);
 	if (ret) {
 		rtw_err(rtwdev, "failed to setup hci\n");
@@ -1506,6 +1509,9 @@ int rtw_core_start(struct rtw_dev *rtwdev)
 
 static void rtw_power_off(struct rtw_dev *rtwdev)
 {
+	if (rtwdev->always_power_on)
+		return;
+
 	rtw_hci_stop(rtwdev);
 	rtw_coex_power_off_setting(rtwdev);
 	rtw_mac_power_off(rtwdev);
diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
index 49894331f7b4..a6125b5e7d53 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -2049,6 +2049,8 @@ struct rtw_dev {
 	bool beacon_loss;
 	struct completion lps_leave_check;
 
+	bool always_power_on;
+
 	struct dentry *debugfs;
 
 	u8 sta_cnt;
diff --git a/drivers/net/wireless/realtek/rtw88/ps.c b/drivers/net/wireless/realtek/rtw88/ps.c
index add5a20b8432..a4092d424eda 100644
--- a/drivers/net/wireless/realtek/rtw88/ps.c
+++ b/drivers/net/wireless/realtek/rtw88/ps.c
@@ -24,8 +24,11 @@ static int rtw_ips_pwr_up(struct rtw_dev *rtwdev)
 	return ret;
 }
 
-int rtw_enter_ips(struct rtw_dev *rtwdev)
+int rtw_enter_ips(struct rtw_dev *rtwdev, bool force)
 {
+	if (!force && rtwdev->always_power_on)
+		return 0;
+
 	if (!test_bit(RTW_FLAG_POWERON, rtwdev->flags))
 		return 0;
 
diff --git a/drivers/net/wireless/realtek/rtw88/ps.h b/drivers/net/wireless/realtek/rtw88/ps.h
index 5ae83d2526cf..92057d01cbec 100644
--- a/drivers/net/wireless/realtek/rtw88/ps.h
+++ b/drivers/net/wireless/realtek/rtw88/ps.h
@@ -15,7 +15,7 @@
 #define LEAVE_LPS_TRY_CNT	5
 #define LEAVE_LPS_TIMEOUT	msecs_to_jiffies(100)
 
-int rtw_enter_ips(struct rtw_dev *rtwdev);
+int rtw_enter_ips(struct rtw_dev *rtwdev, bool force);
 int rtw_leave_ips(struct rtw_dev *rtwdev);
 
 void rtw_power_mode_change(struct rtw_dev *rtwdev, bool enter);
diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index 98f81e3ae13e..e1b66f339cca 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -859,6 +859,9 @@ int rtw_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	rtwdev->hci.ops = &rtw_usb_ops;
 	rtwdev->hci.type = RTW_HCI_TYPE_USB;
 
+	if (rtwdev->chip->id == RTW_CHIP_TYPE_8821C)
+		rtwdev->always_power_on = true;
+
 	rtwusb = rtw_get_usb_priv(rtwdev);
 	rtwusb->rtwdev = rtwdev;
 
diff --git a/drivers/net/wireless/realtek/rtw88/wow.c b/drivers/net/wireless/realtek/rtw88/wow.c
index 16ddee577efe..a90c8b388944 100644
--- a/drivers/net/wireless/realtek/rtw88/wow.c
+++ b/drivers/net/wireless/realtek/rtw88/wow.c
@@ -620,7 +620,7 @@ static int rtw_wow_restore_ps(struct rtw_dev *rtwdev)
 	int ret = 0;
 
 	if (rtw_wow_no_link(rtwdev) && rtwdev->wow.ips_enabled)
-		ret = rtw_enter_ips(rtwdev);
+		ret = rtw_enter_ips(rtwdev, false);
 
 	return ret;
 }
-- 
2.25.1

[PATCH v2 2/2] wifi: rtw88/usb: stop rx work before potential power off

[Marcin Ślusarz]

If power off is disabled (like on 8821CU after previous patch),
the hardware can still fire and deliver data. This may have
undersired impact on the networking stack (e.g.
WARN_ON(!local->started) in ieee80211_rx_list), because hw is
not supposed to do that after power off.

So to prevent that, cancel any pending RX URBs to stop the
completion handlers from being called.

Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
---
v2: start_rx/stop_rx cbs can be NULL; rx_enabled check added to stop_rx
---
 drivers/net/wireless/realtek/rtw88/hci.h  | 14 ++++++++
 drivers/net/wireless/realtek/rtw88/main.c |  7 +++-
 drivers/net/wireless/realtek/rtw88/usb.c  | 44 ++++++++++++++++-------
 drivers/net/wireless/realtek/rtw88/usb.h  |  1 +
 4 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/hci.h b/drivers/net/wireless/realtek/rtw88/hci.h
index 830d7532f2a3..839b9161014f 100644
--- a/drivers/net/wireless/realtek/rtw88/hci.h
+++ b/drivers/net/wireless/realtek/rtw88/hci.h
@@ -18,6 +18,8 @@ struct rtw_hci_ops {
 	void (*deep_ps)(struct rtw_dev *rtwdev, bool enter);
 	void (*link_ps)(struct rtw_dev *rtwdev, bool enter);
 	void (*interface_cfg)(struct rtw_dev *rtwdev);
+	void (*stop_rx)(struct rtw_dev *rtwdev);
+	void (*start_rx)(struct rtw_dev *rtwdev);
 
 	int (*write_data_rsvd_page)(struct rtw_dev *rtwdev, u8 *buf, u32 size);
 	int (*write_data_h2c)(struct rtw_dev *rtwdev, u8 *buf, u32 size);
@@ -57,6 +59,18 @@ static inline void rtw_hci_stop(struct rtw_dev *rtwdev)
 	rtwdev->hci.ops->stop(rtwdev);
 }
 
+static inline void rtw_hci_start_rx(struct rtw_dev *rtwdev)
+{
+	if (rtwdev->hci.ops->start_rx)
+		rtwdev->hci.ops->start_rx(rtwdev);
+}
+
+static inline void rtw_hci_stop_rx(struct rtw_dev *rtwdev)
+{
+	if (rtwdev->hci.ops->stop_rx)
+		rtwdev->hci.ops->stop_rx(rtwdev);
+}
+
 static inline void rtw_hci_deep_ps(struct rtw_dev *rtwdev, bool enter)
 {
 	rtwdev->hci.ops->deep_ps(rtwdev, enter);
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index a48e919adddb..bb0122d19416 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -1357,7 +1357,7 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
 	int ret;
 
 	if (rtwdev->always_power_on && test_bit(RTW_FLAG_POWERON, rtwdev->flags))
-		return 0;
+		goto success;
 
 	ret = rtw_hci_setup(rtwdev);
 	if (ret) {
@@ -1407,6 +1407,9 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
 	rtw_coex_power_on_setting(rtwdev);
 	rtw_coex_init_hw_config(rtwdev, wifi_only);
 
+success:
+	rtw_hci_start_rx(rtwdev);
+
 	return 0;
 
 err_off:
@@ -1509,6 +1512,8 @@ int rtw_core_start(struct rtw_dev *rtwdev)
 
 static void rtw_power_off(struct rtw_dev *rtwdev)
 {
+	rtw_hci_stop_rx(rtwdev);
+
 	if (rtwdev->always_power_on)
 		return;
 
diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index 28ff46e96604..8e784c357ee2 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -713,6 +713,34 @@ static void rtw_usb_interface_cfg(struct rtw_dev *rtwdev)
 	/* empty function for rtw_hci_ops */
 }
 
+static void rtw_usb_stop_rx(struct rtw_dev *rtwdev)
+{
+	struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
+
+	if (!rtwusb->rx_enabled)
+		return;
+
+	rtw_usb_cancel_rx_bufs(rtwusb);
+	rtwusb->rx_enabled = false;
+}
+
+static void rtw_usb_start_rx(struct rtw_dev *rtwdev)
+{
+	struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
+	int i;
+
+	if (rtwusb->rx_enabled)
+		return;
+
+	for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
+		struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
+
+		rtw_usb_rx_resubmit(rtwusb, rxcb);
+	}
+
+	rtwusb->rx_enabled = true;
+}
+
 static struct rtw_hci_ops rtw_usb_ops = {
 	.tx_write = rtw_usb_tx_write,
 	.tx_kick_off = rtw_usb_tx_kick_off,
@@ -722,6 +750,8 @@ static struct rtw_hci_ops rtw_usb_ops = {
 	.deep_ps = rtw_usb_deep_ps,
 	.link_ps = rtw_usb_link_ps,
 	.interface_cfg = rtw_usb_interface_cfg,
+	.stop_rx = rtw_usb_stop_rx,
+	.start_rx = rtw_usb_start_rx,
 
 	.write8  = rtw_usb_write8,
 	.write16 = rtw_usb_write16,
@@ -751,18 +781,6 @@ static int rtw_usb_init_rx(struct rtw_dev *rtwdev)
 	return 0;
 }
 
-static void rtw_usb_setup_rx(struct rtw_dev *rtwdev)
-{
-	struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
-	int i;
-
-	for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
-		struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
-
-		rtw_usb_rx_resubmit(rtwusb, rxcb);
-	}
-}
-
 static void rtw_usb_deinit_rx(struct rtw_dev *rtwdev)
 {
 	struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
@@ -900,7 +918,7 @@ int rtw_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
 		goto err_destroy_rxwq;
 	}
 
-	rtw_usb_setup_rx(rtwdev);
+	rtw_usb_start_rx(rtwdev);
 
 	return 0;
 
diff --git a/drivers/net/wireless/realtek/rtw88/usb.h b/drivers/net/wireless/realtek/rtw88/usb.h
index 86697a5c0103..a6b004d4f74e 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.h
+++ b/drivers/net/wireless/realtek/rtw88/usb.h
@@ -82,6 +82,7 @@ struct rtw_usb {
 	struct rx_usb_ctrl_block rx_cb[RTW_USB_RXCB_NUM];
 	struct sk_buff_head rx_queue;
 	struct work_struct rx_work;
+	bool rx_enabled;
 };
 
 static inline struct rtw_usb_tx_data *rtw_usb_get_tx_data(struct sk_buff *skb)
-- 
2.25.1

RE: [PATCH v2 1/2] wifi: rtw88: 8821cu: keep power on always for 8821CU

[Ping-Ke Shih]

Marcin Ślusarz <marcin.slusarz@gmail.com> wrote: 
> From: Ping-Ke Shih <pkshih@realtek.com>
> 
> This chip fails to reliably wake up from power off.

"so keep power on ...." (more description)

> 
> Change-Id: I295de3c71fe91af37e8cc39b70728a8ba7e94b2f

No change-ID. Please run checkpatch.pl before sending. 

> Reported-by: Marcin Ślusarz <marcin.slusarz@gmail.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

I think you can edit this patch as what you want. Then I can become the
Co-developer (use Co-developed-by). 

RE: [PATCH] wifi: rtw88/usb: stop rx work before potential power off

[Ping-Ke Shih]

Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> wt., 4 cze 2024 o 02:57 Ping-Ke Shih <pkshih@realtek.com> napisał(a):
> >
> > Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> > > --- a/drivers/net/wireless/realtek/rtw88/main.c
> > > +++ b/drivers/net/wireless/realtek/rtw88/main.c
> > > @@ -1357,7 +1357,7 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
> > >         int ret;
> > >
> > >         if (rtwdev->always_power_on && test_bit(RTW_FLAG_POWERON, rtwdev->flags))
> > > -               return 0;
> > > +               goto success;
> >
> > rtw_hci_start_rx(rtwdev) is only needed by this case, so
> >
> > if (rtwdev->always_power_on && test_bit(RTW_FLAG_POWERON, rtwdev->flags)) {
> >         rtw_hci_start_rx(rtwdev);
> >         return 0;
> > }
> 
> Yes, strictly speaking, it's needed only in the always_power_on case,
> but doing that in the common code path ensures that it's tested and
> still works.

For the non- always_power_on case, it calls rtw_hci_start()/rtw_hci_stop() already
so I don't think we should call these duplicates. 

RE: [PATCH v2 2/2] wifi: rtw88/usb: stop rx work before potential power off

[Ping-Ke Shih]

Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> If power off is disabled (like on 8821CU after previous patch),
> the hardware can still fire and deliver data. This may have
> undersired impact on the networking stack (e.g.
> WARN_ON(!local->started) in ieee80211_rx_list), because hw is
> not supposed to do that after power off.
> 
> So to prevent that, cancel any pending RX URBs to stop the
> completion handlers from being called.
> 
> Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
> ---
> v2: start_rx/stop_rx cbs can be NULL; rx_enabled check added to stop_rx
> ---
>  drivers/net/wireless/realtek/rtw88/hci.h  | 14 ++++++++
>  drivers/net/wireless/realtek/rtw88/main.c |  7 +++-
>  drivers/net/wireless/realtek/rtw88/usb.c  | 44 ++++++++++++++++-------
>  drivers/net/wireless/realtek/rtw88/usb.h  |  1 +
>  4 files changed, 52 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/hci.h b/drivers/net/wireless/realtek/rtw88/hci.h
> index 830d7532f2a3..839b9161014f 100644
> --- a/drivers/net/wireless/realtek/rtw88/hci.h
> +++ b/drivers/net/wireless/realtek/rtw88/hci.h
> @@ -18,6 +18,8 @@ struct rtw_hci_ops {
>         void (*deep_ps)(struct rtw_dev *rtwdev, bool enter);
>         void (*link_ps)(struct rtw_dev *rtwdev, bool enter);
>         void (*interface_cfg)(struct rtw_dev *rtwdev);
> +       void (*stop_rx)(struct rtw_dev *rtwdev);
> +       void (*start_rx)(struct rtw_dev *rtwdev);
> 
>         int (*write_data_rsvd_page)(struct rtw_dev *rtwdev, u8 *buf, u32 size);
>         int (*write_data_h2c)(struct rtw_dev *rtwdev, u8 *buf, u32 size);
> @@ -57,6 +59,18 @@ static inline void rtw_hci_stop(struct rtw_dev *rtwdev)
>         rtwdev->hci.ops->stop(rtwdev);
>  }
> 
> +static inline void rtw_hci_start_rx(struct rtw_dev *rtwdev)
> +{
> +       if (rtwdev->hci.ops->start_rx)
> +               rtwdev->hci.ops->start_rx(rtwdev);
> +}
> +
> +static inline void rtw_hci_stop_rx(struct rtw_dev *rtwdev)
> +{
> +       if (rtwdev->hci.ops->stop_rx)
> +               rtwdev->hci.ops->stop_rx(rtwdev);
> +}
> +
>  static inline void rtw_hci_deep_ps(struct rtw_dev *rtwdev, bool enter)
>  {
>         rtwdev->hci.ops->deep_ps(rtwdev, enter);
> diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> index a48e919adddb..bb0122d19416 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.c
> +++ b/drivers/net/wireless/realtek/rtw88/main.c
> @@ -1357,7 +1357,7 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
>         int ret;
> 
>         if (rtwdev->always_power_on && test_bit(RTW_FLAG_POWERON, rtwdev->flags))
> -               return 0;
> +               goto success;
> 
>         ret = rtw_hci_setup(rtwdev);
>         if (ret) {
> @@ -1407,6 +1407,9 @@ static int rtw_power_on(struct rtw_dev *rtwdev)
>         rtw_coex_power_on_setting(rtwdev);
>         rtw_coex_init_hw_config(rtwdev, wifi_only);
> 
> +success:
> +       rtw_hci_start_rx(rtwdev);
> +

As mentioned, this is not a common routine, but only special for always_power_on case.
For normal case, running both rtw_hci_start() and rtw_hci_start_rx() are
confusing. Even I'm thinking the names of these two ops are confusing people. 

>         return 0;
> 
>  err_off:
> @@ -1509,6 +1512,8 @@ int rtw_core_start(struct rtw_dev *rtwdev)
> 
>  static void rtw_power_off(struct rtw_dev *rtwdev)
>  {
> +       rtw_hci_stop_rx(rtwdev);
> +

Ditto. 

>         if (rtwdev->always_power_on)
>                 return;
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> index 28ff46e96604..8e784c357ee2 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -713,6 +713,34 @@ static void rtw_usb_interface_cfg(struct rtw_dev *rtwdev)
>         /* empty function for rtw_hci_ops */
>  }
> 
> +static void rtw_usb_stop_rx(struct rtw_dev *rtwdev)
> +{
> +       struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> +
> +       if (!rtwusb->rx_enabled)
> +               return;
> +
> +       rtw_usb_cancel_rx_bufs(rtwusb);
> +       rtwusb->rx_enabled = false;
> +}
> +
> +static void rtw_usb_start_rx(struct rtw_dev *rtwdev)
> +{
> +       struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> +       int i;
> +
> +       if (rtwusb->rx_enabled)
> +               return;
> +
> +       for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
> +               struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
> +
> +               rtw_usb_rx_resubmit(rtwusb, rxcb);
> +       }
> +
> +       rtwusb->rx_enabled = true;
> +}
> +
>  static struct rtw_hci_ops rtw_usb_ops = {
>         .tx_write = rtw_usb_tx_write,
>         .tx_kick_off = rtw_usb_tx_kick_off,
> @@ -722,6 +750,8 @@ static struct rtw_hci_ops rtw_usb_ops = {
>         .deep_ps = rtw_usb_deep_ps,
>         .link_ps = rtw_usb_link_ps,
>         .interface_cfg = rtw_usb_interface_cfg,
> +       .stop_rx = rtw_usb_stop_rx,
> +       .start_rx = rtw_usb_start_rx,

Please set .stop_rx/.start_rx to NULL for PCI and SDIO explicitly. 


> 
>         .write8  = rtw_usb_write8,
>         .write16 = rtw_usb_write16,
> @@ -751,18 +781,6 @@ static int rtw_usb_init_rx(struct rtw_dev *rtwdev)
>         return 0;
>  }
> 
> -static void rtw_usb_setup_rx(struct rtw_dev *rtwdev)
> -{
> -       struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> -       int i;
> -
> -       for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
> -               struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
> -
> -               rtw_usb_rx_resubmit(rtwusb, rxcb);
> -       }
> -}
> -
>  static void rtw_usb_deinit_rx(struct rtw_dev *rtwdev)
>  {
>         struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> @@ -900,7 +918,7 @@ int rtw_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
>                 goto err_destroy_rxwq;
>         }
> 
> -       rtw_usb_setup_rx(rtwdev);
> +       rtw_usb_start_rx(rtwdev);
> 
>         return 0;
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/usb.h b/drivers/net/wireless/realtek/rtw88/usb.h
> index 86697a5c0103..a6b004d4f74e 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.h
> +++ b/drivers/net/wireless/realtek/rtw88/usb.h
> @@ -82,6 +82,7 @@ struct rtw_usb {
>         struct rx_usb_ctrl_block rx_cb[RTW_USB_RXCB_NUM];
>         struct sk_buff_head rx_queue;
>         struct work_struct rx_work;
> +       bool rx_enabled;
>  };
> 
>  static inline struct rtw_usb_tx_data *rtw_usb_get_tx_data(struct sk_buff *skb)
> --
> 2.25.1
>