Re: [PATCH v4 3/3] wifi: rtw88: ps: trigger firmware recovery only after consecutive LPS leave failures on PCI

Re: [PATCH v4 3/3] wifi: rtw88: ps: trigger firmware recovery only after consecutive LPS leave failures on PCI

[Kalpesh Solanki]

Hi Luka, Ping-Ke,

Thank you both for your patience while I was away, and thank you Luka for
advocating to address the review feedback before pushing v5.

Regarding Luka's two comments on v4:

1. Redundant rtw_hci_type check in rtw_fw_recovery():
   You are completely right. Since rtw_fw_leave_lps_check() already guards the
   call with rtw_hci_type(rtwdev) == RTW_HCI_TYPE_PCIE, and the other callers in
   pci.c are PCI-only, having an HCI check in rtw_fw_recovery() in main.c is
   redundant. I have dropped it in v5.

2. ps_hang_cnt on non-FW_FEATURE_LPS_C2H chipsets:
   Excellent catch! On chipsets without FW_FEATURE_LPS_C2H (such as RTL8822B
   and RTL8723D), the check falls back to polling REG_TCR for
   BIT_PWRMGT_HWDATA_EN. On timeout, the driver forcefully clears this bit via
   rtw_write32_clr(rtwdev, REG_TCR, BIT_PWRMGT_HWDATA_EN) so packets can continue
   to be transmitted. If the firmware is frozen, it will never re-assert this
   bit. On the next check, the register read sees the bit cleared, falsely reports
   success, and resets ps_hang_cnt back to 0. Thus, a stuck firmware on these
   chips could never reach RTW_PS_HANG_MAX_CNT.

   In v5, I have updated patch 3/3 so that ps_hang_cnt is only incremented and
   evaluated when FW_FEATURE_LPS_C2H is supported:
   
       if (rtw_fw_feature_check(fw, FW_FEATURE_LPS_C2H) &&
           rtw_hci_type(rtwdev) == RTW_HCI_TYPE_PCIE) {
               rtwdev->ps_hang_cnt++;
               if (rtwdev->ps_hang_cnt >= RTW_PS_HANG_MAX_CNT)
                       rtw_fw_recovery(rtwdev);
       }

   For C2H-capable devices (such as RTL8822C and RTL8821C), completion signaling
   reliably accumulates consecutive timeouts when the firmware hangs, while
   legacy chipsets without C2H retain the upstream behavior (clear TCR and warn)
   without getting trapped in a broken counter reset loop.

Regarding lore / patchwork:
It appears v4 was either dropped by list filters or delayed during list delivery.
I have sent out the full v5 series [PATCH v5 0/3] incorporating all the above
changes to the mailing list.

Thanks,
Kalpesh

Re: [PATCH v4 3/3] wifi: rtw88: ps: trigger firmware recovery only after consecutive LPS leave failures on PCI

[Luka Gejak]

On November 9, 2026 9:32:09 AM GMT+01:00, KALPESH SOLANKI
<skalpesh942@gmail.com> wrote:
>Hi Luka, Ping-Ke,
>
>Thank you for the reviews and feedback. I have had a bereavement in
>the family yesterday and will be away for a few days.
>
>I have noted the comments and will follow up with v5 once I am back
>and after lore/patchwork is operational.
>
>Best regards,
>Kalpesh
>
>

Of course, take your time, no rush.

Best regards,
Luka Gejak

Re: [PATCH v4 3/3] wifi: rtw88: ps: trigger firmware recovery only after consecutive LPS leave failures on PCI

[KALPESH SOLANKI]

Hi Luka, Ping-Ke,

Thank you for the reviews and feedback. I have had a bereavement in
the family yesterday and will be away for a few days.

I have noted the comments and will follow up with v5 once I am back
and after lore/patchwork is operational.

Best regards,
Kalpesh


On Mon, 9 Nov 2026 at 13:53, KALPESH SOLANKI <skalpesh942@gmail.com> wrote:
>
> Hi Luka, Ping-Ke,
>
> Thank you for the reviews and feedback. I have had a bereavement in the family yesterday and will be away for a few days.
>
> I have noted the comments and will follow up with v5 once I am back and after lore/patchwork is operational.
>
> Best regards,
> Kalpesh
>
> On Mon, 9 Nov 2026 at 12:43, Luka Gejak <luka.gejak@linux.dev> wrote:
>>
>> Hi Kalpesh,
>>
>> Thanks for the new version. Two comments below.
>>
>> On November 8, 2026 3:27:07 PM GMT+01:00, Kalpesh Solanki
>> <skalpesh942@gmail.com> wrote:
>>                                                                             > diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
>> > index a74f199a93ab..7009f8ae3e57 100644                                   > --- a/drivers/net/wireless/realtek/rtw88/main.c                           > +++ b/drivers/net/wireless/realtek/rtw88/main.c                           > @@ -638,6 +638,9 @@ static void rtw_reset_vif_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
>> >
>> >  void rtw_fw_recovery(struct rtw_dev *rtwdev)
>> >  {
>> > +     if (rtw_hci_type(rtwdev) != RTW_HCI_TYPE_PCIE)
>> > +             return;
>> > +
>>
>> The same check is also in rtw_fw_leave_lps_check() below, and the other
>> callers of rtw_fw_recovery() are PCI only, so I think this one can go.
>>
>> [ ... ]
>>
>> > diff --git a/drivers/net/wireless/realtek/rtw88/ps.c b/drivers/net/wireless/realtek/rtw88/ps.c
>> > index add5a20b8432..fa3a4e476cf6 100644
>> > --- a/drivers/net/wireless/realtek/rtw88/ps.c
>> > +++ b/drivers/net/wireless/realtek/rtw88/ps.c
>> > @@ -166,6 +166,13 @@ static void rtw_fw_leave_lps_check(struct rtw_dev *rtwdev)
>> >               rtw_write32_clr(rtwdev, REG_TCR, BIT_PWRMGT_HWDATA_EN);
>> >               rtw_warn(rtwdev, "firmware failed to leave lps state\n");
>> >               rtw_fw_dump_dbg_info(rtwdev);
>> > +
>> > +             rtwdev->ps_hang_cnt++;
>> > +             if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_PCIE &&
>> > +                 rtwdev->ps_hang_cnt >= RTW_PS_HANG_MAX_CNT)
>> > +                     rtw_fw_recovery(rtwdev);
>> > +     } else {
>> > +             rtwdev->ps_hang_cnt = 0;
>> >       }
>>
>> As far as I can see this can't reach the limit on stuck firmware without
>> FW_FEATURE_LPS_C2H, which seems to include rtw8822b and rtw8723d. The
>> register check passes once the bit is clear, the failure path above
>> clears it, and I can only see the firmware setting it again when it
>> enters LPS.
>>
>> So if I read it right, stuck firmware gives one failure, then the next
>> check passes and resets the counter, as long as the registers still read
>> fine. Three in a row would need firmware that is still running.
>>
>> Would it make sense to only count failures when FW_FEATURE_LPS_C2H is set?
>> Ping-Ke, do you know if only the firmware sets this bit?
>>
>>
>> Best regards,
>> Luka Gejak

Re: [PATCH v4 3/3] wifi: rtw88: ps: trigger firmware recovery only after consecutive LPS leave failures on PCI

[Luka Gejak]

Hi Kalpesh,

Thanks for the new version. Two comments below.

On November 8, 2026 3:27:07 PM GMT+01:00, Kalpesh Solanki
<skalpesh942@gmail.com> wrote:
                                                                            > diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> index a74f199a93ab..7009f8ae3e57 100644                                   > --- a/drivers/net/wireless/realtek/rtw88/main.c                           > +++ b/drivers/net/wireless/realtek/rtw88/main.c                           > @@ -638,6 +638,9 @@ static void rtw_reset_vif_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
>
>  void rtw_fw_recovery(struct rtw_dev *rtwdev)
>  {
> +     if (rtw_hci_type(rtwdev) != RTW_HCI_TYPE_PCIE)
> +             return;
> +

The same check is also in rtw_fw_leave_lps_check() below, and the other
callers of rtw_fw_recovery() are PCI only, so I think this one can go.

[ ... ]

> diff --git a/drivers/net/wireless/realtek/rtw88/ps.c b/drivers/net/wireless/realtek/rtw88/ps.c
> index add5a20b8432..fa3a4e476cf6 100644
> --- a/drivers/net/wireless/realtek/rtw88/ps.c
> +++ b/drivers/net/wireless/realtek/rtw88/ps.c
> @@ -166,6 +166,13 @@ static void rtw_fw_leave_lps_check(struct rtw_dev *rtwdev)
>               rtw_write32_clr(rtwdev, REG_TCR, BIT_PWRMGT_HWDATA_EN);
>               rtw_warn(rtwdev, "firmware failed to leave lps state\n");
>               rtw_fw_dump_dbg_info(rtwdev);
> +
> +             rtwdev->ps_hang_cnt++;
> +             if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_PCIE &&
> +                 rtwdev->ps_hang_cnt >= RTW_PS_HANG_MAX_CNT)
> +                     rtw_fw_recovery(rtwdev);
> +     } else {
> +             rtwdev->ps_hang_cnt = 0;
>       }

As far as I can see this can't reach the limit on stuck firmware without
FW_FEATURE_LPS_C2H, which seems to include rtw8822b and rtw8723d. The
register check passes once the bit is clear, the failure path above
clears it, and I can only see the firmware setting it again when it
enters LPS.

So if I read it right, stuck firmware gives one failure, then the next
check passes and resets the counter, as long as the registers still read
fine. Three in a row would need firmware that is still running.

Would it make sense to only count failures when FW_FEATURE_LPS_C2H is set?
Ping-Ke, do you know if only the firmware sets this bit?


Best regards,
Luka Gejak

[PATCH v4 3/3] wifi: rtw88: ps: trigger firmware recovery only after consecutive LPS leave failures on PCI

[Kalpesh Solanki]

A single timeout in rtw_fw_leave_lps_check() does not always indicate a
frozen firmware; in noisy RF environments or during beacon loss, the
802.11 null data frame ACK can be delayed or dropped. Immediately
triggering firmware recovery and a full hardware restart on a single
timeout causes unnecessary disconnections and kernel warning traces on
idle links.

Follow rtw89 by introducing a consecutive failure counter (ps_hang_cnt)
and only triggering rtw_fw_recovery() after RTW_PS_HANG_MAX_CNT (3)
consecutive failures. Reset the counter to 0 on every successful LPS
leave and in rtw_core_start() so that cold starts and hardware restarts
always begin with a clean slate.

Additionally, rtw_fw_recovery() clears keys via rtw_iterate_keys_atomic()
under a spinlock. On USB and SDIO, register writes in key clearing sleep
(via usb_control_msg() and sdio_claim_host()), which would trigger a
BUG: scheduling while atomic if executed. Because USB and SDIO do not
support firmware recovery, restrict rtw_fw_recovery() to PCI devices
via rtw_hci_type().

Signed-off-by: Kalpesh Solanki <skalpesh942@gmail.com>
---
 drivers/net/wireless/realtek/rtw88/main.c | 5 +++++
 drivers/net/wireless/realtek/rtw88/main.h | 1 +
 drivers/net/wireless/realtek/rtw88/ps.c   | 7 +++++++
 drivers/net/wireless/realtek/rtw88/ps.h   | 1 +
 4 files changed, 14 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index a74f199a93a..7009f8ae3e5 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -638,6 +638,9 @@ static void rtw_reset_vif_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
 
 void rtw_fw_recovery(struct rtw_dev *rtwdev)
 {
+	if (rtw_hci_type(rtwdev) != RTW_HCI_TYPE_PCIE)
+		return;
+
 	if (!test_bit(RTW_FLAG_RESTARTING, rtwdev->flags))
 		ieee80211_queue_work(rtwdev->hw, &rtwdev->fw_recovery_work);
 }
@@ -1536,6 +1539,8 @@ int rtw_core_start(struct rtw_dev *rtwdev)
 
 	set_bit(RTW_FLAG_RUNNING, rtwdev->flags);
 
+	rtwdev->ps_hang_cnt = 0;
+
 	return 0;
 }
 
diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
index 6ecbeb4ffe4..525f17590ad 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -2113,6 +2113,7 @@ struct rtw_dev {
 	/* lps power state & handler work */
 	struct rtw_lps_conf lps_conf;
 	bool ps_enabled;
+	u8 ps_hang_cnt;
 	bool beacon_loss;
 	struct completion lps_leave_check;
 
diff --git a/drivers/net/wireless/realtek/rtw88/ps.c b/drivers/net/wireless/realtek/rtw88/ps.c
index add5a20b843..fa3a4e476cf 100644
--- a/drivers/net/wireless/realtek/rtw88/ps.c
+++ b/drivers/net/wireless/realtek/rtw88/ps.c
@@ -166,6 +166,13 @@ static void rtw_fw_leave_lps_check(struct rtw_dev *rtwdev)
 		rtw_write32_clr(rtwdev, REG_TCR, BIT_PWRMGT_HWDATA_EN);
 		rtw_warn(rtwdev, "firmware failed to leave lps state\n");
 		rtw_fw_dump_dbg_info(rtwdev);
+
+		rtwdev->ps_hang_cnt++;
+		if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_PCIE &&
+		    rtwdev->ps_hang_cnt >= RTW_PS_HANG_MAX_CNT)
+			rtw_fw_recovery(rtwdev);
+	} else {
+		rtwdev->ps_hang_cnt = 0;
 	}
 }
 
diff --git a/drivers/net/wireless/realtek/rtw88/ps.h b/drivers/net/wireless/realtek/rtw88/ps.h
index 5ae83d2526c..b088fad7cd8 100644
--- a/drivers/net/wireless/realtek/rtw88/ps.h
+++ b/drivers/net/wireless/realtek/rtw88/ps.h
@@ -14,6 +14,7 @@
 
 #define LEAVE_LPS_TRY_CNT	5
 #define LEAVE_LPS_TIMEOUT	msecs_to_jiffies(100)
+#define RTW_PS_HANG_MAX_CNT	3
 
 int rtw_enter_ips(struct rtw_dev *rtwdev);
 int rtw_leave_ips(struct rtw_dev *rtwdev);
-- 
2.53.0