Hi,
This is the cfg80211 part for CSA multi-interface
support.
It's still missing proper tracing. I'm out of
ideas how to deal with it. I've tried some pustom
trace_seq_ stuff but trace-cmd report doens't
seem to happy with that (it complains about parse
errors). Is this expected or am I missing
something? Any hints are welcome.
Changes since RFC:
* drop rename patch (it is kind of squashed into
the multi-intf combination patch)
* refactor multi-intf combination patch and split
helper functions
* nl80211 command uses now a simple nested array
instead of recursive nesting
* advertise multi-intf csa support to userspace
Michal Kazior (3):
nl80211: make chandef parser take nlattr arg
cfg80211: implement multi-vif interface combination validation
cfg80211: implement multi-interface CSA
include/net/cfg80211.h | 10 +-
include/uapi/linux/nl80211.h | 6 +
net/mac80211/cfg.c | 23 +++-
net/mac80211/ibss.c | 4 +-
net/mac80211/ieee80211_i.h | 7 +-
net/mac80211/mesh.c | 4 +-
net/wireless/core.h | 36 +++++-
net/wireless/nl80211.c | 276 ++++++++++++++++++++++++++++++-------------
net/wireless/rdev-ops.h | 8 +-
net/wireless/trace.h | 37 ++----
net/wireless/util.c | 198 +++++++++++++++++++------------
11 files changed, 405 insertions(+), 204 deletions(-)
--
1.8.4.rc3
On Thu, 2014-01-23 at 08:28 +0200, Luca Coelho wrote:
> Why is this necessary? I think it's easier to treat the CSA's
> separately. When wpa_supplicant decides to switch channel in one
> interface, it asks cfg80211 to do so. Then, if that interface change is
> "approved" (ie. there's no internal reason why it would fail), cfg80211
> sends a notification to userspace that the interface will switch. Then
> wpa_s decides whether any other interfaces need switching as well (ie.
> if it's in the same channel context).
I don't think that makes sense. If userspace requested the CSA
operation, the fact that it will happen should be known as soon as the
request returns 0. No need for a notification (other than when the mlme
code in mac80211 decided to switch by itself). You're mixing the cases
of GO-follow-client-interface and switch-multiple-AP-BSSes, which I'm
not sure is right.
The problem with using the approach you're suggesting to switch multiple
AP interfaces is that when you do that, you open the system up to races
much more than when requesting multiple interfaces at once. Also, I
still think we need to enforce that the interface switch at the same
time, which would practically be impossible with multiple switch
commands.
> > This makes it possible for userspace to request a
> > multi-interface channel switch while allowing
> > sanity checks wrt target interface/channel
> > combination.
>
> Okay, the sanity checks are an interesting issue. But if the
> combination is not good, the whole command will fail and none of the
> interfaces will switch. If wpa_s does this interface-by-interface,
> separately, the triggering interface (ie. the one that decided to switch
> in the first place) may succeed while the subsequent requests may fail
> (eg. if the interface combination is not allowed in the new channel).
> If the second one fails, wpa_s may make another decision, for instance
> trying to switch the second interface to a *different* channel instead,
> or not switch at all.
That makes no sense. You're talking about completely different
scenarios, see above.
johannes
This extends NL80211_CMD_CHANNEL_SWITCH by adding
a new attribute NL80211_ATTR_CH_SWITCH_IFACES. The
attribute holds an array of CSA attributes. Each
entry holds attributes that were used until now:
ifindex, chandef, blocktx, beacon/proberesp.
The new attribute is used as a flag in
send_wiphy() and a driver may advertise the
capability with WIPHY_FLAG_HAS_MULTI_IF_CHSWITCH.
Userspace may now submit CSA in the old way or
with the new attribute set. If the new attribute
is set attributes outside the nested attribute are
ignored.
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 | 6 ++
net/mac80211/cfg.c | 23 ++++-
net/mac80211/ibss.c | 4 +-
net/mac80211/ieee80211_i.h | 7 +-
net/mac80211/mesh.c | 4 +-
net/wireless/nl80211.c | 237 ++++++++++++++++++++++++++++++++-----------
net/wireless/rdev-ops.h | 8 +-
net/wireless/trace.h | 37 +++----
9 files changed, 235 insertions(+), 101 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index d5e57bf..226d1e1 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;
@@ -2266,6 +2267,7 @@ struct cfg80211_qos_map {
* @set_coalesce: Set coalesce parameters.
*
* @channel_switch: initiate channel-switch procedure (with CSA)
+ * num_params is always >= 1.
*
* @set_qos_map: Set QoS mapping information to the driver
*/
@@ -2507,8 +2509,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);
int (*set_qos_map)(struct wiphy *wiphy,
struct net_device *dev,
struct cfg80211_qos_map *qos_map);
@@ -2558,6 +2560,9 @@ struct cfg80211_ops {
* @WIPHY_FLAG_SUPPORTS_5_10_MHZ: Device supports 5 MHz and 10 MHz channels.
* @WIPHY_FLAG_HAS_CHANNEL_SWITCH: Device supports channel switch in
* beaconing mode (AP, IBSS, Mesh, ...).
+ * @WIPHY_FLAG_HAS_MULTI_IF_CHSWITCH: Device supports multi-interface channel
+ * switching in beaconing mode (AP, IBSS, Mesh). If this is set it is
+ * expected that @WIPHY_FLAG_HAS_CHANNEL_SWITCH is set as well.
*/
enum wiphy_flags {
/* use hole at 0 */
@@ -2583,6 +2588,7 @@ enum wiphy_flags {
WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL = BIT(21),
WIPHY_FLAG_SUPPORTS_5_10_MHZ = BIT(22),
WIPHY_FLAG_HAS_CHANNEL_SWITCH = BIT(23),
+ WIPHY_FLAG_HAS_MULTI_IF_CHSWITCH = BIT(24),
};
/**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 53e56cf..ac45e62 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1575,6 +1575,10 @@ enum nl80211_commands {
* advertise values that cannot always be met. In such cases, an attempt
* to add a new station entry with @NL80211_CMD_NEW_STATION may fail.
*
+ * @NL80211_ATTR_CH_SWITCH_IFACES: Nested attribute with channel switch
+ * settings in each entry (ifindex, frequency, beacon IEs). Also used as a
+ * device capability flag in nl80211_send_wiphy().
+ *
* @NL80211_ATTR_MAX: highest attribute number currently defined
* @__NL80211_ATTR_AFTER_LAST: internal use
*/
@@ -1908,6 +1912,8 @@ enum nl80211_attrs {
NL80211_ATTR_MAX_AP_ASSOC_STA,
+ NL80211_ATTR_CH_SWITCH_IFACES,
+
/* 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 032081c..65dac7f 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3071,8 +3071,8 @@ unlock:
sdata_unlock(sdata);
}
-int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
- struct cfg80211_csa_settings *params)
+int __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
+ struct cfg80211_csa_settings *params)
{
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
struct ieee80211_local *local = sdata->local;
@@ -3255,6 +3255,25 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
return 0;
}
+int ieee80211_channel_switch(struct wiphy *wiphy,
+ struct cfg80211_csa_settings *params,
+ int num_params)
+{
+ struct ieee80211_sub_if_data *sdata;
+ int err;
+
+ /* multi-vif CSA is not implemented */
+ if (num_params > 1)
+ return -EOPNOTSUPP;
+
+ sdata = IEEE80211_DEV_TO_SUB_IF(params[0].dev);
+ sdata_lock(sdata);
+ err = __ieee80211_channel_switch(wiphy, params[0].dev, ¶ms[0]);
+ sdata_unlock(sdata);
+
+ return err;
+}
+
static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
struct cfg80211_mgmt_tx_params *params,
u64 *cookie)
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index ed7eec3..5ca5834 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -899,8 +899,8 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata,
params.block_tx = !!csa_ie.mode;
- if (ieee80211_channel_switch(sdata->local->hw.wiphy, sdata->dev,
- ¶ms))
+ if (__ieee80211_channel_switch(sdata->local->hw.wiphy, sdata->dev,
+ ¶ms))
goto disconnect;
ieee80211_ibss_csa_mark_radar(sdata);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 96eb272..a6cda02 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1460,8 +1460,11 @@ void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
/* channel switch handling */
void ieee80211_csa_finalize_work(struct work_struct *work);
-int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
- struct cfg80211_csa_settings *params);
+int __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
+ struct cfg80211_csa_settings *params);
+int ieee80211_channel_switch(struct wiphy *wiphy,
+ struct cfg80211_csa_settings *params,
+ int num_params);
/* interface handling */
int ieee80211_iface_init(void);
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index b02ac33..98bcd2b 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -934,8 +934,8 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata,
ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_REPEATER;
- if (ieee80211_channel_switch(sdata->local->hw.wiphy, sdata->dev,
- ¶ms) < 0)
+ if (__ieee80211_channel_switch(sdata->local->hw.wiphy, sdata->dev,
+ ¶ms) < 0)
return false;
return true;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 8742ed9..fc52992 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -386,6 +386,7 @@ static const struct nla_policy nl80211_policy[NL80211_ATTR_MAX+1] = {
.len = IEEE80211_QOS_MAP_LEN_MAX },
[NL80211_ATTR_MAC_HINT] = { .len = ETH_ALEN },
[NL80211_ATTR_WIPHY_FREQ_HINT] = { .type = NLA_U32 },
+ [NL80211_ATTR_CH_SWITCH_IFACES] = { .type = NLA_NESTED },
};
/* policy for the key attributes */
@@ -1270,6 +1271,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *dev,
if ((dev->wiphy.flags & WIPHY_FLAG_TDLS_EXTERNAL_SETUP) &&
nla_put_flag(msg, NL80211_ATTR_TDLS_EXTERNAL_SETUP))
goto nla_put_failure;
+ if ((dev->wiphy.flags & WIPHY_FLAG_HAS_MULTI_IF_CHSWITCH) &&
+ nla_put_flag(msg, NL80211_ATTR_CH_SWITCH_IFACES))
+ goto nla_put_failure;
state->split_start++;
if (state->split)
break;
@@ -5771,23 +5775,42 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
return err;
}
-static int nl80211_channel_switch(struct sk_buff *skb, struct genl_info *info)
+static int
+nl80211_parse_csa_settings(struct cfg80211_registered_device *rdev,
+ struct nlattr **attrs,
+ struct cfg80211_csa_settings *params,
+ struct cfg80211_iftype_chan_param *ifch_params)
{
- 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;
+ struct net_device *dev = NULL;
+ struct wireless_dev *wdev;
int err;
bool need_new_beacon = false;
- if (!rdev->ops->channel_switch ||
- !(rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH))
- return -EOPNOTSUPP;
+ ASSERT_RTNL();
+
+ if (!attrs[NL80211_ATTR_IFINDEX])
+ return -EINVAL;
+
+ dev = dev_get_by_index(wiphy_net(&rdev->wiphy),
+ nla_get_u32(attrs[NL80211_ATTR_IFINDEX]));
+ if (!dev)
+ return -ENOENT;
+
+ 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:
@@ -5795,105 +5818,195 @@ static int nl80211_channel_switch(struct sk_buff *skb, struct genl_info *info)
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;
- if (info->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;
- wdev_lock(wdev);
- err = rdev_channel_switch(rdev, dev, ¶ms);
- wdev_unlock(wdev);
+out:
+ dev_put(dev);
+ return 0;
+}
+
+static int nl80211_channel_switch(struct sk_buff *skb, struct genl_info *info)
+{
+ struct cfg80211_registered_device *rdev = info->user_ptr[0];
+ struct cfg80211_csa_settings *csa_params;
+ struct cfg80211_iftype_chan_param *ifch_params;
+ struct nlattr *attrs;
+ /* 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];
+ int err, num_params = 0, tmp;
+
+ if (!rdev->ops->channel_switch ||
+ !(rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH))
+ return -EOPNOTSUPP;
+
+ if (info->attrs[NL80211_ATTR_CH_SWITCH_IFACES]) {
+ if (!(rdev->wiphy.flags & NL80211_ATTR_CH_SWITCH_IFACES))
+ return -EOPNOTSUPP;
+
+ nla_for_each_nested(attrs,
+ info->attrs[NL80211_ATTR_CH_SWITCH_IFACES],
+ tmp)
+ num_params++;
+ } else {
+ num_params = 1;
+ }
+
+ csa_params = kzalloc(sizeof(*csa_params) * num_params, GFP_KERNEL);
+ if (!csa_params)
+ return -ENOMEM;
+
+ ifch_params = kzalloc(sizeof(*ifch_params) * num_params, GFP_KERNEL);
+ if (!ifch_params) {
+ kfree(csa_params);
+ return -ENOMEM;
+ }
+
+ if (info->attrs[NL80211_ATTR_CH_SWITCH_IFACES]) {
+ int i = 0;
+
+ nla_for_each_nested(attrs,
+ info->attrs[NL80211_ATTR_CH_SWITCH_IFACES],
+ tmp) {
+ nla_parse(csa_attrs, NL80211_ATTR_MAX, nla_data(attrs),
+ nla_len(attrs), nl80211_policy);
+ err = nl80211_parse_csa_settings(rdev, csa_attrs,
+ &csa_params[i],
+ &ifch_params[i]);
+ if (err)
+ goto out;
+ i++;
+ }
+ } else {
+ err = nl80211_parse_csa_settings(rdev, info->attrs,
+ &csa_params[0],
+ &ifch_params[0]);
+ if (err)
+ goto out;
+ }
+
+ err = cfg80211_can_use_iftype_chan_params(rdev, ifch_params,
+ num_params);
+ if (err)
+ goto out;
+
+ err = rdev_channel_switch(rdev, csa_params, num_params);
+
+out:
+ kfree(csa_params);
+ kfree(ifch_params);
return err;
}
@@ -9918,7 +10031,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 c8e2259..ade70b7 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, num_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 fbcc23e..982246e 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -1865,36 +1865,23 @@ TRACE_EVENT(rdev_crit_proto_stop,
);
TRACE_EVENT(rdev_channel_switch,
- TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
- struct cfg80211_csa_settings *params),
- TP_ARGS(wiphy, netdev, params),
+ TP_PROTO(struct wiphy *wiphy,
+ struct cfg80211_csa_settings *params,
+ int num_params),
+ TP_ARGS(wiphy, params, num_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)
+ __field(int, num_params)
+ __dynamic_array(u8, params, sizeof(*params) * num_params)
),
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)
+ __entry->num_params = num_params;
+ memcpy(__get_dynamic_array(params), params,
+ sizeof(*params) * num_params);
+ ),
+ TP_printk(WIPHY_PR_FMT ", num_params=%d",
+ WIPHY_PR_ARG, __entry->num_params)
);
TRACE_EVENT(rdev_set_qos_map,
--
1.8.4.rc3
On 21 January 2014 16:52, Johannes Berg <[email protected]> wrote:
> On Mon, 2014-01-20 at 15:09 +0100, Michal Kazior wrote:
>
>> @@ -2583,6 +2588,7 @@ enum wiphy_flags {
>> WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL = BIT(21),
>> WIPHY_FLAG_SUPPORTS_5_10_MHZ = BIT(22),
>> WIPHY_FLAG_HAS_CHANNEL_SWITCH = BIT(23),
>> + WIPHY_FLAG_HAS_MULTI_IF_CHSWITCH = BIT(24),
>> };
>
> You could probably use an nl80211 feature flag directly, see e.g.
> NL80211_FEATURE_HT_IBSS (enum nl80211_feature_flags) and save the code
> to set the extra flag attribute.
>
>> + * @NL80211_ATTR_CH_SWITCH_IFACES: Nested attribute with channel switch
>> + * settings in each entry (ifindex, frequency, beacon IEs). Also used as a
>> + * device capability flag in nl80211_send_wiphy().
>
> Which would obviously change this.
>
>> +int ieee80211_channel_switch(struct wiphy *wiphy,
>> + struct cfg80211_csa_settings *params,
>> + int num_params)
>> +{
>> + struct ieee80211_sub_if_data *sdata;
>> + int err;
>> +
>> + /* multi-vif CSA is not implemented */
>> + if (num_params > 1)
>> + return -EOPNOTSUPP;
>
> The capability flag should be checked in cfg80211, so this shouldn't be
> needed?
Okay.
>> +int ieee80211_channel_switch(struct wiphy *wiphy,
>> + struct cfg80211_csa_settings *params,
>> + int num_params);
>
> This function can be static now as the other places call the __ version
> now. Careful with locking though - you just moved it as well, won't the
> callers have to be updated?
Good point. I'll make it static.
ieee80211_channel_switch() was always called with the assumption
wdev->mtx is held. Only cfg80211_channel_switch() caller is changed
wrt to locking of wdev->mtx (since you can't take a bunch of wdev
locks just like that..). That's why other callers use __ version now.
>> + /* 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];
>
> Technically we could even just reuse family->attrs since we're also
> under genl_lock :)
>
>> - u8 radar_detect_width = 0;
>> + struct net_device *dev = NULL;
>> + struct wireless_dev *wdev;
>> int err;
>> bool need_new_beacon = false;
>>
>> - if (!rdev->ops->channel_switch ||
>> - !(rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH))
>> - return -EOPNOTSUPP;
>> + ASSERT_RTNL();
>> +
>> + if (!attrs[NL80211_ATTR_IFINDEX])
>> + return -EINVAL;
>
> Hmm, shouldn't you use the ifindex from the csa_attrs? Or are you
> requiring one to be in the outer data to identify the wiphy? But that
> seems odd too, you can just get the wiphy in whatever way by asking the
> pre_doit for that?
I'm not sure I understand you.
There are two command variants now:
1: the old one, which has ifindex in the root
2: the new one, which has wiphy in the root, and an array of (1)
within CH_SWITCH_IFACES (and each entry has ifindex at its root)
>> + nla_for_each_nested(attrs,
>> + info->attrs[NL80211_ATTR_CH_SWITCH_IFACES],
>> + tmp) {
>> + nla_parse(csa_attrs, NL80211_ATTR_MAX, nla_data(attrs),
>> + nla_len(attrs), nl80211_policy);
>
> I think nla_parse() can fail, you should check that, otherwise there's
> no full validity checking of the attributes contained in it I believe.
Good point. I'll fix that.
Michał
On Mon, 2014-01-20 at 15:09 +0100, Michal Kazior wrote:
> + /* 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;
> +
> + if (params[i].wdev && params[i].wdev->wiphy != &rdev->wiphy)
> + return -EINVAL;
How could you ever have a param without a wdev? That seems like a bug as
well?
> + used_iftypes |= BIT(params[i].iftype);
> + num[params[i].iftype]++;
> +
> + err = cfg80211_is_radar_required(params[i].iftype,
> + params[i].chan,
> + params[i].chanmode,
> + params[i].radar_detect_width);
> + if (err < 0)
> + return err;
> + else if (err && !params[i].radar_detect_width)
> + return -EINVAL;
That's a bit confusing - why do you pass the radar_detect_width in, and
then check it again? Couldn't you just do all the checks in the
function, perhaps renamed a bit?
johannes
On 21 January 2014 16:41, Johannes Berg <[email protected]> wrote:
> On Mon, 2014-01-20 at 15:09 +0100, Michal Kazior wrote:
>
>> + /* 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;
>> +
>> + if (params[i].wdev && params[i].wdev->wiphy != &rdev->wiphy)
>> + return -EINVAL;
>
> How could you ever have a param without a wdev? That seems like a bug as
> well?
cfg80211_can_add_interface() calls with wdev == NULL.
>> + used_iftypes |= BIT(params[i].iftype);
>> + num[params[i].iftype]++;
>> +
>> + err = cfg80211_is_radar_required(params[i].iftype,
>> + params[i].chan,
>> + params[i].chanmode,
>> + params[i].radar_detect_width);
>> + if (err < 0)
>> + return err;
>> + else if (err && !params[i].radar_detect_width)
>> + return -EINVAL;
>
> That's a bit confusing - why do you pass the radar_detect_width in, and
> then check it again? Couldn't you just do all the checks in the
> function, perhaps renamed a bit?
I can move this into the helper function. Any suggestions for the
rename? cfg80211_validate_radar_params() and it would return a 0 or an
errno?
Michał
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 | 198 ++++++++++++++++++++++++++++++++--------------------
2 files changed, 152 insertions(+), 82 deletions(-)
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 37ec16d..faad3e9 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -226,6 +226,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;
@@ -372,12 +380,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);
+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
@@ -400,6 +405,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 d39c371..c4cac9e 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1252,30 +1252,11 @@ 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)
+static int cfg80211_is_radar_required(enum nl80211_iftype iftype,
+ struct ieee80211_channel *chan,
+ enum cfg80211_chan_mode chanmode,
+ u8 radar_detect_width)
{
- struct wireless_dev *wdev_iter;
- u32 used_iftypes = BIT(iftype);
- 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;
- bool radar_required = false;
- int i, j;
-
- ASSERT_RTNL();
-
- if (WARN_ON(hweight32(radar_detect) > 1))
- return -EINVAL;
-
switch (iftype) {
case NL80211_IFTYPE_ADHOC:
case NL80211_IFTYPE_AP:
@@ -1283,58 +1264,132 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
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)
- radar_required = true;
- break;
- }
- radar_required = !!(chan->flags & IEEE80211_CHAN_RADAR);
- break;
+ /* if the interface could potentially choose a DFS
+ * channel, then mark DFS as required. */
+ if (!chan)
+ return (chanmode != CHAN_MODE_UNDEFINED &&
+ radar_detect_width) ? 1 : 0;
+
+ return (chan->flags & IEEE80211_CHAN_RADAR) ? 1 : 0;
case NL80211_IFTYPE_P2P_CLIENT:
case NL80211_IFTYPE_STATION:
case NL80211_IFTYPE_P2P_DEVICE:
case NL80211_IFTYPE_MONITOR:
- break;
+ return 0;
case NUM_NL80211_IFTYPES:
case NL80211_IFTYPE_UNSPECIFIED:
default:
return -EINVAL;
}
+}
- if (radar_required && !radar_detect)
- return -EINVAL;
-
- /* Always allow software iftypes */
- if (rdev->wiphy.software_iftypes & BIT(iftype)) {
- if (radar_detect)
- return -EINVAL;
- return 0;
- }
-
- memset(num, 0, sizeof(num));
- memset(used_channels, 0, sizeof(used_channels));
-
- num[iftype] = 1;
+static int cfg80211_add_used_chan(struct ieee80211_channel **used_chans,
+ int *num_diff_chans,
+ int max_num_diff_chans,
+ struct ieee80211_channel *chan,
+ enum cfg80211_chan_mode chanmode)
+{
+ int i;
switch (chanmode) {
case CHAN_MODE_UNDEFINED:
break;
case CHAN_MODE_SHARED:
- WARN_ON(!chan);
- used_channels[0] = chan;
- num_different_channels++;
+ if (WARN_ON(!chan))
+ return -EINVAL;
+
+ for (i = 0; i < max_num_diff_chans; i++)
+ if (!used_chans[i] || used_chans[i] == chan)
+ break;
+
+ if (i == max_num_diff_chans)
+ return -EBUSY;
+
+ if (used_chans[i] == NULL) {
+ used_chans[i] = chan;
+ (*num_diff_chans)++;
+ }
break;
case CHAN_MODE_EXCLUSIVE:
- num_different_channels++;
+ (*num_diff_chans)++;
break;
}
+ return 0;
+}
+
+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 = 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 = num_params;
+ bool num_software_iftypes = 0;
+ int err, i, j;
+
+ ASSERT_RTNL();
+
+ memset(num, 0, sizeof(num));
+ memset(used_channels, 0, sizeof(used_channels));
+
+ 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;
+
+ if (params[i].wdev && params[i].wdev->wiphy != &rdev->wiphy)
+ return -EINVAL;
+
+ used_iftypes |= BIT(params[i].iftype);
+ num[params[i].iftype]++;
+
+ err = cfg80211_is_radar_required(params[i].iftype,
+ params[i].chan,
+ params[i].chanmode,
+ params[i].radar_detect_width);
+ if (err < 0)
+ return err;
+ else if (err && !params[i].radar_detect_width)
+ return -EINVAL;
+
+ if (rdev->wiphy.software_iftypes & BIT(params[i].iftype)) {
+ num_software_iftypes++;
+ if (params[i].radar_detect_width)
+ return -EINVAL;
+ }
+
+ err = cfg80211_add_used_chan(used_channels,
+ &num_different_channels,
+ ARRAY_SIZE(used_channels),
+ params[i].chan,
+ params[i].chanmode);
+ if (err)
+ return err;
+ }
+
+ /* 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;
@@ -1359,38 +1414,25 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
cfg80211_get_chan_state(wdev_iter, &ch, &chmode);
wdev_unlock(wdev_iter);
- switch (chmode) {
- case CHAN_MODE_UNDEFINED:
- break;
- case CHAN_MODE_SHARED:
- for (i = 0; i < CFG80211_MAX_NUM_DIFFERENT_CHANNELS; i++)
- if (!used_channels[i] || used_channels[i] == ch)
- break;
-
- if (i == CFG80211_MAX_NUM_DIFFERENT_CHANNELS)
- return -EBUSY;
-
- if (used_channels[i] == NULL) {
- used_channels[i] = ch;
- num_different_channels++;
- }
- break;
- case CHAN_MODE_EXCLUSIVE:
- num_different_channels++;
- break;
- }
+ err = cfg80211_add_used_chan(used_channels,
+ &num_different_channels,
+ ARRAY_SIZE(used_channels),
+ ch, chmode);
+ if (err)
+ return err;
num[wdev_iter->iftype]++;
total++;
used_iftypes |= BIT(wdev_iter->iftype);
}
- if (total == 1 && !radar_detect)
+ 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];
@@ -1418,8 +1460,12 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
}
}
- if (radar_detect && !(c->radar_detect_widths & radar_detect))
- goto cont;
+ for (j = 0; j < num_params; j++) {
+ if (params[j].radar_detect_width &&
+ !(c->radar_detect_widths &
+ params[j].radar_detect_width))
+ goto cont;
+ }
/*
* Finally check that all iftypes that we're currently
--
1.8.4.rc3
On Wed, 2014-01-22 at 10:09 +0100, Michal Kazior wrote:
> On 22 January 2014 09:53, Johannes Berg <[email protected]> wrote:
> > On Wed, 2014-01-22 at 07:36 +0100, Michal Kazior wrote:
> >
> >> >> + if (!attrs[NL80211_ATTR_IFINDEX])
> >> >> + return -EINVAL;
> >> 1: the old one, which has ifindex in the root
> >> 2: the new one, which has wiphy in the root, and an array of (1)
> >> within CH_SWITCH_IFACES (and each entry has ifindex at its root)
> >
> > Yeah I think I confused the two. This is handling case 1 only, right?
>
> No. It handles both cases.
In general. I phrased it badly - by "this" I was referring only to the
two lines above.
> > But you have to check the ifindex manually since for case 2 you can only
> > require the pre_doit() to give you the wiphy.
>
> In case 2 ifindex is not available in the attribute root. Can you
> require pre_doit() to check ifindex if it's possible for ifindex to be
> missing (case 2)?
No, the way you're doing it is exactly right, I just got confused.
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 6e78c62..8742ed9 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1840,15 +1840,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;
@@ -1859,11 +1859,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:
@@ -1876,17 +1875,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))
@@ -1918,7 +1915,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;
@@ -3270,7 +3267,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) {
@@ -5736,7 +5733,7 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
if (dfs_region == NL80211_DFS_UNSET)
return -EINVAL;
- err = nl80211_parse_chandef(rdev, info, &chandef);
+ err = nl80211_parse_chandef(rdev, info->attrs, &chandef);
if (err)
return err;
@@ -5863,7 +5860,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;
@@ -6634,7 +6631,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;
@@ -7304,7 +7301,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;
@@ -7686,7 +7683,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;
}
@@ -7985,7 +7982,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 Thu, 2014-01-23 at 16:41 +0100, Johannes Berg wrote:
> On Thu, 2014-01-23 at 11:31 +0200, Luca Coelho wrote:
>
> > > I don't think that makes sense. If userspace requested the CSA
> > > operation, the fact that it will happen should be known as soon as the
> > > request returns 0. No need for a notification (other than when the mlme
> > > code in mac80211 decided to switch by itself). You're mixing the cases
> > > of GO-follow-client-interface and switch-multiple-AP-BSSes, which I'm
> > > not sure is right.
> >
> > I think all cases should be treated in the same way. No need to treat
> > different scenarios differently.
>
> I'm not suggesting that, I'm just saying that I don't think userspace
> should have to ignore the return value. I think if the CSA request
> succeeds, it should have every right to expect that the switch will
> eventually succeed. I'm not saying it shouldn't also get a message
> *when* it succeeded, but I don't think it should have to expect it to
> completely fail still?
We cannot guarantee that the switch will succeed until the actual switch
is done. Let's say a new interface is added to the target channel or if
regulatory changes. It's possible that by the time we really switch
we're not allowed to use the target channel anymore (even though we
*were* allowed when it was requested).
Maybe the userspace can assume that the switch will succeed once it got
the OK response, but it still needs to be able to react if something
fails later.
> > I also like to think that, even though it's probably not a good idea, we
> > *could* have two different processes in userspace handling the different
> > interfaces (eg. two hostapd's). If only the return value of the request
> > is used, any other instances will not know something is happening.
>
> It really seems like a bad idea to run different tools, but hey.
I agree, it's probably a bad idea, but I also think that we shouldn't
enforce a single tool by the way we define the kernel API.
> > I don't see how a notification can hurt. It is just good to align all
> > cases: mlme decided to switch or the userspace decided to switch. In
> > both cases, whoever is listening will get the notification that
> > something is happening.
>
> I'm not saying the notification hurts, I just think it's wrong to make
> userspace look for a success/fail notification, IMHO it should be able
> to expect it to work if the request succeeded.
Maybe we are talking about different notifications here... What I'm
referring to is a notification telling userspace that its interface must
switch channel (because we don't have an extra context).
> > Let's say we have 2 APs with a single channel (your scenario). AP1
> > requests a channel switch with count 7. AP2 gets a notification that
> > its channel must switch at count 7. AP2 decides to either follow or to
> > shut itself down.
>
> I don't think this whole notification scheme makes sense. Realistically,
> we'll only have one hostapd or one wpa_s process controlling it, and
> trying to support anything else will result in a big mess. Therefore,
> hostapd can already know before the fact that the switch will occur -
> it's the one requesting it.
It's just about flexibility. *And*, it would also work if a STA
received a CSA notification (which we handle automatically in the
kernel) and there is an AP in the same context.
> > The same would happen for the GO-follow-client. The client gets a
> > channel switch with count 7. GO gets a notification that its channel
> > must switch at count 7. GO decides to either follow or shut itself
> > down.
>
> This is the only different case, the AP we're connected to is requesting
> the switch, and dictating the behaviour.
Exactly, and I'm trying to address this different case in the same way
as all other cases. ;)
> We *could have* implemented that as a notification to userspace, and
> then made wpa_s request the switch with exactly the matching parameters,
> but we implemented the reacting in mac80211 instead. That seems like a
> reasonable choice since we don't have any real options (other than
> disconnect, which we preserve since wpa_s might decide to do that upon
> getting a "will switch" notification)
Right, but this is irrelevant to my proposal. In this case, we're
reacting to a CSA and telling the userspace that *other* interfaces in
that same channel need to switch.
--
Luca.
On 22 January 2014 09:53, Johannes Berg <[email protected]> wrote:
> On Wed, 2014-01-22 at 07:36 +0100, Michal Kazior wrote:
>
>> >> + if (!attrs[NL80211_ATTR_IFINDEX])
>> >> + return -EINVAL;
>> >
>> > Hmm, shouldn't you use the ifindex from the csa_attrs? Or are you
>> > requiring one to be in the outer data to identify the wiphy? But that
>> > seems odd too, you can just get the wiphy in whatever way by asking the
>> > pre_doit for that?
>>
>> I'm not sure I understand you.
>>
>> There are two command variants now:
>>
>> 1: the old one, which has ifindex in the root
>> 2: the new one, which has wiphy in the root, and an array of (1)
>> within CH_SWITCH_IFACES (and each entry has ifindex at its root)
>
> Yeah I think I confused the two. This is handling case 1 only, right?
No. It handles both cases.
> But you have to check the ifindex manually since for case 2 you can only
> require the pre_doit() to give you the wiphy.
In case 2 ifindex is not available in the attribute root. Can you
require pre_doit() to check ifindex if it's possible for ifindex to be
missing (case 2)?
Michał
Hi,
Sorry for coming to this a bit late, but hopefully not too late. ;)
On Mon, 2014-01-20 at 15:09 +0100, Michal Kazior wrote:
> This extends NL80211_CMD_CHANNEL_SWITCH by adding
> a new attribute NL80211_ATTR_CH_SWITCH_IFACES. The
> attribute holds an array of CSA attributes. Each
> entry holds attributes that were used until now:
> ifindex, chandef, blocktx, beacon/proberesp.
Why is this necessary? I think it's easier to treat the CSA's
separately. When wpa_supplicant decides to switch channel in one
interface, it asks cfg80211 to do so. Then, if that interface change is
"approved" (ie. there's no internal reason why it would fail), cfg80211
sends a notification to userspace that the interface will switch. Then
wpa_s decides whether any other interfaces need switching as well (ie.
if it's in the same channel context).
> The new attribute is used as a flag in
> send_wiphy() and a driver may advertise the
> capability with WIPHY_FLAG_HAS_MULTI_IF_CHSWITCH.
>
> Userspace may now submit CSA in the old way or
> with the new attribute set. If the new attribute
> is set attributes outside the nested attribute are
> ignored.
>
> This makes it possible for userspace to request a
> multi-interface channel switch while allowing
> sanity checks wrt target interface/channel
> combination.
Okay, the sanity checks are an interesting issue. But if the
combination is not good, the whole command will fail and none of the
interfaces will switch. If wpa_s does this interface-by-interface,
separately, the triggering interface (ie. the one that decided to switch
in the first place) may succeed while the subsequent requests may fail
(eg. if the interface combination is not allowed in the new channel).
If the second one fails, wpa_s may make another decision, for instance
trying to switch the second interface to a *different* channel instead,
or not switch at all.
I think this approach makes things more flexible and simpler.
--
Luca.
On Thu, 2014-01-23 at 11:31 +0200, Luca Coelho wrote:
> > I don't think that makes sense. If userspace requested the CSA
> > operation, the fact that it will happen should be known as soon as the
> > request returns 0. No need for a notification (other than when the mlme
> > code in mac80211 decided to switch by itself). You're mixing the cases
> > of GO-follow-client-interface and switch-multiple-AP-BSSes, which I'm
> > not sure is right.
>
> I think all cases should be treated in the same way. No need to treat
> different scenarios differently.
I'm not suggesting that, I'm just saying that I don't think userspace
should have to ignore the return value. I think if the CSA request
succeeds, it should have every right to expect that the switch will
eventually succeed. I'm not saying it shouldn't also get a message
*when* it succeeded, but I don't think it should have to expect it to
completely fail still?
> I also like to think that, even though it's probably not a good idea, we
> *could* have two different processes in userspace handling the different
> interfaces (eg. two hostapd's). If only the return value of the request
> is used, any other instances will not know something is happening.
It really seems like a bad idea to run different tools, but hey.
> I don't see how a notification can hurt. It is just good to align all
> cases: mlme decided to switch or the userspace decided to switch. In
> both cases, whoever is listening will get the notification that
> something is happening.
I'm not saying the notification hurts, I just think it's wrong to make
userspace look for a success/fail notification, IMHO it should be able
to expect it to work if the request succeeded.
> Let's say we have 2 APs with a single channel (your scenario). AP1
> requests a channel switch with count 7. AP2 gets a notification that
> its channel must switch at count 7. AP2 decides to either follow or to
> shut itself down.
I don't think this whole notification scheme makes sense. Realistically,
we'll only have one hostapd or one wpa_s process controlling it, and
trying to support anything else will result in a big mess. Therefore,
hostapd can already know before the fact that the switch will occur -
it's the one requesting it.
> The same would happen for the GO-follow-client. The client gets a
> channel switch with count 7. GO gets a notification that its channel
> must switch at count 7. GO decides to either follow or shut itself
> down.
This is the only different case, the AP we're connected to is requesting
the switch, and dictating the behaviour.
We *could have* implemented that as a notification to userspace, and
then made wpa_s request the switch with exactly the matching parameters,
but we implemented the reacting in mac80211 instead. That seems like a
reasonable choice since we don't have any real options (other than
disconnect, which we preserve since wpa_s might decide to do that upon
getting a "will switch" notification)
johannes
On Wed, 2014-01-22 at 07:11 +0100, Michal Kazior wrote:
> >> + if (params[i].wdev && params[i].wdev->wiphy != &rdev->wiphy)
> >> + return -EINVAL;
> >
> > How could you ever have a param without a wdev? That seems like a bug as
> > well?
>
> cfg80211_can_add_interface() calls with wdev == NULL.
Oh, right, sorry.
> >> + err = cfg80211_is_radar_required(params[i].iftype,
> >> + params[i].chan,
> >> + params[i].chanmode,
> >> + params[i].radar_detect_width);
> >> + if (err < 0)
> >> + return err;
> >> + else if (err && !params[i].radar_detect_width)
> >> + return -EINVAL;
> >
> > That's a bit confusing - why do you pass the radar_detect_width in, and
> > then check it again? Couldn't you just do all the checks in the
> > function, perhaps renamed a bit?
>
> I can move this into the helper function. Any suggestions for the
> rename? cfg80211_validate_radar_params() and it would return a 0 or an
> errno?
Sounds good to me.
johannes
On Wed, 2014-01-22 at 07:36 +0100, Michal Kazior wrote:
> >> + if (!attrs[NL80211_ATTR_IFINDEX])
> >> + return -EINVAL;
> >
> > Hmm, shouldn't you use the ifindex from the csa_attrs? Or are you
> > requiring one to be in the outer data to identify the wiphy? But that
> > seems odd too, you can just get the wiphy in whatever way by asking the
> > pre_doit for that?
>
> I'm not sure I understand you.
>
> There are two command variants now:
>
> 1: the old one, which has ifindex in the root
> 2: the new one, which has wiphy in the root, and an array of (1)
> within CH_SWITCH_IFACES (and each entry has ifindex at its root)
Yeah I think I confused the two. This is handling case 1 only, right?
But you have to check the ifindex manually since for case 2 you can only
require the pre_doit() to give you the wiphy.
johannes
On Thu, 2014-01-23 at 09:26 +0100, Johannes Berg wrote:
> On Thu, 2014-01-23 at 08:28 +0200, Luca Coelho wrote:
>
> > Why is this necessary? I think it's easier to treat the CSA's
> > separately. When wpa_supplicant decides to switch channel in one
> > interface, it asks cfg80211 to do so. Then, if that interface change is
> > "approved" (ie. there's no internal reason why it would fail), cfg80211
> > sends a notification to userspace that the interface will switch. Then
> > wpa_s decides whether any other interfaces need switching as well (ie.
> > if it's in the same channel context).
>
> I don't think that makes sense. If userspace requested the CSA
> operation, the fact that it will happen should be known as soon as the
> request returns 0. No need for a notification (other than when the mlme
> code in mac80211 decided to switch by itself). You're mixing the cases
> of GO-follow-client-interface and switch-multiple-AP-BSSes, which I'm
> not sure is right.
I think all cases should be treated in the same way. No need to treat
different scenarios differently.
I also like to think that, even though it's probably not a good idea, we
*could* have two different processes in userspace handling the different
interfaces (eg. two hostapd's). If only the return value of the request
is used, any other instances will not know something is happening.
I don't see how a notification can hurt. It is just good to align all
cases: mlme decided to switch or the userspace decided to switch. In
both cases, whoever is listening will get the notification that
something is happening.
> The problem with using the approach you're suggesting to switch multiple
> AP interfaces is that when you do that, you open the system up to races
> much more than when requesting multiple interfaces at once. Also, I
> still think we need to enforce that the interface switch at the same
> time, which would practically be impossible with multiple switch
> commands.
This is needed (and may cause races) only for the
"no-more-available-contexts" scenario (eg. single channel).
We could "merge" the multiple switch commands in the kernel. Either by
"dragging" all the interfaces together (as I suggested in another thread
-- still under debate) or by informing the userspace when the actual
switch will happen (with a notification).
Let's say we have 2 APs with a single channel (your scenario). AP1
requests a channel switch with count 7. AP2 gets a notification that
its channel must switch at count 7. AP2 decides to either follow or to
shut itself down.
The same would happen for the GO-follow-client. The client gets a
channel switch with count 7. GO gets a notification that its channel
must switch at count 7. GO decides to either follow or shut itself
down.
Or, another scenario: STA and AP1 with a single channel. STA gets a
channel swith announcement from its AP2 on count 7. AP1 gets a
notification that its channel must switch at count 7. AP1 decides to
either follow or to shut itself down.
There is a higher risk of races with my proposal, but if the count used
is high enough, it shouldn't be too bad. And, in any case, the
multiple-vifs in a single CSA request only solves the races in the first
scenario (2 APs).
> > > This makes it possible for userspace to request a
> > > multi-interface channel switch while allowing
> > > sanity checks wrt target interface/channel
> > > combination.
> >
> > Okay, the sanity checks are an interesting issue. But if the
> > combination is not good, the whole command will fail and none of the
> > interfaces will switch. If wpa_s does this interface-by-interface,
> > separately, the triggering interface (ie. the one that decided to switch
> > in the first place) may succeed while the subsequent requests may fail
> > (eg. if the interface combination is not allowed in the new channel).
> > If the second one fails, wpa_s may make another decision, for instance
> > trying to switch the second interface to a *different* channel instead,
> > or not switch at all.
>
> That makes no sense. You're talking about completely different
> scenarios, see above.
I'm trying to abstract the scenarios as much as possible and only talk
about the interfaces (whichever types they are).
--
Luca.
On Mon, 2014-01-20 at 15:09 +0100, Michal Kazior wrote:
> @@ -2583,6 +2588,7 @@ enum wiphy_flags {
> WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL = BIT(21),
> WIPHY_FLAG_SUPPORTS_5_10_MHZ = BIT(22),
> WIPHY_FLAG_HAS_CHANNEL_SWITCH = BIT(23),
> + WIPHY_FLAG_HAS_MULTI_IF_CHSWITCH = BIT(24),
> };
You could probably use an nl80211 feature flag directly, see e.g.
NL80211_FEATURE_HT_IBSS (enum nl80211_feature_flags) and save the code
to set the extra flag attribute.
> + * @NL80211_ATTR_CH_SWITCH_IFACES: Nested attribute with channel switch
> + * settings in each entry (ifindex, frequency, beacon IEs). Also used as a
> + * device capability flag in nl80211_send_wiphy().
Which would obviously change this.
> +int ieee80211_channel_switch(struct wiphy *wiphy,
> + struct cfg80211_csa_settings *params,
> + int num_params)
> +{
> + struct ieee80211_sub_if_data *sdata;
> + int err;
> +
> + /* multi-vif CSA is not implemented */
> + if (num_params > 1)
> + return -EOPNOTSUPP;
The capability flag should be checked in cfg80211, so this shouldn't be
needed?
> +int ieee80211_channel_switch(struct wiphy *wiphy,
> + struct cfg80211_csa_settings *params,
> + int num_params);
This function can be static now as the other places call the __ version
now. Careful with locking though - you just moved it as well, won't the
callers have to be updated?
> @@ -1270,6 +1271,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *dev,
> if ((dev->wiphy.flags & WIPHY_FLAG_TDLS_EXTERNAL_SETUP) &&
> nla_put_flag(msg, NL80211_ATTR_TDLS_EXTERNAL_SETUP))
> goto nla_put_failure;
> + if ((dev->wiphy.flags & WIPHY_FLAG_HAS_MULTI_IF_CHSWITCH) &&
> + nla_put_flag(msg, NL80211_ATTR_CH_SWITCH_IFACES))
> + goto nla_put_failure;
> state->split_start++;
> if (state->split)
> break;
This goes away, which also saves you the trouble of moving it to a
correct place (due to size constraints, we can't increase the amount of
data available to really old iw/wpa_s versions)
> + /* 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];
Technically we could even just reuse family->attrs since we're also
under genl_lock :)
> - u8 radar_detect_width = 0;
> + struct net_device *dev = NULL;
> + struct wireless_dev *wdev;
> int err;
> bool need_new_beacon = false;
>
> - if (!rdev->ops->channel_switch ||
> - !(rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH))
> - return -EOPNOTSUPP;
> + ASSERT_RTNL();
> +
> + if (!attrs[NL80211_ATTR_IFINDEX])
> + return -EINVAL;
Hmm, shouldn't you use the ifindex from the csa_attrs? Or are you
requiring one to be in the outer data to identify the wiphy? But that
seems odd too, you can just get the wiphy in whatever way by asking the
pre_doit for that?
> + nla_for_each_nested(attrs,
> + info->attrs[NL80211_ATTR_CH_SWITCH_IFACES],
> + tmp) {
> + nla_parse(csa_attrs, NL80211_ATTR_MAX, nla_data(attrs),
> + nla_len(attrs), nl80211_policy);
I think nla_parse() can fail, you should check that, otherwise there's
no full validity checking of the attributes contained in it I believe.
johannes