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

From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
To: Loic Poulain <loic.poulain@linaro.org>, kvalo@codeaurora.org
Cc: linux-wireless@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] Revert "wcn36xx: Enable firmware link monitoring"
Date: Tue, 31 Aug 2021 02:15:29 +0100	[thread overview]
Message-ID: <401fd67a-4965-d4ae-8b6c-8c81a179a8c7@linaro.org> (raw)
In-Reply-To: <1630343360-5942-1-git-send-email-loic.poulain@linaro.org>

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 <loic.poulain@linaro.org>
> ---
>   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_

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

Given we don't have roam offload enabled either, in this reference, it 
also means we won't roam to a new AP until Linux resumes, which is 
certainly not the way we want this to work.

In any case, for now until we solve this bug

Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>