[PATCH] mac80211: implement STA CSA for drivers using channel contexts

[PATCH] mac80211: implement STA CSA for drivers using channel contexts

[Arik Nemtsov]

Limit the current implementation to a single channel context used by
a single vif, thereby avoiding multi-vif/channel complexities.

Reuse the main function from AP CSA code, but move a portion out in
order to fit the STA scenario.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
This isn't very well tested yet, but seems to work with a TI wl18xx card.
The single-role/single-channel support is enough to pass the 11h certification.

 net/mac80211/cfg.c  |  5 +++++
 net/mac80211/chan.c |  5 -----
 net/mac80211/mlme.c | 55 +++++++++++++++++++++++++++++++++++++++--------------
 3 files changed, 46 insertions(+), 19 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 7aa38ce..beb1c2a 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2872,6 +2872,11 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 	if (WARN_ON(err < 0))
 		return;
 
+	if (!local->use_chanctx) {
+		local->_oper_chandef = local->csa_chandef;
+		ieee80211_hw_config(local, 0);
+	}
+
 	err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
 	if (err < 0)
 		return;
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 3a4764b..03ba6b5 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -453,11 +453,6 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
 	chanctx_changed |= IEEE80211_CHANCTX_CHANGE_CHANNEL;
 	drv_change_chanctx(local, ctx, chanctx_changed);
 
-	if (!local->use_chanctx) {
-		local->_oper_chandef = *chandef;
-		ieee80211_hw_config(local, 0);
-	}
-
 	ieee80211_recalc_chanctx_chantype(local, ctx);
 	ieee80211_recalc_smps_chanctx(local, ctx);
 	ieee80211_recalc_radar_chanctx(local, ctx);
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 45a87ee..750bf55 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -926,6 +926,8 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 		container_of(work, struct ieee80211_sub_if_data, u.mgd.chswitch_work);
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
+	u32 changed = 0;
+	int ret;
 
 	if (!ieee80211_sdata_running(sdata))
 		return;
@@ -934,24 +936,38 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 	if (!ifmgd->associated)
 		goto out;
 
-	local->_oper_chandef = local->csa_chandef;
+	ret = ieee80211_vif_change_channel(sdata, &local->csa_chandef,
+					   &changed);
+	if (ret) {
+		sdata_info(sdata,
+			   "vif channel switch failed, disconnecting\n");
+		ieee80211_queue_work(&sdata->local->hw,
+				     &ifmgd->csa_connection_drop_work);
+		goto out;
+	}
 
-	if (!local->ops->channel_switch) {
-		/* call "hw_config" only if doing sw channel switch */
-		ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
-	} else {
-		/* update the device channel directly */
-		local->hw.conf.chandef = local->_oper_chandef;
+	if (!local->use_chanctx) {
+		local->_oper_chandef = local->csa_chandef;
+		/* Call "hw_config" only if doing sw channel switch.
+		 * Otherwise update the channel directly */
+		if (!local->ops->channel_switch)
+			ieee80211_hw_config(local, 0);
+		else
+			local->hw.conf.chandef = local->_oper_chandef;
 	}
 
 	/* XXX: shouldn't really modify cfg80211-owned data! */
-	ifmgd->associated->channel = local->_oper_chandef.chan;
+	ifmgd->associated->channel = local->csa_chandef.chan;
 
 	/* XXX: wait for a beacon first? */
 	ieee80211_wake_queues_by_reason(&local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
 					IEEE80211_QUEUE_STOP_REASON_CSA);
+
+	ieee80211_bss_info_change_notify(sdata, changed);
+
  out:
+	sdata->vif.csa_active = false;
 	ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
 	sdata_unlock(sdata);
 }
@@ -1180,17 +1196,27 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	}
 
 	ifmgd->flags |= IEEE80211_STA_CSA_RECEIVED;
+	sdata->vif.csa_active = true;
 
+	mutex_lock(&local->chanctx_mtx);
 	if (local->use_chanctx) {
-		sdata_info(sdata,
-			   "not handling channel switch with channel contexts\n");
-		ieee80211_queue_work(&local->hw,
-				     &ifmgd->csa_connection_drop_work);
-		return;
+		u32 num_chanctx = 0;
+		list_for_each_entry(chanctx, &local->chanctx_list, list)
+		       num_chanctx++;
+
+		if (num_chanctx > 1) {
+			sdata_info(sdata,
+				   "not handling chan-switch with channel contexts multi-vif\n");
+			ieee80211_queue_work(&local->hw,
+					     &ifmgd->csa_connection_drop_work);
+			mutex_unlock(&local->chanctx_mtx);
+			return;
+		}
 	}
 
-	mutex_lock(&local->chanctx_mtx);
 	if (WARN_ON(!rcu_access_pointer(sdata->vif.chanctx_conf))) {
+		ieee80211_queue_work(&local->hw,
+				     &ifmgd->csa_connection_drop_work);
 		mutex_unlock(&local->chanctx_mtx);
 		return;
 	}
@@ -2163,6 +2189,7 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
 			       WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
 			       true, frame_buf);
 	ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
+	sdata->vif.csa_active = false;
 	ieee80211_wake_queues_by_reason(&sdata->local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
 					IEEE80211_QUEUE_STOP_REASON_CSA);
-- 
1.8.1.2

Re: [PATCH] mac80211: implement STA CSA for drivers using channel contexts

[Johannes Berg]

On Tue, 2013-08-13 at 15:03 +0300, Arik Nemtsov wrote:

> This isn't very well tested yet, but seems to work with a TI wl18xx card.
> The single-role/single-channel support is enough to pass the 11h certification.

:-)

>  net/mac80211/cfg.c  |  5 +++++
>  net/mac80211/chan.c |  5 -----
>  net/mac80211/mlme.c | 55 +++++++++++++++++++++++++++++++++++++++--------------
>  3 files changed, 46 insertions(+), 19 deletions(-)
> 
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 7aa38ce..beb1c2a 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -2872,6 +2872,11 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
>  	if (WARN_ON(err < 0))
>  		return;
>  
> +	if (!local->use_chanctx) {
> +		local->_oper_chandef = local->csa_chandef;
> +		ieee80211_hw_config(local, 0);
> +	}

I don't really understand this part - I think you should add some
documentation or something?

> @@ -453,11 +453,6 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
>  	chanctx_changed |= IEEE80211_CHANCTX_CHANGE_CHANNEL;
>  	drv_change_chanctx(local, ctx, chanctx_changed);
>  
> -	if (!local->use_chanctx) {
> -		local->_oper_chandef = *chandef;
> -		ieee80211_hw_config(local, 0);
> -	}

I really don't like it either - this was here so that no other code
really needed to be worried about non-chanctx drivers.

> @@ -1180,17 +1196,27 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
>  	}
>  
>  	ifmgd->flags |= IEEE80211_STA_CSA_RECEIVED;
> +	sdata->vif.csa_active = true;

I don't think we can just do this - this isn't going to result in good
behaviour for multi-vif drivers and in particular I'm pretty sure with
the MVM driver this would result in bad behaviour.

If you really want to do this I think it needs to be optional.

Also the documentation for the chanctx channel change probably needs to
be updated, etc.

johannes

Re: [PATCH] mac80211: implement STA CSA for drivers using channel contexts

[Arik Nemtsov]

On Thu, Aug 15, 2013 at 3:03 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>> @@ -2872,6 +2872,11 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
>>       if (WARN_ON(err < 0))
>>               return;
>>
>> +     if (!local->use_chanctx) {
>> +             local->_oper_chandef = local->csa_chandef;
>> +             ieee80211_hw_config(local, 0);
>> +     }
>
> I don't really understand this part - I think you should add some
> documentation or something?

Basically I removed this chunk of code from
ieee80211_vif_change_channel() and put it here - a bit later in the AP
CSA flow.
I don't think it does any harm.

>
>> @@ -453,11 +453,6 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
>>       chanctx_changed |= IEEE80211_CHANCTX_CHANGE_CHANNEL;
>>       drv_change_chanctx(local, ctx, chanctx_changed);
>>
>> -     if (!local->use_chanctx) {
>> -             local->_oper_chandef = *chandef;
>> -             ieee80211_hw_config(local, 0);
>> -     }
>
> I really don't like it either - this was here so that no other code
> really needed to be worried about non-chanctx drivers.

Well right now ieee80211_chswitch_work() takes care of it, and does
something a bit different there to accommodate the legacy behavior -
if the chan_switch op is defined, ieee80211_hw_config is not called.
Would you prefer that ieee80211_vif_change_channel() handle all this,
checking interface type to do the right thing?

>
>> @@ -1180,17 +1196,27 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
>>       }
>>
>>       ifmgd->flags |= IEEE80211_STA_CSA_RECEIVED;
>> +     sdata->vif.csa_active = true;
>
> I don't think we can just do this - this isn't going to result in good
> behaviour for multi-vif drivers and in particular I'm pretty sure with
> the MVM driver this would result in bad behaviour.
>
> If you really want to do this I think it needs to be optional.

I only added it since the current implementation of
ieee80211_vif_change_channel() bails if it's false. That said, I'm not
sure what's wrong here. This setting is per-vif.

>
> Also the documentation for the chanctx channel change probably needs to
> be updated, etc.

Is there any current documentation? The AP CSA patches by Simon didn't
have any IIRC.
I guess I'm not sure what's special here, we just replace the chandef
and that's it.

Arik

Re: [PATCH] mac80211: implement STA CSA for drivers using channel contexts

[Johannes Berg]

On Thu, 2013-08-15 at 19:07 +0300, Arik Nemtsov wrote:
> On Thu, Aug 15, 2013 at 3:03 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> >> @@ -2872,6 +2872,11 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
> >>       if (WARN_ON(err < 0))
> >>               return;
> >>
> >> +     if (!local->use_chanctx) {
> >> +             local->_oper_chandef = local->csa_chandef;
> >> +             ieee80211_hw_config(local, 0);
> >> +     }
> >
> > I don't really understand this part - I think you should add some
> > documentation or something?
> 
> Basically I removed this chunk of code from
> ieee80211_vif_change_channel() and put it here - a bit later in the AP
> CSA flow.
> I don't think it does any harm.

Well, technically not, but I'm trying to reduce the places that touch
the old non-chanctx stuff, not increase them :-)

> >> @@ -453,11 +453,6 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
> >>       chanctx_changed |= IEEE80211_CHANCTX_CHANGE_CHANNEL;
> >>       drv_change_chanctx(local, ctx, chanctx_changed);
> >>
> >> -     if (!local->use_chanctx) {
> >> -             local->_oper_chandef = *chandef;
> >> -             ieee80211_hw_config(local, 0);
> >> -     }
> >
> > I really don't like it either - this was here so that no other code
> > really needed to be worried about non-chanctx drivers.
> 
> Well right now ieee80211_chswitch_work() takes care of it, and does
> something a bit different there to accommodate the legacy behavior -
> if the chan_switch op is defined, ieee80211_hw_config is not called.
> Would you prefer that ieee80211_vif_change_channel() handle all this,
> checking interface type to do the right thing?

Well, it can't. If you look carefully then the old chan_switch op
behaviour is to let the driver switch, not switch in software
afterwards.

> I only added it since the current implementation of
> ieee80211_vif_change_channel() bails if it's false. That said, I'm not
> sure what's wrong here. This setting is per-vif.

Yeah but it's (currently) meant for interfaces controlling the CSA (i.e.
AP only right now) ... so I really think we need to make this
controllable, I think that when we want to implement it for Intel MVM
firmware then we'll let the firmware control the switch timing, etc.
None of this can even be done today or with your patch.

johannes

Re: [PATCH] mac80211: implement STA CSA for drivers using channel contexts

[Arik Nemtsov]

On Fri, Aug 16, 2013 at 1:56 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>> Well right now ieee80211_chswitch_work() takes care of it, and does
>> something a bit different there to accommodate the legacy behavior -
>> if the chan_switch op is defined, ieee80211_hw_config is not called.
>> Would you prefer that ieee80211_vif_change_channel() handle all this,
>> checking interface type to do the right thing?
>
> Well, it can't. If you look carefully then the old chan_switch op
> behaviour is to let the driver switch, not switch in software
> afterwards.

The right thing for chan_switch drivers would be not to call hw_config()..

>
>> I only added it since the current implementation of
>> ieee80211_vif_change_channel() bails if it's false. That said, I'm not
>> sure what's wrong here. This setting is per-vif.
>
> Yeah but it's (currently) meant for interfaces controlling the CSA (i.e.
> AP only right now) ... so I really think we need to make this
> controllable, I think that when we want to implement it for Intel MVM
> firmware then we'll let the firmware control the switch timing, etc.
> None of this can even be done today or with your patch.

The TI driver implements the chan_switch op and uses channel contexts.
If I catch your drift you would rather these kind of drivers (which
include MVM) use a new API exposed by mac80211 to complete the channel
switch. This API would basically be ieee80211_vif_change_channel(). Do
we still need to touch the cfg80211 chandef definition in ifmgd?

The above is maybe cleaner, but it's functionally equivalent to the
solution today - the low level driver decides when the channel switch
is completed, and ieee80211_chswitch_work() is called, which does the
right thing for all cases.
Note that with the above, the channel_contexts + software chan-switch
drivers will still need the kind of code that I wrote. So it would
just lead to replicated code. Or maybe you meant something else?

Also, where would you put csa_active = true (if at all) for a STA
interface? Unlike AP, the trigger here is mac80211 code. So putting it
there seemed appropriate.

Arik

Re: [PATCH] mac80211: implement STA CSA for drivers using channel contexts

[Johannes Berg]

On Fri, 2013-08-16 at 23:09 +0300, Arik Nemtsov wrote:

> > Well, it can't. If you look carefully then the old chan_switch op
> > behaviour is to let the driver switch, not switch in software
> > afterwards.
> 
> The right thing for chan_switch drivers would be not to call hw_config()..

chan_switch? or chanctx?

> > Yeah but it's (currently) meant for interfaces controlling the CSA (i.e.
> > AP only right now) ... so I really think we need to make this
> > controllable, I think that when we want to implement it for Intel MVM
> > firmware then we'll let the firmware control the switch timing, etc.
> > None of this can even be done today or with your patch.
> 
> The TI driver implements the chan_switch op and uses channel contexts.

Huh, ok, that was a combination I didn't think was going to exist, since
the chanswitch API doesn't really tell you what channel context etc.
OTOH, it does give you the vif so you have the chanctx implicitly.

> If I catch your drift you would rather these kind of drivers (which
> include MVM) use a new API exposed by mac80211 to complete the channel
> switch. This API would basically be ieee80211_vif_change_channel(). Do
> we still need to touch the cfg80211 chandef definition in ifmgd?

That's what I was thinking, yes.

> The above is maybe cleaner, but it's functionally equivalent to the
> solution today - the low level driver decides when the channel switch
> is completed, and ieee80211_chswitch_work() is called, which does the
> right thing for all cases.

Right ... I guess that works.

> Note that with the above, the channel_contexts + software chan-switch
> drivers will still need the kind of code that I wrote. So it would
> just lead to replicated code. Or maybe you meant something else?

We have too many possibilities I guess ... I think for MVM I want the
disconnect, not the channel context change in software. You're taking
that possibility away, hence my suggestion of a new hardware flag for it
or so.

> Also, where would you put csa_active = true (if at all) for a STA
> interface? Unlike AP, the trigger here is mac80211 code. So putting it
> there seemed appropriate.

Yeah, I was really just trying to say that current chanctx drivers need
not really expect a chanctx to change channel unless they implement CSA,
but that currently means AP-CSA - basically what I just said above with
taking away the possibility of doing the deauth instead.

johannes

Re: [PATCH] mac80211: implement STA CSA for drivers using channel contexts

[Arik Nemtsov]

On Fri, Aug 23, 2013 at 5:01 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2013-08-16 at 23:09 +0300, Arik Nemtsov wrote:
>
>> > Well, it can't. If you look carefully then the old chan_switch op
>> > behaviour is to let the driver switch, not switch in software
>> > afterwards.
>>
>> The right thing for chan_switch drivers would be not to call hw_config()..
>
> chan_switch? or chanctx?

both. the hw_config(channel) is meaningless for chanctx drivers. The
legacy code for op_chan_switch drivers didn't call hw_config() as
well, assuming they'd already get notified internally by their op.

>> The TI driver implements the chan_switch op and uses channel contexts.
>
> Huh, ok, that was a combination I didn't think was going to exist, since
> the chanswitch API doesn't really tell you what channel context etc.
> OTOH, it does give you the vif so you have the chanctx implicitly.

Yep, it's good enough. The driver gets the chandef from the vif.

>
>> Note that with the above, the channel_contexts + software chan-switch
>> drivers will still need the kind of code that I wrote. So it would
>> just lead to replicated code. Or maybe you meant something else?
>
> We have too many possibilities I guess ... I think for MVM I want the
> disconnect, not the channel context change in software. You're taking
> that possibility away, hence my suggestion of a new hardware flag for it
> or so.

I was thinking it's equivalent to to AP case - currently mac80211 is
the dictator and simply changes the chandef once the lower drv
completes the switch.

Anyway IMHO the simplest approach to handle all the legacy stuff +
chanctx it to keep the current patch as is, and add a mac80211 HW flag
to support it, keeping the deauth option as default.
That's what I think you're suggesting?

>
>> Also, where would you put csa_active = true (if at all) for a STA
>> interface? Unlike AP, the trigger here is mac80211 code. So putting it
>> there seemed appropriate.
>
> Yeah, I was really just trying to say that current chanctx drivers need
> not really expect a chanctx to change channel unless they implement CSA,
> but that currently means AP-CSA - basically what I just said above with
> taking away the possibility of doing the deauth instead.

Let's keep the csa_active as I've used it, and just do a deauth when
the csa-support HW flag is not given?

Arik

Re: [PATCH] mac80211: implement STA CSA for drivers using channel contexts

[Johannes Berg]

On Sun, 2013-08-25 at 16:14 +0300, Arik Nemtsov wrote:

> >> The right thing for chan_switch drivers would be not to call hw_config()..
> >
> > chan_switch? or chanctx?
> 
> both. the hw_config(channel) is meaningless for chanctx drivers. The
> legacy code for op_chan_switch drivers didn't call hw_config() as
> well, assuming they'd already get notified internally by their op.

Right.

> >> The TI driver implements the chan_switch op and uses channel contexts.
> >
> > Huh, ok, that was a combination I didn't think was going to exist, since
> > the chanswitch API doesn't really tell you what channel context etc.
> > OTOH, it does give you the vif so you have the chanctx implicitly.
> 
> Yep, it's good enough. The driver gets the chandef from the vif.

Ok, fair enough.

> >> Note that with the above, the channel_contexts + software chan-switch
> >> drivers will still need the kind of code that I wrote. So it would
> >> just lead to replicated code. Or maybe you meant something else?
> >
> > We have too many possibilities I guess ... I think for MVM I want the
> > disconnect, not the channel context change in software. You're taking
> > that possibility away, hence my suggestion of a new hardware flag for it
> > or so.
> 
> I was thinking it's equivalent to to AP case - currently mac80211 is
> the dictator and simply changes the chandef once the lower drv
> completes the switch.

Yeah but the driver in this case has a choice to not supported it.

> Anyway IMHO the simplest approach to handle all the legacy stuff +
> chanctx it to keep the current patch as is, and add a mac80211 HW flag
> to support it, keeping the deauth option as default.
> That's what I think you're suggesting?

Yeah, that sounds about right.

> >> Also, where would you put csa_active = true (if at all) for a STA
> >> interface? Unlike AP, the trigger here is mac80211 code. So putting it
> >> there seemed appropriate.
> >
> > Yeah, I was really just trying to say that current chanctx drivers need
> > not really expect a chanctx to change channel unless they implement CSA,
> > but that currently means AP-CSA - basically what I just said above with
> > taking away the possibility of doing the deauth instead.
> 
> Let's keep the csa_active as I've used it, and just do a deauth when
> the csa-support HW flag is not given?

Works for me - I wasn't particularly worried about the csa_active
handling per se, but rather what it implied about the calls and driver
support etc.

johannes