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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 01/09/2021 00:51, Bryan O'Donoghue wrote:
> On 31/08/2021 07:44, Kalle Valo wrote:
>> Loic Poulain <loic.poulain@linaro.org> writes:
>>
>>> This reverts commit wcn->hw, CONNECTION_MONITOR.
>>>
>>> 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>
>>
>> I'll queue this to v5.15.
>>
> 
> Might want to hold off on that.
> 
> Its been reported by testing that wcn36xx_smd_delete_sta_context_ind() 
> actually _is_ firing.
> 
> But if you look at wcn36xx_smd_delete_sta_context_ind() it doesn't seem 
> to do ieee80211_connection_loss() like it presumably should.
> 
> I think the right fix here might be to call ieee80211_connection_loss() 
> in wcn36xx_smd_delete_sta_context_ind() if (wcn->hw & CONNECTION_MONITOR)
> 
> ---
> bod

In that case right now if (wcn->hw & CONNECTION_MONITOR) we'd presumably 
rely on a subsequent wcn36xx_smd_missed_beacon_ind() but...

its not clear that wcn36xx_smd_missed_beacon_ind() would fire subsequent 
to wcn36xx_smd_delete_sta_context_ind()

In fact, it would be pretty illogical if it did.

I think instead of revert - we want to just do better processing here in 
wcn36xx_smd_delete_sta_context_ind() alright.

---
bod