Re: [PATCH] Revert "wcn36xx: Enable firmware link monitoring"

Loic Poulain [off-list ref] writes:

Hi Bryan,

On Tue, 31 Aug 2021 at 03:13, Bryan O'Donoghue [off-list ref] wrote:

 On 30/08/2021 18:09, Loic Poulain wrote:
 > This reverts commit 8def9ec46a5fafc0abcf34489a9e8a787bca984d.
 > 
 > The firmware keep-alive does not cause any event in case of error
 > such as non acked. It's just a basic keep alive to prevent the AP
 > to kick-off the station due to inactivity. So let mac80211 submit
 > its own monitoring packet (probe/null) and disconnect on timeout.
 > 
 > Note: We want to keep firmware keep alive to prevent kick-off
 > when host is in suspend-to-mem (no mac80211 monitor packet).
 > Ideally fw keep alive should be enabled in suspend path and disabled
 > in resume path to prevent having both firmware and mac80211 submitting
 > periodic null packets.
 > 
 > This fixes non detected AP leaving issues in active mode (nothing
 > monitors beacon or connection).
 > 
 > Cc: stable@vger.kernel.org
 > Fixes: 8def9ec46a5f ("wcn36xx: Enable firmware link monitoring")
 > Signed-off-by: Loic Poulain [off-list ref]
 > ---
 >   drivers/net/wireless/ath/wcn36xx/main.c | 1 -
 >   1 file changed, 1 deletion(-)
 > 
 > diff --git a/drivers/net/wireless/ath/wcn36xx/main.c
 b/drivers/net/wireless/ath/wcn36xx/main.c
 > index 216bc34..128d25d 100644
 > --- a/drivers/net/wireless/ath/wcn36xx/main.c
 > +++ b/drivers/net/wireless/ath/wcn36xx/main.c
 > @@ -1362,7 +1362,6 @@ static int wcn36xx_init_ieee80211(struct wcn36xx *wcn)
 >       ieee80211_hw_set(wcn->hw, HAS_RATE_CONTROL);
 >       ieee80211_hw_set(wcn->hw, SINGLE_SCAN_ON_ALL_BANDS);
 >       ieee80211_hw_set(wcn->hw, REPORTS_TX_ACK_STATUS);
 > -     ieee80211_hw_set(wcn->hw, CONNECTION_MONITOR);
 >   
 >       wcn->hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
 >               BIT(NL80211_IFTYPE_AP) |
 > 

 But why is BSS heartbeat offload not running, it should be.

 I agree we should switch this bit off for now since its obviously not 
 working as intended.

 But we need to root cause _why_

I think it has just not be designed as a connection tracking mechanism but as a simple
keep alive, which is submitted every 30s unconditionally.

 In suspend absent a working heartbeat monitor - if the AP goes away we 
 stay in suspend indefinitely.

We shouldn't because the firmware is monitoring beacons and would cause a beacon miss
indication, waking up the host.

No HTML please, our lists drop emails using HTML.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches