From: Karl Beldan <[email protected]>
Signed-off-by: Karl Beldan <[email protected]>
Cc: Simon Wunderlich <[email protected]>
---
drivers/net/wireless/mac80211_hwsim.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index a11dc7c..cc30acd 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -168,6 +168,7 @@ struct hwsim_vif_priv {
u8 bssid[ETH_ALEN];
bool assoc;
bool bcn_en;
+ int csa_bcn_cnt;
u16 aid;
};
@@ -1058,6 +1059,18 @@ static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac,
mac80211_hwsim_tx_frame(hw, skb,
rcu_dereference(vif->chanctx_conf)->def.chan);
+
+ if (vif->csa_active) {
+ struct hwsim_vif_priv *vp = (void *)vif->drv_priv;
+
+ vp->csa_bcn_cnt++;
+ if (ieee80211_csa_is_complete(vif)) {
+ wiphy_debug(hw->wiphy,
+ "%s CSA complete after %d beacons\n",
+ __func__, vp->csa_bcn_cnt);
+ ieee80211_csa_finish(vif);
+ }
+ }
}
static enum hrtimer_restart
@@ -1692,6 +1705,19 @@ static void mac80211_hwsim_unassign_vif_chanctx(struct ieee80211_hw *hw,
hwsim_check_chanctx_magic(ctx);
}
+static void mac80211_hwsim_channel_switch_beacon(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ struct cfg80211_chan_def *chandef)
+{
+ struct hwsim_vif_priv *vp = (void *)vif->drv_priv;
+
+ hwsim_check_magic(vif);
+ vp->csa_bcn_cnt = 0;
+ wiphy_debug(hw->wiphy, "%s (freq=%d(%d - %d)/%s)\n", __func__,
+ chandef->chan->center_freq, chandef->center_freq1,
+ chandef->center_freq2, hwsim_chanwidths[chandef->width]);
+}
+
static struct ieee80211_ops mac80211_hwsim_ops =
{
.tx = mac80211_hwsim_tx,
@@ -1716,6 +1742,7 @@ static struct ieee80211_ops mac80211_hwsim_ops =
.flush = mac80211_hwsim_flush,
.get_tsf = mac80211_hwsim_get_tsf,
.set_tsf = mac80211_hwsim_set_tsf,
+ .channel_switch_beacon = mac80211_hwsim_channel_switch_beacon,
};
@@ -2359,7 +2386,9 @@ static int __init init_mac80211_hwsim(void)
hw->wiphy->flags |= WIPHY_FLAG_SUPPORTS_TDLS |
WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL |
- WIPHY_FLAG_AP_UAPSD;
+ WIPHY_FLAG_AP_UAPSD |
+ WIPHY_FLAG_HAS_CHANNEL_SWITCH;
+
hw->wiphy->features |= NL80211_FEATURE_ACTIVE_MONITOR;
/* ask mac80211 to reserve space for magic */
--
1.8.2
On Fri, Nov 22, 2013 at 10:06:26AM +0100, Karl Beldan wrote:
> From: Karl Beldan <[email protected]>
>
> Signed-off-by: Karl Beldan <[email protected]>
> Cc: Simon Wunderlich <[email protected]>
> ---
> drivers/net/wireless/mac80211_hwsim.c | 31 ++++++++++++++++++++++++++++++-
> 1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> index a11dc7c..cc30acd 100644
> --- a/drivers/net/wireless/mac80211_hwsim.c
> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -168,6 +168,7 @@ struct hwsim_vif_priv {
> u8 bssid[ETH_ALEN];
> bool assoc;
> bool bcn_en;
> + int csa_bcn_cnt;
> u16 aid;
> };
>
> @@ -1058,6 +1059,18 @@ static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac,
>
> mac80211_hwsim_tx_frame(hw, skb,
> rcu_dereference(vif->chanctx_conf)->def.chan);
> +
> + if (vif->csa_active) {
> + struct hwsim_vif_priv *vp = (void *)vif->drv_priv;
> +
> + vp->csa_bcn_cnt++;
> + if (ieee80211_csa_is_complete(vif)) {
> + wiphy_debug(hw->wiphy,
> + "%s CSA complete after %d beacons\n",
> + __func__, vp->csa_bcn_cnt);
> + ieee80211_csa_finish(vif);
> + }
> + }
> }
>
Hmm, I thought the CSA code would make ieee80211_beacon_get_tim return
NULL (or do something alike) after the last ieee80211_beacon_get_tim
returned a beacon with a null CSA count until the config is done - but
it seems it doesn't - in that case this change would be race prone.
Did I miss something ?
Karl
On Fri, Nov 22, 2013 at 01:08:11PM +0100, Simon Wunderlich wrote:
> Hello Karl,
>
> >
> > Hmm, I thought the CSA code would make ieee80211_beacon_get_tim return
> > NULL (or do something alike) after the last ieee80211_beacon_get_tim
> > returned a beacon with a null CSA count until the config is done - but
> > it seems it doesn't - in that case this change would be race prone.
> > Did I miss something ?
>
> No, you have to do the check yourself (as you appearently did). In ath9k I'm
> checking if the CSA finished before scheduling the next beacon. Can you do the
> same for hwsim? Where do you see the race?
>
Hi Simon,
I would see a race in such scenario: {{{
cmd: stack/channel_switch(count=1);
bcn-intr: driver/beacon_get();
vif->csa_active == true
ieee80211_csa_is_complete(); // == true
ieee80211_csa_finish(); // schedules csa work
bcn-intr: driver/beacon_get(); // beacon but on the old channel
vif->csa_active still true
ieee80211_csa_is_complete(); // still true
csa worker: csa_finalize_work(); ----> too late
change_channel(); ----> too late
}}}
To prevent this, I thought that there was something like : {{{
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 3e2dfcb..97b0382 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2405,7 +2405,7 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif)
}
EXPORT_SYMBOL(ieee80211_csa_finish);
-static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
+static bool ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
struct beacon_data *beacon)
{
struct probe_resp *resp;
@@ -2428,14 +2428,14 @@ static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
beacon_data_len = beacon->head_len;
break;
default:
- return;
+ return false;
}
if (WARN_ON(counter_offset_beacon >= beacon_data_len))
- return;
+ return false;
/* warn if the driver did not check for/react to csa completeness */
if (WARN_ON(beacon_data[counter_offset_beacon] == 0))
- return;
+ return false;
beacon_data[counter_offset_beacon]--;
@@ -2446,11 +2446,13 @@ static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
/* if nl80211 accepted the offset, this should not happen. */
if (WARN_ON(!resp)) {
rcu_read_unlock();
- return;
+ return true;
}
resp->data[counter_offset_presp]--;
rcu_read_unlock();
}
+
+ return true;
}
bool ieee80211_csa_is_complete(struct ieee80211_vif *vif)
@@ -2540,7 +2542,8 @@ struct sk_buff *ieee80211_beacon_get_tim(struct ieee80211_hw *hw,
if (beacon) {
if (sdata->vif.csa_active)
- ieee80211_update_csa(sdata, beacon);
+ if (!ieee80211_update_csa(sdata, beacon))
+ goto out;
... blah blah for mesh .. ibss ..
}}}
.. Of course there are other means to achieve this, just an example.
However I might have overlooked some things in the CSA code so ..
> Simon
>
> P.S.: Please use my new e-mail address, the old one will go out of service by
> end of the year.
Sure
Karl
On Tue, Nov 26, 2013 at 04:43:22PM +0100, Simon Wunderlich wrote:
> Hey Karl,
> > Sorry, I was not very clear.
> > What I meant is that, my change for hwsim is not correct because it
> > assumed something like what's above that would prevent beacon_get() to
> > return a beacon when csa is under completion. Instead, it should check
> > for csa completion prior to getting the beacon.
> >
> > Checking your ath9k change for CSA, I am under the impression this is
> > possible ? : {{{
> > ath9k_channel_switch_beacon()
> > ath9k_beacon_tasklet()
> > ath9k_csa_is_finished() // ret false
> > ..
> > ath9k_beacon_tasklet()
> > ath9k_csa_is_finished() // sets sc->csa_vif to NULL, ret true
> > ieee80211_csa_finish() // schedules csa work
> >
> > ath9k_beacon_tasklet()
> > ath9k_csa_is_finished() // ret false because sc->csa_vif==NULL
> >
> > csa_finalize_work() // too late
> > }}}
>
> Yeah, you are right, if the worker is too late ath9k still generates a beacon
> and will hit a warning that the csa was already finished. Actually that helped
> quite a lot while developing the feature.
>
> In practice, I don't think it's that bad - the worker shouldn't take so long
> too respond, and also it does not hurt to generate/send the beacon one more
> time on the same channel with count = 0 (or 1). It's useful to get a warning
> in this case so that the root cause for the delay can be worked on.
>
I think it's always nice to add a clean-room use of a new stack feature.
I was more expecting a WARN_ON(!vif && vif->csa_active) in the driver
than hitting a "forbidden" path in the stack.
The v4 I sent for hwsim is immune to this race but I'd have expected the
CSA code to handle this (e.g. with a vif->csa_finishing, or by letting
ieee80211_beacon_get* return an ERR_PTR) since many drivers would? have
to do this ATM.
> What do you think? I can change that easily in ath9k otherwise. :)
>
I have no strong opinion on this, no pb with me keeping it as is, plus
CSA being a recent addition, I doubt new ideas won't chime in.
Karl
Hello Karl,
>
> Hmm, I thought the CSA code would make ieee80211_beacon_get_tim return
> NULL (or do something alike) after the last ieee80211_beacon_get_tim
> returned a beacon with a null CSA count until the config is done - but
> it seems it doesn't - in that case this change would be race prone.
> Did I miss something ?
No, you have to do the check yourself (as you appearently did). In ath9k I'm
checking if the CSA finished before scheduling the next beacon. Can you do the
same for hwsim? Where do you see the race?
Simon
P.S.: Please use my new e-mail address, the old one will go out of service by
end of the year.
On Fri, Nov 22, 2013 at 03:05:33PM +0100, Karl Beldan wrote:
> On Fri, Nov 22, 2013 at 01:08:11PM +0100, Simon Wunderlich wrote:
> > Hello Karl,
> >
> > >
> > > Hmm, I thought the CSA code would make ieee80211_beacon_get_tim return
> > > NULL (or do something alike) after the last ieee80211_beacon_get_tim
> > > returned a beacon with a null CSA count until the config is done - but
> > > it seems it doesn't - in that case this change would be race prone.
> > > Did I miss something ?
> >
> > No, you have to do the check yourself (as you appearently did). In ath9k I'm
> > checking if the CSA finished before scheduling the next beacon. Can you do the
> > same for hwsim? Where do you see the race?
> >
> Hi Simon,
>
> I would see a race in such scenario: {{{
> cmd: stack/channel_switch(count=1);
>
> bcn-intr: driver/beacon_get();
> vif->csa_active == true
> ieee80211_csa_is_complete(); // == true
> ieee80211_csa_finish(); // schedules csa work
>
> bcn-intr: driver/beacon_get(); // beacon but on the old channel
> vif->csa_active still true
> ieee80211_csa_is_complete(); // still true
>
> csa worker: csa_finalize_work(); ----> too late
> change_channel(); ----> too late
> }}}
>
> To prevent this, I thought that there was something like : {{{
>
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 3e2dfcb..97b0382 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -2405,7 +2405,7 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif)
> }
> EXPORT_SYMBOL(ieee80211_csa_finish);
>
> -static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
> +static bool ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
> struct beacon_data *beacon)
> {
> struct probe_resp *resp;
> @@ -2428,14 +2428,14 @@ static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
> beacon_data_len = beacon->head_len;
> break;
> default:
> - return;
> + return false;
> }
> if (WARN_ON(counter_offset_beacon >= beacon_data_len))
> - return;
> + return false;
>
> /* warn if the driver did not check for/react to csa completeness */
> if (WARN_ON(beacon_data[counter_offset_beacon] == 0))
> - return;
> + return false;
>
> beacon_data[counter_offset_beacon]--;
>
> @@ -2446,11 +2446,13 @@ static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
> /* if nl80211 accepted the offset, this should not happen. */
> if (WARN_ON(!resp)) {
> rcu_read_unlock();
> - return;
> + return true;
> }
> resp->data[counter_offset_presp]--;
> rcu_read_unlock();
> }
> +
> + return true;
> }
>
> bool ieee80211_csa_is_complete(struct ieee80211_vif *vif)
> @@ -2540,7 +2542,8 @@ struct sk_buff *ieee80211_beacon_get_tim(struct ieee80211_hw *hw,
>
> if (beacon) {
> if (sdata->vif.csa_active)
> - ieee80211_update_csa(sdata, beacon);
> + if (!ieee80211_update_csa(sdata, beacon))
> + goto out;
>
> ... blah blah for mesh .. ibss ..
> }}}
> .. Of course there are other means to achieve this, just an example.
>
> However I might have overlooked some things in the CSA code so ..
>
Sorry, I was not very clear.
What I meant is that, my change for hwsim is not correct because it
assumed something like what's above that would prevent beacon_get() to
return a beacon when csa is under completion. Instead, it should check
for csa completion prior to getting the beacon.
Checking your ath9k change for CSA, I am under the impression this is
possible ? : {{{
ath9k_channel_switch_beacon()
ath9k_beacon_tasklet()
ath9k_csa_is_finished() // ret false
..
ath9k_beacon_tasklet()
ath9k_csa_is_finished() // sets sc->csa_vif to NULL, ret true
ieee80211_csa_finish() // schedules csa work
ath9k_beacon_tasklet()
ath9k_csa_is_finished() // ret false because sc->csa_vif==NULL
csa_finalize_work() // too late
}}}
Karl
Hey Karl,
> Sorry, I was not very clear.
> What I meant is that, my change for hwsim is not correct because it
> assumed something like what's above that would prevent beacon_get() to
> return a beacon when csa is under completion. Instead, it should check
> for csa completion prior to getting the beacon.
>
> Checking your ath9k change for CSA, I am under the impression this is
> possible ? : {{{
> ath9k_channel_switch_beacon()
> ath9k_beacon_tasklet()
> ath9k_csa_is_finished() // ret false
> ..
> ath9k_beacon_tasklet()
> ath9k_csa_is_finished() // sets sc->csa_vif to NULL, ret true
> ieee80211_csa_finish() // schedules csa work
>
> ath9k_beacon_tasklet()
> ath9k_csa_is_finished() // ret false because sc->csa_vif==NULL
>
> csa_finalize_work() // too late
> }}}
Yeah, you are right, if the worker is too late ath9k still generates a beacon
and will hit a warning that the csa was already finished. Actually that helped
quite a lot while developing the feature.
In practice, I don't think it's that bad - the worker shouldn't take so long
too respond, and also it does not hurt to generate/send the beacon one more
time on the same channel with count = 0 (or 1). It's useful to get a warning
in this case so that the root cause for the delay can be worked on.
What do you think? I can change that easily in ath9k otherwise. :)
Thanks,
Simon