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: [email protected]
Fixes: 8def9ec46a5f ("wcn36xx: Enable firmware link monitoring")
Signed-off-by: Loic Poulain <[email protected]>
---
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) |
--
2.7.4
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: [email protected]
> Fixes: 8def9ec46a5f ("wcn36xx: Enable firmware link monitoring")
> Signed-off-by: Loic Poulain <[email protected]>
> ---
> 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 <[email protected]>
Loic Poulain <[email protected]> writes:
> 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: [email protected]
> Fixes: 8def9ec46a5f ("wcn36xx: Enable firmware link monitoring")
> Signed-off-by: Loic Poulain <[email protected]>
I'll queue this to v5.15.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Loic Poulain <[email protected]> writes:
> Hi Bryan,
>
> On Tue, 31 Aug 2021 at 03:13, Bryan O'Donoghue <[email protected]> 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: [email protected]
> > Fixes: 8def9ec46a5f ("wcn36xx: Enable firmware link monitoring")
> > Signed-off-by: Loic Poulain <[email protected]>
> > ---
> > 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
On 31/08/2021 07:44, Kalle Valo wrote:
> Loic Poulain <[email protected]> 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: [email protected]
>> Fixes: 8def9ec46a5f ("wcn36xx: Enable firmware link monitoring")
>> Signed-off-by: Loic Poulain <[email protected]>
>
> 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
On 01/09/2021 00:51, Bryan O'Donoghue wrote:
> On 31/08/2021 07:44, Kalle Valo wrote:
>> Loic Poulain <[email protected]> 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: [email protected]
>>> Fixes: 8def9ec46a5f ("wcn36xx: Enable firmware link monitoring")
>>> Signed-off-by: Loic Poulain <[email protected]>
>>
>> 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
Bryan O'Donoghue <[email protected]> writes:
> On 01/09/2021 00:51, Bryan O'Donoghue wrote:
>> On 31/08/2021 07:44, Kalle Valo wrote:
>>> Loic Poulain <[email protected]> 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: [email protected]
>>>> Fixes: 8def9ec46a5f ("wcn36xx: Enable firmware link monitoring")
>>>> Signed-off-by: Loic Poulain <[email protected]>
>>>
>>> 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.
Ok, I'll drop this patch.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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: [email protected]
> Fixes: 8def9ec46a5f ("wcn36xx: Enable firmware link monitoring")
> Signed-off-by: Loic Poulain <[email protected]>
> ---
> 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) |
>
OK.
We've made a good effort to resolve offloaded link monitoring but, it
feels like spaghetti code and isn't as reliable as letting Linux do the
link monitoring.
Let's go ahead with the reversion.
Acked-by: Bryan O'Donoghue <[email protected]>