[v4] mac80211_hwsim: claim CSA support for AP

Commit Message

Karl Beldan Jan. 6, 2014, 6:01 p.m. UTC

On Tue, Dec 17, 2013 at 06:01:00PM +0200, Kalle Valo wrote:
> Karl Beldan <[e-mail]> writes:
> 
> > From: Karl Beldan <[e-mail]>
> >
> > 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 <[e-mail]>
> > Cc: Simon Wunderlich <[e-mail]>
> 
> 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: {{{

}}}

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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Johannes Berg Jan. 7, 2014, 3:33 p.m. UTC | #1

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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Karl Beldan Jan. 7, 2014, 5:34 p.m. UTC | #2

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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Johannes Berg Jan. 20, 2014, 11:09 a.m. UTC | #3

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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Karl Beldan Jan. 20, 2014, 1:51 p.m. UTC | #4

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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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 */