Hi,
Here's another respin of channel context patches.
Changes:
- moved ieee80211_chan_mode
- updated chanctx_conf comment
- replaced vif_list with refcount
- added lockdeps
- channel type downgrading support
(when a channel context is unassigned from a vif
channel type is recalculated for the remaining vifs
sharing the same channel context)
-- Pozdrawiam / Best regards, Michal Kazior.
On Thu, 2012-06-28 at 09:54 +0200, Michal Kazior wrote:
> >> Yes, this is more or less what I also had in mind. I was just thinking
> >> about solving the issue of channel context and hw.conf.channel
> >> consistency. If we switch a channel we either modify channel in channel
> >> context directly (violating the immutability of channel contexts) or we
> >> iterate and re-set the new channel on each interface (because
> >> single-channel drivers may still have multiple interfaces and we
> >> probably want to use sdata->vif.chanctx_conf->channel instead of
> >> hw.conf.channel inside mac80211).
> >>
> >> Now that I think about it I guess violating the immutability for the
> >> single-channel case is okay. It would greatly simplify the code and we'd
> >> just put a comment down in hw_config where the only violation would occur.
> >
> > I'm not sure why we would violate it? The way I see it, you'd never
> > change the channel context channel since internally in mac80211 you'd
> > never want to see a different channel, just like today we use
> > local->oper_channel everywhere we'd then use sdata->vif.chanctx->channel
> > throughout, right?
> >
> > I think the only thing we need to do is put something like this into
> > hw_config:
> >
> > if (local->tmp_channel) {
> > local->hw.conf.channel = local->tmp_channel;
> > ...
> > } else {
> > local->hw.conf.channel = chanctx->channel;
> > }
> >
> > No?
>
> Using sdata->vif.chanctx_conf->channel instead of local->oper_channel
> doesn't make any sense to me.
>
> Take ieee80211_tx() for example. It does:
>
> tx.channel = local->hw.conf.channel;
>
> We don't use oper_channel here, but hw.conf.channel. TX can happen on
> different interfaces so for multi-channel operation it should be saying:
>
> tx.channel = sdata->vif.chanctx_conf->channel;
>
> In this case if we want to support the swscan/tmpchan through
> hw_config() we need update the channel context's channel somehow.
>
> I'm more thinking of hw.conf.channel becoming more of a backup value for
> single-channel drivers while we internally focus on channel contexts.
Yes, makes sense. I forgot all about the TX code. I'm a little wary of
making the contexts mutable, even in this case, because a lot of code
uses local->oper_channel as well, and that is expected to really be the
operating channel all of the time, even if we're scanning at some point
in time.
Luckily, tx.channel isn't actually used much, only for the band. So if
we tag the SKBs with the band earlier (info->band), maybe we don't need
to use hw.conf.channel as much there for tx.channel?
Other uses where we do need the current channel are
* ieee80211_build_probe_req
* ieee80211_add_srates_ie/ieee80211_add_ext_srates_ie
* __ieee80211_start_scan uses it but need not, could use oper_channel
instead and the code never executes for multi-channel
* ieee80211_set_tx_power() is interesting, may need to make it all
per-sdata now through nl80211 etc.
* rate_idx_to_bitrate can use the sta's sdata's channel
* ieee80211_change_bss can use the sdata's channel
* debugfs stuff probably just moves to per-sdata files
* ibss code all uses sdata channel
* ieee80211_if_change_type ... probably just set basic_rates = 0
* mesh can use sdata channel
* mlme.c should use sdata channel, but there's the channel switch stuff
* rate.h should use sta->sdata channel
Much of this is actually means we have bugs today! Whenever we use
hw.conf.channel and should be using sdata channel soon, we should be
using local->oper_channel today!
Maybe it's worth fixing that first, and getting rid of *most* instances
of hw.conf.channel, so we have a clearer idea of which changes in what
ways?
And then we can analyse the remaining instances of hw.conf.channel more
closely to see why they need to have the current channel (like tx code)
and see what needs to be done about them?
johannes
On Thu, 2012-06-28 at 11:20 +0200, Michal Kazior wrote:
> > Yes, makes sense. I forgot all about the TX code. I'm a little wary of
> > making the contexts mutable, even in this case, because a lot of code
> > uses local->oper_channel as well, and that is expected to really be the
> > operating channel all of the time, even if we're scanning at some point
> > in time.
>
> Yeah. The other option (maintaining the immutability) is to iterate
> through all interfaces and call ieee80211_vif_use_channel when switching
> channel for single-channel operation. Or do you have something else in
> mind maybe?
I don't think we should do that, we want to maintain chanctx.channel,
what today is oper_channel. But I guess I was just expressing myself
badly, seems below you understood it better :)
> > Luckily, tx.channel isn't actually used much, only for the band. So if
> > we tag the SKBs with the band earlier (info->band), maybe we don't need
> > to use hw.conf.channel as much there for tx.channel?
> >
> > Other uses where we do need the current channel are
> > * ieee80211_build_probe_req
> > * ieee80211_add_srates_ie/ieee80211_add_ext_srates_ie
> > * __ieee80211_start_scan uses it but need not, could use oper_channel
> > instead and the code never executes for multi-channel
> > * ieee80211_set_tx_power() is interesting, may need to make it all
> > per-sdata now through nl80211 etc.
>
> What will drivers that don't support per-sdata tx_power do? Do all
> multi-vif (not multi-channel) drivers support per-interface tx power?
>
> I guess we'd have to manage:
> a) common tx power value in ieee80211_local
> b) provide a function that calculates the common value
> so drivers may use it (and avoid code duplication)
> c) ..or else drivers would need to implement the calculation on
> their own
Yeah, good question. I'm not too worried about TX power though TBH, I
think it's fair to assume that if a driver supports multi-channel, it
supports per-channel TX power as well, if it even supports TX power
adjustment at all.
> > * rate_idx_to_bitrate can use the sta's sdata's channel
> > * ieee80211_change_bss can use the sdata's channel
> > * debugfs stuff probably just moves to per-sdata files
> > * ibss code all uses sdata channel
> > * ieee80211_if_change_type ... probably just set basic_rates = 0
> > * mesh can use sdata channel
> > * mlme.c should use sdata channel, but there's the channel switch stuff
> > * rate.h should use sta->sdata channel
> >
> > Much of this is actually means we have bugs today! Whenever we use
> > hw.conf.channel and should be using sdata channel soon, we should be
> > using local->oper_channel today!
>
> Oh! Now I understand why you wanted to use channel contexts in place of
> oper_channel. This makes sense.
:-)
> > Maybe it's worth fixing that first, and getting rid of *most* instances
> > of hw.conf.channel, so we have a clearer idea of which changes in what
> > ways?
>
> Sounds like a good idea.
Do you want to take that up? I think it would be worth doing it in front
of all the other mac80211 patches (those are incomplete anyway in that
the new code doesn't get used), and then we can shake out any bugs here
before we switch over to multi-channel?
Or I can work on it too if you prefer.
johannes
Johannes Berg wrote:
> On Wed, 2012-06-27 at 12:13 +0200, Michal Kazior wrote:
>
>>> Ok. Thinking about this a bit I get a feeling that we should internally
>>> fully convert mac80211 over to channel contexts and not make things
>>> conditional. Then, whenever a new channel context is allocated it will
>>> be the only one and we can set hw.conf.channel/type to its channel(type)
>>> and call hw_config() instead of ... add/change chanctx?
>>>
>>> That way, we really only need to make a distinction in the low-level
>>> code that actually calls into the driver, and on a higher level we can
>>> just use the channel contexts everywhere.
>>
>> But wouldn't that make hw.conf.channel have a different value then the
>> channel context after running hw_config() (in case of sw scan for
>> example)? You want to copy the value back from hw.conf.channel back to
>> (an immutable) context channel in such a case? Could you elaborate more
>> on this, please?
>
> Good questions :-)
>
> I was thinking that today we have tmp_channel & oper_channel (in local),
> and oper_channel would follow the (single) channel context (if there
> even is one, otherwise any random channel is fine), and hw.conf.channel
> would still be tmp_channel when that is in used for scanning/roc?
I'm still confused.
Okay here's what I think:
Our main concern right now is swscan and tmpchan for legacy operation.
We need to know whether a driver requires us to support swscan/tmpchan
or not. We could possibly:
a) if .hw_scan and .remain_on_channel are implemented use we
channel contexts only.
We then automatically require driver to handle all chanctx related
ieee80211_ops'. If it's missing we fail with -EINVAL in the
ieee80211_hw_register.
b) we could add a new flag a driver may report, e.g.
IEEE80211_HW_SUPPORTS_CHANCTX
(along with appropriate checks mentioned above)
For legacy operation we'd need to iterate through active interface in
hw_config() and call ieee80211_vif_use_channel() for each. This would
allow us to have the same channel context across all interfaces (so we
can virtually use channel context everywhere instead of
hw.conf.channel). This means the .assign_vif_chanctx cannot sleep if my
understanding is correct (we'd need to use RCU locking for iteration
since devlist lock may be held while hw_config() is caled).
What do you think?
>>> The biggest issue here will be CSA handling. I have no idea yet how this
>>> will work in multi-channel, maybe we need to disconnect all other
>>> interfaces on the same channel, or so?
>>
>> We don't care about CSA in cfg80211 right now, do we? We probably should
>> as it can break interface combinations right now too.
>
> Good point.
>
>> We could maybe use the cfg80211_ch_switch_notify.
>
> That was intended for AP mode, I'm not sure it's good for station mode,
> in station mode we need to have current_bss->channel updated as well in
> that case.
>
>> This would probably require some more
>> additional changes too (maybe with regard to channel tracking too).
>>
>> Worst-case we disconnect other interfaces. We might be able to create a
>> new channel context (provided num_different_channels hasn't been reached
>> yet) or reuse an existing channel context (provided CSA happens to
>> target a channel we have a channel context for already).
>
> Yeah ... we need to think about it more. I thought we could put it off a
> bit longer, but I guess with the channel tracking we already need it?
Hmm.. Yeah we need to now, sort of. FullMAC drivers could suffer, but we
could drop connections upon CSA in mac80211 for the time being.
cfg80211 could provide:
* cfg80211_sta_can_switch_chan
* cfg80211_sta_ch_switch_notify
mac80211 could then be able to know whether num_different_channels has
been reached (cfg80211_sta_can_switch_chan) and eventually notify upon
channel switch (cfg80211_sta_ch_switch_notify).
Channel switch would happen if either:
* cfg80211_sta_can_switch_chan is true
* channel context for target CSA channel already exists
I'm just unsure about the current_bss thing - whether we should e.g.
initiate a scan to update it, or can we shamelessly update the channel
in the structure directly?
--
Pozdrawiam / Best regards, Michal Kazior.
Johannes Berg wrote:
> On Tue, 2012-06-26 at 14:37 +0200, Michal Kazior wrote:
>> Hi,
>>
>> Here's another respin of channel context patches.
>>
>> Changes:
>>
>> - moved ieee80211_chan_mode
>> - updated chanctx_conf comment
>> - replaced vif_list with refcount
>> - added lockdeps
>> - channel type downgrading support
>> (when a channel context is unassigned from a vif
>> channel type is recalculated for the remaining vifs
>> sharing the same channel context)
>
> Except for that last point which seems to have implementation issues,
> this looks really good too!
>
> How are you planning to proceed from here? I'm happy to pick these up
> even if you're not going to work on using it right away since I think in
> that case I would probably be working on using it pretty soon, but if
> you are planning to work on it that's even better :-)
The next step is to start using channel contexts in mac80211. I was
thinking of using chanctx->channel instead of hw.conf.channel if
.assign_vif_chanctx is defined. I expect some issues like using channel
before it is set.
Hwsim requires .hw_scan and ROC to be implemented to test channel
contexts. hw_scan should be simple (if we skip a few things..) but
haven't looked into the ROC case yet. It also requires some changes to
move channel logic per-vif.
Depending on how things turn out I may start to tackle on mac80211 a bit
more and hw_scan for hwsim. I may send a few patches on monday.
I'll of course resend this patchset today once I fix the pointed out issues.
--
Pozdrawiam / Best regards, Michal Kazior.
Johannes Berg wrote:
> On Wed, 2012-06-27 at 09:30 +0200, Michal Kazior wrote:
>
>>> How are you planning to proceed from here? I'm happy to pick these up
>>> even if you're not going to work on using it right away since I think in
>>> that case I would probably be working on using it pretty soon, but if
>>> you are planning to work on it that's even better :-)
>>
>> The next step is to start using channel contexts in mac80211. I was
>> thinking of using chanctx->channel instead of hw.conf.channel if
>> .assign_vif_chanctx is defined. I expect some issues like using channel
>> before it is set.
>
> Ok. Thinking about this a bit I get a feeling that we should internally
> fully convert mac80211 over to channel contexts and not make things
> conditional. Then, whenever a new channel context is allocated it will
> be the only one and we can set hw.conf.channel/type to its channel(type)
> and call hw_config() instead of ... add/change chanctx?
>
> That way, we really only need to make a distinction in the low-level
> code that actually calls into the driver, and on a higher level we can
> just use the channel contexts everywhere.
But wouldn't that make hw.conf.channel have a different value then the
channel context after running hw_config() (in case of sw scan for
example)? You want to copy the value back from hw.conf.channel back to
(an immutable) context channel in such a case? Could you elaborate more
on this, please?
> The biggest issue here will be CSA handling. I have no idea yet how this
> will work in multi-channel, maybe we need to disconnect all other
> interfaces on the same channel, or so?
We don't care about CSA in cfg80211 right now, do we? We probably should
as it can break interface combinations right now too. We could maybe use
the cfg80211_ch_switch_notify. This would probably require some more
additional changes too (maybe with regard to channel tracking too).
Worst-case we disconnect other interfaces. We might be able to create a
new channel context (provided num_different_channels hasn't been reached
yet) or reuse an existing channel context (provided CSA happens to
target a channel we have a channel context for already).
--
Pozdrawiam / Best regards, Michal Kazior.
From: Kazior Michal <[email protected]>
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/driver-ops.h | 51 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 51 insertions(+), 0 deletions(-)
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 6d33a0c..e79f06d 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -845,4 +845,55 @@ drv_allow_buffered_frames(struct ieee80211_local *local,
more_data);
trace_drv_return_void(local);
}
+
+static inline void
+drv_add_chanctx(struct ieee80211_local *local,
+ struct ieee80211_chanctx *ctx)
+{
+ if (local->ops->add_chanctx)
+ local->ops->add_chanctx(&local->hw, &ctx->conf);
+}
+
+static inline void
+drv_remove_chanctx(struct ieee80211_local *local,
+ struct ieee80211_chanctx *ctx)
+{
+ if (local->ops->remove_chanctx)
+ local->ops->remove_chanctx(&local->hw, &ctx->conf);
+}
+
+static inline void
+drv_change_chantype(struct ieee80211_local *local,
+ struct ieee80211_chanctx *ctx)
+{
+ if (local->ops->change_chantype)
+ local->ops->change_chantype(&local->hw, &ctx->conf);
+}
+
+static inline void
+drv_assign_vif_chanctx(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_chanctx *ctx)
+{
+ check_sdata_in_driver(sdata);
+
+ if (local->ops->assign_vif_chanctx)
+ local->ops->assign_vif_chanctx(&local->hw,
+ &sdata->vif,
+ &ctx->conf);
+}
+
+static inline void
+drv_unassign_vif_chanctx(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_chanctx *ctx)
+{
+ check_sdata_in_driver(sdata);
+
+ if (local->ops->unassign_vif_chanctx)
+ local->ops->unassign_vif_chanctx(&local->hw,
+ &sdata->vif,
+ &ctx->conf);
+}
+
#endif /* __MAC80211_DRIVER_OPS */
--
1.7.0.4
Johannes Berg wrote:
> On Wed, 2012-06-27 at 14:43 +0200, Michal Kazior wrote:
> I think we should do
> c) if the driver advertises support for multiple channels in its
> interface combinations or implements the channel context callbacks,
> it must have hw_scan/roc, otherwise we fail hw registration; if it
> advertises support for multiple channels obviously it must have all
> the channel context callbacks
>
> This would leave all existing drivers operating as-is, the next step
> would be adding channel context support to a driver (must have hw
> scan/roc in that case), and the next step after that would be actually
> advertising support for multiple channels -- in practice it's probably
> just a single patch doing both but that's the logical order then.
Right.
>> For legacy operation we'd need to iterate through active interface in
>> hw_config() and call ieee80211_vif_use_channel() for each. This would
>> allow us to have the same channel context across all interfaces (so we
>> can virtually use channel context everywhere instead of
>> hw.conf.channel). This means the .assign_vif_chanctx cannot sleep if my
>> understanding is correct (we'd need to use RCU locking for iteration
>> since devlist lock may be held while hw_config() is caled).
>>
>> What do you think?
>
> What 'legacy operation' are you referring to?
The case when we support swscan and tmpchan.
> In any case, I think you're turning it upside down. I think we should
> get rid of local->oper_channel(_type) completely, and instead use the
> channel contexts in mac80211 everywhere. If the driver doesn't implement
> channel contexts it can only support a single channel. Thus, we can have
> at most one channel context, so whenever a new context is added (there
> could be zero) or any context is modified (the only one) we can set
> hw.conf.channel and call hw_config() with the CHANNEL_CHANGE flag.
>
> IOW, nothing in mac80211 would ever call hw_config() for the channel or
> channel type change, it would all do channel contexts, but the channel
> context code would see that if the driver doesn't support channel
> contexts
> 1) there will be at most one context in mac80211
> 2) this context is programmed into the device by using hw_config()
> instead of the context callbacks
Yes, this is more or less what I also had in mind. I was just thinking
about solving the issue of channel context and hw.conf.channel
consistency. If we switch a channel we either modify channel in channel
context directly (violating the immutability of channel contexts) or we
iterate and re-set the new channel on each interface (because
single-channel drivers may still have multiple interfaces and we
probably want to use sdata->vif.chanctx_conf->channel instead of
hw.conf.channel inside mac80211).
Now that I think about it I guess violating the immutability for the
single-channel case is okay. It would greatly simplify the code and we'd
just put a comment down in hw_config where the only violation would occur.
--
Pozdrawiam / Best regards, Michal Kazior.
Johannes Berg wrote:
> On Thu, 2012-06-28 at 11:20 +0200, Michal Kazior wrote:
>>> Maybe it's worth fixing that first, and getting rid of *most* instances
>>> of hw.conf.channel, so we have a clearer idea of which changes in what
>>> ways?
>>
>> Sounds like a good idea.
>
> Do you want to take that up? I think it would be worth doing it in front
> of all the other mac80211 patches (those are incomplete anyway in that
> the new code doesn't get used), and then we can shake out any bugs here
> before we switch over to multi-channel?
Sure, I'll take it up.
--
Pozdrawiam / Best regards, Michal Kazior.
On Tue, 2012-06-26 at 14:37 +0200, Michal Kazior wrote:
> From: Kazior Michal <[email protected]>
In this and a few other patches the From: seems to be wrong?
> Channel context pointer will be accessiable on
typo: accessible
johannes
Johannes Berg wrote:
> On Tue, 2012-06-26 at 14:37 +0200, Michal Kazior wrote:
>
>> +static enum nl80211_channel_type
>> +ieee80211_calc_chantype(struct ieee80211_local *local,
>> + struct ieee80211_chanctx *ctx)
>> +{
>> + struct ieee80211_chanctx_conf *conf = &ctx->conf;
>> + struct ieee80211_sub_if_data *sdata;
>> + enum nl80211_channel_type chantype = NL80211_CHAN_NO_HT;
>> + enum nl80211_channel_type compat;
>> +
>> + lockdep_assert_held(&local->chanctx_mtx);
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
>> + if (!ieee80211_sdata_running(sdata))
>> + continue;
>> + if (sdata->vif.chanctx_conf != conf)
>> + continue;
>> +
>> + BUG_ON(!ieee80211_channel_types_are_compatible(
>> + conf->channel_type, chantype, &compat));
>
> Please no BUG_ON, maybe only WARN_ON_ONCE even?
Okay. I guess returning NL80211_CHAN_NO_HT in such case is okay.
>> + chantype = compat;
>> + }
>> + rcu_read_unlock();
>> +
>> + return chantype;
>> +}
>
> I don't think I understand this, wouldn't it need some per-vif requested
> channel type to work correctly? I don't see any chantype values coming
> from the sdata here, so I don't think this could work?
>
> If say vif1 needs ht40+ and vif2 ht20, the channel context will be set
> to ht40+, but then if vif1 goes away and you don't know anything about
> vif2 at all, then how can this work? The way I see it, your code here
> doesn't really do anything, except recalculate that ht40+ is compatible
> with ht40+, or something like that? I guess I'm a bit confused, it seems
> to me that this cannot work even in theory unless you have per-vif data.
It seems I must've had a mind-derp. I was planning on using
sdata->vif.bss_conf.channel_type which should be ok, right?
>> static void
>> ieee80211_unassign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
>> struct ieee80211_chanctx *ctx)
>> @@ -248,6 +302,8 @@ ieee80211_unassign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
>>
>> drv_unassign_vif_chanctx(sdata->local, sdata, ctx);
>>
>> + ieee80211_recalc_chanctx_chantype(sdata->local, ctx);
>> +
>> ctx->refcount--;
>> sdata->vif.chanctx_conf = NULL;
>
> And then shouldn't you recalc *after* setting chanctx_conf = NULL so you
> skip this vif?
You're absolutely right.
--
Pozdrawiam / Best regards, Michal Kazior.
On Wed, 2012-06-27 at 12:13 +0200, Michal Kazior wrote:
> > Ok. Thinking about this a bit I get a feeling that we should internally
> > fully convert mac80211 over to channel contexts and not make things
> > conditional. Then, whenever a new channel context is allocated it will
> > be the only one and we can set hw.conf.channel/type to its channel(type)
> > and call hw_config() instead of ... add/change chanctx?
> >
> > That way, we really only need to make a distinction in the low-level
> > code that actually calls into the driver, and on a higher level we can
> > just use the channel contexts everywhere.
>
> But wouldn't that make hw.conf.channel have a different value then the
> channel context after running hw_config() (in case of sw scan for
> example)? You want to copy the value back from hw.conf.channel back to
> (an immutable) context channel in such a case? Could you elaborate more
> on this, please?
Good questions :-)
I was thinking that today we have tmp_channel & oper_channel (in local),
and oper_channel would follow the (single) channel context (if there
even is one, otherwise any random channel is fine), and hw.conf.channel
would still be tmp_channel when that is in used for scanning/roc?
> > The biggest issue here will be CSA handling. I have no idea yet how this
> > will work in multi-channel, maybe we need to disconnect all other
> > interfaces on the same channel, or so?
>
> We don't care about CSA in cfg80211 right now, do we? We probably should
> as it can break interface combinations right now too.
Good point.
> We could maybe use the cfg80211_ch_switch_notify.
That was intended for AP mode, I'm not sure it's good for station mode,
in station mode we need to have current_bss->channel updated as well in
that case.
> This would probably require some more
> additional changes too (maybe with regard to channel tracking too).
>
> Worst-case we disconnect other interfaces. We might be able to create a
> new channel context (provided num_different_channels hasn't been reached
> yet) or reuse an existing channel context (provided CSA happens to
> target a channel we have a channel context for already).
Yeah ... we need to think about it more. I thought we could put it off a
bit longer, but I guess with the channel tracking we already need it?
johannes
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/driver-ops.h | 10 +++++
net/mac80211/driver-trace.h | 88 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 98 insertions(+), 0 deletions(-)
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index e79f06d..dde9f33 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -850,24 +850,30 @@ static inline void
drv_add_chanctx(struct ieee80211_local *local,
struct ieee80211_chanctx *ctx)
{
+ trace_drv_add_chanctx(local, ctx);
if (local->ops->add_chanctx)
local->ops->add_chanctx(&local->hw, &ctx->conf);
+ trace_drv_return_void(local);
}
static inline void
drv_remove_chanctx(struct ieee80211_local *local,
struct ieee80211_chanctx *ctx)
{
+ trace_drv_remove_chanctx(local, ctx);
if (local->ops->remove_chanctx)
local->ops->remove_chanctx(&local->hw, &ctx->conf);
+ trace_drv_return_void(local);
}
static inline void
drv_change_chantype(struct ieee80211_local *local,
struct ieee80211_chanctx *ctx)
{
+ trace_drv_change_chantype(local, ctx);
if (local->ops->change_chantype)
local->ops->change_chantype(&local->hw, &ctx->conf);
+ trace_drv_return_void(local);
}
static inline void
@@ -877,10 +883,12 @@ drv_assign_vif_chanctx(struct ieee80211_local *local,
{
check_sdata_in_driver(sdata);
+ trace_drv_assign_vif_chanctx(local, sdata, ctx);
if (local->ops->assign_vif_chanctx)
local->ops->assign_vif_chanctx(&local->hw,
&sdata->vif,
&ctx->conf);
+ trace_drv_return_void(local);
}
static inline void
@@ -890,10 +898,12 @@ drv_unassign_vif_chanctx(struct ieee80211_local *local,
{
check_sdata_in_driver(sdata);
+ trace_drv_unassign_vif_chanctx(local, sdata, ctx);
if (local->ops->unassign_vif_chanctx)
local->ops->unassign_vif_chanctx(&local->hw,
&sdata->vif,
&ctx->conf);
+ trace_drv_return_void(local);
}
#endif /* __MAC80211_DRIVER_OPS */
diff --git a/net/mac80211/driver-trace.h b/net/mac80211/driver-trace.h
index 6de00b2..16b0e43 100644
--- a/net/mac80211/driver-trace.h
+++ b/net/mac80211/driver-trace.h
@@ -1606,6 +1606,94 @@ TRACE_EVENT(stop_queue,
LOCAL_PR_ARG, __entry->queue, __entry->reason
)
);
+
+DECLARE_EVENT_CLASS(local_chanctx,
+ TP_PROTO(struct ieee80211_local *local,
+ struct ieee80211_chanctx *ctx),
+
+ TP_ARGS(local, ctx),
+
+ TP_STRUCT__entry(
+ LOCAL_ENTRY
+ __field(void *, ctx)
+ __field(int, freq)
+ __field(int, chantype)
+ ),
+
+ TP_fast_assign(
+ LOCAL_ASSIGN;
+ __entry->ctx = ctx;
+ __entry->freq = ctx->conf.channel->center_freq;
+ __entry->chantype = ctx->conf.channel_type;
+ ),
+
+ TP_printk(
+ LOCAL_PR_FMT " ctx:%p freq:%dMhz chantype:%d",
+ LOCAL_PR_ARG, __entry->ctx, __entry->freq, __entry->chantype
+ )
+);
+
+DEFINE_EVENT(local_chanctx, drv_add_chanctx,
+ TP_PROTO(struct ieee80211_local *local,
+ struct ieee80211_chanctx *ctx),
+ TP_ARGS(local, ctx)
+);
+
+DEFINE_EVENT(local_chanctx, drv_remove_chanctx,
+ TP_PROTO(struct ieee80211_local *local,
+ struct ieee80211_chanctx *ctx),
+ TP_ARGS(local, ctx)
+);
+
+DEFINE_EVENT(local_chanctx, drv_change_chantype,
+ TP_PROTO(struct ieee80211_local *local,
+ struct ieee80211_chanctx *ctx),
+ TP_ARGS(local, ctx)
+);
+
+DECLARE_EVENT_CLASS(local_sdata_chanctx,
+ TP_PROTO(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_chanctx *ctx),
+
+ TP_ARGS(local, sdata, ctx),
+
+ TP_STRUCT__entry(
+ LOCAL_ENTRY
+ VIF_ENTRY
+ __field(void *, ctx)
+ __field(int, freq)
+ __field(int, chantype)
+ ),
+
+ TP_fast_assign(
+ LOCAL_ASSIGN;
+ VIF_ASSIGN;
+ __entry->ctx = ctx;
+ __entry->freq = ctx->conf.channel->center_freq;
+ __entry->chantype = ctx->conf.channel_type;
+ ),
+
+ TP_printk(
+ LOCAL_PR_FMT VIF_PR_FMT " ctx:%p freq:%dMhz chantype:%d",
+ LOCAL_PR_ARG, VIF_PR_ARG,
+ __entry->ctx, __entry->freq, __entry->chantype
+ )
+);
+
+DEFINE_EVENT(local_sdata_chanctx, drv_assign_vif_chanctx,
+ TP_PROTO(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_chanctx *ctx),
+ TP_ARGS(local, sdata, ctx)
+);
+
+DEFINE_EVENT(local_sdata_chanctx, drv_unassign_vif_chanctx,
+ TP_PROTO(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_chanctx *ctx),
+ TP_ARGS(local, sdata, ctx)
+);
#endif /* !__MAC80211_DRIVER_TRACE || TRACE_HEADER_MULTI_READ */
#undef TRACE_INCLUDE_PATH
--
1.7.0.4
Johannes Berg wrote:
> On Thu, 2012-06-28 at 08:04 +0200, Michal Kazior wrote:
>
>>> In any case, I think you're turning it upside down. I think we should
>>> get rid of local->oper_channel(_type) completely, and instead use the
>>> channel contexts in mac80211 everywhere. If the driver doesn't implement
>>> channel contexts it can only support a single channel. Thus, we can have
>>> at most one channel context, so whenever a new context is added (there
>>> could be zero) or any context is modified (the only one) we can set
>>> hw.conf.channel and call hw_config() with the CHANNEL_CHANGE flag.
>>>
>>> IOW, nothing in mac80211 would ever call hw_config() for the channel or
>>> channel type change, it would all do channel contexts, but the channel
>>> context code would see that if the driver doesn't support channel
>>> contexts
>>> 1) there will be at most one context in mac80211
>>> 2) this context is programmed into the device by using hw_config()
>>> instead of the context callbacks
>>
>> Yes, this is more or less what I also had in mind. I was just thinking
>> about solving the issue of channel context and hw.conf.channel
>> consistency. If we switch a channel we either modify channel in channel
>> context directly (violating the immutability of channel contexts) or we
>> iterate and re-set the new channel on each interface (because
>> single-channel drivers may still have multiple interfaces and we
>> probably want to use sdata->vif.chanctx_conf->channel instead of
>> hw.conf.channel inside mac80211).
>>
>> Now that I think about it I guess violating the immutability for the
>> single-channel case is okay. It would greatly simplify the code and we'd
>> just put a comment down in hw_config where the only violation would occur.
>
> I'm not sure why we would violate it? The way I see it, you'd never
> change the channel context channel since internally in mac80211 you'd
> never want to see a different channel, just like today we use
> local->oper_channel everywhere we'd then use sdata->vif.chanctx->channel
> throughout, right?
>
> I think the only thing we need to do is put something like this into
> hw_config:
>
> if (local->tmp_channel) {
> local->hw.conf.channel = local->tmp_channel;
> ...
> } else {
> local->hw.conf.channel = chanctx->channel;
> }
>
> No?
Using sdata->vif.chanctx_conf->channel instead of local->oper_channel
doesn't make any sense to me.
Take ieee80211_tx() for example. It does:
tx.channel = local->hw.conf.channel;
We don't use oper_channel here, but hw.conf.channel. TX can happen on
different interfaces so for multi-channel operation it should be saying:
tx.channel = sdata->vif.chanctx_conf->channel;
In this case if we want to support the swscan/tmpchan through
hw_config() we need update the channel context's channel somehow.
I'm more thinking of hw.conf.channel becoming more of a backup value for
single-channel drivers while we internally focus on channel contexts.
--
Pozdrawiam / Best regards, Michal Kazior.
From: Kazior Michal <[email protected]>
Channel context pointer will be accessiable on
both assign and unassign events.
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/chan.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index dc397a8..1aa553d 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -5,6 +5,7 @@
#include <linux/nl80211.h>
#include <net/cfg80211.h>
#include "ieee80211_i.h"
+#include "driver-ops.h"
static enum ieee80211_chan_mode
__ieee80211_get_channel_mode(struct ieee80211_local *local,
@@ -190,6 +191,8 @@ ieee80211_new_chanctx(struct ieee80211_local *local,
list_add(&ctx->list, &local->chanctx_list);
+ drv_add_chanctx(local, ctx);
+
return ctx;
}
@@ -200,6 +203,8 @@ ieee80211_free_chanctx(struct ieee80211_chanctx *ctx)
BUG_ON(ctx->refcount != 0);
+ drv_remove_chanctx(ctx->local, ctx);
+
list_del(&ctx->list);
kfree(ctx);
}
@@ -212,6 +217,8 @@ ieee80211_assign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
sdata->vif.chanctx_conf = &ctx->conf;
ctx->refcount++;
+
+ drv_assign_vif_chanctx(sdata->local, sdata, ctx);
}
static void
@@ -220,6 +227,8 @@ ieee80211_unassign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
{
lockdep_assert_held(&local->chanctx_mtx);
+ drv_unassign_vif_chanctx(sdata->local, sdata, ctx);
+
ctx->refcount--;
sdata->vif.chanctx_conf = NULL;
}
--
1.7.0.4
On Thu, 2012-06-28 at 08:04 +0200, Michal Kazior wrote:
> > In any case, I think you're turning it upside down. I think we should
> > get rid of local->oper_channel(_type) completely, and instead use the
> > channel contexts in mac80211 everywhere. If the driver doesn't implement
> > channel contexts it can only support a single channel. Thus, we can have
> > at most one channel context, so whenever a new context is added (there
> > could be zero) or any context is modified (the only one) we can set
> > hw.conf.channel and call hw_config() with the CHANNEL_CHANGE flag.
> >
> > IOW, nothing in mac80211 would ever call hw_config() for the channel or
> > channel type change, it would all do channel contexts, but the channel
> > context code would see that if the driver doesn't support channel
> > contexts
> > 1) there will be at most one context in mac80211
> > 2) this context is programmed into the device by using hw_config()
> > instead of the context callbacks
>
> Yes, this is more or less what I also had in mind. I was just thinking
> about solving the issue of channel context and hw.conf.channel
> consistency. If we switch a channel we either modify channel in channel
> context directly (violating the immutability of channel contexts) or we
> iterate and re-set the new channel on each interface (because
> single-channel drivers may still have multiple interfaces and we
> probably want to use sdata->vif.chanctx_conf->channel instead of
> hw.conf.channel inside mac80211).
>
> Now that I think about it I guess violating the immutability for the
> single-channel case is okay. It would greatly simplify the code and we'd
> just put a comment down in hw_config where the only violation would occur.
I'm not sure why we would violate it? The way I see it, you'd never
change the channel context channel since internally in mac80211 you'd
never want to see a different channel, just like today we use
local->oper_channel everywhere we'd then use sdata->vif.chanctx->channel
throughout, right?
I think the only thing we need to do is put something like this into
hw_config:
if (local->tmp_channel) {
local->hw.conf.channel = local->tmp_channel;
...
} else {
local->hw.conf.channel = chanctx->channel;
}
No?
johannes
Johannes Berg wrote:
> On Tue, 2012-06-26 at 14:37 +0200, Michal Kazior wrote:
>> From: Kazior Michal <[email protected]>
>
> In this and a few other patches the From: seems to be wrong?
The mail server does tricks on me. I'll have to check msmtp config.
>> Channel context pointer will be accessiable on
>
> typo: accessible
Oh, thanks!
--
Pozdrawiam / Best regards, Michal Kazior.
On Tue, 2012-06-26 at 14:37 +0200, Michal Kazior wrote:
> Hi,
>
> Here's another respin of channel context patches.
>
> Changes:
>
> - moved ieee80211_chan_mode
> - updated chanctx_conf comment
> - replaced vif_list with refcount
> - added lockdeps
> - channel type downgrading support
> (when a channel context is unassigned from a vif
> channel type is recalculated for the remaining vifs
> sharing the same channel context)
Except for that last point which seems to have implementation issues,
this looks really good too!
How are you planning to proceed from here? I'm happy to pick these up
even if you're not going to work on using it right away since I think in
that case I would probably be working on using it pretty soon, but if
you are planning to work on it that's even better :-)
johannes
Johannes Berg wrote:
> On Wed, 2012-06-27 at 14:43 +0200, Michal Kazior wrote:
>>> That was intended for AP mode, I'm not sure it's good for station mode,
>>> in station mode we need to have current_bss->channel updated as well in
>>> that case.
>>>
>>>> This would probably require some more
>>>> additional changes too (maybe with regard to channel tracking too).
>>>>
>>>> Worst-case we disconnect other interfaces. We might be able to create a
>>>> new channel context (provided num_different_channels hasn't been reached
>>>> yet) or reuse an existing channel context (provided CSA happens to
>>>> target a channel we have a channel context for already).
>>>
>>> Yeah ... we need to think about it more. I thought we could put it off a
>>> bit longer, but I guess with the channel tracking we already need it?
>>
>> Hmm.. Yeah we need to now, sort of. FullMAC drivers could suffer, but we
>> could drop connections upon CSA in mac80211 for the time being.
>>
>> cfg80211 could provide:
>> * cfg80211_sta_can_switch_chan
>> * cfg80211_sta_ch_switch_notify
>>
>> mac80211 could then be able to know whether num_different_channels has
>> been reached (cfg80211_sta_can_switch_chan) and eventually notify upon
>> channel switch (cfg80211_sta_ch_switch_notify).
>>
>> Channel switch would happen if either:
>> * cfg80211_sta_can_switch_chan is true
>> * channel context for target CSA channel already exists
>>
>> I'm just unsure about the current_bss thing - whether we should e.g.
>> initiate a scan to update it, or can we shamelessly update the channel
>> in the structure directly?
>
> Today mac80211 just updates it, yeah.
Should I send the ,,respect channels in iface combinations'' as a
[PATCH] now, or should the CSA issue (in cfg80211) be addressed first?
--
Pozdrawiam / Best regards, Michal Kazior.
On Tue, 2012-06-26 at 15:55 +0200, Michal Kazior wrote:
> > I don't think I understand this, wouldn't it need some per-vif requested
> > channel type to work correctly? I don't see any chantype values coming
> > from the sdata here, so I don't think this could work?
> >
> > If say vif1 needs ht40+ and vif2 ht20, the channel context will be set
> > to ht40+, but then if vif1 goes away and you don't know anything about
> > vif2 at all, then how can this work? The way I see it, your code here
> > doesn't really do anything, except recalculate that ht40+ is compatible
> > with ht40+, or something like that? I guess I'm a bit confused, it seems
> > to me that this cannot work even in theory unless you have per-vif data.
>
> It seems I must've had a mind-derp. I was planning on using
> sdata->vif.bss_conf.channel_type which should be ok, right?
Yeah, unless you were planning to not go through
ieee80211_set_channel_type() any more in the future?
johannes
Johannes Berg wrote:
> On Thu, 2012-06-28 at 09:01 +0200, Michal Kazior wrote:
>
>>>> Hmm.. Yeah we need to now, sort of. FullMAC drivers could suffer, but we
>>>> could drop connections upon CSA in mac80211 for the time being.
>>>>
>>>> cfg80211 could provide:
>>>> * cfg80211_sta_can_switch_chan
>>>> * cfg80211_sta_ch_switch_notify
>>>>
>>>> mac80211 could then be able to know whether num_different_channels has
>>>> been reached (cfg80211_sta_can_switch_chan) and eventually notify upon
>>>> channel switch (cfg80211_sta_ch_switch_notify).
>>>>
>>>> Channel switch would happen if either:
>>>> * cfg80211_sta_can_switch_chan is true
>>>> * channel context for target CSA channel already exists
>>>>
>>>> I'm just unsure about the current_bss thing - whether we should e.g.
>>>> initiate a scan to update it, or can we shamelessly update the channel
>>>> in the structure directly?
>>>
>>> Today mac80211 just updates it, yeah.
>>
>> Should I send the ,,respect channels in iface combinations'' as a
>> [PATCH] now, or should the CSA issue (in cfg80211) be addressed first?
>
> Good question. Even if we don't do CSA first, I guess we don't really
> break anything right now, do we? And if the driver/mac80211 just changes
> the channel, that means it could seem to go over the limit of 1 channel
> but things just break down like they do today?
Yeah, probably. We might end up in a funny state (e.g. rejecting some
actions which would otherwise be possible) after doing CSA until we
disconnect.
> I guess I'd like to have the CSA stuff addressed, but it seems we
> haven't really closed on the best way yet, and I don't want to hold off
> your work for it for too long?
If you're okay with that then I'll just send out the [PATCH].
--
Pozdrawiam / Best regards, Michal Kazior.
On Thu, 2012-06-28 at 10:54 +0200, Michal Kazior wrote:
> >>> Today mac80211 just updates it, yeah.
> >>
> >> Should I send the ,,respect channels in iface combinations'' as a
> >> [PATCH] now, or should the CSA issue (in cfg80211) be addressed first?
> >
> > Good question. Even if we don't do CSA first, I guess we don't really
> > break anything right now, do we? And if the driver/mac80211 just changes
> > the channel, that means it could seem to go over the limit of 1 channel
> > but things just break down like they do today?
>
> Yeah, probably. We might end up in a funny state (e.g. rejecting some
> actions which would otherwise be possible) after doing CSA until we
> disconnect.
I'm not all that worried about that.
> > I guess I'd like to have the CSA stuff addressed, but it seems we
> > haven't really closed on the best way yet, and I don't want to hold off
> > your work for it for too long?
>
> If you're okay with that then I'll just send out the [PATCH].
Sounds fine, thanks.
johannes
From: Kazior Michal <[email protected]>
Channel context are the foundation for
multi-channel operation.
Channel contexts are immutable and are re-created
(or re-used if other interfaces are bound to a
certain channel and a compatible channel type) on
channel switching.
This is an initial implementation and more
features will come in separate patches.
Signed-off-by: Michal Kazior <[email protected]>
---
include/net/mac80211.h | 25 ++++++++
net/mac80211/chan.c | 134 ++++++++++++++++++++++++++++++++++++++++++++
net/mac80211/ieee80211_i.h | 36 ++++++++++++
net/mac80211/main.c | 3 +
4 files changed, 198 insertions(+), 0 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 6914f99..d64fa82 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -144,6 +144,24 @@ struct ieee80211_low_level_stats {
};
/**
+ * struct ieee80211_chanctx_conf - channel context that vifs may be tuned to
+ *
+ * This is the driver-visible part. The ieee80211_chanctx
+ * that contains it is visible in mac80211 only.
+ *
+ * @channel: the channel to tune to
+ * @channel_type: the channel (HT) type
+ * @drv_priv: data area for driver use, will always be aligned to
+ * sizeof(void *), size is determined in hw information.
+ */
+struct ieee80211_chanctx_conf {
+ struct ieee80211_channel *channel;
+ enum nl80211_channel_type channel_type;
+
+ u8 drv_priv[0] __attribute__((__aligned__(sizeof(void *))));
+};
+
+/**
* enum ieee80211_bss_change - BSS change notification flags
*
* These flags are used with the bss_info_changed() callback
@@ -897,6 +915,8 @@ enum ieee80211_vif_flags {
* at runtime, mac80211 will never touch this field
* @hw_queue: hardware queue for each AC
* @cab_queue: content-after-beacon (DTIM beacon really) queue, AP mode only
+ * @chanctx_conf: will be %NULL before the interface is assigned to a channel
+ * context
* @drv_priv: data area for driver use, will always be aligned to
* sizeof(void *).
*/
@@ -909,6 +929,8 @@ struct ieee80211_vif {
u8 cab_queue;
u8 hw_queue[IEEE80211_NUM_ACS];
+ struct ieee80211_chanctx_conf *chanctx_conf;
+
u32 driver_flags;
/* must be last */
@@ -1268,6 +1290,8 @@ enum ieee80211_hw_flags {
* within &struct ieee80211_vif.
* @sta_data_size: size (in bytes) of the drv_priv data area
* within &struct ieee80211_sta.
+ * @chanctx_data_size: size (in bytes) of the drv_priv data area
+ * within &struct ieee80211_chanctx_conf.
*
* @max_rates: maximum number of alternate rate retry stages the hw
* can handle.
@@ -1312,6 +1336,7 @@ struct ieee80211_hw {
int channel_change_time;
int vif_data_size;
int sta_data_size;
+ int chanctx_data_size;
int napi_weight;
u16 queues;
u16 max_listen_interval;
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index f0f87e5..dc397a8 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -139,3 +139,137 @@ bool ieee80211_set_channel_type(struct ieee80211_local *local,
return result;
}
+
+static struct ieee80211_chanctx *
+ieee80211_find_chanctx(struct ieee80211_local *local,
+ struct ieee80211_channel *channel,
+ enum nl80211_channel_type channel_type,
+ enum ieee80211_chanctx_mode mode)
+{
+ struct ieee80211_chanctx *ctx;
+
+ lockdep_assert_held(&local->chanctx_mtx);
+
+ if (mode == IEEE80211_CHANCTX_EXCLUSIVE)
+ return NULL;
+ if (WARN_ON(!channel))
+ return NULL;
+
+ list_for_each_entry(ctx, &local->chanctx_list, list) {
+ if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE)
+ continue;
+ if (ctx->conf.channel != channel)
+ continue;
+ if (ctx->conf.channel_type != channel_type)
+ continue;
+
+ return ctx;
+ }
+
+ return NULL;
+}
+
+static struct ieee80211_chanctx *
+ieee80211_new_chanctx(struct ieee80211_local *local,
+ struct ieee80211_channel *channel,
+ enum nl80211_channel_type channel_type,
+ enum ieee80211_chanctx_mode mode)
+{
+ struct ieee80211_chanctx *ctx;
+
+ lockdep_assert_held(&local->chanctx_mtx);
+
+ ctx = kzalloc(sizeof(*ctx) + local->hw.chanctx_data_size, GFP_KERNEL);
+ if (!ctx)
+ return NULL;
+
+ ctx->conf.channel = channel;
+ ctx->conf.channel_type = channel_type;
+ ctx->mode = mode;
+ ctx->local = local;
+
+ list_add(&ctx->list, &local->chanctx_list);
+
+ return ctx;
+}
+
+static void
+ieee80211_free_chanctx(struct ieee80211_chanctx *ctx)
+{
+ lockdep_assert_held(&local->chanctx_mtx);
+
+ BUG_ON(ctx->refcount != 0);
+
+ list_del(&ctx->list);
+ kfree(ctx);
+}
+
+static void
+ieee80211_assign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_chanctx *ctx)
+{
+ lockdep_assert_held(&local->chanctx_mtx);
+
+ sdata->vif.chanctx_conf = &ctx->conf;
+ ctx->refcount++;
+}
+
+static void
+ieee80211_unassign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_chanctx *ctx)
+{
+ lockdep_assert_held(&local->chanctx_mtx);
+
+ ctx->refcount--;
+ sdata->vif.chanctx_conf = NULL;
+}
+
+static void
+__ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
+{
+ struct ieee80211_chanctx_conf *conf = sdata->vif.chanctx_conf;
+ struct ieee80211_chanctx *ctx =
+ container_of(conf, struct ieee80211_chanctx, conf);
+
+ lockdep_assert_held(&local->chanctx_mtx);
+
+ if (!conf)
+ return;
+
+ ieee80211_unassign_vif_chanctx(sdata, ctx);
+ if (ctx->refcount == 0)
+ ieee80211_free_chanctx(ctx);
+}
+
+bool
+ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_channel *channel,
+ enum nl80211_channel_type channel_type,
+ enum ieee80211_chanctx_mode mode)
+{
+ struct ieee80211_local *local = sdata->local;
+ struct ieee80211_chanctx *ctx;
+
+ mutex_lock(&local->chanctx_mtx);
+ __ieee80211_vif_release_channel(sdata);
+
+ ctx = ieee80211_find_chanctx(local, channel, channel_type, mode);
+ if (!ctx)
+ ctx = ieee80211_new_chanctx(local, channel, channel_type, mode);
+ if (!ctx) {
+ mutex_unlock(&local->chanctx_mtx);
+ return false;
+ }
+
+ ieee80211_assign_vif_chanctx(sdata, ctx);
+ mutex_unlock(&local->chanctx_mtx);
+ return true;
+}
+
+void
+ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
+{
+ mutex_lock(&sdata->local->chanctx_mtx);
+ __ieee80211_vif_release_channel(sdata);
+ mutex_unlock(&sdata->local->chanctx_mtx);
+}
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index e6cbf5b..0781568 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -645,6 +645,30 @@ enum ieee80211_sdata_state_bits {
SDATA_STATE_OFFCHANNEL,
};
+/**
+ * enum ieee80211_chanctx_mode - channel context configuration mode
+ *
+ * @IEEE80211_CHANCTX_SHARED: channel context may be used by
+ * multiple interfaces
+ * @IEEE80211_CHANCTX_EXCLUSIVE: channel context can be used
+ * only by a single interface. This can be used for example for
+ * non-fixed channel IBSS.
+ */
+enum ieee80211_chanctx_mode {
+ IEEE80211_CHANCTX_SHARED,
+ IEEE80211_CHANCTX_EXCLUSIVE
+};
+
+struct ieee80211_chanctx {
+ struct list_head list;
+
+ struct ieee80211_local *local;
+ enum ieee80211_chanctx_mode mode;
+ int refcount;
+
+ struct ieee80211_chanctx_conf conf;
+};
+
struct ieee80211_sub_if_data {
struct list_head list;
@@ -969,6 +993,10 @@ struct ieee80211_local {
struct ieee80211_channel *tmp_channel;
enum nl80211_channel_type tmp_channel_type;
+ /* channel contexts */
+ struct list_head chanctx_list;
+ struct mutex chanctx_mtx;
+
/* SNMP counters */
/* dot11CountersTable */
u32 dot11TransmittedFragmentCount;
@@ -1488,6 +1516,14 @@ bool ieee80211_set_channel_type(struct ieee80211_local *local,
enum nl80211_channel_type
ieee80211_ht_oper_to_channel_type(struct ieee80211_ht_operation *ht_oper);
+bool
+ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_channel *channel,
+ enum nl80211_channel_type channel_type,
+ enum ieee80211_chanctx_mode mode);
+void
+ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata);
+
#ifdef CONFIG_MAC80211_NOINLINE
#define debug_noinline noinline
#else
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index d81c178..735edd5 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -613,6 +613,9 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
spin_lock_init(&local->filter_lock);
spin_lock_init(&local->queue_stop_reason_lock);
+ INIT_LIST_HEAD(&local->chanctx_list);
+ mutex_init(&local->chanctx_mtx);
+
/*
* The rx_skb_queue is only accessed from tasklets,
* but other SKB queues are used from within IRQ
--
1.7.0.4
hi Michal,
On Tue, Jun 26, 2012 at 3:37 PM, Michal Kazior <[email protected]> wrote:
> From: Kazior Michal <[email protected]>
>
> Split functionality for further reuse.
>
> Will prevent code duplication when channel context
> channel_type merging is introduced.
>
> Signed-off-by: Michal Kazior <[email protected]>
> ---
[...]
> +static bool
> +ieee80211_channel_types_are_compatible(enum nl80211_channel_type chantype1,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?enum nl80211_channel_type chantype2,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?enum nl80211_channel_type *compat)
> +{
> + ? ? ? switch (chantype1) {
> ? ? ? ?case NL80211_CHAN_NO_HT:
> ? ? ? ?case NL80211_CHAN_HT20:
> ? ? ? ? ? ? ? ?/*
> ? ? ? ? ? ? ? ? * allow any change that doesn't go to no-HT
> ? ? ? ? ? ? ? ? * (if it already is no-HT no change is needed)
> ? ? ? ? ? ? ? ? */
> - ? ? ? ? ? ? ? if (chantype == NL80211_CHAN_NO_HT)
> + ? ? ? ? ? ? ? if (chantype2 == NL80211_CHAN_NO_HT)
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> - ? ? ? ? ? ? ? superchan = chantype;
> + ? ? ? ? ? ? ? *compat = chantype2;
> ? ? ? ? ? ? ? ?break;
> ? ? ? ?case NL80211_CHAN_HT40PLUS:
> ? ? ? ?case NL80211_CHAN_HT40MINUS:
> + ? ? ? ? ? ? ? *compat = chantype1;
this should be outside of switch, as it applies to the first cases as
well (chantype1=ht20, chantype2=no_ht will return the original *compat
value (no_ht) instead of ht20).
Eliad.
Johannes Berg wrote:
> On Thu, 2012-06-28 at 09:54 +0200, Michal Kazior wrote:
>
>>>> Yes, this is more or less what I also had in mind. I was just thinking
>>>> about solving the issue of channel context and hw.conf.channel
>>>> consistency. If we switch a channel we either modify channel in channel
>>>> context directly (violating the immutability of channel contexts) or we
>>>> iterate and re-set the new channel on each interface (because
>>>> single-channel drivers may still have multiple interfaces and we
>>>> probably want to use sdata->vif.chanctx_conf->channel instead of
>>>> hw.conf.channel inside mac80211).
>>>>
>>>> Now that I think about it I guess violating the immutability for the
>>>> single-channel case is okay. It would greatly simplify the code and we'd
>>>> just put a comment down in hw_config where the only violation would occur.
>>>
>>> I'm not sure why we would violate it? The way I see it, you'd never
>>> change the channel context channel since internally in mac80211 you'd
>>> never want to see a different channel, just like today we use
>>> local->oper_channel everywhere we'd then use sdata->vif.chanctx->channel
>>> throughout, right?
>>>
>>> I think the only thing we need to do is put something like this into
>>> hw_config:
>>>
>>> if (local->tmp_channel) {
>>> local->hw.conf.channel = local->tmp_channel;
>>> ...
>>> } else {
>>> local->hw.conf.channel = chanctx->channel;
>>> }
>>>
>>> No?
>>
>> Using sdata->vif.chanctx_conf->channel instead of local->oper_channel
>> doesn't make any sense to me.
>>
>> Take ieee80211_tx() for example. It does:
>>
>> tx.channel = local->hw.conf.channel;
>>
>> We don't use oper_channel here, but hw.conf.channel. TX can happen on
>> different interfaces so for multi-channel operation it should be saying:
>>
>> tx.channel = sdata->vif.chanctx_conf->channel;
>>
>> In this case if we want to support the swscan/tmpchan through
>> hw_config() we need update the channel context's channel somehow.
>>
>> I'm more thinking of hw.conf.channel becoming more of a backup value for
>> single-channel drivers while we internally focus on channel contexts.
>
> Yes, makes sense. I forgot all about the TX code. I'm a little wary of
> making the contexts mutable, even in this case, because a lot of code
> uses local->oper_channel as well, and that is expected to really be the
> operating channel all of the time, even if we're scanning at some point
> in time.
Yeah. The other option (maintaining the immutability) is to iterate
through all interfaces and call ieee80211_vif_use_channel when switching
channel for single-channel operation. Or do you have something else in
mind maybe?
> Luckily, tx.channel isn't actually used much, only for the band. So if
> we tag the SKBs with the band earlier (info->band), maybe we don't need
> to use hw.conf.channel as much there for tx.channel?
>
> Other uses where we do need the current channel are
> * ieee80211_build_probe_req
> * ieee80211_add_srates_ie/ieee80211_add_ext_srates_ie
> * __ieee80211_start_scan uses it but need not, could use oper_channel
> instead and the code never executes for multi-channel
> * ieee80211_set_tx_power() is interesting, may need to make it all
> per-sdata now through nl80211 etc.
What will drivers that don't support per-sdata tx_power do? Do all
multi-vif (not multi-channel) drivers support per-interface tx power?
I guess we'd have to manage:
a) common tx power value in ieee80211_local
b) provide a function that calculates the common value
so drivers may use it (and avoid code duplication)
c) ..or else drivers would need to implement the calculation on
their own
> * rate_idx_to_bitrate can use the sta's sdata's channel
> * ieee80211_change_bss can use the sdata's channel
> * debugfs stuff probably just moves to per-sdata files
> * ibss code all uses sdata channel
> * ieee80211_if_change_type ... probably just set basic_rates = 0
> * mesh can use sdata channel
> * mlme.c should use sdata channel, but there's the channel switch stuff
> * rate.h should use sta->sdata channel
>
> Much of this is actually means we have bugs today! Whenever we use
> hw.conf.channel and should be using sdata channel soon, we should be
> using local->oper_channel today!
Oh! Now I understand why you wanted to use channel contexts in place of
oper_channel. This makes sense.
> Maybe it's worth fixing that first, and getting rid of *most* instances
> of hw.conf.channel, so we have a clearer idea of which changes in what
> ways?
Sounds like a good idea.
--
Pozdrawiam / Best regards, Michal Kazior.
On Wed, 2012-06-27 at 09:30 +0200, Michal Kazior wrote:
> > How are you planning to proceed from here? I'm happy to pick these up
> > even if you're not going to work on using it right away since I think in
> > that case I would probably be working on using it pretty soon, but if
> > you are planning to work on it that's even better :-)
>
> The next step is to start using channel contexts in mac80211. I was
> thinking of using chanctx->channel instead of hw.conf.channel if
> .assign_vif_chanctx is defined. I expect some issues like using channel
> before it is set.
Ok. Thinking about this a bit I get a feeling that we should internally
fully convert mac80211 over to channel contexts and not make things
conditional. Then, whenever a new channel context is allocated it will
be the only one and we can set hw.conf.channel/type to its channel(type)
and call hw_config() instead of ... add/change chanctx?
That way, we really only need to make a distinction in the low-level
code that actually calls into the driver, and on a higher level we can
just use the channel contexts everywhere.
The biggest issue here will be CSA handling. I have no idea yet how this
will work in multi-channel, maybe we need to disconnect all other
interfaces on the same channel, or so?
> Hwsim requires .hw_scan and ROC to be implemented to test channel
> contexts. hw_scan should be simple (if we skip a few things..) but
> haven't looked into the ROC case yet. It also requires some changes to
> move channel logic per-vif.
Good points.
johannes
From: Kazior Michal <[email protected]>
Reuse channels with compatible channel types. Some
channel types are compatible and can be used
concurrently.
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/chan.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 56 insertions(+), 0 deletions(-)
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index fe1ea01..1a445da 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -167,6 +167,7 @@ ieee80211_find_chanctx(struct ieee80211_local *local,
enum ieee80211_chanctx_mode mode)
{
struct ieee80211_chanctx *ctx;
+ enum nl80211_channel_type compat_type;
lockdep_assert_held(&local->chanctx_mtx);
@@ -176,12 +177,22 @@ ieee80211_find_chanctx(struct ieee80211_local *local,
return NULL;
list_for_each_entry(ctx, &local->chanctx_list, list) {
+ compat_type = ctx->conf.channel_type;
+
if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE)
continue;
if (ctx->conf.channel != channel)
continue;
if (ctx->conf.channel_type != channel_type)
continue;
+ if (!ieee80211_channel_types_are_compatible(ctx->conf.channel_type,
+ channel_type,
+ &compat_type))
+ continue;
+ if (ctx->conf.channel_type != compat_type) {
+ ctx->conf.channel_type = compat_type;
+ drv_change_chantype(local, ctx);
+ }
return ctx;
}
@@ -240,6 +251,49 @@ ieee80211_assign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
drv_assign_vif_chanctx(sdata->local, sdata, ctx);
}
+static enum nl80211_channel_type
+ieee80211_calc_chantype(struct ieee80211_local *local,
+ struct ieee80211_chanctx *ctx)
+{
+ struct ieee80211_chanctx_conf *conf = &ctx->conf;
+ struct ieee80211_sub_if_data *sdata;
+ enum nl80211_channel_type chantype = NL80211_CHAN_NO_HT;
+ enum nl80211_channel_type compat;
+
+ lockdep_assert_held(&local->chanctx_mtx);
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+ if (!ieee80211_sdata_running(sdata))
+ continue;
+ if (sdata->vif.chanctx_conf != conf)
+ continue;
+
+ BUG_ON(!ieee80211_channel_types_are_compatible(
+ conf->channel_type, chantype, &compat));
+
+ chantype = compat;
+ }
+ rcu_read_unlock();
+
+ return chantype;
+}
+
+static void
+ieee80211_recalc_chanctx_chantype(struct ieee80211_local *local,
+ struct ieee80211_chanctx *ctx)
+{
+ enum nl80211_channel_type chantype;
+
+ lockdep_assert_held(&local->chanctx_mtx);
+
+ chantype = ieee80211_calc_chantype(local, ctx);
+ if (chantype != ctx->conf.channel_type) {
+ ctx->conf.channel_type = chantype;
+ drv_change_chantype(local, ctx);
+ }
+}
+
static void
ieee80211_unassign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
struct ieee80211_chanctx *ctx)
@@ -248,6 +302,8 @@ ieee80211_unassign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
drv_unassign_vif_chanctx(sdata->local, sdata, ctx);
+ ieee80211_recalc_chanctx_chantype(sdata->local, ctx);
+
ctx->refcount--;
sdata->vif.chanctx_conf = NULL;
}
--
1.7.0.4
On Thu, 2012-06-28 at 09:01 +0200, Michal Kazior wrote:
> >> Hmm.. Yeah we need to now, sort of. FullMAC drivers could suffer, but we
> >> could drop connections upon CSA in mac80211 for the time being.
> >>
> >> cfg80211 could provide:
> >> * cfg80211_sta_can_switch_chan
> >> * cfg80211_sta_ch_switch_notify
> >>
> >> mac80211 could then be able to know whether num_different_channels has
> >> been reached (cfg80211_sta_can_switch_chan) and eventually notify upon
> >> channel switch (cfg80211_sta_ch_switch_notify).
> >>
> >> Channel switch would happen if either:
> >> * cfg80211_sta_can_switch_chan is true
> >> * channel context for target CSA channel already exists
> >>
> >> I'm just unsure about the current_bss thing - whether we should e.g.
> >> initiate a scan to update it, or can we shamelessly update the channel
> >> in the structure directly?
> >
> > Today mac80211 just updates it, yeah.
>
> Should I send the ,,respect channels in iface combinations'' as a
> [PATCH] now, or should the CSA issue (in cfg80211) be addressed first?
Good question. Even if we don't do CSA first, I guess we don't really
break anything right now, do we? And if the driver/mac80211 just changes
the channel, that means it could seem to go over the limit of 1 channel
but things just break down like they do today?
I guess I'd like to have the CSA stuff addressed, but it seems we
haven't really closed on the best way yet, and I don't want to hold off
your work for it for too long?
johannes
On Tue, 2012-06-26 at 16:01 +0200, Michal Kazior wrote:
> Johannes Berg wrote:
> > On Tue, 2012-06-26 at 14:37 +0200, Michal Kazior wrote:
> >> From: Kazior Michal <[email protected]>
> >
> > In this and a few other patches the From: seems to be wrong?
>
> The mail server does tricks on me. I'll have to check msmtp config.
No, not the From in the email, the From you put as the first line into
the patch -- if you're using git that comes from the commit author
field.
johannes
From: Kazior Michal <[email protected]>
Introduces channel context callbacks. Channel on a
context channel is immutable. Channel type will be
changable later though, thus change_chantype is
adveristed.
Signed-off-by: Michal Kazior <[email protected]>
---
include/net/mac80211.h | 22 ++++++++++++++++++++++
1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d64fa82..1cdce79 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2279,6 +2279,15 @@ enum ieee80211_rate_control_changed {
* @get_et_strings: Ethtool API to get a set of strings to describe stats
* and perhaps other supported types of ethtool data-sets.
*
+ * @add_chanctx: Notifies device driver about new channel context creation.
+ * @remove_chanctx: Notifies device driver about channel context destruction.
+ * @change_chantype: Notifies device driver about channel context channel_type
+ * change which may happen when combining different vifs on a same channel
+ * with different HTs.
+ * @assign_vif_chanctx: Notifies device driver about channel context being bound
+ * to vif. Possible use is for hw queue remapping.
+ * @unassign_vif_chanctx: Notifies device driver about channel context being
+ * unbound from vif.
*/
struct ieee80211_ops {
void (*tx)(struct ieee80211_hw *hw, struct sk_buff *skb);
@@ -2418,6 +2427,19 @@ struct ieee80211_ops {
void (*get_et_strings)(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
u32 sset, u8 *data);
+
+ void (*add_chanctx)(struct ieee80211_hw *hw,
+ struct ieee80211_chanctx_conf *ctx);
+ void (*remove_chanctx)(struct ieee80211_hw *hw,
+ struct ieee80211_chanctx_conf *ctx);
+ void (*change_chantype)(struct ieee80211_hw *hw,
+ struct ieee80211_chanctx_conf *ctx);
+ void (*assign_vif_chanctx)(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ struct ieee80211_chanctx_conf *ctx);
+ void (*unassign_vif_chanctx)(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ struct ieee80211_chanctx_conf *ctx);
};
/**
--
1.7.0.4
On Tue, 2012-06-26 at 14:37 +0200, Michal Kazior wrote:
> +static enum nl80211_channel_type
> +ieee80211_calc_chantype(struct ieee80211_local *local,
> + struct ieee80211_chanctx *ctx)
> +{
> + struct ieee80211_chanctx_conf *conf = &ctx->conf;
> + struct ieee80211_sub_if_data *sdata;
> + enum nl80211_channel_type chantype = NL80211_CHAN_NO_HT;
> + enum nl80211_channel_type compat;
> +
> + lockdep_assert_held(&local->chanctx_mtx);
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> + if (!ieee80211_sdata_running(sdata))
> + continue;
> + if (sdata->vif.chanctx_conf != conf)
> + continue;
> +
> + BUG_ON(!ieee80211_channel_types_are_compatible(
> + conf->channel_type, chantype, &compat));
Please no BUG_ON, maybe only WARN_ON_ONCE even?
> + chantype = compat;
> + }
> + rcu_read_unlock();
> +
> + return chantype;
> +}
I don't think I understand this, wouldn't it need some per-vif requested
channel type to work correctly? I don't see any chantype values coming
from the sdata here, so I don't think this could work?
If say vif1 needs ht40+ and vif2 ht20, the channel context will be set
to ht40+, but then if vif1 goes away and you don't know anything about
vif2 at all, then how can this work? The way I see it, your code here
doesn't really do anything, except recalculate that ht40+ is compatible
with ht40+, or something like that? I guess I'm a bit confused, it seems
to me that this cannot work even in theory unless you have per-vif data.
> static void
> ieee80211_unassign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
> struct ieee80211_chanctx *ctx)
> @@ -248,6 +302,8 @@ ieee80211_unassign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
>
> drv_unassign_vif_chanctx(sdata->local, sdata, ctx);
>
> + ieee80211_recalc_chanctx_chantype(sdata->local, ctx);
> +
> ctx->refcount--;
> sdata->vif.chanctx_conf = NULL;
And then shouldn't you recalc *after* setting chanctx_conf = NULL so you
skip this vif?
johannes
From: Kazior Michal <[email protected]>
Split functionality for further reuse.
Will prevent code duplication when channel context
channel_type merging is introduced.
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/chan.c | 57 ++++++++++++++++++++++++++++++++++-----------------
1 files changed, 38 insertions(+), 19 deletions(-)
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 1aa553d..fe1ea01 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -69,16 +69,14 @@ ieee80211_get_channel_mode(struct ieee80211_local *local,
return mode;
}
-bool ieee80211_set_channel_type(struct ieee80211_local *local,
- struct ieee80211_sub_if_data *sdata,
- enum nl80211_channel_type chantype)
+static enum nl80211_channel_type
+ieee80211_get_superchan(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata)
{
- struct ieee80211_sub_if_data *tmp;
enum nl80211_channel_type superchan = NL80211_CHAN_NO_HT;
- bool result;
+ struct ieee80211_sub_if_data *tmp;
mutex_lock(&local->iflist_mtx);
-
list_for_each_entry(tmp, &local->interfaces, list) {
if (tmp == sdata)
continue;
@@ -104,41 +102,62 @@ bool ieee80211_set_channel_type(struct ieee80211_local *local,
break;
}
}
+ mutex_unlock(&local->iflist_mtx);
+
+ return superchan;
+}
- switch (superchan) {
+static bool
+ieee80211_channel_types_are_compatible(enum nl80211_channel_type chantype1,
+ enum nl80211_channel_type chantype2,
+ enum nl80211_channel_type *compat)
+{
+ switch (chantype1) {
case NL80211_CHAN_NO_HT:
case NL80211_CHAN_HT20:
/*
* allow any change that doesn't go to no-HT
* (if it already is no-HT no change is needed)
*/
- if (chantype == NL80211_CHAN_NO_HT)
+ if (chantype2 == NL80211_CHAN_NO_HT)
break;
- superchan = chantype;
+ *compat = chantype2;
break;
case NL80211_CHAN_HT40PLUS:
case NL80211_CHAN_HT40MINUS:
+ *compat = chantype1;
/* allow smaller bandwidth and same */
- if (chantype == NL80211_CHAN_NO_HT)
+ if (chantype2 == NL80211_CHAN_NO_HT)
break;
- if (chantype == NL80211_CHAN_HT20)
+ if (chantype2 == NL80211_CHAN_HT20)
break;
- if (superchan == chantype)
+ if (chantype2 == chantype1)
break;
- result = false;
- goto out;
+ return false;
}
- local->_oper_channel_type = superchan;
+ return true;
+}
+
+bool ieee80211_set_channel_type(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ enum nl80211_channel_type chantype)
+{
+ enum nl80211_channel_type superchan;
+ enum nl80211_channel_type compatchan = NL80211_CHAN_NO_HT;
+
+ superchan = ieee80211_get_superchan(local, sdata);
+ if (!ieee80211_channel_types_are_compatible(superchan, chantype,
+ &compatchan))
+ return false;
+
+ local->_oper_channel_type = compatchan;
if (sdata)
sdata->vif.bss_conf.channel_type = chantype;
- result = true;
- out:
- mutex_unlock(&local->iflist_mtx);
+ return true;
- return result;
}
static struct ieee80211_chanctx *
--
1.7.0.4
On Wed, 2012-06-27 at 14:43 +0200, Michal Kazior wrote:
> > Good questions :-)
> >
> > I was thinking that today we have tmp_channel & oper_channel (in local),
> > and oper_channel would follow the (single) channel context (if there
> > even is one, otherwise any random channel is fine), and hw.conf.channel
> > would still be tmp_channel when that is in used for scanning/roc?
>
> I'm still confused.
>
> Okay here's what I think:
>
> Our main concern right now is swscan and tmpchan for legacy operation.
> We need to know whether a driver requires us to support swscan/tmpchan
> or not. We could possibly:
>
> a) if .hw_scan and .remain_on_channel are implemented use we
> channel contexts only.
> We then automatically require driver to handle all chanctx related
> ieee80211_ops'. If it's missing we fail with -EINVAL in the
> ieee80211_hw_register.
No, I don't think we can do that, since some drivers today have
hw_scan/roc but don't want multi-channel.
> b) we could add a new flag a driver may report, e.g.
> IEEE80211_HW_SUPPORTS_CHANCTX
> (along with appropriate checks mentioned above)
I don't think that's needed?
I think we should do
c) if the driver advertises support for multiple channels in its
interface combinations or implements the channel context callbacks,
it must have hw_scan/roc, otherwise we fail hw registration; if it
advertises support for multiple channels obviously it must have all
the channel context callbacks
This would leave all existing drivers operating as-is, the next step
would be adding channel context support to a driver (must have hw
scan/roc in that case), and the next step after that would be actually
advertising support for multiple channels -- in practice it's probably
just a single patch doing both but that's the logical order then.
> For legacy operation we'd need to iterate through active interface in
> hw_config() and call ieee80211_vif_use_channel() for each. This would
> allow us to have the same channel context across all interfaces (so we
> can virtually use channel context everywhere instead of
> hw.conf.channel). This means the .assign_vif_chanctx cannot sleep if my
> understanding is correct (we'd need to use RCU locking for iteration
> since devlist lock may be held while hw_config() is caled).
>
> What do you think?
What 'legacy operation' are you referring to?
In any case, I think you're turning it upside down. I think we should
get rid of local->oper_channel(_type) completely, and instead use the
channel contexts in mac80211 everywhere. If the driver doesn't implement
channel contexts it can only support a single channel. Thus, we can have
at most one channel context, so whenever a new context is added (there
could be zero) or any context is modified (the only one) we can set
hw.conf.channel and call hw_config() with the CHANNEL_CHANGE flag.
IOW, nothing in mac80211 would ever call hw_config() for the channel or
channel type change, it would all do channel contexts, but the channel
context code would see that if the driver doesn't support channel
contexts
1) there will be at most one context in mac80211
2) this context is programmed into the device by using hw_config()
instead of the context callbacks
> > That was intended for AP mode, I'm not sure it's good for station mode,
> > in station mode we need to have current_bss->channel updated as well in
> > that case.
> >
> >> This would probably require some more
> >> additional changes too (maybe with regard to channel tracking too).
> >>
> >> Worst-case we disconnect other interfaces. We might be able to create a
> >> new channel context (provided num_different_channels hasn't been reached
> >> yet) or reuse an existing channel context (provided CSA happens to
> >> target a channel we have a channel context for already).
> >
> > Yeah ... we need to think about it more. I thought we could put it off a
> > bit longer, but I guess with the channel tracking we already need it?
>
> Hmm.. Yeah we need to now, sort of. FullMAC drivers could suffer, but we
> could drop connections upon CSA in mac80211 for the time being.
>
> cfg80211 could provide:
> * cfg80211_sta_can_switch_chan
> * cfg80211_sta_ch_switch_notify
>
> mac80211 could then be able to know whether num_different_channels has
> been reached (cfg80211_sta_can_switch_chan) and eventually notify upon
> channel switch (cfg80211_sta_ch_switch_notify).
>
> Channel switch would happen if either:
> * cfg80211_sta_can_switch_chan is true
> * channel context for target CSA channel already exists
>
> I'm just unsure about the current_bss thing - whether we should e.g.
> initiate a scan to update it, or can we shamelessly update the channel
> in the structure directly?
Today mac80211 just updates it, yeah.
johannes
Just FYI, Michal said he doesn't have time to work on this right now so
I'm adopting these patches.
johannes