Driver is supposed to call it only once for each
scheduled channel_switch_beacon().
Signed-off-by: Michal Kazior <[email protected]>
---
This is required for multi-vif csa but since that
isn't merged yet I'm not including any specifics
in the commit log itself.
Preventing multiple ieee80211_csa_finish() calls
sounds like a good idea in general to me.
drivers/net/wireless/ath/ath10k/core.h | 1 +
drivers/net/wireless/ath/ath10k/mac.c | 5 +++--
drivers/net/wireless/ath/ath10k/wmi.c | 5 ++++-
3 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index ad209a6..cf4f756 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -228,6 +228,7 @@ struct ath10k_vif {
struct ath10k *ar;
struct ieee80211_vif *vif;
+ bool csa_finished;
bool is_started;
bool is_up;
u32 aid;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 52ae380..583daba 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4090,8 +4090,9 @@ static void ath10k_channel_switch_beacon(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct cfg80211_chan_def *chandef)
{
- /* there's no need to do anything here. vif->csa_active is enough */
- return;
+ struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+
+ arvif->csa_finished = false;
}
static void ath10k_sta_rc_update(struct ieee80211_hw *hw,
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 48130fe..1630cd8 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -1414,7 +1414,10 @@ static void ath10k_wmi_event_host_swba(struct ath10k *ar, struct sk_buff *skb)
* actual channel switch is done */
if (arvif->vif->csa_active &&
ieee80211_csa_is_complete(arvif->vif)) {
- ieee80211_csa_finish(arvif->vif);
+ if (!arvif->csa_finished) {
+ ieee80211_csa_finish(arvif->vif);
+ arvif->csa_finished = true;
+ }
continue;
}
--
1.8.5.3
On Fri, 2014-03-21 at 15:58 +0100, Michal Kazior wrote:
> > Didn't we have this discussion on the hwsim patch before?
>
> Oh. I'm not aware of the discussion. Probably missed it.
Or I got it wrong :)
> > I'd argue that if all drivers must track this, then the mac80211 API is
> > defective.
>
> I can make this a mac80211 patch then?
Yeah I guess. What was the reason for this though? Here it's multi-vif?
Before I think we were talking about races?
johannes
On 21 March 2014 15:17, Johannes Berg <[email protected]> wrote:
> On Fri, 2014-03-21 at 15:02 +0100, Michal Kazior wrote:
>> Driver is supposed to call it only once for each
>> scheduled channel_switch_beacon().
>>
>> Signed-off-by: Michal Kazior <[email protected]>
>> ---
>> This is required for multi-vif csa but since that
>> isn't merged yet I'm not including any specifics
>> in the commit log itself.
>>
>> Preventing multiple ieee80211_csa_finish() calls
>> sounds like a good idea in general to me.
>
> Didn't we have this discussion on the hwsim patch before?
Oh. I'm not aware of the discussion. Probably missed it.
> I'd argue that if all drivers must track this, then the mac80211 API is
> defective.
I can make this a mac80211 patch then?
Michał
On Fri, 2014-03-21 at 15:02 +0100, Michal Kazior wrote:
> Driver is supposed to call it only once for each
> scheduled channel_switch_beacon().
>
> Signed-off-by: Michal Kazior <[email protected]>
> ---
> This is required for multi-vif csa but since that
> isn't merged yet I'm not including any specifics
> in the commit log itself.
>
> Preventing multiple ieee80211_csa_finish() calls
> sounds like a good idea in general to me.
Didn't we have this discussion on the hwsim patch before?
I'd argue that if all drivers must track this, then the mac80211 API is
defective.
johannes
On Mon, 2014-03-24 at 07:37 +0100, Michal Kazior wrote:
> > Yeah I guess. What was the reason for this though? Here it's multi-vif?
> > Before I think we were talking about races?
>
> Before multi-vif if ieee80211_csa_finalize() failed and left
> csa_active==true driver could end up poking the csa_finalize_work()
> over and over again I guess?
I think the concern previously was about races, where finalize_work
doesn't actually get to run before the next beacon.
> For multi-vif csa using reservation there are 2 reasons:
> a) channel switches across interfaces may not be tightly
> synchronized. I did tests where 2 long (cs_count=100) channel switches
> were a few seconds apart. In that case driver poked csa_finalize work
> quite a lot resulting in a mess.
> b) my current approach has the following flow:
>
> 1. reserve_chanctx()
> 2. (some time later) driver calls csa_finish, and csa_finalize calls
> use_reserved_chanctx() because reserved_chanctx is set
> 3. (possibly some time later, for multi-vif reservations) csa_finalize
> is called again but this time without reserved_chanctx, so it verifies
> if bss_conf.chandef is csa_chandef (after successful reservation it
> should be).
>
> This means if driver would poke csa_finalize() more than once then
> use_reserved_chanctx() would be called again and again as a result.
Yeah but that seems better to catch in mac80211?
johannes
On 21 March 2014 16:07, Johannes Berg <[email protected]> wrote:
> On Fri, 2014-03-21 at 15:58 +0100, Michal Kazior wrote:
>
>> > Didn't we have this discussion on the hwsim patch before?
>>
>> Oh. I'm not aware of the discussion. Probably missed it.
>
> Or I got it wrong :)
>
>> > I'd argue that if all drivers must track this, then the mac80211 API is
>> > defective.
>>
>> I can make this a mac80211 patch then?
>
> Yeah I guess. What was the reason for this though? Here it's multi-vif?
> Before I think we were talking about races?
Before multi-vif if ieee80211_csa_finalize() failed and left
csa_active==true driver could end up poking the csa_finalize_work()
over and over again I guess?
For multi-vif csa using reservation there are 2 reasons:
a) channel switches across interfaces may not be tightly
synchronized. I did tests where 2 long (cs_count=100) channel switches
were a few seconds apart. In that case driver poked csa_finalize work
quite a lot resulting in a mess.
b) my current approach has the following flow:
1. reserve_chanctx()
2. (some time later) driver calls csa_finish, and csa_finalize calls
use_reserved_chanctx() because reserved_chanctx is set
3. (possibly some time later, for multi-vif reservations) csa_finalize
is called again but this time without reserved_chanctx, so it verifies
if bss_conf.chandef is csa_chandef (after successful reservation it
should be).
This means if driver would poke csa_finalize() more than once then
use_reserved_chanctx() would be called again and again as a result.
Michał
Michal Kazior <[email protected]> writes:
> Driver is supposed to call it only once for each
> scheduled channel_switch_beacon().
>
> Signed-off-by: Michal Kazior <[email protected]>
> ---
> This is required for multi-vif csa but since that
> isn't merged yet I'm not including any specifics
> in the commit log itself.
>
> Preventing multiple ieee80211_csa_finish() calls
> sounds like a good idea in general to me.
Based on the discussion I'm dropping this. Please resend if we actually
want to have this in ath10k.
--
Kalle Valo