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

[PATCH v4 0/3] wifi: rtw88: firmware recovery fixes and LPS hang handling

[Kalpesh Solanki]

This series addresses firmware recovery robustness and LPS hang handling
in rtw88 based on review feedback from Luka Gejak and Ping-Ke Shih.

Patch 1/2 from the v2 series (DMI quirk for HP Laptop 15s-fr5xxx) was
already applied to rtw-next. The previous LPS leave recovery patch was
dropped from rtw-next by Ping-Ke Shih for respin.

This v4 series splits the recovery fixes into three focused patches:

1. Patch 1/3: Clear RTW_FLAG_RESTARTING on stop and start failure.
   mac80211's drv_stop() skips local->ops->stop() when local->started
   is false (such as after a failed drv_start()). If rtw_core_start()
   fails during restart, clearing the flag only in rtw_ops_stop() left
   it permanently set. We now clear RTW_FLAG_RESTARTING in both
   rtw_ops_stop() and in rtw_ops_start() upon rtw_core_start() failure.

2. Patch 2/3: Skip already reset station in rtw_sta_remove().
   During station disconnect (e.g., ieee80211_set_disassoc()), mac80211
   leaves LPS before removing the AP station. If recovery is triggered by
   LPS leave failure, __fw_recovery_work() removes all stations via
   rtw_reset_sta_iter(). When mac80211 subsequently calls
   rtw_ops_sta_remove(), rtw_sta_remove() ran a second time, resulting in
   a double free of si->mask and underflow of rtwdev->sta_cnt. We check
   si->vif at entry and clear si->vif and si->mask upon removal.

3. Patch 3/3: Trigger firmware recovery only after consecutive LPS leave
   failures on PCI.
   Restricts recovery to PCI devices (rtw_hci_type == RTW_HCI_TYPE_PCIE)
   because USB (usb_control_msg()) and SDIO (sdio_claim_host()) sleep,
   which triggers a "scheduling while atomic" panic under
   local->key_lock during key clearing in __fw_recovery_work(). Retains
   rtw_fw_dump_dbg_info() on the first failure (guarded by RTW_DBG_FW),
   and requires 3 consecutive timeouts (RTW_PS_HANG_MAX_CNT) before
   triggering hardware recovery. ps_hang_cnt is reset on success and in
   rtw_core_start().

Changes in v4:
- In patch 1/3: Also clear RTW_FLAG_RESTARTING in rtw_ops_start() if
  rtw_core_start() fails.
- Add patch 2/3: Prevent double free and sta_cnt underflow in
  rtw_sta_remove() when recovery races with station disconnect.
- In patch 3/3:
  - Keep rtw_fw_dump_dbg_info() in place on first failure.
  - Restrict firmware recovery invocation to PCIe devices to prevent
    scheduling while atomic panic on USB and SDIO.
  - Explicitly check rtw_hci_type() in rtw_fw_recovery().

Changes in v3:
- Note: v2 patch 1/2 (DMI quirk) is already applied in rtw-next.
- Add patch 1/2 to clear RTW_FLAG_RESTARTING in rtw_ops_stop()
  with Fixes tag.
- In patch 2/2:
  - Count consecutive failures before triggering recovery
    (RTW_PS_HANG_MAX_CNT = 3).
  - Reset ps_hang_cnt on successful LPS leave and in rtw_core_start().
  - Suppress raw register dump on transient failures to avoid log spam.

Changes in v2:
- Rebase on top of rtw-next.

Kalpesh Solanki (3):
  wifi: rtw88: clear RTW_FLAG_RESTARTING on stop and start failure
  wifi: rtw88: skip already reset station in rtw_sta_remove()
  wifi: rtw88: ps: trigger firmware recovery only after consecutive LPS
    leave failures on PCI

 drivers/net/wireless/realtek/rtw88/mac80211.c |  3 +++
 drivers/net/wireless/realtek/rtw88/main.c     | 10 ++++++++++
 drivers/net/wireless/realtek/rtw88/main.h     |  1 +
 drivers/net/wireless/realtek/rtw88/ps.c       |  7 +++++++
 drivers/net/wireless/realtek/rtw88/ps.h       |  1 +
 5 files changed, 22 insertions(+)

-- 
2.53.0

[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

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

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]

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