From: Luciano Coelho <[email protected]>
Hi,
Some checkpatch warnings slipped through my fingers. :(
In v5:
* fix checkpatch warnings;
In v4:
* add chanctx mode parameter to ieee80211_vif_reserve_chanctx();
* compare the vif's previous BSS width with the reserved new width
instead of comparing it to the chanctx's combined width;
* remove IEEE80211_CHANCTX_RESERVED and the reserved_mode element;
* increase refcount also for "in-place" changes;
* stop queues also before doing an "in-place" change;
* refactor ieee80211_use_reserved_chanctx() a bit to fit "in-place"
better;
In v3:
* promoted to PATCH ;)
* applied comments by Michał (see inlined in the patches);
* removed the "add usage of CS channel reservation for STA" patch,
since it's independent and is not ready;
This series is based on top of mac80211-next/master, without including
my combination check changes. When the combination check patchset
gets applied, I can either send the "merge" of the two features as a
separate patchset or I can modify this patchset accordingly (though I
prefer the former, so I can do things in small steps ;).
--
Cheers,
Luca.
Luciano Coelho (3):
mac80211: split ieee80211_vif_change_channel in two
mac80211: implement chanctx reservation
mac80211: allow reservation of a running chanctx
include/net/mac80211.h | 7 ++
net/mac80211/chan.c | 214 ++++++++++++++++++++++++++++++++++++++++-----
net/mac80211/ieee80211_i.h | 14 +++
3 files changed, 215 insertions(+), 20 deletions(-)
--
1.8.5.3
On Fri, 2014-03-07 at 08:43 +0200, Luca Coelho wrote:
> On Wed, 2014-03-05 at 12:32 +0100, Michal Kazior wrote:
> > On 5 March 2014 12:11, Luca Coelho <[email protected]> wrote:
> > > With single-channel drivers, we need to be able to change a running
> > > chanctx if we want to use chanctx reservation. Not all drivers may be
> > > able to do this, so add a flag that indicates support for it.
[]
> > > diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
[]
> > > @@ -162,6 +162,26 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
> > > }
> > > }
> > >
> > > +static bool ieee80211_chanctx_is_reserved(struct ieee80211_local *local,
> > > + struct ieee80211_chanctx *ctx)
> > > +{
> > > + struct ieee80211_sub_if_data *sdata;
> > > + bool ret = false;
> > > +
> > > + lockdep_assert_held(&local->chanctx_mtx);
> > > + rcu_read_lock();
> > > + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > > + if (sdata->reserved_chanctx == ctx) {
> > > + ret = true;
> > > + goto out;
> > > + }
> > > + }
> > > +
> > > +out:
> > > + rcu_read_unlock();
> > > + return false;
> >
> > `return ret` ;-)
>
> Gack! I'll fix.
trivia: using break is more traditional
static bool ieee80211_chanctx_is_reserved(struct ieee80211_local *local,
struct ieee80211_chanctx *ctx)
{
struct ieee80211_sub_if_data *sdata;
bool reserved = false;
lockdep_assert_held(&local->chanctx_mtx);
rcu_read_lock();
list_for_each_entry_rcu(sdata, &local->interfaces, list) {
if (sdata->reserved_chanctx == ctx) {
reserved = true;
break;
}
}
rcu_read_unlock();
return reserved;
}
On Wed, 2014-03-05 at 12:32 +0100, Michal Kazior wrote:
> On 5 March 2014 12:11, Luca Coelho <[email protected]> wrote:
> > From: Luciano Coelho <[email protected]>
> >
> > With single-channel drivers, we need to be able to change a running
> > chanctx if we want to use chanctx reservation. Not all drivers may be
> > able to do this, so add a flag that indicates support for it.
> >
> > Changing a running chanctx can also be used as an optimization in
> > multi-channel drivers when the context needs to be reserved for future
> > usage.
> >
> > Introduce IEEE80211_CHANCTX_RESERVED chanctx mode to mark a channel as
> > reserved so nobody else can use it (since we know it's going to
> > change). In the future, we may allow several vifs to use the same
> > reservation as long as they plan to use the chanctx on the same
> > future channel.
> >
> > Signed-off-by: Luciano Coelho <[email protected]>
> > ---
> > In v3:
> >
> > * reworded the TODO, slightly;
> >
> > In v4:
> >
> > * remove IEEE80211_CHANCTX_RESERVED and the reserved_mode element;
> > * increase refcount also for "in-place" changes;
> > * stop queues also before doing an "in-place" change;
> > * refactor ieee80211_use_reserved_chanctx() a bit to fit "in-place"
> > better;
> >
> > In v5:
> >
> > * fix checkpatch warnings;
> > ---
> > include/net/mac80211.h | 7 ++++
> > net/mac80211/chan.c | 97 +++++++++++++++++++++++++++++++++++---------------
> > 2 files changed, 75 insertions(+), 29 deletions(-)
> >
> > diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> > index 86faa41..b35c608 100644
> > --- a/include/net/mac80211.h
> > +++ b/include/net/mac80211.h
> > @@ -1553,6 +1553,12 @@ struct ieee80211_tx_control {
> > * for a single active channel while using channel contexts. When support
> > * is not enabled the default action is to disconnect when getting the
> > * CSA frame.
> > + *
> > + * @IEEE80211_HW_CHANGE_RUNNING_CHANCTX: The hardware can change a
> > + * channel context on-the-fly. This is needed for channel switch
> > + * on single-channel hardware. It can also be used as an
> > + * optimization in certain channel switch cases with
> > + * multi-channel.
> > */
> > enum ieee80211_hw_flags {
> > IEEE80211_HW_HAS_RATE_CONTROL = 1<<0,
> > @@ -1584,6 +1590,7 @@ enum ieee80211_hw_flags {
> > IEEE80211_HW_TIMING_BEACON_ONLY = 1<<26,
> > IEEE80211_HW_SUPPORTS_HT_CCK_RATES = 1<<27,
> > IEEE80211_HW_CHANCTX_STA_CSA = 1<<28,
> > + IEEE80211_HW_CHANGE_RUNNING_CHANCTX = 1<<29,
> > };
> >
> > /**
> > diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
> > index 9dfdba5..d634f41 100644
> > --- a/net/mac80211/chan.c
> > +++ b/net/mac80211/chan.c
> > @@ -162,6 +162,26 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
> > }
> > }
> >
> > +static bool ieee80211_chanctx_is_reserved(struct ieee80211_local *local,
> > + struct ieee80211_chanctx *ctx)
> > +{
> > + struct ieee80211_sub_if_data *sdata;
> > + bool ret = false;
> > +
> > + lockdep_assert_held(&local->chanctx_mtx);
> > + rcu_read_lock();
> > + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > + if (sdata->reserved_chanctx == ctx) {
> > + ret = true;
> > + goto out;
> > + }
> > + }
> > +
> > +out:
> > + rcu_read_unlock();
> > + return false;
>
> `return ret` ;-)
Gack! I'll fix.
> It's probably a good idea to check ieee80211_sdata_running() before
> even considering reserved_chanctx?
Yes, good point. reserved->chanctx should not be set if the sdata is
not running, but for consistency, if nothing else, I'll add the check
here.
>
> > +}
> > +
> > static struct ieee80211_chanctx *
> > ieee80211_find_chanctx(struct ieee80211_local *local,
> > const struct cfg80211_chan_def *chandef,
> > @@ -177,7 +197,11 @@ ieee80211_find_chanctx(struct ieee80211_local *local,
> > list_for_each_entry(ctx, &local->chanctx_list, list) {
> > const struct cfg80211_chan_def *compat;
> >
> > - if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE)
> > + /* We don't support chanctx reservation for multiple
> > + * vifs yet, so don't allow reserved chanctxs to be
> > + * reused.
> > + */
> > + if (ieee80211_chanctx_is_reserved(local, ctx))
> > continue;
>
> Are you really sure you want to drop the ctx->mode == EXCLUSIVE check here?
Nope, I don't. I should teach myself to be more careful when I'm trying
to multitask. :\
>
> > - /* reserve the new or existing context */
> > sdata->reserved_chanctx = new_ctx;
> > new_ctx->refcount++;
> > -
> > sdata->reserved_chandef = *chandef;
>
> Shouldn't this be in the [2/3]?
Yeah.
>
> > out:
> > mutex_unlock(&local->chanctx_mtx);
> > @@ -703,37 +734,45 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
> > ieee80211_stop_queues_by_reason(&local->hw,
> > IEEE80211_MAX_QUEUE_MAP,
> > IEEE80211_QUEUE_STOP_REASON_CHANCTX);
> > + /* unref our reservation */
> > + ctx->refcount--;
> > + sdata->reserved_chanctx = NULL;
> >
> > - ieee80211_unassign_vif_chanctx(sdata, old_ctx);
> > - if (old_ctx->refcount == 0)
> > - ieee80211_free_chanctx(local, old_ctx);
> > + if (old_ctx == ctx) {
> > + /* This is our own context, just change it */
> > + ret = __ieee80211_vif_change_channel(sdata, old_ctx,
> > + &local_changed);
> > + if (ret)
> > + goto out;
> > + } else {
> > + ieee80211_unassign_vif_chanctx(sdata, old_ctx);
> > + if (old_ctx->refcount == 0)
> > + ieee80211_free_chanctx(local, old_ctx);
> >
> > - if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
> > - local_changed |= BSS_CHANGED_BANDWIDTH;
> > + if (sdata->vif.bss_conf.chandef.width !=
> > + sdata->reserved_chandef.width)
> > + local_changed |= BSS_CHANGED_BANDWIDTH;
> >
> > - sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
> > + sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
> >
> > - /* unref our reservation before assigning */
> > - ctx->refcount--;
> > - sdata->reserved_chanctx = NULL;
> > - ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> > - if (ret) {
> > - /* if assign fails refcount stays the same */
> > - if (ctx->refcount == 0)
> > - ieee80211_free_chanctx(local, ctx);
> > - goto out_wake;
> > + ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> > + if (ret) {
> > + /* if assign fails refcount stays the same */
> > + if (ctx->refcount == 0)
> > + ieee80211_free_chanctx(local, ctx);
> > + goto out;
> > + }
> > +
> > + ieee80211_recalc_chanctx_chantype(local, ctx);
> > + ieee80211_recalc_smps_chanctx(local, ctx);
> > + ieee80211_recalc_radar_chanctx(local, ctx);
> > }
>
> Not really sure if you need to `else` and re-indent the whole thing
> because you already do a `goto` in the `if`..
No, I don't do a goto in the 'if', unless
__ieee80211_vif_change_channel() fails. I just reckoned this would be a
bit cleaner like this.
> > *changed = local_changed;
> > -
> > - ieee80211_recalc_chanctx_chantype(local, ctx);
> > - ieee80211_recalc_smps_chanctx(local, ctx);
> > - ieee80211_recalc_radar_chanctx(local, ctx);
> > -out_wake:
> > +out:
> > ieee80211_wake_queues_by_reason(&sdata->local->hw,
> > IEEE80211_MAX_QUEUE_MAP,
> > IEEE80211_QUEUE_STOP_REASON_CHANCTX);
> > -out:
> > mutex_unlock(&local->chanctx_mtx);
> > return ret;
> > }
>
> Are you sure you want to remove the `out_wake` from here? Why not in the [2/3]?
Well, this patch is somewhat refactoring this function, so I think it's
okay to have this here as well.
--
Luca.
On March 7, 2014 10:19:49 PM GMT+02:00, Joe Perches <[email protected]> wrote:
>On Fri, 2014-03-07 at 08:43 +0200, Luca Coelho wrote:
>> On Wed, 2014-03-05 at 12:32 +0100, Michal Kazior wrote:
>> > On 5 March 2014 12:11, Luca Coelho <[email protected]> wrote:
>> > > With single-channel drivers, we need to be able to change a
>running
>> > > chanctx if we want to use chanctx reservation. Not all drivers
>may be
>> > > able to do this, so add a flag that indicates support for it.
>[]
>> > > diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
>[]
>> > > @@ -162,6 +162,26 @@ static void ieee80211_change_chanctx(struct
>ieee80211_local *local,
>> > > }
>> > > }
>> > >
>> > > +static bool ieee80211_chanctx_is_reserved(struct ieee80211_local
>*local,
>> > > + struct ieee80211_chanctx
>*ctx)
>> > > +{
>> > > + struct ieee80211_sub_if_data *sdata;
>> > > + bool ret = false;
>> > > +
>> > > + lockdep_assert_held(&local->chanctx_mtx);
>> > > + rcu_read_lock();
>> > > + list_for_each_entry_rcu(sdata, &local->interfaces, list)
>{
>> > > + if (sdata->reserved_chanctx == ctx) {
>> > > + ret = true;
>> > > + goto out;
>> > > + }
>> > > + }
>> > > +
>> > > +out:
>> > > + rcu_read_unlock();
>> > > + return false;
>> >
>> > `return ret` ;-)
>>
>> Gack! I'll fix.
>
>trivia: using break is more traditional
Yeah, good point. I'll change to break.
--
Luca.
On Wed, 2014-03-05 at 13:04 +0100, Michal Kazior wrote:
> On 5 March 2014 12:11, Luca Coelho <[email protected]> wrote:
>
> [...]
>
> > + /* unref our reservation before assigning */
> > + ctx->refcount--;
> > + sdata->reserved_chanctx = NULL;
> > + ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> > + if (ret) {
> > + /* if assign fails refcount stays the same */
> > + if (ctx->refcount == 0)
> > + ieee80211_free_chanctx(local, ctx);
> > + goto out_wake;
> > + }
>
> This actually won't work if there are AP VLANs (I noticed that yesterday).
>
> You can't call ieee80211_vif_copy_chanctx_to_vlans() here due to
> current locking requirements. I have a patch for that though ;-) I'll
> send it soon.
How is this related to this patch?
Anyway, good to know that you have fixed it. :)
--
Luca.
On 5 March 2014 12:11, Luca Coelho <[email protected]> wrote:
[...]
> + /* unref our reservation before assigning */
> + ctx->refcount--;
> + sdata->reserved_chanctx = NULL;
> + ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> + if (ret) {
> + /* if assign fails refcount stays the same */
> + if (ctx->refcount == 0)
> + ieee80211_free_chanctx(local, ctx);
> + goto out_wake;
> + }
This actually won't work if there are AP VLANs (I noticed that yesterday).
You can't call ieee80211_vif_copy_chanctx_to_vlans() here due to
current locking requirements. I have a patch for that though ;-) I'll
send it soon.
Michał
On 5 March 2014 12:11, Luca Coelho <[email protected]> wrote:
> From: Luciano Coelho <[email protected]>
>
> With single-channel drivers, we need to be able to change a running
> chanctx if we want to use chanctx reservation. Not all drivers may be
> able to do this, so add a flag that indicates support for it.
>
> Changing a running chanctx can also be used as an optimization in
> multi-channel drivers when the context needs to be reserved for future
> usage.
>
> Introduce IEEE80211_CHANCTX_RESERVED chanctx mode to mark a channel as
> reserved so nobody else can use it (since we know it's going to
> change). In the future, we may allow several vifs to use the same
> reservation as long as they plan to use the chanctx on the same
> future channel.
>
> Signed-off-by: Luciano Coelho <[email protected]>
> ---
> In v3:
>
> * reworded the TODO, slightly;
>
> In v4:
>
> * remove IEEE80211_CHANCTX_RESERVED and the reserved_mode element;
> * increase refcount also for "in-place" changes;
> * stop queues also before doing an "in-place" change;
> * refactor ieee80211_use_reserved_chanctx() a bit to fit "in-place"
> better;
>
> In v5:
>
> * fix checkpatch warnings;
> ---
> include/net/mac80211.h | 7 ++++
> net/mac80211/chan.c | 97 +++++++++++++++++++++++++++++++++++---------------
> 2 files changed, 75 insertions(+), 29 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 86faa41..b35c608 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1553,6 +1553,12 @@ struct ieee80211_tx_control {
> * for a single active channel while using channel contexts. When support
> * is not enabled the default action is to disconnect when getting the
> * CSA frame.
> + *
> + * @IEEE80211_HW_CHANGE_RUNNING_CHANCTX: The hardware can change a
> + * channel context on-the-fly. This is needed for channel switch
> + * on single-channel hardware. It can also be used as an
> + * optimization in certain channel switch cases with
> + * multi-channel.
> */
> enum ieee80211_hw_flags {
> IEEE80211_HW_HAS_RATE_CONTROL = 1<<0,
> @@ -1584,6 +1590,7 @@ enum ieee80211_hw_flags {
> IEEE80211_HW_TIMING_BEACON_ONLY = 1<<26,
> IEEE80211_HW_SUPPORTS_HT_CCK_RATES = 1<<27,
> IEEE80211_HW_CHANCTX_STA_CSA = 1<<28,
> + IEEE80211_HW_CHANGE_RUNNING_CHANCTX = 1<<29,
> };
>
> /**
> diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
> index 9dfdba5..d634f41 100644
> --- a/net/mac80211/chan.c
> +++ b/net/mac80211/chan.c
> @@ -162,6 +162,26 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
> }
> }
>
> +static bool ieee80211_chanctx_is_reserved(struct ieee80211_local *local,
> + struct ieee80211_chanctx *ctx)
> +{
> + struct ieee80211_sub_if_data *sdata;
> + bool ret = false;
> +
> + lockdep_assert_held(&local->chanctx_mtx);
> + rcu_read_lock();
> + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> + if (sdata->reserved_chanctx == ctx) {
> + ret = true;
> + goto out;
> + }
> + }
> +
> +out:
> + rcu_read_unlock();
> + return false;
`return ret` ;-)
It's probably a good idea to check ieee80211_sdata_running() before
even considering reserved_chanctx?
> +}
> +
> static struct ieee80211_chanctx *
> ieee80211_find_chanctx(struct ieee80211_local *local,
> const struct cfg80211_chan_def *chandef,
> @@ -177,7 +197,11 @@ ieee80211_find_chanctx(struct ieee80211_local *local,
> list_for_each_entry(ctx, &local->chanctx_list, list) {
> const struct cfg80211_chan_def *compat;
>
> - if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE)
> + /* We don't support chanctx reservation for multiple
> + * vifs yet, so don't allow reserved chanctxs to be
> + * reused.
> + */
> + if (ieee80211_chanctx_is_reserved(local, ctx))
> continue;
Are you really sure you want to drop the ctx->mode == EXCLUSIVE check here?
> - /* reserve the new or existing context */
> sdata->reserved_chanctx = new_ctx;
> new_ctx->refcount++;
> -
> sdata->reserved_chandef = *chandef;
Shouldn't this be in the [2/3]?
> out:
> mutex_unlock(&local->chanctx_mtx);
> @@ -703,37 +734,45 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
> ieee80211_stop_queues_by_reason(&local->hw,
> IEEE80211_MAX_QUEUE_MAP,
> IEEE80211_QUEUE_STOP_REASON_CHANCTX);
> + /* unref our reservation */
> + ctx->refcount--;
> + sdata->reserved_chanctx = NULL;
>
> - ieee80211_unassign_vif_chanctx(sdata, old_ctx);
> - if (old_ctx->refcount == 0)
> - ieee80211_free_chanctx(local, old_ctx);
> + if (old_ctx == ctx) {
> + /* This is our own context, just change it */
> + ret = __ieee80211_vif_change_channel(sdata, old_ctx,
> + &local_changed);
> + if (ret)
> + goto out;
> + } else {
> + ieee80211_unassign_vif_chanctx(sdata, old_ctx);
> + if (old_ctx->refcount == 0)
> + ieee80211_free_chanctx(local, old_ctx);
>
> - if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
> - local_changed |= BSS_CHANGED_BANDWIDTH;
> + if (sdata->vif.bss_conf.chandef.width !=
> + sdata->reserved_chandef.width)
> + local_changed |= BSS_CHANGED_BANDWIDTH;
>
> - sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
> + sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
>
> - /* unref our reservation before assigning */
> - ctx->refcount--;
> - sdata->reserved_chanctx = NULL;
> - ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> - if (ret) {
> - /* if assign fails refcount stays the same */
> - if (ctx->refcount == 0)
> - ieee80211_free_chanctx(local, ctx);
> - goto out_wake;
> + ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> + if (ret) {
> + /* if assign fails refcount stays the same */
> + if (ctx->refcount == 0)
> + ieee80211_free_chanctx(local, ctx);
> + goto out;
> + }
> +
> + ieee80211_recalc_chanctx_chantype(local, ctx);
> + ieee80211_recalc_smps_chanctx(local, ctx);
> + ieee80211_recalc_radar_chanctx(local, ctx);
> }
Not really sure if you need to `else` and re-indent the whole thing
because you already do a `goto` in the `if`..
> *changed = local_changed;
> -
> - ieee80211_recalc_chanctx_chantype(local, ctx);
> - ieee80211_recalc_smps_chanctx(local, ctx);
> - ieee80211_recalc_radar_chanctx(local, ctx);
> -out_wake:
> +out:
> ieee80211_wake_queues_by_reason(&sdata->local->hw,
> IEEE80211_MAX_QUEUE_MAP,
> IEEE80211_QUEUE_STOP_REASON_CHANCTX);
> -out:
> mutex_unlock(&local->chanctx_mtx);
> return ret;
> }
Are you sure you want to remove the `out_wake` from here? Why not in the [2/3]?
Michał
On 7 March 2014 07:48, Luca Coelho <[email protected]> wrote:
> On Wed, 2014-03-05 at 13:04 +0100, Michal Kazior wrote:
>> On 5 March 2014 12:11, Luca Coelho <[email protected]> wrote:
>>
>> [...]
>>
>> > + /* unref our reservation before assigning */
>> > + ctx->refcount--;
>> > + sdata->reserved_chanctx = NULL;
>> > + ret = ieee80211_assign_vif_chanctx(sdata, ctx);
>> > + if (ret) {
>> > + /* if assign fails refcount stays the same */
>> > + if (ctx->refcount == 0)
>> > + ieee80211_free_chanctx(local, ctx);
>> > + goto out_wake;
>> > + }
>>
>> This actually won't work if there are AP VLANs (I noticed that yesterday).
>>
>> You can't call ieee80211_vif_copy_chanctx_to_vlans() here due to
>> current locking requirements. I have a patch for that though ;-) I'll
>> send it soon.
>
> How is this related to this patch?
>
> Anyway, good to know that you have fixed it. :)
If you re-assign a chanctx of an AP that has VLANs you leave VLANs
with the old chanctx pointer. Those pointers should be updated or else
you'll end up dereferencing an invalid pointer.
Michał
From: Luciano Coelho <[email protected]>
ieee80211_vif_change_channel() locks chanctx_mtx. When implementing
channel reservation for CS, we will need to call the function to
change channel when the lock is already held, so split the part that
requires the lock out and leave the locking in the original function.
Signed-off-by: Luciano Coelho <[email protected]>
---
net/mac80211/chan.c | 58 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 33 insertions(+), 25 deletions(-)
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 42c6592..a27c6ec 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -547,39 +547,20 @@ int ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
return ret;
}
-int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
- u32 *changed)
+static int __ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_chanctx *ctx,
+ u32 *changed)
{
struct ieee80211_local *local = sdata->local;
- struct ieee80211_chanctx_conf *conf;
- struct ieee80211_chanctx *ctx;
const struct cfg80211_chan_def *chandef = &sdata->csa_chandef;
- int ret;
u32 chanctx_changed = 0;
- lockdep_assert_held(&local->mtx);
-
- /* should never be called if not performing a channel switch. */
- if (WARN_ON(!sdata->vif.csa_active))
- return -EINVAL;
-
if (!cfg80211_chandef_usable(sdata->local->hw.wiphy, chandef,
IEEE80211_CHAN_DISABLED))
return -EINVAL;
- mutex_lock(&local->chanctx_mtx);
- conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
- lockdep_is_held(&local->chanctx_mtx));
- if (!conf) {
- ret = -EINVAL;
- goto out;
- }
-
- ctx = container_of(conf, struct ieee80211_chanctx, conf);
- if (ctx->refcount != 1) {
- ret = -EINVAL;
- goto out;
- }
+ if (ctx->refcount != 1)
+ return -EINVAL;
if (sdata->vif.bss_conf.chandef.width != chandef->width) {
chanctx_changed = IEEE80211_CHANCTX_CHANGE_WIDTH;
@@ -597,7 +578,34 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
ieee80211_recalc_radar_chanctx(local, ctx);
ieee80211_recalc_chanctx_min_def(local, ctx);
- ret = 0;
+ return 0;
+}
+
+int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
+ u32 *changed)
+{
+ struct ieee80211_local *local = sdata->local;
+ struct ieee80211_chanctx_conf *conf;
+ struct ieee80211_chanctx *ctx;
+ int ret;
+
+ lockdep_assert_held(&local->mtx);
+
+ /* should never be called if not performing a channel switch. */
+ if (WARN_ON(!sdata->vif.csa_active))
+ return -EINVAL;
+
+ mutex_lock(&local->chanctx_mtx);
+ conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+ lockdep_is_held(&local->chanctx_mtx));
+ if (!conf) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ctx = container_of(conf, struct ieee80211_chanctx, conf);
+
+ ret = __ieee80211_vif_change_channel(sdata, ctx, changed);
out:
mutex_unlock(&local->chanctx_mtx);
return ret;
--
1.8.5.3
From: Luciano Coelho <[email protected]>
With single-channel drivers, we need to be able to change a running
chanctx if we want to use chanctx reservation. Not all drivers may be
able to do this, so add a flag that indicates support for it.
Changing a running chanctx can also be used as an optimization in
multi-channel drivers when the context needs to be reserved for future
usage.
Introduce IEEE80211_CHANCTX_RESERVED chanctx mode to mark a channel as
reserved so nobody else can use it (since we know it's going to
change). In the future, we may allow several vifs to use the same
reservation as long as they plan to use the chanctx on the same
future channel.
Signed-off-by: Luciano Coelho <[email protected]>
---
In v3:
* reworded the TODO, slightly;
In v4:
* remove IEEE80211_CHANCTX_RESERVED and the reserved_mode element;
* increase refcount also for "in-place" changes;
* stop queues also before doing an "in-place" change;
* refactor ieee80211_use_reserved_chanctx() a bit to fit "in-place"
better;
In v5:
* fix checkpatch warnings;
---
include/net/mac80211.h | 7 ++++
net/mac80211/chan.c | 97 +++++++++++++++++++++++++++++++++++---------------
2 files changed, 75 insertions(+), 29 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 86faa41..b35c608 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1553,6 +1553,12 @@ struct ieee80211_tx_control {
* for a single active channel while using channel contexts. When support
* is not enabled the default action is to disconnect when getting the
* CSA frame.
+ *
+ * @IEEE80211_HW_CHANGE_RUNNING_CHANCTX: The hardware can change a
+ * channel context on-the-fly. This is needed for channel switch
+ * on single-channel hardware. It can also be used as an
+ * optimization in certain channel switch cases with
+ * multi-channel.
*/
enum ieee80211_hw_flags {
IEEE80211_HW_HAS_RATE_CONTROL = 1<<0,
@@ -1584,6 +1590,7 @@ enum ieee80211_hw_flags {
IEEE80211_HW_TIMING_BEACON_ONLY = 1<<26,
IEEE80211_HW_SUPPORTS_HT_CCK_RATES = 1<<27,
IEEE80211_HW_CHANCTX_STA_CSA = 1<<28,
+ IEEE80211_HW_CHANGE_RUNNING_CHANCTX = 1<<29,
};
/**
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 9dfdba5..d634f41 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -162,6 +162,26 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
}
}
+static bool ieee80211_chanctx_is_reserved(struct ieee80211_local *local,
+ struct ieee80211_chanctx *ctx)
+{
+ struct ieee80211_sub_if_data *sdata;
+ bool ret = false;
+
+ lockdep_assert_held(&local->chanctx_mtx);
+ rcu_read_lock();
+ list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+ if (sdata->reserved_chanctx == ctx) {
+ ret = true;
+ goto out;
+ }
+ }
+
+out:
+ rcu_read_unlock();
+ return false;
+}
+
static struct ieee80211_chanctx *
ieee80211_find_chanctx(struct ieee80211_local *local,
const struct cfg80211_chan_def *chandef,
@@ -177,7 +197,11 @@ ieee80211_find_chanctx(struct ieee80211_local *local,
list_for_each_entry(ctx, &local->chanctx_list, list) {
const struct cfg80211_chan_def *compat;
- if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE)
+ /* We don't support chanctx reservation for multiple
+ * vifs yet, so don't allow reserved chanctxs to be
+ * reused.
+ */
+ if (ieee80211_chanctx_is_reserved(local, ctx))
continue;
compat = cfg80211_chandef_compatible(&ctx->conf.def, chandef);
@@ -652,18 +676,25 @@ int ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
/* try to find another context with the chandef we want */
new_ctx = ieee80211_find_chanctx(local, chandef, mode);
if (!new_ctx) {
- /* create a new context */
- new_ctx = ieee80211_new_chanctx(local, chandef, mode);
- if (IS_ERR(new_ctx)) {
- ret = PTR_ERR(new_ctx);
- goto out;
+ if (curr_ctx->refcount == 1 &&
+ (local->hw.flags & IEEE80211_HW_CHANGE_RUNNING_CHANCTX)) {
+ /* if we're the only users of the chanctx and
+ * the driver supports changing a running
+ * context, reserve our current context
+ */
+ new_ctx = curr_ctx;
+ } else {
+ /* create a new context and reserve it */
+ new_ctx = ieee80211_new_chanctx(local, chandef, mode);
+ if (IS_ERR(new_ctx)) {
+ ret = PTR_ERR(new_ctx);
+ goto out;
+ }
}
}
- /* reserve the new or existing context */
sdata->reserved_chanctx = new_ctx;
new_ctx->refcount++;
-
sdata->reserved_chandef = *chandef;
out:
mutex_unlock(&local->chanctx_mtx);
@@ -703,37 +734,45 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
ieee80211_stop_queues_by_reason(&local->hw,
IEEE80211_MAX_QUEUE_MAP,
IEEE80211_QUEUE_STOP_REASON_CHANCTX);
+ /* unref our reservation */
+ ctx->refcount--;
+ sdata->reserved_chanctx = NULL;
- ieee80211_unassign_vif_chanctx(sdata, old_ctx);
- if (old_ctx->refcount == 0)
- ieee80211_free_chanctx(local, old_ctx);
+ if (old_ctx == ctx) {
+ /* This is our own context, just change it */
+ ret = __ieee80211_vif_change_channel(sdata, old_ctx,
+ &local_changed);
+ if (ret)
+ goto out;
+ } else {
+ ieee80211_unassign_vif_chanctx(sdata, old_ctx);
+ if (old_ctx->refcount == 0)
+ ieee80211_free_chanctx(local, old_ctx);
- if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
- local_changed |= BSS_CHANGED_BANDWIDTH;
+ if (sdata->vif.bss_conf.chandef.width !=
+ sdata->reserved_chandef.width)
+ local_changed |= BSS_CHANGED_BANDWIDTH;
- sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
+ sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
- /* unref our reservation before assigning */
- ctx->refcount--;
- sdata->reserved_chanctx = NULL;
- ret = ieee80211_assign_vif_chanctx(sdata, ctx);
- if (ret) {
- /* if assign fails refcount stays the same */
- if (ctx->refcount == 0)
- ieee80211_free_chanctx(local, ctx);
- goto out_wake;
+ ret = ieee80211_assign_vif_chanctx(sdata, ctx);
+ if (ret) {
+ /* if assign fails refcount stays the same */
+ if (ctx->refcount == 0)
+ ieee80211_free_chanctx(local, ctx);
+ goto out;
+ }
+
+ ieee80211_recalc_chanctx_chantype(local, ctx);
+ ieee80211_recalc_smps_chanctx(local, ctx);
+ ieee80211_recalc_radar_chanctx(local, ctx);
}
*changed = local_changed;
-
- ieee80211_recalc_chanctx_chantype(local, ctx);
- ieee80211_recalc_smps_chanctx(local, ctx);
- ieee80211_recalc_radar_chanctx(local, ctx);
-out_wake:
+out:
ieee80211_wake_queues_by_reason(&sdata->local->hw,
IEEE80211_MAX_QUEUE_MAP,
IEEE80211_QUEUE_STOP_REASON_CHANCTX);
-out:
mutex_unlock(&local->chanctx_mtx);
return ret;
}
--
1.8.5.3
From: Luciano Coelho <[email protected]>
In order to support channel switch with multiple vifs and multiple
contexts, we implement a concept of channel context reservation. This
allows us to reserve a channel context to be used later.
The reservation functionality is not tied directly to channel switch
and may be used in other situations (eg. reserving a channel context
during IBSS join).
We first check if an existing compatible context exists and if it
does, we reserve it. If there is no compatible context we create a
new one and reserve it.
Signed-off-by: Luciano Coelho <[email protected]>
---
In RFC v2:
* reformulated the patch to make channel reservation generic and
not only for channel switch, as we may find other uses for it in
the future;
* added ieee80211_vif_unreserve_chanctx();
* removed reservation of the current chanctx from this patch;
* added a stop_queue_reason for chanctx reservation;
* added a few TODOs according to Michal's comments to be handled in
future patch series;
* set sdata->csa_chandef to the received chandef in all cases (it
was only happening in the alone-in-the-vif case;
In v3:
* removed stray empty line;
* reworded the commit message;
* added reserved_chandef;
* added comment about reserved_chanctx and reserved_chandef protection;
* wake the queues if ieee80211_assign_vif_chanctx() fails;
In v4:
* add chanctx mode parameter to ieee80211_vif_reserve_chanctx();
* compare the vif's previous BSS width with the reserved new width
instead of comparing it to the chanctx's combined width;
---
net/mac80211/chan.c | 127 +++++++++++++++++++++++++++++++++++++++++++++
net/mac80211/ieee80211_i.h | 14 +++++
2 files changed, 141 insertions(+)
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index a27c6ec..9dfdba5 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -419,6 +419,9 @@ static void __ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
ctx = container_of(conf, struct ieee80211_chanctx, conf);
+ if (sdata->reserved_chanctx)
+ ieee80211_vif_unreserve_chanctx(sdata);
+
ieee80211_unassign_vif_chanctx(sdata, ctx);
if (ctx->refcount == 0)
ieee80211_free_chanctx(local, ctx);
@@ -611,6 +614,130 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
return ret;
}
+int ieee80211_vif_unreserve_chanctx(struct ieee80211_sub_if_data *sdata)
+{
+ lockdep_assert_held(&sdata->local->chanctx_mtx);
+
+ if (WARN_ON(!sdata->reserved_chanctx))
+ return -EINVAL;
+
+ if (--sdata->reserved_chanctx->refcount == 0)
+ ieee80211_free_chanctx(sdata->local, sdata->reserved_chanctx);
+
+ sdata->reserved_chanctx = NULL;
+
+ return 0;
+}
+
+int ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
+ const struct cfg80211_chan_def *chandef,
+ enum ieee80211_chanctx_mode mode)
+{
+ struct ieee80211_local *local = sdata->local;
+ struct ieee80211_chanctx_conf *conf;
+ struct ieee80211_chanctx *new_ctx, *curr_ctx;
+ int ret = 0;
+
+ mutex_lock(&local->chanctx_mtx);
+
+ conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+ lockdep_is_held(&local->chanctx_mtx));
+ if (!conf) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ curr_ctx = container_of(conf, struct ieee80211_chanctx, conf);
+
+ /* try to find another context with the chandef we want */
+ new_ctx = ieee80211_find_chanctx(local, chandef, mode);
+ if (!new_ctx) {
+ /* create a new context */
+ new_ctx = ieee80211_new_chanctx(local, chandef, mode);
+ if (IS_ERR(new_ctx)) {
+ ret = PTR_ERR(new_ctx);
+ goto out;
+ }
+ }
+
+ /* reserve the new or existing context */
+ sdata->reserved_chanctx = new_ctx;
+ new_ctx->refcount++;
+
+ sdata->reserved_chandef = *chandef;
+out:
+ mutex_unlock(&local->chanctx_mtx);
+ return ret;
+}
+
+int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
+ u32 *changed)
+{
+ struct ieee80211_local *local = sdata->local;
+ struct ieee80211_chanctx *ctx;
+ struct ieee80211_chanctx *old_ctx;
+ struct ieee80211_chanctx_conf *conf;
+ int ret, local_changed = *changed;
+
+ /* TODO: need to recheck if the chandef is usable etc.? */
+
+ lockdep_assert_held(&local->mtx);
+
+ mutex_lock(&local->chanctx_mtx);
+
+ ctx = sdata->reserved_chanctx;
+ if (WARN_ON(!ctx)) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+ lockdep_is_held(&local->chanctx_mtx));
+ if (!conf) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ old_ctx = container_of(conf, struct ieee80211_chanctx, conf);
+
+ ieee80211_stop_queues_by_reason(&local->hw,
+ IEEE80211_MAX_QUEUE_MAP,
+ IEEE80211_QUEUE_STOP_REASON_CHANCTX);
+
+ ieee80211_unassign_vif_chanctx(sdata, old_ctx);
+ if (old_ctx->refcount == 0)
+ ieee80211_free_chanctx(local, old_ctx);
+
+ if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
+ local_changed |= BSS_CHANGED_BANDWIDTH;
+
+ sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
+
+ /* unref our reservation before assigning */
+ ctx->refcount--;
+ sdata->reserved_chanctx = NULL;
+ ret = ieee80211_assign_vif_chanctx(sdata, ctx);
+ if (ret) {
+ /* if assign fails refcount stays the same */
+ if (ctx->refcount == 0)
+ ieee80211_free_chanctx(local, ctx);
+ goto out_wake;
+ }
+
+ *changed = local_changed;
+
+ ieee80211_recalc_chanctx_chantype(local, ctx);
+ ieee80211_recalc_smps_chanctx(local, ctx);
+ ieee80211_recalc_radar_chanctx(local, ctx);
+out_wake:
+ ieee80211_wake_queues_by_reason(&sdata->local->hw,
+ IEEE80211_MAX_QUEUE_MAP,
+ IEEE80211_QUEUE_STOP_REASON_CHANCTX);
+out:
+ mutex_unlock(&local->chanctx_mtx);
+ return ret;
+}
+
int ieee80211_vif_change_bandwidth(struct ieee80211_sub_if_data *sdata,
const struct cfg80211_chan_def *chandef,
u32 *changed)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 8603dfb..ae8acc2 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -756,6 +756,10 @@ struct ieee80211_sub_if_data {
bool csa_radar_required;
struct cfg80211_chan_def csa_chandef;
+ /* context reservation -- protected with chanctx_mtx */
+ struct ieee80211_chanctx *reserved_chanctx;
+ struct cfg80211_chan_def reserved_chandef;
+
/* used to reconfigure hardware SM PS */
struct work_struct recalc_smps;
@@ -899,6 +903,7 @@ enum queue_stop_reason {
IEEE80211_QUEUE_STOP_REASON_SKB_ADD,
IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL,
IEEE80211_QUEUE_STOP_REASON_FLUSH,
+ IEEE80211_QUEUE_STOP_REASON_CHANCTX,
};
#ifdef CONFIG_MAC80211_LEDS
@@ -1776,6 +1781,15 @@ ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
const struct cfg80211_chan_def *chandef,
enum ieee80211_chanctx_mode mode);
int __must_check
+ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
+ const struct cfg80211_chan_def *chandef,
+ enum ieee80211_chanctx_mode mode);
+int __must_check
+ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
+ u32 *changed);
+int ieee80211_vif_unreserve_chanctx(struct ieee80211_sub_if_data *sdata);
+
+int __must_check
ieee80211_vif_change_bandwidth(struct ieee80211_sub_if_data *sdata,
const struct cfg80211_chan_def *chandef,
u32 *changed);
--
1.8.5.3
On Mon, 2014-03-10 at 08:03 +0100, Michal Kazior wrote:
> On 7 March 2014 07:48, Luca Coelho <[email protected]> wrote:
> > On Wed, 2014-03-05 at 13:04 +0100, Michal Kazior wrote:
> >> On 5 March 2014 12:11, Luca Coelho <[email protected]> wrote:
> >>
> >> [...]
> >>
> >> > + /* unref our reservation before assigning */
> >> > + ctx->refcount--;
> >> > + sdata->reserved_chanctx = NULL;
> >> > + ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> >> > + if (ret) {
> >> > + /* if assign fails refcount stays the same */
> >> > + if (ctx->refcount == 0)
> >> > + ieee80211_free_chanctx(local, ctx);
> >> > + goto out_wake;
> >> > + }
> >>
> >> This actually won't work if there are AP VLANs (I noticed that yesterday).
> >>
> >> You can't call ieee80211_vif_copy_chanctx_to_vlans() here due to
> >> current locking requirements. I have a patch for that though ;-) I'll
> >> send it soon.
> >
> > How is this related to this patch?
> >
> > Anyway, good to know that you have fixed it. :)
>
> If you re-assign a chanctx of an AP that has VLANs you leave VLANs
> with the old chanctx pointer. Those pointers should be updated or else
> you'll end up dereferencing an invalid pointer.
I see. So, I need to call ieee80211_vif_copy_chanctx_to_vlans() here,
but to do that I need your patch that allows it to be called without the
RTNL.
So, now this series will depend on that patch, I'll talk to Johannes.
--
Luca.
On 10 March 2014 10:08, Luca Coelho <[email protected]> wrote:
> On Mon, 2014-03-10 at 08:03 +0100, Michal Kazior wrote:
>> On 7 March 2014 07:48, Luca Coelho <[email protected]> wrote:
>> > On Wed, 2014-03-05 at 13:04 +0100, Michal Kazior wrote:
>> >> On 5 March 2014 12:11, Luca Coelho <[email protected]> wrote:
>> >>
>> >> [...]
>> >>
>> >> > + /* unref our reservation before assigning */
>> >> > + ctx->refcount--;
>> >> > + sdata->reserved_chanctx = NULL;
>> >> > + ret = ieee80211_assign_vif_chanctx(sdata, ctx);
>> >> > + if (ret) {
>> >> > + /* if assign fails refcount stays the same */
>> >> > + if (ctx->refcount == 0)
>> >> > + ieee80211_free_chanctx(local, ctx);
>> >> > + goto out_wake;
>> >> > + }
>> >>
>> >> This actually won't work if there are AP VLANs (I noticed that yesterday).
>> >>
>> >> You can't call ieee80211_vif_copy_chanctx_to_vlans() here due to
>> >> current locking requirements. I have a patch for that though ;-) I'll
>> >> send it soon.
>> >
>> > How is this related to this patch?
>> >
>> > Anyway, good to know that you have fixed it. :)
>>
>> If you re-assign a chanctx of an AP that has VLANs you leave VLANs
>> with the old chanctx pointer. Those pointers should be updated or else
>> you'll end up dereferencing an invalid pointer.
>
> I see. So, I need to call ieee80211_vif_copy_chanctx_to_vlans() here,
> but to do that I need your patch that allows it to be called without the
> RTNL.
>
> So, now this series will depend on that patch, I'll talk to Johannes.
There's also another catch with having unassign-assign chanctx
swapping. There's a brief period of time where chanctx_mtx unprotected
code (think rcu based sections) access the chanctx_conf to get
band/sband. This ends up with bloody NULL dereferences if you have a
5GHz-only hw. I'm currently patching those up. This would be another
(although indirect) dependency for the patchset.
We might also want to have a ieee80211_vif_chanctx() wrapper that
would return either assigned chanctx or reserved chanctx before the
reservation patchset. Otherwise, even with those NULL dereferences
patched you risk WARN_ONs and other (logic) failures.
Michał