[PATCH v4] mac80211_hwsim: claim CSA support for AP

[PATCH v4] mac80211_hwsim: claim CSA support for AP

[Karl Beldan]

From: Karl Beldan <karl.beldan@rivierawaves.com>

This assigns the channel_switch_beacon op.
For hwsim, it comes down to calling ieee80211_csa_finish once
ieee80211_csa_is_complete is true.
Since channel_switch_beacon is not called if CSA count starts @ 0 or 1,
the check for ieee80211_csa_is_complete can be done after getting the
beacon (and this way it might trigger helpful warnings).

This adds a per vif bool csa_finished that is set after a call to
ieee80211_csa_finish() and used to skip beaconing while csa_active is
set in case a beacon is scheduled prior to csa_finalize_work completion.
This bool and the number of beacons transmitted during the CSA up to the
call to ieee80211_csa_finish() are reset in channel_switch_beacon op.

Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
Cc: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
---
 drivers/net/wireless/mac80211_hwsim.c | 39 ++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index a11dc7c..0fb6bb4 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -169,6 +169,8 @@ struct hwsim_vif_priv {
 	bool assoc;
 	bool bcn_en;
 	u16 aid;
+	int csa_bcn_cnt;
+	bool csa_finished;
 };
 
 #define HWSIM_VIF_MAGIC	0x69537748
@@ -1024,6 +1026,7 @@ static void mac80211_hwsim_tx_frame(struct ieee80211_hw *hw,
 static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac,
 				     struct ieee80211_vif *vif)
 {
+	struct hwsim_vif_priv *vp = (void *)vif->drv_priv;
 	struct mac80211_hwsim_data *data = arg;
 	struct ieee80211_hw *hw = data->hw;
 	struct ieee80211_tx_info *info;
@@ -1038,6 +1041,12 @@ static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac,
 	    vif->type != NL80211_IFTYPE_ADHOC)
 		return;
 
+	if (vif->csa_active && vp->csa_finished) {
+		wiphy_debug(hw->wiphy, "%s skip (CSA is active & finished)\n",
+			    __func__);
+		return;
+	}
+
 	skb = ieee80211_beacon_get(hw, vif);
 	if (skb == NULL)
 		return;
@@ -1058,6 +1067,17 @@ 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) {
+		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);
+			vp->csa_finished = 1;
+		}
+	}
 }
 
 static enum hrtimer_restart
@@ -1692,6 +1712,20 @@ 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_finished = 0;
+	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 +1750,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 +2394,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

Re: [PATCH v4] mac80211_hwsim: claim CSA support for AP

[Johannes Berg]

On Sat, 2013-11-23 at 17:38 +0100, Karl Beldan wrote:

> This adds a per vif bool csa_finished that is set after a call to
> ieee80211_csa_finish() and used to skip beaconing while csa_active is
> set in case a beacon is scheduled prior to csa_finalize_work completion.
> This bool and the number of beacons transmitted during the CSA up to the
> call to ieee80211_csa_finish() are reset in channel_switch_beacon op.

Andrei says:

Overall it seems ok, but all the purpose of csa_finished is not very
clear..
It looks that this patch tries to avoid extra beaconing on the previous
channel/hitting the warning..
But the problem is much bigger here, it means that we didn't switch in
time (before the next beacon) so it's ok to hit the warning here and
transmit extra beacon with count==1.
So if we see a lot of such warnings, maybe we need to fix
ieee80211_csa_finish instead (not using work for example)

johannes

Re: [PATCH v4] mac80211_hwsim: claim CSA support for AP

[Karl Beldan]

On Tue, Dec 03, 2013 at 01:55:33PM +0100, Johannes Berg wrote:
> On Sat, 2013-11-23 at 17:38 +0100, Karl Beldan wrote:
> 
> > This adds a per vif bool csa_finished that is set after a call to
> > ieee80211_csa_finish() and used to skip beaconing while csa_active is
> > set in case a beacon is scheduled prior to csa_finalize_work completion.
> > This bool and the number of beacons transmitted during the CSA up to the
> > call to ieee80211_csa_finish() are reset in channel_switch_beacon op.
> 
> Andrei says:
> 
> Overall it seems ok, but all the purpose of csa_finished is not very
> clear..
> It looks that this patch tries to avoid extra beaconing on the previous
> channel/hitting the warning..
Yes, it avoids extra beacons.

> But the problem is much bigger here, it means that we didn't switch in
> time (before the next beacon) so it's ok to hit the warning here and
> transmit extra beacon with count==1.
Precisely what I discussed in the previous emails.
However, I'd expect WARN*s to trigger on unexpected conditions .. though
I haven't seen any warning, the codepath nearly seems to beg for it.

> So if we see a lot of such warnings, maybe we need to fix
> ieee80211_csa_finish instead (not using work for example)
> 
The replies I got from the race I detailed felt like ppl wanted to
prevent this in-driver, now we agree this race prone work is a flaw.
Maybe I'll send a v5 for hwsim relying on future in-stack improvement to
avoid extra beacon instead of doing this in-driver, with some
appropriate comments.
 
Karl

Re: [PATCH v4] mac80211_hwsim: claim CSA support for AP

[Johannes Berg]

On Thu, 2013-12-05 at 18:26 +0100, Karl Beldan wrote:

> > Overall it seems ok, but all the purpose of csa_finished is not very
> > clear..
> > It looks that this patch tries to avoid extra beaconing on the previous
> > channel/hitting the warning..
> Yes, it avoids extra beacons.
> 
> > But the problem is much bigger here, it means that we didn't switch in
> > time (before the next beacon) so it's ok to hit the warning here and
> > transmit extra beacon with count==1.

> Precisely what I discussed in the previous emails.
> However, I'd expect WARN*s to trigger on unexpected conditions .. though
> I haven't seen any warning, the codepath nearly seems to beg for it.
> 
> > So if we see a lot of such warnings, maybe we need to fix
> > ieee80211_csa_finish instead (not using work for example)

> The replies I got from the race I detailed felt like ppl wanted to
> prevent this in-driver, now we agree this race prone work is a flaw.
> Maybe I'll send a v5 for hwsim relying on future in-stack improvement to
> avoid extra beacon instead of doing this in-driver, with some
> appropriate comments.

Generally, I think I'd prefer simpler code in the driver(s) over simpler
code in the stack, since the former tends to get duplicated a lot.

johannes

Re: [PATCH v4] mac80211_hwsim: claim CSA support for AP

[Kalle Valo]

Karl Beldan <karl.beldan@gmail.com> writes:

> From: Karl Beldan <karl.beldan@rivierawaves.com>
>
> This assigns the channel_switch_beacon op.
> For hwsim, it comes down to calling ieee80211_csa_finish once
> ieee80211_csa_is_complete is true.
> Since channel_switch_beacon is not called if CSA count starts @ 0 or 1,
> the check for ieee80211_csa_is_complete can be done after getting the
> beacon (and this way it might trigger helpful warnings).
>
> This adds a per vif bool csa_finished that is set after a call to
> ieee80211_csa_finish() and used to skip beaconing while csa_active is
> set in case a beacon is scheduled prior to csa_finalize_work completion.
> This bool and the number of beacons transmitted during the CSA up to the
> call to ieee80211_csa_finish() are reset in channel_switch_beacon op.
>
> Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
> Cc: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>

What are we going to do with this patch? It would be convenient to get
hwsim support CSA as it would help working with DFS issues.

Was the issue blocking this patch the race when transmitting beacons?
IMHO we can live with that and fix it properly later. It's not the only
bug in DFS code right now ;)

> @@ -1058,6 +1067,17 @@ 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) {
> +		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);
> +			vp->csa_finished = 1;
> +		}
> +	}

csa_finished is a bool so 'true' should be used here.

-- 
Kalle Valo

Re: [PATCH v4] mac80211_hwsim: claim CSA support for AP

[Karl Beldan]

On Tue, Dec 17, 2013 at 06:01:00PM +0200, Kalle Valo wrote:
> Karl Beldan <karl.beldan@gmail.com> writes:
> 
> > From: Karl Beldan <karl.beldan@rivierawaves.com>
> >
> > This assigns the channel_switch_beacon op.
> > For hwsim, it comes down to calling ieee80211_csa_finish once
> > ieee80211_csa_is_complete is true.
> > Since channel_switch_beacon is not called if CSA count starts @ 0 or 1,
> > the check for ieee80211_csa_is_complete can be done after getting the
> > beacon (and this way it might trigger helpful warnings).
> >
> > This adds a per vif bool csa_finished that is set after a call to
> > ieee80211_csa_finish() and used to skip beaconing while csa_active is
> > set in case a beacon is scheduled prior to csa_finalize_work completion.
> > This bool and the number of beacons transmitted during the CSA up to the
> > call to ieee80211_csa_finish() are reset in channel_switch_beacon op.
> >
> > Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
> > Cc: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
> 
> What are we going to do with this patch? It would be convenient to get
> hwsim support CSA as it would help working with DFS issues.
> 
> Was the issue blocking this patch the race when transmitting beacons?
> IMHO we can live with that and fix it properly later. It's not the only
> bug in DFS code right now ;)
> 

Hi,

Sorry for the delay.
I am under the impression ppl who have expressed themselves would
prefer something like: {{{

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 9c0cc8d..b76de14 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -169,6 +169,8 @@ struct hwsim_vif_priv {
 	bool assoc;
 	bool bcn_en;
 	u16 aid;
+	int csa_bcn_cnt;
+	bool csa_finished;
 };
 
 #define HWSIM_VIF_MAGIC	0x69537748
@@ -1032,6 +1034,7 @@ static void mac80211_hwsim_tx_frame(struct ieee80211_hw *hw,
 static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac,
 				     struct ieee80211_vif *vif)
 {
+	struct hwsim_vif_priv *vp = (void *)vif->drv_priv;
 	struct mac80211_hwsim_data *data = arg;
 	struct ieee80211_hw *hw = data->hw;
 	struct ieee80211_tx_info *info;
@@ -1066,6 +1069,19 @@ 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) {
+		vp->csa_bcn_cnt++;
+		if (vp->csa_finished) {
+			wiphy_debug(hw->wiphy,"%s extra CSA-beacon\n", __func__);
+		} else 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);
+			vp->csa_finished = true;
+		}
+	}
 }
 
 static enum hrtimer_restart
@@ -1700,6 +1716,20 @@ 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_finished = false;
+	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,
@@ -1724,6 +1754,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,
 };
 
 
@@ -2366,7 +2397,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 */
}}}

I understand having this is convenient for DFS so feel free to
adjust/submit this patch as you see fit.


> > @@ -1058,6 +1067,17 @@ 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) {
> > +		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);
> > +			vp->csa_finished = 1;
> > +		}
> > +	}
> 
> csa_finished is a bool so 'true' should be used here.
> 
Thanks,
 
Karl

Re: [PATCH v4] mac80211_hwsim: claim CSA support for AP

[Johannes Berg]

On Mon, 2014-01-06 at 19:01 +0100, Karl Beldan wrote:

> +	if (vif->csa_active) {
> +		vp->csa_bcn_cnt++;

I don't see any readers of this variable?

> +		if (vp->csa_finished) {
> +			wiphy_debug(hw->wiphy,"%s extra CSA-beacon\n", __func__);
> +		} else if (ieee80211_csa_is_complete(vif)) {
> +			wiphy_debug(hw->wiphy,
> +				    "%s CSA complete after %d beacons\n",
> +				    __func__, vp->csa_bcn_cnt);

except for the debug, but is that really useful?

> +			ieee80211_csa_finish(vif);
> +			vp->csa_finished = true;

and if we remove the other debug we can also remove the csa_finished
variable, that seems reasonable to me since mac80211 would already warn
anyway, no?

johannes

Re: [PATCH v4] mac80211_hwsim: claim CSA support for AP

[Karl Beldan]

On Tue, Jan 07, 2014 at 04:33:38PM +0100, Johannes Berg wrote:
> On Mon, 2014-01-06 at 19:01 +0100, Karl Beldan wrote:
> 
> > +	if (vif->csa_active) {
> > +		vp->csa_bcn_cnt++;
> 
> I don't see any readers of this variable?
> 
> > +		if (vp->csa_finished) {
> > +			wiphy_debug(hw->wiphy,"%s extra CSA-beacon\n", __func__);
> > +		} else if (ieee80211_csa_is_complete(vif)) {
> > +			wiphy_debug(hw->wiphy,
> > +				    "%s CSA complete after %d beacons\n",
> > +				    __func__, vp->csa_bcn_cnt);
> 
> except for the debug, but is that really useful?
> 
In a sim/debug module ? I found it appropriate ;)
Seriously I have no strong opinion on this matter, as I said, feel free
to do whatever you want with this, there's nothing critical here.

> > +			ieee80211_csa_finish(vif);
> > +			vp->csa_finished = true;
> 
> and if we remove the other debug we can also remove the csa_finished
> variable, that seems reasonable to me since mac80211 would already warn
> anyway, no?
> 
I just got rid of a big blahblah I got for this, please, adjust as you
see fit as I have no strong opinion on this matter.

 
Karl

Re: [PATCH v4] mac80211_hwsim: claim CSA support for AP

[Johannes Berg]

On Tue, 2014-01-07 at 18:34 +0100, Karl Beldan wrote:
> On Tue, Jan 07, 2014 at 04:33:38PM +0100, Johannes Berg wrote:
> > On Mon, 2014-01-06 at 19:01 +0100, Karl Beldan wrote:
> > 
> > > +	if (vif->csa_active) {
> > > +		vp->csa_bcn_cnt++;
> > 
> > I don't see any readers of this variable?
> > 
> > > +		if (vp->csa_finished) {
> > > +			wiphy_debug(hw->wiphy,"%s extra CSA-beacon\n", __func__);
> > > +		} else if (ieee80211_csa_is_complete(vif)) {
> > > +			wiphy_debug(hw->wiphy,
> > > +				    "%s CSA complete after %d beacons\n",
> > > +				    __func__, vp->csa_bcn_cnt);
> > 
> > except for the debug, but is that really useful?
> > 
> In a sim/debug module ? I found it appropriate ;)
> Seriously I have no strong opinion on this matter, as I said, feel free
> to do whatever you want with this, there's nothing critical here.
> 
> > > +			ieee80211_csa_finish(vif);
> > > +			vp->csa_finished = true;
> > 
> > and if we remove the other debug we can also remove the csa_finished
> > variable, that seems reasonable to me since mac80211 would already warn
> > anyway, no?
> > 
> I just got rid of a big blahblah I got for this, please, adjust as you
> see fit as I have no strong opinion on this matter.

Well, heh. That'd mean learning all about the API? I think I have a
patch somewhere from somebody else that looked simpler, I think I'll
take that and see what happens.

johannes

Re: [PATCH v4] mac80211_hwsim: claim CSA support for AP

[Karl Beldan]

On Mon, Jan 20, 2014 at 12:09:40PM +0100, Johannes Berg wrote:
> On Tue, 2014-01-07 at 18:34 +0100, Karl Beldan wrote:
> > On Tue, Jan 07, 2014 at 04:33:38PM +0100, Johannes Berg wrote:
> > > On Mon, 2014-01-06 at 19:01 +0100, Karl Beldan wrote:
[...]
> 
> Well, heh. That'd mean learning all about the API? I think I have a
> patch somewhere from somebody else that looked simpler, I think I'll
> take that and see what happens.
> 

Arf, I wasn't aware of the trouble.

 
Karl