Re: [PATCH v2 2/2] wifi: rtw88: ps: Trigger firmware recovery when failing to leave LPS state

[Kalpesh Solanki <skalpesh942@gmail.com>]

Hi Luka,

Thanks for taking a look. You make very good points on both issues.

> As far as I can see, this timeout doesn't always mean the firmware is
> stuck. The comment in __rtw_fw_leave_lps_check_reg() says REG_TCR is
> only restored once the AP has acked the null frame, and 3a2dd6b7cadf
> ("rtw88: check firmware leave lps successfully") says that frame can
> take much longer in a noisy environment. That is why the old code just
> cleared the bit and warned.
> 
> So if I read it right, a busy channel or a weak link now ends in the
> WARN() in __fw_recovery_work() and a full ieee80211_restart_hw(). The
> watchdog leaves LPS every two seconds on an idle link, and
> BCN_FILTER_NOTIFY_BEACON_LOSS leaves it right when the AP may be gone.
> This also reaches USB and SDIO now, which had no way to trigger
> rtw_fw_recovery() before.
> 
> rtw89 only recovers after RTW89_PS_HANG_MAX_CNT failures in a row.
> Would you be willing to send a follow up that does the same here?

Yeah, resetting the whole hardware right away on a single timeout is
definitely too aggressive if the frame just got delayed or dropped due
to RF noise or beacon loss. Using a counter like rtw89 does
(RTW89_PS_HANG_MAX_CNT = 3) and only triggering recovery after consecutive
failures makes much more sense to prevent spurious restarts.

I'm happy to write and send a follow-up patch implementing this.

> There also seems to be a path where recovery never completes.
> rtw_leave_lps() also runs from ieee80211_set_disassoc() through
> rtw_recalc_lps(). If the check times out there while the last interface
> goes down, or on a suspend without WoWLAN, the queued work runs before
> drv_stop(). mac80211 then never calls drv_reconfig_complete() with
> IEEE80211_RECONFIG_TYPE_RESTART, so RTW_FLAG_RESTARTING stays set and
> rtw_fw_recovery() does nothing until the driver is reloaded, also for
> real firmware crashes. Maybe rtw_ops_stop() could clear it as well?

Good catch on the mac80211 lifecycle path. If open_count is zero or during
suspend without WoWLAN, drv_reconfig_complete() never gets called, which
leaves RTW_FLAG_RESTARTING stuck and permanently blocks recovery until module
reload.

Clearing RTW_FLAG_RESTARTING (and resetting the hang counter) in
rtw_ops_stop() sounds like the right way to handle it.

I can prepare a 2-patch follow-up series:
1. wifi: rtw88: ps: trigger recovery only after consecutive LPS leave failures
2. wifi: rtw88: clear RTW_FLAG_RESTARTING in rtw_ops_stop()

Ping-Ke, does that sound good to you?

Thanks,
Kalpesh