Hi,
I'm trying to implement multi-BSS CSA. So far I've
prepared some cfg80211 patches and I'd like to get
feedback whether this kind of apprach is okay or
not.
Michal Kazior (4):
nl80211: make chandef parser take nlattr arg
cfg80211: rename function argument to be more readable
cfg80211: implement multi-vif interface combination validation
cfg80211: implement multi-BSS CSA
include/net/cfg80211.h | 10 +-
include/uapi/linux/nl80211.h | 2 +
net/mac80211/cfg.c | 10 +-
net/wireless/core.h | 36 +++++--
net/wireless/nl80211.c | 227 +++++++++++++++++++++++++++++--------------
net/wireless/rdev-ops.h | 8 +-
net/wireless/trace.h | 29 +-----
net/wireless/util.c | 158 ++++++++++++++++++------------
8 files changed, 306 insertions(+), 174 deletions(-)
--
1.8.4.rc3
On 15 January 2014 10:39, Johannes Berg <[email protected]> wrote:
> On Wed, 2014-01-15 at 07:52 +0100, Michal Kazior wrote:
>
>> So you're basically saying the new attribute should be used as an
>> array attribute instead. Sounds good, but..
>>
>> What about backward compatibility? Do we not care? Or should we
>> preserve old parsing for single-interface CSA (i.e. older hostapd)?
>
> I think we have to preserve the old parsing if the new isn't present,
> but that shouldn't be all that difficult?
>
> if (info->attrs[MULTI])
> for_each_attr(a, info->attrs[MULTI])
> parse_nested(a, attrs);
> parse(attrs);
> else
> parse(info->attrs);
>
> or something like that, right?
Agreed.
>> > is more error prone as it would allow older kernels to parse the whole
>> > thing while ignoring the next/more/whatever, so you'd get some weird
>> > subset of the intended behaviour. Forcing *all* interfaces into the
>> > sub-attribute when more than one is desired (or in fact for a single
>> > one, if you're ok with requiring a kernel with support) would IMHO be
>> > less error prone.
>>
>> From a practical point of view cfg80211 should deny such a request due
>> to multi-channel (interface combination) and by (the only CSA
>> implementator in upstream) mac80211 due to chanctx->refcount > 1.
>
> Hmm, true. I'd still prefer the other version, the stacked/nested one
> kinda makes me uncomfortable ... It'll work, no doubt about it, but the
> deep nesting I'm not really happy with.
I'll work on that then.
Michał
On 14 January 2014 17:21, Johannes Berg <[email protected]> wrote:
> On Wed, 2014-01-08 at 16:26 +0100, Michal Kazior wrote:
>> This extends NL80211_CMD_CHANNEL_SWITCH by adding
>> a new attribute NL80211_ATTR_SWITCH_NEXT. The
>> attribute holds nested channel switch attributes
>> (including itself) allowing
>> multi-interface/chained CSA.
>
> That's ... creative? Interesting? I'm not sure I like that API much.
>
> OTOH, it's a neat way of reusing all the attributes that we have, etc.
>
> However, I think it'd be easier to implement on both sides of the fence
> if we structured this differently and made the "more vifs" to be a
> nested attribute with data inside each one of them.
>
> Actually, I'd suggest that in the multi-vif case all of the attributes
> should be inside the list, like this:
>
> wiphy, chswitch=[1=[ifidx,...], 2=[ifidx,...], 3=[ifidx,...]]
>
> alternatively, maybe something that would be nice would be
>
> wiphy, chswitch=[ifidx1=[...], ifidx2=[...], ifidx3=[...]]
>
> but the common way would be to ignore the array index completely.
So you're basically saying the new attribute should be used as an
array attribute instead. Sounds good, but..
What about backward compatibility? Do we not care? Or should we
preserve old parsing for single-interface CSA (i.e. older hostapd)?
> Today you get
>
> wiphy, ifidx, ..., next=[ifidx, ..., next=[ifidx, ..., next=[...]]]
>
> and I don't really like the deep nesting.
>
> I would also argue that doing
> wiphy, ifidx1, ..., more=[1=[ifidx2, ...], ...]
> or
> wiphy, ifidx1, ..., next=[...]
>
> is more error prone as it would allow older kernels to parse the whole
> thing while ignoring the next/more/whatever, so you'd get some weird
> subset of the intended behaviour. Forcing *all* interfaces into the
> sub-attribute when more than one is desired (or in fact for a single
> one, if you're ok with requiring a kernel with support) would IMHO be
> less error prone.
>From a practical point of view cfg80211 should deny such a request due
to multi-channel (interface combination) and by (the only CSA
implementator in upstream) mac80211 due to chanctx->refcount > 1.
> Haven't looked over the code in much detail, but a few small comments:
> * I think you should preserve tracing, it has facilities for arrays too
I didn't want to bother with that for the recursively nested structure
for the initial implementation.
> * hard-coding a limit of 8 vifs seems pointless, you can easily
> allocate dynamic
> memory, no?
I wanted to avoid that for the initial implementation.
Also, there's a locking bug in this patch - wdev_lock shouldn't be
taken by the channel_switch in nl80211. The lock should be taken by
the callee for each interface it processes later on.
Michal
On Wed, 2014-01-08 at 16:26 +0100, Michal Kazior wrote:
> This refactors cfg80211_can_use_iftype_chan() so
> that it can process multi-interface combination
> changes.
>
> With this it will be possible to handle, e.g.
> multi-BSS channel switching.
This seems reasonable, though I'd say that this code:
> + for (i = 0; i < num_params; i++) {
> + if (WARN_ON(hweight32(params[i].radar_detect_width) > 1))
> + return -EINVAL;
> +
> + /* sanity check - make sure all wdevs in params[] are unique */
> + for (j = 0; j < num_params; j++)
> + if (WARN_ON(i != j && params[i].wdev == params[j].wdev))
> + return -EINVAL;
could then use some refactoring, e.g. by moving the inside of the loop
to another helper function.
johannes
Unifies variable naming regarding radar detection
width.
Signed-off-by: Michal Kazior <[email protected]>
---
net/wireless/core.h | 2 +-
net/wireless/util.c | 14 +++++++-------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 0a277c3..38f74fe 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -379,7 +379,7 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
enum nl80211_iftype iftype,
struct ieee80211_channel *chan,
enum cfg80211_chan_mode chanmode,
- u8 radar_detect);
+ u8 radar_detect_width);
/**
* cfg80211_chandef_dfs_usable - checks if chandef is DFS usable
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 935dea9..41013f1 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1238,7 +1238,7 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
enum nl80211_iftype iftype,
struct ieee80211_channel *chan,
enum cfg80211_chan_mode chanmode,
- u8 radar_detect)
+ u8 radar_detect_width)
{
struct wireless_dev *wdev_iter;
u32 used_iftypes = BIT(iftype);
@@ -1254,7 +1254,7 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
ASSERT_RTNL();
- if (WARN_ON(hweight32(radar_detect) > 1))
+ if (WARN_ON(hweight32(radar_detect_width) > 1))
return -EINVAL;
switch (iftype) {
@@ -1268,7 +1268,7 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
* then mark DFS as required.
*/
if (!chan) {
- if (chanmode != CHAN_MODE_UNDEFINED && radar_detect)
+ if (chanmode != CHAN_MODE_UNDEFINED && radar_detect_width)
radar_required = true;
break;
}
@@ -1285,12 +1285,12 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
return -EINVAL;
}
- if (radar_required && !radar_detect)
+ if (radar_required && !radar_detect_width)
return -EINVAL;
/* Always allow software iftypes */
if (rdev->wiphy.software_iftypes & BIT(iftype)) {
- if (radar_detect)
+ if (radar_detect_width)
return -EINVAL;
return 0;
}
@@ -1366,7 +1366,7 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
used_iftypes |= BIT(wdev_iter->iftype);
}
- if (total == 1 && !radar_detect)
+ if (total == 1 && !radar_detect_width)
return 0;
for (i = 0; i < rdev->wiphy.n_iface_combinations; i++) {
@@ -1399,7 +1399,7 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
}
}
- if (radar_detect && !(c->radar_detect_widths & radar_detect))
+ if (radar_detect_width && !(c->radar_detect_widths & radar_detect_width))
goto cont;
/*
--
1.8.4.rc3
On Wed, 2014-01-08 at 16:26 +0100, Michal Kazior wrote:
> Unifies variable naming regarding radar detection
> width.
I think this is dangerous - "radar_detect_width" could easily be read as
an enum value, rather than a bitfield. I do wonder why
cfg80211_can_use_iftype_chan() gets passed a bitfield though, it might
be smarter to make that get an enum value for added type checking.
Actually I guess the reason is that there's no "no radar checking" value
in the enum then ...
At least for _widths, it's clear that it must be a bitfield since
otherwise more than one width wouldn't be possible.
> - if (radar_detect && !(c->radar_detect_widths & radar_detect))
> + if (radar_detect_width && !(c->radar_detect_widths & radar_detect_width))
That also makes it extend far beyond 80 columns.
johannes
This extends NL80211_CMD_CHANNEL_SWITCH by adding
a new attribute NL80211_ATTR_SWITCH_NEXT. The
attribute holds nested channel switch attributes
(including itself) allowing
multi-interface/chained CSA.
This makes it possible for userspace to request a
multi-interface channel switch while allowing
sanity checks wrt target interface/channel
combination.
This changes channel_switch() cfg80211 op to pass
different set of parameters. The only driver that
is affected by this is mac80211.
This change should not affect any prior CSA
behaviour.
Signed-off-by: Michal Kazior <[email protected]>
---
include/net/cfg80211.h | 10 ++-
include/uapi/linux/nl80211.h | 2 +
net/mac80211/cfg.c | 10 ++-
net/wireless/nl80211.c | 188 +++++++++++++++++++++++++++++++------------
net/wireless/rdev-ops.h | 8 +-
net/wireless/trace.h | 29 ++-----
6 files changed, 161 insertions(+), 86 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index e9abc7b..1d57f97 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -688,6 +688,7 @@ struct cfg80211_ap_settings {
* @count: number of beacons until switch
*/
struct cfg80211_csa_settings {
+ struct net_device *dev;
struct cfg80211_chan_def chandef;
struct cfg80211_beacon_data beacon_csa;
u16 counter_offset_beacon, counter_offset_presp;
@@ -697,6 +698,8 @@ struct cfg80211_csa_settings {
u8 count;
};
+#define CFG80211_MAX_NUM_CSA_SETTINGS 8
+
/**
* enum station_parameters_apply_mask - station parameter values to apply
* @STATION_PARAM_APPLY_UAPSD: apply new uAPSD parameters (uapsd_queues, max_sp)
@@ -2207,7 +2210,8 @@ struct cfg80211_mgmt_tx_params {
* reliability. This operation can not fail.
* @set_coalesce: Set coalesce parameters.
*
- * @channel_switch: initiate channel-switch procedure (with CSA)
+ * @channel_switch: initiate channel-switch procedure (with CSA).
+ * num_params is always >= 1.
*/
struct cfg80211_ops {
int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -2447,8 +2451,8 @@ struct cfg80211_ops {
struct cfg80211_coalesce *coalesce);
int (*channel_switch)(struct wiphy *wiphy,
- struct net_device *dev,
- struct cfg80211_csa_settings *params);
+ struct cfg80211_csa_settings *params,
+ int num_params);
};
/*
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 129b7b0..6d3b59c 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1839,6 +1839,8 @@ enum nl80211_attrs {
NL80211_ATTR_SUPPORT_5_MHZ,
NL80211_ATTR_SUPPORT_10_MHZ,
+ NL80211_ATTR_CH_SWITCH_NEXT,
+
/* add attributes here, update the policy in nl80211.c */
__NL80211_ATTR_AFTER_LAST,
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index f80e8c4..ef13120 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3047,9 +3047,11 @@ unlock:
sdata_unlock(sdata);
}
-static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
- struct cfg80211_csa_settings *params)
+static int ieee80211_channel_switch(struct wiphy *wiphy,
+ struct cfg80211_csa_settings *params,
+ int num_params)
{
+ struct net_device *dev = params[0].dev;
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
struct ieee80211_local *local = sdata->local;
struct ieee80211_chanctx_conf *chanctx_conf;
@@ -3059,6 +3061,10 @@ static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
lockdep_assert_held(&sdata->wdev.mtx);
+ /* multi-vif CSA is not implemented */
+ if (num_params > 1)
+ return -EOPNOTSUPP;
+
if (!list_empty(&local->roc_list) || local->scanning)
return -EBUSY;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 33962ef..20cbcbd 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5709,12 +5709,18 @@ static int nl80211_channel_switch(struct sk_buff *skb, struct genl_info *info)
struct cfg80211_registered_device *rdev = info->user_ptr[0];
struct net_device *dev = info->user_ptr[1];
struct wireless_dev *wdev = dev->ieee80211_ptr;
- struct cfg80211_csa_settings params;
- /* csa_attrs is defined static to avoid waste of stack size - this
+ /* static variables avoid waste of stack size - this
* function is called under RTNL lock, so this should not be a problem.
*/
static struct nlattr *csa_attrs[NL80211_ATTR_MAX+1];
- u8 radar_detect_width = 0;
+ static struct nlattr *next_attrs[NL80211_ATTR_MAX+1];
+ static struct cfg80211_csa_settings
+ all_params[CFG80211_MAX_NUM_CSA_SETTINGS] = {};
+ static struct cfg80211_iftype_chan_param
+ all_ifch_params[CFG80211_MAX_NUM_CSA_SETTINGS] = {};
+ struct cfg80211_csa_settings *params = &all_params[0];
+ struct cfg80211_iftype_chan_param *ifch_params = &all_ifch_params[0];
+ struct nlattr **attrs, *next_csa;
int err;
bool need_new_beacon = false;
@@ -5722,111 +5728,187 @@ static int nl80211_channel_switch(struct sk_buff *skb, struct genl_info *info)
!(rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH))
return -EOPNOTSUPP;
+ attrs = info->attrs;
+
+again:
+ if (!attrs[NL80211_ATTR_IFINDEX]) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ dev = dev_get_by_index(genl_info_net(info),
+ nla_get_u32(attrs[NL80211_ATTR_IFINDEX]));
+ if (!dev) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ params->dev = dev;
+ wdev = dev->ieee80211_ptr;
+ if (!wdev) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ if (!netif_running(dev)) {
+ err = -EINVAL;
+ goto out;
+ }
+
switch (dev->ieee80211_ptr->iftype) {
case NL80211_IFTYPE_AP:
case NL80211_IFTYPE_P2P_GO:
need_new_beacon = true;
/* useless if AP is not running */
- if (!wdev->beacon_interval)
- return -EINVAL;
+ if (!wdev->beacon_interval) {
+ err = -EINVAL;
+ goto out;
+ }
break;
case NL80211_IFTYPE_ADHOC:
case NL80211_IFTYPE_MESH_POINT:
break;
default:
- return -EOPNOTSUPP;
+ err = -EOPNOTSUPP;
+ goto out;
}
- memset(¶ms, 0, sizeof(params));
-
- if (!info->attrs[NL80211_ATTR_WIPHY_FREQ] ||
- !info->attrs[NL80211_ATTR_CH_SWITCH_COUNT])
- return -EINVAL;
+ if (!attrs[NL80211_ATTR_WIPHY_FREQ] ||
+ !attrs[NL80211_ATTR_CH_SWITCH_COUNT]) {
+ err = -EINVAL;
+ goto out;
+ }
/* only important for AP, IBSS and mesh create IEs internally */
- if (need_new_beacon && !info->attrs[NL80211_ATTR_CSA_IES])
- return -EINVAL;
+ if (need_new_beacon && !attrs[NL80211_ATTR_CSA_IES]) {
+ err = -EINVAL;
+ goto out;
+ }
- params.count = nla_get_u32(info->attrs[NL80211_ATTR_CH_SWITCH_COUNT]);
+ params->count = nla_get_u32(attrs[NL80211_ATTR_CH_SWITCH_COUNT]);
if (!need_new_beacon)
goto skip_beacons;
- err = nl80211_parse_beacon(info->attrs, ¶ms.beacon_after);
+ err = nl80211_parse_beacon(attrs, ¶ms->beacon_after);
if (err)
- return err;
+ goto out;
err = nla_parse_nested(csa_attrs, NL80211_ATTR_MAX,
- info->attrs[NL80211_ATTR_CSA_IES],
+ attrs[NL80211_ATTR_CSA_IES],
nl80211_policy);
if (err)
- return err;
+ goto out;
- err = nl80211_parse_beacon(csa_attrs, ¶ms.beacon_csa);
+ err = nl80211_parse_beacon(csa_attrs, ¶ms->beacon_csa);
if (err)
- return err;
+ goto out;
- if (!csa_attrs[NL80211_ATTR_CSA_C_OFF_BEACON])
- return -EINVAL;
+ if (!csa_attrs[NL80211_ATTR_CSA_C_OFF_BEACON]) {
+ err = -EINVAL;
+ goto out;
+ }
- params.counter_offset_beacon =
+ params->counter_offset_beacon =
nla_get_u16(csa_attrs[NL80211_ATTR_CSA_C_OFF_BEACON]);
- if (params.counter_offset_beacon >= params.beacon_csa.tail_len)
- return -EINVAL;
+ if (params->counter_offset_beacon >= params->beacon_csa.tail_len) {
+ err = -EINVAL;
+ goto out;
+ }
/* sanity check - counters should be the same */
- if (params.beacon_csa.tail[params.counter_offset_beacon] !=
- params.count)
- return -EINVAL;
+ if (params->beacon_csa.tail[params->counter_offset_beacon] !=
+ params->count) {
+ err = -EINVAL;
+ goto out;
+ }
if (csa_attrs[NL80211_ATTR_CSA_C_OFF_PRESP]) {
- params.counter_offset_presp =
+ params->counter_offset_presp =
nla_get_u16(csa_attrs[NL80211_ATTR_CSA_C_OFF_PRESP]);
- if (params.counter_offset_presp >=
- params.beacon_csa.probe_resp_len)
- return -EINVAL;
+ if (params->counter_offset_presp >=
+ params->beacon_csa.probe_resp_len) {
+ err = -EINVAL;
+ goto out;
+ }
- if (params.beacon_csa.probe_resp[params.counter_offset_presp] !=
- params.count)
- return -EINVAL;
+ if (params->beacon_csa.probe_resp[params->counter_offset_presp] !=
+ params->count) {
+ err = -EINVAL;
+ goto out;
+ }
}
skip_beacons:
- err = nl80211_parse_chandef(rdev, info->attrs, ¶ms.chandef);
+ err = nl80211_parse_chandef(rdev, attrs, ¶ms->chandef);
if (err)
- return err;
+ goto out;
- if (!cfg80211_reg_can_beacon(&rdev->wiphy, ¶ms.chandef))
- return -EINVAL;
+ if (!cfg80211_reg_can_beacon(&rdev->wiphy, ¶ms->chandef)) {
+ err = -EINVAL;
+ goto out;
+ }
if (dev->ieee80211_ptr->iftype == NL80211_IFTYPE_AP ||
dev->ieee80211_ptr->iftype == NL80211_IFTYPE_P2P_GO ||
dev->ieee80211_ptr->iftype == NL80211_IFTYPE_ADHOC) {
err = cfg80211_chandef_dfs_required(wdev->wiphy,
- ¶ms.chandef);
+ ¶ms->chandef);
if (err < 0) {
- return err;
+ err = -EINVAL;
+ goto out;
} else if (err) {
- radar_detect_width = BIT(params.chandef.width);
- params.radar_required = true;
+ params->radar_required = true;
}
}
- err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
- params.chandef.chan,
- CHAN_MODE_SHARED,
- radar_detect_width);
- if (err)
- return err;
+ if (attrs[NL80211_ATTR_CH_SWITCH_BLOCK_TX])
+ params->block_tx = true;
+
+ ifch_params->wdev = wdev;
+ ifch_params->iftype = wdev->iftype;
+ ifch_params->chan = params->chandef.chan;
+ ifch_params->chanmode = CHAN_MODE_SHARED;
+ ifch_params->radar_detect_width = params->radar_required
+ ? BIT(params->chandef.width)
+ : 0;
+ ifch_params++;
+ params++;
+
+ next_csa = attrs[NL80211_ATTR_CH_SWITCH_NEXT];
+ if (next_csa) {
+ memset(next_attrs, 0, sizeof(next_attrs));
- if (info->attrs[NL80211_ATTR_CH_SWITCH_BLOCK_TX])
- params.block_tx = true;
+ if (params - all_params >= ARRAY_SIZE(all_params)) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ err = nla_parse_nested(next_attrs, NL80211_ATTR_MAX, next_csa,
+ nl80211_policy);
+ if (err)
+ goto out;
+
+ attrs = next_attrs;
+ goto again;
+ }
+
+ err = cfg80211_can_use_iftype_chan_params(rdev, all_ifch_params,
+ ifch_params -
+ all_ifch_params);
+ if (err)
+ goto out;
wdev_lock(wdev);
- err = rdev_channel_switch(rdev, dev, ¶ms);
+ err = rdev_channel_switch(rdev, all_params, params - all_params);
wdev_unlock(wdev);
+out:
+ for (; params - all_params >= 0; params--)
+ if (params->dev)
+ dev_put(params->dev);
+
return err;
}
@@ -9593,7 +9675,7 @@ static const struct genl_ops nl80211_ops[] = {
.doit = nl80211_channel_switch,
.policy = nl80211_policy,
.flags = GENL_ADMIN_PERM,
- .internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+ .internal_flags = NL80211_FLAG_NEED_WIPHY |
NL80211_FLAG_NEED_RTNL,
},
};
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index a6c03ab..6d7cacf 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -921,13 +921,13 @@ static inline void rdev_crit_proto_stop(struct cfg80211_registered_device *rdev,
}
static inline int rdev_channel_switch(struct cfg80211_registered_device *rdev,
- struct net_device *dev,
- struct cfg80211_csa_settings *params)
+ struct cfg80211_csa_settings *params,
+ int num_params)
{
int ret;
- trace_rdev_channel_switch(&rdev->wiphy, dev, params);
- ret = rdev->ops->channel_switch(&rdev->wiphy, dev, params);
+ trace_rdev_channel_switch(&rdev->wiphy, params);
+ ret = rdev->ops->channel_switch(&rdev->wiphy, params, num_params);
trace_rdev_return_int(&rdev->wiphy, ret);
return ret;
}
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index f7aa7a7..daaa806 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -1843,36 +1843,17 @@ TRACE_EVENT(rdev_crit_proto_stop,
);
TRACE_EVENT(rdev_channel_switch,
- TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
+ TP_PROTO(struct wiphy *wiphy,
struct cfg80211_csa_settings *params),
- TP_ARGS(wiphy, netdev, params),
+ TP_ARGS(wiphy, params),
TP_STRUCT__entry(
WIPHY_ENTRY
- NETDEV_ENTRY
- CHAN_DEF_ENTRY
- __field(u16, counter_offset_beacon)
- __field(u16, counter_offset_presp)
- __field(bool, radar_required)
- __field(bool, block_tx)
- __field(u8, count)
),
TP_fast_assign(
WIPHY_ASSIGN;
- NETDEV_ASSIGN;
- CHAN_DEF_ASSIGN(¶ms->chandef);
- __entry->counter_offset_beacon = params->counter_offset_beacon;
- __entry->counter_offset_presp = params->counter_offset_presp;
- __entry->radar_required = params->radar_required;
- __entry->block_tx = params->block_tx;
- __entry->count = params->count;
- ),
- TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", " CHAN_DEF_PR_FMT
- ", block_tx: %d, count: %u, radar_required: %d"
- ", counter offsets (beacon/presp): %u/%u",
- WIPHY_PR_ARG, NETDEV_PR_ARG, CHAN_DEF_PR_ARG,
- __entry->block_tx, __entry->count, __entry->radar_required,
- __entry->counter_offset_beacon,
- __entry->counter_offset_presp)
+ ),
+ TP_printk(WIPHY_PR_FMT,
+ WIPHY_PR_ARG)
);
/*************************************************************
--
1.8.4.rc3
On Wed, 2014-01-08 at 16:26 +0100, Michal Kazior wrote:
> The chandef parsing function doesn't need to
> access genl_info. It just needs the attrs field.
This seems reasonable regardless of everything else, I'll apply it.
johannes
The chandef parsing function doesn't need to
access genl_info. It just needs the attrs field.
This makes it easier for further reuse.
Signed-off-by: Michal Kazior <[email protected]>
---
net/wireless/nl80211.c | 41 +++++++++++++++++++----------------------
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index a693f86..33962ef 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1775,15 +1775,15 @@ static bool nl80211_can_set_dev_channel(struct wireless_dev *wdev)
}
static int nl80211_parse_chandef(struct cfg80211_registered_device *rdev,
- struct genl_info *info,
+ struct nlattr **attrs,
struct cfg80211_chan_def *chandef)
{
u32 control_freq;
- if (!info->attrs[NL80211_ATTR_WIPHY_FREQ])
+ if (!attrs[NL80211_ATTR_WIPHY_FREQ])
return -EINVAL;
- control_freq = nla_get_u32(info->attrs[NL80211_ATTR_WIPHY_FREQ]);
+ control_freq = nla_get_u32(attrs[NL80211_ATTR_WIPHY_FREQ]);
chandef->chan = ieee80211_get_channel(&rdev->wiphy, control_freq);
chandef->width = NL80211_CHAN_WIDTH_20_NOHT;
@@ -1794,11 +1794,10 @@ static int nl80211_parse_chandef(struct cfg80211_registered_device *rdev,
if (!chandef->chan || chandef->chan->flags & IEEE80211_CHAN_DISABLED)
return -EINVAL;
- if (info->attrs[NL80211_ATTR_WIPHY_CHANNEL_TYPE]) {
+ if (attrs[NL80211_ATTR_WIPHY_CHANNEL_TYPE]) {
enum nl80211_channel_type chantype;
- chantype = nla_get_u32(
- info->attrs[NL80211_ATTR_WIPHY_CHANNEL_TYPE]);
+ chantype = nla_get_u32(attrs[NL80211_ATTR_WIPHY_CHANNEL_TYPE]);
switch (chantype) {
case NL80211_CHAN_NO_HT:
@@ -1811,17 +1810,15 @@ static int nl80211_parse_chandef(struct cfg80211_registered_device *rdev,
default:
return -EINVAL;
}
- } else if (info->attrs[NL80211_ATTR_CHANNEL_WIDTH]) {
+ } else if (attrs[NL80211_ATTR_CHANNEL_WIDTH]) {
chandef->width =
- nla_get_u32(info->attrs[NL80211_ATTR_CHANNEL_WIDTH]);
- if (info->attrs[NL80211_ATTR_CENTER_FREQ1])
+ nla_get_u32(attrs[NL80211_ATTR_CHANNEL_WIDTH]);
+ if (attrs[NL80211_ATTR_CENTER_FREQ1])
chandef->center_freq1 =
- nla_get_u32(
- info->attrs[NL80211_ATTR_CENTER_FREQ1]);
- if (info->attrs[NL80211_ATTR_CENTER_FREQ2])
+ nla_get_u32(attrs[NL80211_ATTR_CENTER_FREQ1]);
+ if (attrs[NL80211_ATTR_CENTER_FREQ2])
chandef->center_freq2 =
- nla_get_u32(
- info->attrs[NL80211_ATTR_CENTER_FREQ2]);
+ nla_get_u32(attrs[NL80211_ATTR_CENTER_FREQ2]);
}
if (!cfg80211_chandef_valid(chandef))
@@ -1853,7 +1850,7 @@ static int __nl80211_set_channel(struct cfg80211_registered_device *rdev,
if (!nl80211_can_set_dev_channel(wdev))
return -EOPNOTSUPP;
- result = nl80211_parse_chandef(rdev, info, &chandef);
+ result = nl80211_parse_chandef(rdev, info->attrs, &chandef);
if (result)
return result;
@@ -3205,7 +3202,7 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
}
if (info->attrs[NL80211_ATTR_WIPHY_FREQ]) {
- err = nl80211_parse_chandef(rdev, info, ¶ms.chandef);
+ err = nl80211_parse_chandef(rdev, info->attrs, ¶ms.chandef);
if (err)
return err;
} else if (wdev->preset_chandef.chan) {
@@ -5669,7 +5666,7 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
struct cfg80211_chan_def chandef;
int err;
- err = nl80211_parse_chandef(rdev, info, &chandef);
+ err = nl80211_parse_chandef(rdev, info->attrs, &chandef);
if (err)
return err;
@@ -5796,7 +5793,7 @@ static int nl80211_channel_switch(struct sk_buff *skb, struct genl_info *info)
}
skip_beacons:
- err = nl80211_parse_chandef(rdev, info, ¶ms.chandef);
+ err = nl80211_parse_chandef(rdev, info->attrs, ¶ms.chandef);
if (err)
return err;
@@ -6567,7 +6564,7 @@ static int nl80211_join_ibss(struct sk_buff *skb, struct genl_info *info)
ibss.ie_len = nla_len(info->attrs[NL80211_ATTR_IE]);
}
- err = nl80211_parse_chandef(rdev, info, &ibss.chandef);
+ err = nl80211_parse_chandef(rdev, info->attrs, &ibss.chandef);
if (err)
return err;
@@ -7223,7 +7220,7 @@ static int nl80211_remain_on_channel(struct sk_buff *skb,
duration > rdev->wiphy.max_remain_on_channel_duration)
return -EINVAL;
- err = nl80211_parse_chandef(rdev, info, &chandef);
+ err = nl80211_parse_chandef(rdev, info->attrs, &chandef);
if (err)
return err;
@@ -7517,7 +7514,7 @@ static int nl80211_tx_mgmt(struct sk_buff *skb, struct genl_info *info)
*/
chandef.chan = NULL;
if (info->attrs[NL80211_ATTR_WIPHY_FREQ]) {
- err = nl80211_parse_chandef(rdev, info, &chandef);
+ err = nl80211_parse_chandef(rdev, info->attrs, &chandef);
if (err)
return err;
}
@@ -7816,7 +7813,7 @@ static int nl80211_join_mesh(struct sk_buff *skb, struct genl_info *info)
cfg.auto_open_plinks = false;
if (info->attrs[NL80211_ATTR_WIPHY_FREQ]) {
- err = nl80211_parse_chandef(rdev, info, &setup.chandef);
+ err = nl80211_parse_chandef(rdev, info->attrs, &setup.chandef);
if (err)
return err;
} else {
--
1.8.4.rc3
On Tue, 2014-01-14 at 17:04 +0100, Johannes Berg wrote:
> On Wed, 2014-01-08 at 16:26 +0100, Michal Kazior wrote:
> > The chandef parsing function doesn't need to
> > access genl_info. It just needs the attrs field.
>
> This seems reasonable regardless of everything else, I'll apply it.
Actually I changed my mind again - if we need to change other things
that'd be just churn. I have no objection to this though, so if it turns
out to be useful or needed we can do it.
johannes
On Wed, 2014-01-15 at 07:52 +0100, Michal Kazior wrote:
> So you're basically saying the new attribute should be used as an
> array attribute instead. Sounds good, but..
>
> What about backward compatibility? Do we not care? Or should we
> preserve old parsing for single-interface CSA (i.e. older hostapd)?
I think we have to preserve the old parsing if the new isn't present,
but that shouldn't be all that difficult?
if (info->attrs[MULTI])
for_each_attr(a, info->attrs[MULTI])
parse_nested(a, attrs);
parse(attrs);
else
parse(info->attrs);
or something like that, right?
> > is more error prone as it would allow older kernels to parse the whole
> > thing while ignoring the next/more/whatever, so you'd get some weird
> > subset of the intended behaviour. Forcing *all* interfaces into the
> > sub-attribute when more than one is desired (or in fact for a single
> > one, if you're ok with requiring a kernel with support) would IMHO be
> > less error prone.
>
> From a practical point of view cfg80211 should deny such a request due
> to multi-channel (interface combination) and by (the only CSA
> implementator in upstream) mac80211 due to chanctx->refcount > 1.
Hmm, true. I'd still prefer the other version, the stacked/nested one
kinda makes me uncomfortable ... It'll work, no doubt about it, but the
deep nesting I'm not really happy with.
johannes
On Wed, 2014-01-08 at 16:26 +0100, Michal Kazior wrote:
> This extends NL80211_CMD_CHANNEL_SWITCH by adding
> a new attribute NL80211_ATTR_SWITCH_NEXT. The
> attribute holds nested channel switch attributes
> (including itself) allowing
> multi-interface/chained CSA.
That's ... creative? Interesting? I'm not sure I like that API much.
OTOH, it's a neat way of reusing all the attributes that we have, etc.
However, I think it'd be easier to implement on both sides of the fence
if we structured this differently and made the "more vifs" to be a
nested attribute with data inside each one of them.
Actually, I'd suggest that in the multi-vif case all of the attributes
should be inside the list, like this:
wiphy, chswitch=[1=[ifidx,...], 2=[ifidx,...], 3=[ifidx,...]]
alternatively, maybe something that would be nice would be
wiphy, chswitch=[ifidx1=[...], ifidx2=[...], ifidx3=[...]]
but the common way would be to ignore the array index completely.
Today you get
wiphy, ifidx, ..., next=[ifidx, ..., next=[ifidx, ..., next=[...]]]
and I don't really like the deep nesting.
I would also argue that doing
wiphy, ifidx1, ..., more=[1=[ifidx2, ...], ...]
or
wiphy, ifidx1, ..., next=[...]
is more error prone as it would allow older kernels to parse the whole
thing while ignoring the next/more/whatever, so you'd get some weird
subset of the intended behaviour. Forcing *all* interfaces into the
sub-attribute when more than one is desired (or in fact for a single
one, if you're ok with requiring a kernel with support) would IMHO be
less error prone.
Haven't looked over the code in much detail, but a few small comments:
* I think you should preserve tracing, it has facilities for arrays too
* hard-coding a limit of 8 vifs seems pointless, you can easily
allocate dynamic
memory, no?
johannes
This refactors cfg80211_can_use_iftype_chan() so
that it can process multi-interface combination
changes.
With this it will be possible to handle, e.g.
multi-BSS channel switching.
Signed-off-by: Michal Kazior <[email protected]>
---
net/wireless/core.h | 36 ++++++++++--
net/wireless/util.c | 158 ++++++++++++++++++++++++++++++++--------------------
2 files changed, 127 insertions(+), 67 deletions(-)
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 38f74fe..ee19118 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -228,6 +228,14 @@ enum cfg80211_chan_mode {
CHAN_MODE_EXCLUSIVE,
};
+struct cfg80211_iftype_chan_param {
+ struct wireless_dev *wdev;
+ enum nl80211_iftype iftype;
+ struct ieee80211_channel *chan;
+ enum cfg80211_chan_mode chanmode;
+ u8 radar_detect_width;
+};
+
struct cfg80211_beacon_registration {
struct list_head list;
u32 nlportid;
@@ -374,12 +382,9 @@ int cfg80211_change_iface(struct cfg80211_registered_device *rdev,
void cfg80211_process_rdev_events(struct cfg80211_registered_device *rdev);
void cfg80211_process_wdev_events(struct wireless_dev *wdev);
-int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
- struct wireless_dev *wdev,
- enum nl80211_iftype iftype,
- struct ieee80211_channel *chan,
- enum cfg80211_chan_mode chanmode,
- u8 radar_detect_width);
+int cfg80211_can_use_iftype_chan_params(struct cfg80211_registered_device *rdev,
+ const struct cfg80211_iftype_chan_param *params,
+ int num_params);
/**
* cfg80211_chandef_dfs_usable - checks if chandef is DFS usable
@@ -402,6 +407,25 @@ void cfg80211_dfs_channels_update_work(struct work_struct *work);
static inline int
+cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
+ struct wireless_dev *wdev,
+ enum nl80211_iftype iftype,
+ struct ieee80211_channel *chan,
+ enum cfg80211_chan_mode chanmode,
+ u8 radar_detect_width)
+{
+ struct cfg80211_iftype_chan_param param = {
+ .wdev = wdev,
+ .iftype = iftype,
+ .chan = chan,
+ .chanmode = chanmode,
+ .radar_detect_width = radar_detect_width,
+ };
+
+ return cfg80211_can_use_iftype_chan_params(rdev, ¶m, 1);
+}
+
+static inline int
cfg80211_can_change_interface(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev,
enum nl80211_iftype iftype)
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 41013f1..099f1da 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1233,89 +1233,118 @@ int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
return res;
}
-int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
- struct wireless_dev *wdev,
- enum nl80211_iftype iftype,
- struct ieee80211_channel *chan,
- enum cfg80211_chan_mode chanmode,
- u8 radar_detect_width)
+int cfg80211_can_use_iftype_chan_params(struct cfg80211_registered_device *rdev,
+ const struct cfg80211_iftype_chan_param *params,
+ int num_params)
{
struct wireless_dev *wdev_iter;
- u32 used_iftypes = BIT(iftype);
+ u32 used_iftypes = 0;
int num[NUM_NL80211_IFTYPES];
struct ieee80211_channel
*used_channels[CFG80211_MAX_NUM_DIFFERENT_CHANNELS];
struct ieee80211_channel *ch;
enum cfg80211_chan_mode chmode;
int num_different_channels = 0;
- int total = 1;
+ int total = num_params;
bool radar_required = false;
+ bool num_software_iftypes = 0;
int i, j;
ASSERT_RTNL();
- if (WARN_ON(hweight32(radar_detect_width) > 1))
- return -EINVAL;
+ memset(num, 0, sizeof(num));
+ memset(used_channels, 0, sizeof(used_channels));
- switch (iftype) {
- case NL80211_IFTYPE_ADHOC:
- case NL80211_IFTYPE_AP:
- case NL80211_IFTYPE_AP_VLAN:
- case NL80211_IFTYPE_MESH_POINT:
- case NL80211_IFTYPE_P2P_GO:
- case NL80211_IFTYPE_WDS:
- /* if the interface could potentially choose a DFS channel,
- * then mark DFS as required.
- */
- if (!chan) {
- if (chanmode != CHAN_MODE_UNDEFINED && radar_detect_width)
- radar_required = true;
+ for (i = 0; i < num_params; i++) {
+ if (WARN_ON(hweight32(params[i].radar_detect_width) > 1))
+ return -EINVAL;
+
+ /* sanity check - make sure all wdevs in params[] are unique */
+ for (j = 0; j < num_params; j++)
+ if (WARN_ON(i != j && params[i].wdev == params[j].wdev))
+ return -EINVAL;
+
+ radar_required = false;
+ used_iftypes |= BIT(params[i].iftype);
+
+ switch (params[i].iftype) {
+ case NL80211_IFTYPE_ADHOC:
+ case NL80211_IFTYPE_AP:
+ case NL80211_IFTYPE_AP_VLAN:
+ case NL80211_IFTYPE_MESH_POINT:
+ case NL80211_IFTYPE_P2P_GO:
+ case NL80211_IFTYPE_WDS:
+ /* if the interface could potentially choose a DFS channel,
+ * then mark DFS as required.
+ */
+ if (!params[i].chan) {
+ if (params[i].chanmode != CHAN_MODE_UNDEFINED &&
+ params[i].radar_detect_width)
+ radar_required = true;
+ break;
+ }
+ radar_required = !!(params[i].chan->flags & IEEE80211_CHAN_RADAR);
+ break;
+ case NL80211_IFTYPE_P2P_CLIENT:
+ case NL80211_IFTYPE_STATION:
+ case NL80211_IFTYPE_P2P_DEVICE:
+ case NL80211_IFTYPE_MONITOR:
break;
+ case NUM_NL80211_IFTYPES:
+ case NL80211_IFTYPE_UNSPECIFIED:
+ default:
+ return -EINVAL;
}
- radar_required = !!(chan->flags & IEEE80211_CHAN_RADAR);
- break;
- case NL80211_IFTYPE_P2P_CLIENT:
- case NL80211_IFTYPE_STATION:
- case NL80211_IFTYPE_P2P_DEVICE:
- case NL80211_IFTYPE_MONITOR:
- break;
- case NUM_NL80211_IFTYPES:
- case NL80211_IFTYPE_UNSPECIFIED:
- default:
- return -EINVAL;
- }
-
- if (radar_required && !radar_detect_width)
- return -EINVAL;
- /* Always allow software iftypes */
- if (rdev->wiphy.software_iftypes & BIT(iftype)) {
- if (radar_detect_width)
+ if (radar_required && !params[i].radar_detect_width)
return -EINVAL;
- return 0;
- }
- memset(num, 0, sizeof(num));
- memset(used_channels, 0, sizeof(used_channels));
+ if (rdev->wiphy.software_iftypes & BIT(params[i].iftype)) {
+ num_software_iftypes++;
+ if (params[i].radar_detect_width)
+ return -EINVAL;
+ }
- num[iftype] = 1;
+ num[params[i].iftype]++;
- switch (chanmode) {
- case CHAN_MODE_UNDEFINED:
- break;
- case CHAN_MODE_SHARED:
- WARN_ON(!chan);
- used_channels[0] = chan;
- num_different_channels++;
- break;
- case CHAN_MODE_EXCLUSIVE:
- num_different_channels++;
- break;
+ switch (params[i].chanmode) {
+ case CHAN_MODE_UNDEFINED:
+ break;
+ case CHAN_MODE_SHARED:
+ if (WARN_ON(!params[i].chan))
+ return -EINVAL;
+
+ for (j = 0; j < CFG80211_MAX_NUM_DIFFERENT_CHANNELS; j++)
+ if (!used_channels[j] ||
+ used_channels[j] == params[i].chan)
+ break;
+
+ if (j == CFG80211_MAX_NUM_DIFFERENT_CHANNELS)
+ return -EBUSY;
+
+ if (used_channels[j] == NULL) {
+ used_channels[j] = params[i].chan;
+ num_different_channels++;
+ }
+ break;
+ case CHAN_MODE_EXCLUSIVE:
+ num_different_channels++;
+ break;
+ }
}
+ /* Always allow software iftypes */
+ if (num_params == num_software_iftypes)
+ return 0;
+
list_for_each_entry(wdev_iter, &rdev->wdev_list, list) {
- if (wdev_iter == wdev)
+ /* skip wdevs which are in params[] */
+ for (i = 0; i < num_params; i++)
+ if (wdev_iter == params[i].wdev)
+ break;
+ if (i < num_params)
continue;
+
if (wdev_iter->iftype == NL80211_IFTYPE_P2P_DEVICE) {
if (!wdev_iter->p2p_started)
continue;
@@ -1366,12 +1395,13 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
used_iftypes |= BIT(wdev_iter->iftype);
}
- if (total == 1 && !radar_detect_width)
+ if (total == 1 && num_params == 1 && !params[0].radar_detect_width)
return 0;
for (i = 0; i < rdev->wiphy.n_iface_combinations; i++) {
const struct ieee80211_iface_combination *c;
struct ieee80211_iface_limit *limits;
+ enum nl80211_iftype iftype;
u32 all_iftypes = 0;
c = &rdev->wiphy.iface_combinations[i];
@@ -1399,8 +1429,14 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
}
}
- if (radar_detect_width && !(c->radar_detect_widths & radar_detect_width))
- goto cont;
+ for (j = 0; j < num_params; j++) {
+ if (!params[j].radar_detect_width)
+ continue;
+
+ if (!(c->radar_detect_widths &
+ params[j].radar_detect_width))
+ goto cont;
+ }
/*
* Finally check that all iftypes that we're currently
--
1.8.4.rc3