From: Marcin Ślusarz <[email protected]>
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
From: Marcin Ślusarz <[email protected]>
Needed by the next patch that disables power off operation for one chip.
Signed-off-by: Marcin Ślusarz <[email protected]>
Cc: Ping-Ke Shih <[email protected]>
Cc: Larry Finger <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: [email protected]
---
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
From: Marcin Ślusarz <[email protected]>
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 <[email protected]>
Cc: Ping-Ke Shih <[email protected]>
Cc: Larry Finger <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: [email protected]
---
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
On 27/05/2024 20:34, Marcin Ślusarz wrote:
> From: Marcin Ślusarz <[email protected]>
>
> 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?
Marcin Ślusarz <[email protected]> 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.
Marcin Ślusarz <[email protected]> wrote:
> From: Marcin Ślusarz <[email protected]>
>
> Needed by the next patch that disables power off operation for one chip.
>
> Signed-off-by: Marcin Ślusarz <[email protected]>
> Cc: Ping-Ke Shih <[email protected]>
> Cc: Larry Finger <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: [email protected]
> ---
> 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.
wt., 28 maj 2024 o 05:52 Ping-Ke Shih <[email protected]> napisał(a):
>
> Marcin Ślusarz <[email protected]> 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).
wt., 28 maj 2024 o 05:56 Ping-Ke Shih <[email protected]> napisał(a):
>
> Marcin Ślusarz <[email protected]> wrote:
> > From: Marcin Ślusarz <[email protected]>
> >
> > Needed by the next patch that disables power off operation for one chip.
> >
> > Signed-off-by: Marcin Ślusarz <[email protected]>
> > Cc: Ping-Ke Shih <[email protected]>
> > Cc: Larry Finger <[email protected]>
> > Cc: Kalle Valo <[email protected]>
> > Cc: [email protected]
> > ---
> > 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.
pon., 27 maj 2024 o 20:43 Bitterblue Smith <[email protected]> napisał(a):
>
> On 27/05/2024 20:34, Marcin Ślusarz wrote:
> > From: Marcin Ślusarz <[email protected]>
> >
> > 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).
On 28/05/2024 13:42, Marcin Ślusarz wrote:
> pon., 27 maj 2024 o 20:43 Bitterblue Smith <[email protected]> napisał(a):
>>
>> On 27/05/2024 20:34, Marcin Ślusarz wrote:
>>> From: Marcin Ślusarz <[email protected]>
>>>
>>> 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/
wt., 28 maj 2024 o 14:25 Bitterblue Smith <[email protected]> napisał(a):
>
> On 28/05/2024 13:42, Marcin Ślusarz wrote:
> > pon., 27 maj 2024 o 20:43 Bitterblue Smith <[email protected]> napisał(a):
> >>
> >> On 27/05/2024 20:34, Marcin Ślusarz wrote:
> >>> From: Marcin Ślusarz <[email protected]>
> >>>
> >>> 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.
Marcin Ślusarz <[email protected]> wrote:
> wt., 28 maj 2024 o 05:52 Ping-Ke Shih <[email protected]> napisał(a):
> >
> > Marcin Ślusarz <[email protected]> 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.
śr., 29 maj 2024 o 03:52 Ping-Ke Shih <[email protected]> napisał(a):
>
> Marcin Ślusarz <[email protected]> wrote:
> > wt., 28 maj 2024 o 05:52 Ping-Ke Shih <[email protected]> napisał(a):
> > >
> > > Marcin Ślusarz <[email protected]> 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
Marcin Ślusarz <[email protected]> wrote:
> śr., 29 maj 2024 o 03:52 Ping-Ke Shih <[email protected]> napisał(a):
> >
> > Marcin Ślusarz <[email protected]> wrote:
> > > wt., 28 maj 2024 o 05:52 Ping-Ke Shih <[email protected]> napisał(a):
> > > >
> > > > Marcin Ślusarz <[email protected]> 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
czw., 30 maj 2024 o 05:13 Ping-Ke Shih <[email protected]> napisał(a):
>
> Marcin Ślusarz <[email protected]> wrote:
> > śr., 29 maj 2024 o 03:52 Ping-Ke Shih <[email protected]> napisał(a):
> > >
> > > Marcin Ślusarz <[email protected]> wrote:
> > > > wt., 28 maj 2024 o 05:52 Ping-Ke Shih <[email protected]> napisał(a):
> > > > >
> > > > > Marcin Ślusarz <[email protected]> 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 :).
From: Marcin Ślusarz <[email protected]>
Avoids WARN_ON(!local->started) in ieee80211_rx_list, after
the patch that disables power management of 8821CU.
Signed-off-by: Marcin Ślusarz <[email protected]>
---
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
From: Marcin Ślusarz <[email protected]>
Avoids WARN_ON(!local->started) in ieee80211_rx_list, after
the patch that disables power management of 8821CU.
Signed-off-by: Marcin Ślusarz <[email protected]>
---
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
Marcin Ślusarz <[email protected]> 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
Marcin Ślusarz <[email protected]> wrote:
> From: Marcin Ślusarz <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
wt., 4 cze 2024 o 02:57 Ping-Ke Shih <[email protected]> napisał(a):
>
> Marcin Ślusarz <[email protected]> wrote:
> > From: Marcin Ślusarz <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
> >
>
wt., 4 cze 2024 o 02:50 Ping-Ke Shih <[email protected]> napisał(a):
>
> Marcin Ślusarz <[email protected]> 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
>
From: Ping-Ke Shih <[email protected]>
This chip fails to reliably wake up from power off.
Change-Id: I295de3c71fe91af37e8cc39b70728a8ba7e94b2f
Reported-by: Marcin Ślusarz <[email protected]>
Signed-off-by: Ping-Ke Shih <[email protected]>
---
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