This is a follow up from PATCHv1 last week. This patchset adds generic
channel switch support for AP. This is required for DFS operation
(e.g. Wi-Fi Alliance requires this for 802.11h certification). This will also
be required for IBSS-DFS later.
Changes from PATCHv1:
* integrate Felix suggestion to perform the channel switch after the beacon send
has been completed. This required a few minor design changes (i.e. split into
functions for decrementation and check for csa completion in mac80211)
* few minor changes (e.g. check for CSA in mac80211, documentation)
The rough design is:
* userspace asks kernel to switch a channel using the new NL80211_CMD_CHANNEL_SWITCH
command. It supplies IE information for the time while staying on the old channel and
announcing the switch, and IE information for after the switch to the new channel.
* IE information contains the beacon and optionally probe responses, which should
include (E)CSA IEs for the CSA case. Furthermore an offset is provided (for beacon
and probe response) to point to the counter field within the channel switch IEs.
* The driver gets the new beacons passed and must set them, and decrement the
counter field. When it reaches 0, the channel is changed and userspace notified.
Discussion points:
* Assembling all these IE information is a little bit tedious but doable (I've
patched hostapd).
* Other future users like IBSS/MESH will not get the beacon/probe response IEs, as they
generate these beacons themselves. Therefore they need the COUNT attribute, which
is kind of duplicate right now.
* Userspace must generate/handle all IEs, which lifts the previous limitations of
the RFC (e.g. no change of band allowed, no operation mode change allowed).
* it currently works for me [TM] on my ath9k based machine
As always, any comments are appreciated.
Cheers,
Simon
Simon Wunderlich (5):
nl80211: use attributes to parse beacons
nl80211/cfg80211: add channel switch command
mac80211: add functions to duplicate a cfg80211_beacon
mac80211: add channel switch command and beacon callbacks
ath9k: enable CSA functionality in ath9k
drivers/net/wireless/ath/ath9k/ath9k.h | 2 +
drivers/net/wireless/ath/ath9k/beacon.c | 21 ++++
drivers/net/wireless/ath/ath9k/main.c | 17 +++
drivers/net/wireless/ath/ath9k/xmit.c | 2 +
include/net/cfg80211.h | 26 +++++
include/net/mac80211.h | 33 ++++++
include/uapi/linux/nl80211.h | 29 +++++
net/mac80211/cfg.c | 185 ++++++++++++++++++++++++++++++-
net/mac80211/driver-ops.h | 11 ++
net/mac80211/ieee80211_i.h | 11 ++
net/mac80211/iface.c | 2 +
net/mac80211/trace.h | 20 ++++
net/mac80211/tx.c | 68 ++++++++++++
net/wireless/nl80211.c | 171 +++++++++++++++++++++++-----
14 files changed, 567 insertions(+), 31 deletions(-)
--
1.7.10.4
> + * @NL80211_CMD_CHANNEL_SWITCH: Perform a channel switch by announcing the
> + * the new channel information (Channel Switch Announcement - CSA)
> + * in the beacon for some time (as defined in the
> + * %NL80211_ATTR_CH_SWITCH_COUNT parameter) and then change to the
> + * new channel. Userspace provides the new channel information (using
> + * %NL80211_ATTR_WIPHY_FREQ and the attributes determining channel
> + * width). %NL80211_ATTR_CH_SWITCH_BLOCK_TX may be supplied to inform
> + * other station that transmission must be blocked until the channel
> + * switch is complete.
We probably need a flag to indicate that the command is available, since
mac80211 might implement it but not all drivers will?
johannes
To allow channel switch announcements within beacons, add
the channel switch command to nl80211/cfg80211. This is
implementation is intended for AP and (later) IBSS mode.
Signed-off-by: Simon Wunderlich <[email protected]>
Signed-off-by: Mathias Kretschmer <[email protected]>
---
Changes to RFCv1:
* accept and pass CSA IEs and after-change IEs
* use parameter structure instead of individual parameters
---
include/net/cfg80211.h | 26 ++++++++++
include/uapi/linux/nl80211.h | 29 +++++++++++
net/wireless/nl80211.c | 118 +++++++++++++++++++++++++++++++++++++++++-
3 files changed, 172 insertions(+), 1 deletion(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 3c3a563..0de5384 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -610,6 +610,29 @@ struct cfg80211_ap_settings {
bool radar_required;
};
+/* struct cfg80211_csa_settings - channel switch settings
+ *
+ * Used for channel switch
+ *
+ * @chandef: defines the channel to use after the switch
+ * @beacon_csa: beacon data while performing the switch
+ * @counter_offset_beacon: offset for the counter within the beacon (tail)
+ * @counter_offset_presp: offset for the counter within the probe response
+ * @beacon_after: beacon data to be used on the new channel
+ * @radar_required: whether radar detection is required on the new channel
+ * @block_tx: whether transmissions should be blocked while changing
+ * @count: number of beacons until switch
+ */
+struct cfg80211_csa_settings {
+ struct cfg80211_chan_def chandef;
+ struct cfg80211_beacon_data beacon_csa;
+ u16 counter_offset_beacon, counter_offset_presp;
+ struct cfg80211_beacon_data beacon_after;
+ bool radar_required;
+ bool block_tx;
+ u8 count;
+};
+
/**
* enum station_parameters_apply_mask - station parameter values to apply
* @STATION_PARAM_APPLY_UAPSD: apply new uAPSD parameters (uapsd_queues, max_sp)
@@ -2278,6 +2301,9 @@ struct cfg80211_ops {
u16 duration);
void (*crit_proto_stop)(struct wiphy *wiphy,
struct wireless_dev *wdev);
+ int (*channel_switch)(struct wiphy *wiphy,
+ struct net_device *dev,
+ struct cfg80211_csa_settings *params);
};
/*
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index ca6facf..ddf3b9e 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -648,6 +648,16 @@
* @NL80211_CMD_CRIT_PROTOCOL_STOP: Indicates the connection reliability can
* return back to normal.
*
+ * @NL80211_CMD_CHANNEL_SWITCH: Perform a channel switch by announcing the
+ * the new channel information (Channel Switch Announcement - CSA)
+ * in the beacon for some time (as defined in the
+ * %NL80211_ATTR_CH_SWITCH_COUNT parameter) and then change to the
+ * new channel. Userspace provides the new channel information (using
+ * %NL80211_ATTR_WIPHY_FREQ and the attributes determining channel
+ * width). %NL80211_ATTR_CH_SWITCH_BLOCK_TX may be supplied to inform
+ * other station that transmission must be blocked until the channel
+ * switch is complete.
+ *
* @NL80211_CMD_MAX: highest used command number
* @__NL80211_CMD_AFTER_LAST: internal use
*/
@@ -810,6 +820,7 @@ enum nl80211_commands {
NL80211_CMD_CRIT_PROTOCOL_START,
NL80211_CMD_CRIT_PROTOCOL_STOP,
+ NL80211_CMD_CHANNEL_SWITCH,
/* add new commands above here */
/* used to define NL80211_CMD_MAX below */
@@ -1436,6 +1447,18 @@ enum nl80211_commands {
* allowed to be used with the first @NL80211_CMD_SET_STATION command to
* update a TDLS peer STA entry.
*
+ * @NL80211_ATTR_CH_SWITCH_COUNT: u32 attribute specifying the number of TBTT's
+ * until the channel switch event.
+ * @NL80211_ATTR_CH_SWITCH_BLOCK_TX: flag attribute specifying that transmission
+ * must be blocked on the current channel (before the channel switch
+ * operation).
+ * @NL80211_ATTR_CSA_IES: Nested set of attributes containing the IE information
+ * for the time while performing a channel switch.
+ * @NL80211_ATTR_CSA_C_OFF_BEACON: Offset of the channel switch counter
+ * field in the beacons tail (%NL80211_ATTR_BEACON_TAIL).
+ * @NL80211_ATTR_CSA_C_OFF_PRESP: Offset of the channel switch counter
+ * field in the probe response (%NL80211_ATTR_PROBE_RESP).
+ *
* @NL80211_ATTR_MAX: highest attribute number currently defined
* @__NL80211_ATTR_AFTER_LAST: internal use
*/
@@ -1736,6 +1759,12 @@ enum nl80211_attrs {
NL80211_ATTR_PEER_AID,
+ NL80211_ATTR_CH_SWITCH_COUNT,
+ NL80211_ATTR_CH_SWITCH_BLOCK_TX,
+ NL80211_ATTR_CSA_IES,
+ NL80211_ATTR_CSA_C_OFF_BEACON,
+ NL80211_ATTR_CSA_C_OFF_PRESP,
+
/* add attributes here, update the policy in nl80211.c */
__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 1c4f7da..d48a00d 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -349,6 +349,11 @@ static const struct nla_policy nl80211_policy[NL80211_ATTR_MAX+1] = {
[NL80211_ATTR_IE_RIC] = { .type = NLA_BINARY,
.len = IEEE80211_MAX_DATA_LEN },
[NL80211_ATTR_PEER_AID] = { .type = NLA_U16 },
+ [NL80211_ATTR_CH_SWITCH_COUNT] = { .type = NLA_U32 },
+ [NL80211_ATTR_CH_SWITCH_BLOCK_TX] = { .type = NLA_FLAG },
+ [NL80211_ATTR_CSA_IES] = { .type = NLA_NESTED },
+ [NL80211_ATTR_CSA_C_OFF_BEACON] = { .type = NLA_U16 },
+ [NL80211_ATTR_CSA_C_OFF_PRESP] = { .type = NLA_U16 },
};
/* policy for the key attributes */
@@ -5540,6 +5545,109 @@ 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)
+{
+ 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;
+ struct nlattr *csa_ies[NL80211_ATTR_MAX+1];
+ u8 radar_detect_width = 0;
+ int err;
+
+ if (!rdev->ops->channel_switch)
+ return -EOPNOTSUPP;
+
+ /* may add IBSS support later */
+ if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_AP &&
+ dev->ieee80211_ptr->iftype != NL80211_IFTYPE_P2P_GO)
+ return -EOPNOTSUPP;
+
+ memset(¶ms, 0, sizeof(params));
+
+ if (!info->attrs[NL80211_ATTR_WIPHY_FREQ] ||
+ !info->attrs[NL80211_ATTR_CH_SWITCH_COUNT])
+ return -EINVAL;
+
+ /* only important for AP, IBSS and mesh create IEs internally */
+ if (!info->attrs[NL80211_ATTR_CSA_IES] ||
+ !info->attrs[NL80211_ATTR_CSA_C_OFF_BEACON])
+ return -EINVAL;
+
+ /* useless if AP is not running */
+ if (!wdev->beacon_interval)
+ return -EINVAL;
+
+ params.count = nla_get_u32(info->attrs[NL80211_ATTR_CH_SWITCH_COUNT]);
+
+ err = nl80211_parse_beacon(info->attrs, ¶ms.beacon_after);
+ if (err)
+ return err;
+
+ err = nla_parse_nested(csa_ies, NL80211_ATTR_MAX,
+ info->attrs[NL80211_ATTR_CSA_IES],
+ nl80211_policy);
+ if (err)
+ return err;
+
+ err = nl80211_parse_beacon(csa_ies, ¶ms.beacon_csa);
+ if (err)
+ return err;
+
+ params.counter_offset_beacon =
+ nla_get_u16(info->attrs[NL80211_ATTR_CSA_C_OFF_BEACON]);
+ if (params.counter_offset_beacon > params.beacon_csa.tail_len)
+ return -EINVAL;
+
+ /* sanity check - counter should be the same this should be the same */
+ if (params.beacon_csa.tail[params.counter_offset_beacon] !=
+ params.count)
+ return -EINVAL;
+
+ if (info->attrs[NL80211_ATTR_CSA_C_OFF_PRESP]) {
+ params.counter_offset_presp =
+ nla_get_u16(info->attrs[NL80211_ATTR_CSA_C_OFF_PRESP]);
+ if (params.counter_offset_presp >
+ params.beacon_csa.probe_resp_len)
+ return -EINVAL;
+
+ if (params.beacon_csa.probe_resp[params.counter_offset_presp] !=
+ params.count)
+ return -EINVAL;
+ }
+
+ err = nl80211_parse_chandef(rdev, info, ¶ms.chandef);
+ if (err)
+ return err;
+
+ if (!cfg80211_reg_can_beacon(&rdev->wiphy, ¶ms.chandef))
+ return -EINVAL;
+
+ err = cfg80211_chandef_dfs_required(wdev->wiphy, ¶ms.chandef);
+ if (err < 0)
+ return err;
+
+ if (err)
+ radar_detect_width = BIT(params.chandef.width);
+ else
+ radar_detect_width = 0;
+
+ err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
+ params.chandef.chan,
+ CHAN_MODE_SHARED,
+ radar_detect_width);
+ if (err)
+ return err;
+
+ if (info->attrs[NL80211_ATTR_CH_SWITCH_BLOCK_TX])
+ params.block_tx = true;
+
+ err = rdev->ops->channel_switch(&rdev->wiphy, dev, ¶ms);
+
+ return err;
+}
+
static int nl80211_send_bss(struct sk_buff *msg, struct netlink_callback *cb,
u32 seq, int flags,
struct cfg80211_registered_device *rdev,
@@ -8995,7 +9103,15 @@ static struct genl_ops nl80211_ops[] = {
.flags = GENL_ADMIN_PERM,
.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
NL80211_FLAG_NEED_RTNL,
- }
+ },
+ {
+ .cmd = NL80211_CMD_CHANNEL_SWITCH,
+ .doit = nl80211_channel_switch,
+ .policy = nl80211_policy,
+ .flags = GENL_ADMIN_PERM,
+ .internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+ NL80211_FLAG_NEED_RTNL,
+ },
};
static struct genl_multicast_group nl80211_mlme_mcgrp = {
--
1.7.10.4
On Fri, 2013-06-14 at 14:15 +0200, Simon Wunderlich wrote:
> + * @channel_switch_beacon: Starts a channel switch for the to a new channel.
for the ?
> + * Beacons are modified to include CSA or ECSA IEs before calling this
> + * function. The corresponding count fields in these IEs must be
> + * decremented, and when they reach zero the driver must call
> + * ieee80211_csa_finish(). Drivers which use ieee80211_beacon_get()
> + * get the csa counter decremented by mac80211, but must check if it is
> + * zero using ieee80211_csa_is_complete() and then call
> + * ieee80211_csa_finish().
... when the beacon was transmitted, or something like that, right?
> @@ -2818,6 +2830,8 @@ struct ieee80211_ops {
> struct ieee80211_vif *vif,
> struct inet6_dev *idev);
> #endif
> + void (*channel_switch_beacon)(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif);
What about channel contexts? Actually I don't really understand this?
Shouldn't it say which channel to switch to?
> /**
> + * ieee80211_csa_finish - notify mac80211 about channel switch
> + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> + *
> + * After a channel switch announcement was scheduled and the counter in this
> + * announcement hit zero, this function must be called by the driver to
> + * notify mac80211 that the channel can be changed.
> + */
> +void ieee80211_csa_finish(struct ieee80211_vif *vif);
If there are multiple interfaces, should it be called multiple times?
etc. Maybe it should be on a channel context instead?
> +/**
> + * ieee80211_csa_is_complete - find out if counters reached zero
> + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> + *
> + * This function checks the registered beacon if the counters reached zero.
> + */
> +int ieee80211_csa_is_complete(struct ieee80211_vif *vif);
Huh? Return value? Maybe it should be bool?
> + netif_carrier_off(sdata->dev);
> + err = ieee80211_vif_use_channel(sdata, &local->csa_chandef,
> + IEEE80211_CHANCTX_SHARED);
> + netif_carrier_on(sdata->dev);
That seems like a really bad idea, deleting a channel context might tear
down all kinds of device state and might require deleting the interface
first ... I think the chan context API needs to be extended to switch
instead.
> + if (WARN_ON(err < 0))
> + return;
This can fail _easily_ too, e.g. if some other vif stays on the channel
and you're now using too many channel contexts.
> + /* don't handle if chanctx is used */
> + if (local->use_chanctx)
> + return -EBUSY;
Still don't really like the way you've implemented it :-)
> + rcu_read_lock();
> + mutex_lock(&local->chanctx_mtx);
Yeah right.
> +static inline void
> +drv_channel_switch_beacon(struct ieee80211_sub_if_data *sdata)
> +{
> + struct ieee80211_local *local = sdata->local;
> + if (local->ops->channel_switch_beacon) {
blank line between these two please
> + trace_drv_channel_switch_beacon(sdata);
> + local->ops->channel_switch_beacon(&local->hw, &sdata->vif);
> + }
> +}
> +
> +
steal one from here ;-)
> void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
> +/* channel switch handling */
I'd prefer a blank line before the comment
> +TRACE_EVENT(drv_channel_switch_beacon,
> + TP_PROTO(struct ieee80211_sub_if_data *sdata),
Use local_sdata_evt I think.
> +void ieee80211_csa_finish(struct ieee80211_vif *vif)
> +{
> + struct ieee80211_sub_if_data *sdata = NULL;
> + sdata = vif_to_sdata(vif);
ahem.
> + vif->csa_active = 0;
is that a counter, or should it be a bool?
> +static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
> + struct beacon_data *beacon)
> +{
> + struct probe_resp *resp;
> + int counter_beacon = sdata->csa_counter_offset_beacon;
> + int counter_presp = sdata->csa_counter_offset_presp;
> +
> + if (WARN_ON(counter_beacon > beacon->tail_len))
> + return;
> +
> + if (WARN_ON(((u8 *)beacon->tail)[counter_beacon] == 0))
> + return;
How can these happen?
> + ((u8 *)beacon->tail)[counter_beacon]--;
> +
> + if (counter_presp && sdata->vif.type == NL80211_IFTYPE_AP) {
> + resp = rcu_dereference(sdata->u.ap.probe_resp);
Who guarantees RCU protection?
> + if (WARN_ON(!resp))
> + return;
That can legimitately happen, no? At least userspace is allowed to not
set probe_resp now, if you want to change that ...
> + if (WARN_ON(counter_presp > resp->len))
> + return;
?
I guess counter_presp should be called "counter_offset_presp" or so
> + ((u8 *)resp->data)[counter_presp]--;
resp->data is already a u8 *, no?
> +int ieee80211_csa_is_complete(struct ieee80211_vif *vif)
> +{
> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> + struct beacon_data *beacon = NULL;
> + int counter_beacon = sdata->csa_counter_offset_beacon;
> +
> + if (!sdata || !ieee80211_sdata_running(sdata))
> + return 0;
yeah right ... container_of returned NULL?
> + if (vif->type == NL80211_IFTYPE_AP) {
> + struct ieee80211_if_ap *ap = &sdata->u.ap;
> + beacon = rcu_dereference(ap->beacon);
> +
swap those last two lines :)
johannes
On Tue, Jun 18, 2013 at 04:48:10PM +0200, Johannes Berg wrote:
>
> > +/* struct cfg80211_csa_settings - channel switch settings
> > + *
>
> /**
> * struct ...
> * ...
>
OK
> > + * @NL80211_ATTR_CH_SWITCH_COUNT: u32 attribute specifying the number of TBTT's
> > + * until the channel switch event.
> > + * @NL80211_ATTR_CH_SWITCH_BLOCK_TX: flag attribute specifying that transmission
> > + * must be blocked on the current channel (before the channel switch
> > + * operation).
> > + * @NL80211_ATTR_CSA_IES: Nested set of attributes containing the IE information
> > + * for the time while performing a channel switch.
> > + * @NL80211_ATTR_CSA_C_OFF_BEACON: Offset of the channel switch counter
> > + * field in the beacons tail (%NL80211_ATTR_BEACON_TAIL).
> > + * @NL80211_ATTR_CSA_C_OFF_PRESP: Offset of the channel switch counter
> > + * field in the probe response (%NL80211_ATTR_PROBE_RESP).
>
> Shouldn't the offset be into the CSA_IES?
>
Hmm ... yeah you are right, that would be better. Will fix that.
> > + struct nlattr *csa_ies[NL80211_ATTR_MAX+1];
>
> Hmm, this doesn't seem right.
>
Why? If it's about namespace, see below. :)
> > + err = nla_parse_nested(csa_ies, NL80211_ATTR_MAX,
> > + info->attrs[NL80211_ATTR_CSA_IES],
> > + nl80211_policy);
>
> (Indentation)
oops...
>
> If you're going to use nested IEs, shouldn't you define a separate
> namespace to be used within the nesting?
The idea was to recycle the nl80211_parse_beacon() function for the CSA IEs,
but if you prefer I can define an own namespace and create a separate function
to check.
Cheers,
Simon
> +/* struct cfg80211_csa_settings - channel switch settings
> + *
/**
* struct ...
* ...
> + * @NL80211_ATTR_CH_SWITCH_COUNT: u32 attribute specifying the number of TBTT's
> + * until the channel switch event.
> + * @NL80211_ATTR_CH_SWITCH_BLOCK_TX: flag attribute specifying that transmission
> + * must be blocked on the current channel (before the channel switch
> + * operation).
> + * @NL80211_ATTR_CSA_IES: Nested set of attributes containing the IE information
> + * for the time while performing a channel switch.
> + * @NL80211_ATTR_CSA_C_OFF_BEACON: Offset of the channel switch counter
> + * field in the beacons tail (%NL80211_ATTR_BEACON_TAIL).
> + * @NL80211_ATTR_CSA_C_OFF_PRESP: Offset of the channel switch counter
> + * field in the probe response (%NL80211_ATTR_PROBE_RESP).
Shouldn't the offset be into the CSA_IES?
> + struct nlattr *csa_ies[NL80211_ATTR_MAX+1];
Hmm, this doesn't seem right.
> + err = nla_parse_nested(csa_ies, NL80211_ATTR_MAX,
> + info->attrs[NL80211_ATTR_CSA_IES],
> + nl80211_policy);
(Indentation)
If you're going to use nested IEs, shouldn't you define a separate
namespace to be used within the nesting?
johannes
On Fri, 2013-06-14 at 14:15 +0200, Simon Wunderlich wrote:
> +static struct cfg80211_beacon_data *
> +cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
> +{
> + struct cfg80211_beacon_data *new_beacon;
> + new_beacon = kzalloc(sizeof(*new_beacon), GFP_KERNEL);
> + if (!beacon)
> + return NULL;
Why not allocate one bigger block and use pointers into it? There's no
(reasonable) way that it'd get really big ...
johannes
On Tue, Jun 18, 2013 at 04:50:30PM +0200, Johannes Berg wrote:
>
> > + * @NL80211_CMD_CHANNEL_SWITCH: Perform a channel switch by announcing the
> > + * the new channel information (Channel Switch Announcement - CSA)
> > + * in the beacon for some time (as defined in the
> > + * %NL80211_ATTR_CH_SWITCH_COUNT parameter) and then change to the
> > + * new channel. Userspace provides the new channel information (using
> > + * %NL80211_ATTR_WIPHY_FREQ and the attributes determining channel
> > + * width). %NL80211_ATTR_CH_SWITCH_BLOCK_TX may be supplied to inform
> > + * other station that transmission must be blocked until the channel
> > + * switch is complete.
>
> We probably need a flag to indicate that the command is available, since
> mac80211 might implement it but not all drivers will?
Yeah ... shall we announce it as CMD() in nl80211_send_wiphy()? Or feature
flag?
Cheers,
Simon
Taking the easy stuff first ...
On Fri, 2013-06-14 at 14:15 +0200, Simon Wunderlich wrote:
> only the attributes are required and not the whole netlink info, as the
> function accesses the attributes only anyway. This makes it easier to
> parse nested beacon IEs later.
I applied this.
johannes
On Tue, 2013-06-18 at 17:18 +0200, Simon Wunderlich wrote:
> On Tue, Jun 18, 2013 at 04:50:30PM +0200, Johannes Berg wrote:
> >
> > > + * @NL80211_CMD_CHANNEL_SWITCH: Perform a channel switch by announcing the
> > > + * the new channel information (Channel Switch Announcement - CSA)
> > > + * in the beacon for some time (as defined in the
> > > + * %NL80211_ATTR_CH_SWITCH_COUNT parameter) and then change to the
> > > + * new channel. Userspace provides the new channel information (using
> > > + * %NL80211_ATTR_WIPHY_FREQ and the attributes determining channel
> > > + * width). %NL80211_ATTR_CH_SWITCH_BLOCK_TX may be supplied to inform
> > > + * other station that transmission must be blocked until the channel
> > > + * switch is complete.
> >
> > We probably need a flag to indicate that the command is available, since
> > mac80211 might implement it but not all drivers will?
>
> Yeah ... shall we announce it as CMD() in nl80211_send_wiphy()? Or feature
> flag?
CMD() works (make sure to only put it if split), but need a wiphy flag
since mac80211 always implements it, I think.
johannes
The count field in CSA must be decremented with each beacon
transmitted. This patch implements the functionality for drivers
using ieee80211_beacon_get(). Other drivers must call back manually
after reaching count == 0.
This patch also contains the handling and finish worker for the channel
switch command.
Signed-off-by: Simon Wunderlich <[email protected]>
Signed-off-by: Mathias Kretschmer <[email protected]>
---
Changes to PATCHv1:
* don't switch when CAC is active
* add hw to parameters for driver
Changes to RFCv1:
* use beacons as supplied from nl80211 without generating/parsing them
* update beacons using offsets
* report back by calling the channel switch event in cfg80211
---
include/net/mac80211.h | 33 ++++++++++++++
net/mac80211/cfg.c | 109 +++++++++++++++++++++++++++++++++++++++++++-
net/mac80211/driver-ops.h | 11 +++++
net/mac80211/ieee80211_i.h | 11 +++++
net/mac80211/iface.c | 2 +
net/mac80211/trace.h | 20 ++++++++
net/mac80211/tx.c | 68 +++++++++++++++++++++++++++
7 files changed, 252 insertions(+), 2 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index a405a7a..f0592cd 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1078,6 +1078,7 @@ enum ieee80211_vif_flags {
* @addr: address of this interface
* @p2p: indicates whether this AP or STA interface is a p2p
* interface, i.e. a GO or p2p-sta respectively
+ * @csa_active: marks whether a channel switch is going on
* @driver_flags: flags/capabilities the driver has for this interface,
* these need to be set (or cleared) when the interface is added
* or, if supported by the driver, the interface type is changed
@@ -1100,6 +1101,7 @@ struct ieee80211_vif {
struct ieee80211_bss_conf bss_conf;
u8 addr[ETH_ALEN];
bool p2p;
+ bool csa_active;
u8 cab_queue;
u8 hw_queue[IEEE80211_NUM_ACS];
@@ -2631,6 +2633,16 @@ enum ieee80211_roc_type {
* @ipv6_addr_change: IPv6 address assignment on the given interface changed.
* Currently, this is only called for managed or P2P client interfaces.
* This callback is optional; it must not sleep.
+ *
+ * @channel_switch_beacon: Starts a channel switch for the to a new channel.
+ * Beacons are modified to include CSA or ECSA IEs before calling this
+ * function. The corresponding count fields in these IEs must be
+ * decremented, and when they reach zero the driver must call
+ * ieee80211_csa_finish(). Drivers which use ieee80211_beacon_get()
+ * get the csa counter decremented by mac80211, but must check if it is
+ * zero using ieee80211_csa_is_complete() and then call
+ * ieee80211_csa_finish().
+ *
*/
struct ieee80211_ops {
void (*tx)(struct ieee80211_hw *hw,
@@ -2818,6 +2830,8 @@ struct ieee80211_ops {
struct ieee80211_vif *vif,
struct inet6_dev *idev);
#endif
+ void (*channel_switch_beacon)(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif);
};
/**
@@ -3313,6 +3327,25 @@ static inline struct sk_buff *ieee80211_beacon_get(struct ieee80211_hw *hw,
}
/**
+ * ieee80211_csa_finish - notify mac80211 about channel switch
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback.
+ *
+ * After a channel switch announcement was scheduled and the counter in this
+ * announcement hit zero, this function must be called by the driver to
+ * notify mac80211 that the channel can be changed.
+ */
+void ieee80211_csa_finish(struct ieee80211_vif *vif);
+
+/**
+ * ieee80211_csa_is_complete - find out if counters reached zero
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback.
+ *
+ * This function checks the registered beacon if the counters reached zero.
+ */
+int ieee80211_csa_is_complete(struct ieee80211_vif *vif);
+
+
+/**
* ieee80211_proberesp_get - retrieve a Probe Response template
* @hw: pointer obtained from ieee80211_alloc_hw().
* @vif: &struct ieee80211_vif pointer from the add_interface callback.
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 5ffc7d9..e468465 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -854,8 +854,8 @@ static int ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata,
return 0;
}
-static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
- struct cfg80211_beacon_data *params)
+int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_beacon_data *params)
{
struct beacon_data *new, *old;
int new_head_len, new_tail_len;
@@ -2854,6 +2854,110 @@ error:
return NULL;
}
+void ieee80211_csa_finalize_work(struct work_struct *work)
+{
+ struct ieee80211_sub_if_data *sdata =
+ container_of(work, struct ieee80211_sub_if_data,
+ csa_finalize_work);
+ struct ieee80211_local *local = sdata->local;
+ int err;
+
+ if (!ieee80211_sdata_running(sdata))
+ return;
+
+ if (WARN_ON(sdata->vif.type != NL80211_IFTYPE_AP))
+ return;
+
+ netif_carrier_off(sdata->dev);
+ err = ieee80211_vif_use_channel(sdata, &local->csa_chandef,
+ IEEE80211_CHANCTX_SHARED);
+ netif_carrier_on(sdata->dev);
+ if (WARN_ON(err < 0))
+ return;
+ netif_carrier_on(sdata->dev);
+
+ err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
+ cfg80211_beacon_free(sdata->u.ap.next_beacon);
+ sdata->u.ap.next_beacon = NULL;
+
+ ieee80211_wake_queues_by_reason(&sdata->local->hw,
+ IEEE80211_MAX_QUEUE_MAP,
+ IEEE80211_QUEUE_STOP_REASON_CSA);
+
+ ieee80211_bss_info_change_notify(sdata, err);
+
+ cfg80211_ch_switch_notify(sdata->dev, &local->csa_chandef);
+}
+
+static 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;
+ struct ieee80211_chanctx_conf *chanctx_conf;
+ struct ieee80211_chanctx *chanctx;
+ int err;
+
+ if (!list_empty(&local->roc_list) || local->scanning)
+ return -EBUSY;
+
+ if (sdata->wdev.cac_started)
+ return -EBUSY;
+
+ /* don't handle if chanctx is used */
+ if (local->use_chanctx)
+ return -EBUSY;
+
+ rcu_read_lock();
+ mutex_lock(&local->chanctx_mtx);
+ chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
+ if (!chanctx_conf) {
+ mutex_unlock(&local->chanctx_mtx);
+ rcu_read_unlock();
+ return -EBUSY;
+ }
+
+ /* don't handle for multi-VIF cases */
+ chanctx = container_of(chanctx_conf, struct ieee80211_chanctx, conf);
+ if (chanctx->refcount > 1) {
+ mutex_unlock(&local->chanctx_mtx);
+ rcu_read_unlock();
+ return -EBUSY;
+ }
+ mutex_unlock(&local->chanctx_mtx);
+ rcu_read_unlock();
+
+ /* only handle AP for now. */
+ switch (sdata->vif.type) {
+ case NL80211_IFTYPE_AP:
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ sdata->csa_counter_offset_beacon = params->counter_offset_beacon;
+ sdata->csa_counter_offset_presp = params->counter_offset_presp;
+ sdata->u.ap.next_beacon = cfg80211_beacon_dup(¶ms->beacon_after);
+ if (!sdata->u.ap.next_beacon)
+ return -ENOMEM;
+
+ if (params->block_tx)
+ ieee80211_stop_queues_by_reason(&local->hw,
+ IEEE80211_MAX_QUEUE_MAP,
+ IEEE80211_QUEUE_STOP_REASON_CSA);
+
+ err = ieee80211_assign_beacon(sdata, ¶ms->beacon_csa);
+ if (err < 0)
+ return err;
+
+ local->csa_chandef = params->chandef;
+
+ ieee80211_bss_info_change_notify(sdata, err);
+ drv_channel_switch_beacon(sdata);
+
+ return 0;
+}
+
static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
struct ieee80211_channel *chan, bool offchan,
unsigned int wait, const u8 *buf, size_t len,
@@ -3581,4 +3685,5 @@ struct cfg80211_ops mac80211_config_ops = {
.get_et_strings = ieee80211_get_et_strings,
.get_channel = ieee80211_cfg_get_channel,
.start_radar_detection = ieee80211_start_radar_detection,
+ .channel_switch = ieee80211_channel_switch,
};
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index b931c96..a8952f6 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1072,4 +1072,15 @@ static inline void drv_ipv6_addr_change(struct ieee80211_local *local,
}
#endif
+static inline void
+drv_channel_switch_beacon(struct ieee80211_sub_if_data *sdata)
+{
+ struct ieee80211_local *local = sdata->local;
+ if (local->ops->channel_switch_beacon) {
+ trace_drv_channel_switch_beacon(sdata);
+ local->ops->channel_switch_beacon(&local->hw, &sdata->vif);
+ }
+}
+
+
#endif /* __MAC80211_DRIVER_OPS */
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 7a6f1a0..d34d816 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -258,6 +258,8 @@ struct ieee80211_if_ap {
struct beacon_data __rcu *beacon;
struct probe_resp __rcu *probe_resp;
+ /* to be used after channel switch. */
+ struct cfg80211_beacon_data *next_beacon;
struct list_head vlans;
struct ps_data ps;
@@ -713,6 +715,10 @@ struct ieee80211_sub_if_data {
struct ieee80211_tx_queue_params tx_conf[IEEE80211_NUM_ACS];
+ struct work_struct csa_finalize_work;
+ int csa_counter_offset_beacon;
+ int csa_counter_offset_presp;
+
/* used to reconfigure hardware SM PS */
struct work_struct recalc_smps;
@@ -1340,6 +1346,8 @@ void ieee80211_roc_purge(struct ieee80211_local *local,
void ieee80211_roc_notify_destroy(struct ieee80211_roc_work *roc, bool free);
void ieee80211_sw_roc_work(struct work_struct *work);
void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
+/* channel switch handling */
+void ieee80211_csa_finalize_work(struct work_struct *work);
/* interface handling */
int ieee80211_iface_init(void);
@@ -1362,6 +1370,9 @@ void ieee80211_del_virtual_monitor(struct ieee80211_local *local);
bool __ieee80211_recalc_txpower(struct ieee80211_sub_if_data *sdata);
void ieee80211_recalc_txpower(struct ieee80211_sub_if_data *sdata);
+int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_beacon_data *params);
+
static inline bool ieee80211_sdata_running(struct ieee80211_sub_if_data *sdata)
{
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 7cabaf2..d17692d 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -800,6 +800,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
cancel_work_sync(&local->dynamic_ps_enable_work);
cancel_work_sync(&sdata->recalc_smps);
+ cancel_work_sync(&sdata->csa_finalize_work);
cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
@@ -1263,6 +1264,7 @@ static void ieee80211_setup_sdata(struct ieee80211_sub_if_data *sdata,
skb_queue_head_init(&sdata->skb_queue);
INIT_WORK(&sdata->work, ieee80211_iface_work);
INIT_WORK(&sdata->recalc_smps, ieee80211_recalc_smps_work);
+ INIT_WORK(&sdata->csa_finalize_work, ieee80211_csa_finalize_work);
switch (type) {
case NL80211_IFTYPE_P2P_GO:
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index c215fafd7..6b2af54 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -1906,6 +1906,26 @@ TRACE_EVENT(api_radar_detected,
)
);
+TRACE_EVENT(drv_channel_switch_beacon,
+ TP_PROTO(struct ieee80211_sub_if_data *sdata),
+
+ TP_ARGS(sdata),
+
+ TP_STRUCT__entry(
+ VIF_ENTRY
+ ),
+
+ TP_fast_assign(
+ VIF_ASSIGN;
+ ),
+
+ TP_printk(
+ VIF_PR_FMT " channel switch",
+ VIF_PR_ARG
+ )
+);
+
+
#ifdef CONFIG_MAC80211_MESSAGE_TRACING
#undef TRACE_SYSTEM
#define TRACE_SYSTEM mac80211_msg
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 4105d0c..97751e4 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2320,6 +2320,71 @@ static int ieee80211_beacon_add_tim(struct ieee80211_sub_if_data *sdata,
return 0;
}
+void ieee80211_csa_finish(struct ieee80211_vif *vif)
+{
+ struct ieee80211_sub_if_data *sdata = NULL;
+ sdata = vif_to_sdata(vif);
+
+ vif->csa_active = 0;
+ ieee80211_queue_work(&sdata->local->hw,
+ &sdata->csa_finalize_work);
+}
+EXPORT_SYMBOL(ieee80211_csa_finish);
+
+static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
+ struct beacon_data *beacon)
+{
+ struct probe_resp *resp;
+ int counter_beacon = sdata->csa_counter_offset_beacon;
+ int counter_presp = sdata->csa_counter_offset_presp;
+
+ if (WARN_ON(counter_beacon > beacon->tail_len))
+ return;
+
+ if (WARN_ON(((u8 *)beacon->tail)[counter_beacon] == 0))
+ return;
+
+ ((u8 *)beacon->tail)[counter_beacon]--;
+
+ if (counter_presp && sdata->vif.type == NL80211_IFTYPE_AP) {
+ resp = rcu_dereference(sdata->u.ap.probe_resp);
+ if (WARN_ON(!resp))
+ return;
+ if (WARN_ON(counter_presp > resp->len))
+ return;
+
+ ((u8 *)resp->data)[counter_presp]--;
+ }
+}
+
+int ieee80211_csa_is_complete(struct ieee80211_vif *vif)
+{
+ struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+ struct beacon_data *beacon = NULL;
+ int counter_beacon = sdata->csa_counter_offset_beacon;
+
+ if (!sdata || !ieee80211_sdata_running(sdata))
+ return 0;
+
+ if (vif->type == NL80211_IFTYPE_AP) {
+ struct ieee80211_if_ap *ap = &sdata->u.ap;
+ beacon = rcu_dereference(ap->beacon);
+
+ } else {
+ WARN_ON(1);
+ return 0;
+ }
+
+ if (WARN_ON(counter_beacon > beacon->tail_len))
+ return 0;
+
+ if (((u8 *)beacon->tail)[counter_beacon] == 0)
+ return 1;
+
+ return 0;
+}
+EXPORT_SYMBOL(ieee80211_csa_is_complete);
+
struct sk_buff *ieee80211_beacon_get_tim(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
u16 *tim_offset, u16 *tim_length)
@@ -2350,6 +2415,9 @@ struct sk_buff *ieee80211_beacon_get_tim(struct ieee80211_hw *hw,
struct beacon_data *beacon = rcu_dereference(ap->beacon);
if (beacon) {
+ if (sdata->vif.csa_active)
+ ieee80211_update_csa(sdata, beacon);
+
/*
* headroom, head length,
* tail length and maximum TIM length
--
1.7.10.4
CSA is only enabled for one interface, but the same limitation applies
for mac80211 too. It checks whether the beacon has been sent (different
approaches for non-EDMA-enabled and EDMA-enabled devices), and completes
the channel switch after that.
Signed-off-by: Simon Wunderlich <[email protected]>
Signed-off-by: Mathias Kretschmer <[email protected]>
---
Changes to PATCHv1:
* complete channel switch after the last beacon has been sent
---
drivers/net/wireless/ath/ath9k/ath9k.h | 2 ++
drivers/net/wireless/ath/ath9k/beacon.c | 21 +++++++++++++++++++++
drivers/net/wireless/ath/ath9k/main.c | 17 +++++++++++++++++
drivers/net/wireless/ath/ath9k/xmit.c | 2 ++
4 files changed, 42 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 579ed9c..3745487 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -424,6 +424,7 @@ void ath9k_beacon_assign_slot(struct ath_softc *sc, struct ieee80211_vif *vif);
void ath9k_beacon_remove_slot(struct ath_softc *sc, struct ieee80211_vif *vif);
void ath9k_set_tsfadjust(struct ath_softc *sc, struct ieee80211_vif *vif);
void ath9k_set_beacon(struct ath_softc *sc);
+bool ath9k_csa_is_finished(struct ath_softc *sc);
/*******************/
/* Link Monitoring */
@@ -751,6 +752,7 @@ struct ath_softc {
#endif
struct ath_descdma txsdma;
+ struct ieee80211_vif *csa_vif;
struct ath_ant_comb ant_comb;
u8 ant_tx, ant_rx;
diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
index fd1eeba..e400151 100644
--- a/drivers/net/wireless/ath/ath9k/beacon.c
+++ b/drivers/net/wireless/ath/ath9k/beacon.c
@@ -310,6 +310,23 @@ void ath9k_set_tsfadjust(struct ath_softc *sc, struct ieee80211_vif *vif)
(unsigned long long)tsfadjust, avp->av_bslot);
}
+bool ath9k_csa_is_finished(struct ath_softc *sc)
+{
+ struct ieee80211_vif *vif;
+
+ vif = sc->csa_vif;
+ if (!vif || !vif->csa_active)
+ return false;
+
+ if (!ieee80211_csa_is_complete(vif))
+ return false;
+
+ ieee80211_csa_finish(vif);
+ vif->csa_active = 0;
+ sc->csa_vif = NULL;
+ return true;
+}
+
void ath9k_beacon_tasklet(unsigned long data)
{
struct ath_softc *sc = (struct ath_softc *)data;
@@ -355,6 +372,10 @@ void ath9k_beacon_tasklet(unsigned long data)
return;
}
+ /* EDMA devices check that in the tx completion function. */
+ if (!edma && ath9k_csa_is_finished(sc))
+ return;
+
slot = ath9k_beacon_choose_slot(sc);
vif = sc->beacon.bslot[slot];
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index cc5a98b..57d0642 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1026,6 +1026,9 @@ static void ath9k_remove_interface(struct ieee80211_hw *hw,
if (ath9k_uses_beacons(vif->type))
ath9k_beacon_remove_slot(sc, vif);
+ if (sc->csa_vif == vif)
+ sc->csa_vif = NULL;
+
ath9k_ps_wakeup(sc);
ath9k_calculate_summary_state(hw, NULL);
ath9k_ps_restore(sc);
@@ -2319,6 +2322,19 @@ static void ath9k_sw_scan_complete(struct ieee80211_hw *hw)
clear_bit(SC_OP_SCANNING, &sc->sc_flags);
}
+static void ath9k_channel_switch_beacon(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif)
+{
+ struct ath_softc *sc = hw->priv;
+
+ /* mac80211 does not support CSA in multi-if cases (yet) */
+ if (WARN_ON(sc->csa_vif))
+ return;
+
+ vif->csa_active = 1;
+ sc->csa_vif = vif;
+}
+
struct ieee80211_ops ath9k_ops = {
.tx = ath9k_tx,
.start = ath9k_start,
@@ -2366,4 +2382,5 @@ struct ieee80211_ops ath9k_ops = {
#endif
.sw_scan_start = ath9k_sw_scan_start,
.sw_scan_complete = ath9k_sw_scan_complete,
+ .channel_switch_beacon = ath9k_channel_switch_beacon,
};
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index eab0fcb..cec670d 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -2282,6 +2282,8 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
if (ts.qid == sc->beacon.beaconq) {
sc->beacon.tx_processed = true;
sc->beacon.tx_last = !(ts.ts_status & ATH9K_TXERR_MASK);
+
+ ath9k_csa_is_finished(sc);
continue;
}
--
1.7.10.4
> > If you're going to use nested IEs, shouldn't you define a separate
> > namespace to be used within the nesting?
>
> The idea was to recycle the nl80211_parse_beacon() function for the CSA IEs,
> but if you prefer I can define an own namespace and create a separate function
> to check.
Hmm, ok. Not sure. This just seems like an awful lot of stack space,
increasing with each new attribute ...
johannes
On Tue, Jun 18, 2013 at 04:49:32PM +0200, Johannes Berg wrote:
> On Fri, 2013-06-14 at 14:15 +0200, Simon Wunderlich wrote:
>
> > +static struct cfg80211_beacon_data *
> > +cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
> > +{
> > + struct cfg80211_beacon_data *new_beacon;
> > + new_beacon = kzalloc(sizeof(*new_beacon), GFP_KERNEL);
> > + if (!beacon)
> > + return NULL;
>
> Why not allocate one bigger block and use pointers into it? There's no
> (reasonable) way that it'd get really big ...
Yeah, can do that.
For the channel switch announcement, it is required to set a new beacon
after the driver arrived on the new channel. The new beacon will be set
by nl80211, but is required to duplicate the beacon as the original
memory within the netlink message will be gone. Add functions to
duplicate and free these cfg80211 beacons.
Signed-off-by: Simon Wunderlich <[email protected]>
Signed-off-by: Mathias Kretschmer <[email protected]>
---
net/mac80211/cfg.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 64cf294..5ffc7d9 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2778,6 +2778,82 @@ static int ieee80211_start_radar_detection(struct wiphy *wiphy,
return 0;
}
+static void cfg80211_beacon_free(struct cfg80211_beacon_data *beacon)
+{
+ kfree(beacon->head);
+ kfree(beacon->tail);
+ kfree(beacon->beacon_ies);
+ kfree(beacon->proberesp_ies);
+ kfree(beacon->assocresp_ies);
+ kfree(beacon->probe_resp);
+ kfree(beacon);
+}
+
+static struct cfg80211_beacon_data *
+cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
+{
+ struct cfg80211_beacon_data *new_beacon;
+ new_beacon = kzalloc(sizeof(*new_beacon), GFP_KERNEL);
+ if (!beacon)
+ return NULL;
+
+ if (beacon->head_len) {
+ new_beacon->head = kmalloc(beacon->head_len, GFP_KERNEL);
+ new_beacon->head_len = beacon->head_len;
+ if (!new_beacon->head)
+ goto error;
+ memcpy((u8 *)new_beacon->head, beacon->head, beacon->head_len);
+ }
+ if (beacon->tail_len) {
+ new_beacon->tail = kmalloc(beacon->tail_len, GFP_KERNEL);
+ new_beacon->tail_len = beacon->tail_len;
+ if (!new_beacon->tail)
+ goto error;
+ memcpy((u8 *)new_beacon->tail, beacon->tail, beacon->tail_len);
+ }
+ if (beacon->beacon_ies_len) {
+ new_beacon->beacon_ies = kmalloc(beacon->beacon_ies_len,
+ GFP_KERNEL);
+ new_beacon->beacon_ies_len = beacon->beacon_ies_len;
+ if (!new_beacon->beacon_ies)
+ goto error;
+ memcpy((u8 *)new_beacon->beacon_ies, beacon->beacon_ies,
+ beacon->beacon_ies_len);
+ }
+ if (beacon->proberesp_ies_len) {
+ new_beacon->proberesp_ies = kmalloc(beacon->proberesp_ies_len,
+ GFP_KERNEL);
+ new_beacon->proberesp_ies_len = beacon->proberesp_ies_len;
+ if (!new_beacon->proberesp_ies)
+ goto error;
+ memcpy((u8 *)new_beacon->proberesp_ies, beacon->proberesp_ies,
+ beacon->proberesp_ies_len);
+ }
+ if (beacon->assocresp_ies_len) {
+ new_beacon->assocresp_ies = kmalloc(beacon->assocresp_ies_len,
+ GFP_KERNEL);
+ new_beacon->assocresp_ies_len = beacon->assocresp_ies_len;
+ if (!new_beacon->assocresp_ies)
+ goto error;
+ memcpy((u8 *)new_beacon->assocresp_ies, beacon->assocresp_ies,
+ beacon->assocresp_ies_len);
+ }
+ if (beacon->probe_resp_len) {
+ new_beacon->probe_resp = kmalloc(beacon->probe_resp_len,
+ GFP_KERNEL);
+ new_beacon->probe_resp_len = beacon->probe_resp_len;
+ if (!new_beacon->probe_resp)
+ goto error;
+ memcpy((u8 *)new_beacon->probe_resp, beacon->probe_resp,
+ beacon->probe_resp_len);
+ }
+
+ return new_beacon;
+error:
+ cfg80211_beacon_free(new_beacon);
+ return NULL;
+}
+
static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
struct ieee80211_channel *chan, bool offchan,
unsigned int wait, const u8 *buf, size_t len,
--
1.7.10.4
only the attributes are required and not the whole netlink info, as the
function accesses the attributes only anyway. This makes it easier to
parse nested beacon IEs later.
Signed-off-by: Simon Wunderlich <[email protected]>
Signed-off-by: Mathias Kretschmer <[email protected]>
---
net/wireless/nl80211.c | 53 +++++++++++++++++++++++-------------------------
1 file changed, 25 insertions(+), 28 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index e402819..1c4f7da 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2882,61 +2882,58 @@ static int nl80211_set_mac_acl(struct sk_buff *skb, struct genl_info *info)
return err;
}
-static int nl80211_parse_beacon(struct genl_info *info,
+static int nl80211_parse_beacon(struct nlattr *attrs[],
struct cfg80211_beacon_data *bcn)
{
bool haveinfo = false;
- if (!is_valid_ie_attr(info->attrs[NL80211_ATTR_BEACON_TAIL]) ||
- !is_valid_ie_attr(info->attrs[NL80211_ATTR_IE]) ||
- !is_valid_ie_attr(info->attrs[NL80211_ATTR_IE_PROBE_RESP]) ||
- !is_valid_ie_attr(info->attrs[NL80211_ATTR_IE_ASSOC_RESP]))
+ if (!is_valid_ie_attr(attrs[NL80211_ATTR_BEACON_TAIL]) ||
+ !is_valid_ie_attr(attrs[NL80211_ATTR_IE]) ||
+ !is_valid_ie_attr(attrs[NL80211_ATTR_IE_PROBE_RESP]) ||
+ !is_valid_ie_attr(attrs[NL80211_ATTR_IE_ASSOC_RESP]))
return -EINVAL;
memset(bcn, 0, sizeof(*bcn));
- if (info->attrs[NL80211_ATTR_BEACON_HEAD]) {
- bcn->head = nla_data(info->attrs[NL80211_ATTR_BEACON_HEAD]);
- bcn->head_len = nla_len(info->attrs[NL80211_ATTR_BEACON_HEAD]);
+ if (attrs[NL80211_ATTR_BEACON_HEAD]) {
+ bcn->head = nla_data(attrs[NL80211_ATTR_BEACON_HEAD]);
+ bcn->head_len = nla_len(attrs[NL80211_ATTR_BEACON_HEAD]);
if (!bcn->head_len)
return -EINVAL;
haveinfo = true;
}
- if (info->attrs[NL80211_ATTR_BEACON_TAIL]) {
- bcn->tail = nla_data(info->attrs[NL80211_ATTR_BEACON_TAIL]);
- bcn->tail_len =
- nla_len(info->attrs[NL80211_ATTR_BEACON_TAIL]);
+ if (attrs[NL80211_ATTR_BEACON_TAIL]) {
+ bcn->tail = nla_data(attrs[NL80211_ATTR_BEACON_TAIL]);
+ bcn->tail_len = nla_len(attrs[NL80211_ATTR_BEACON_TAIL]);
haveinfo = true;
}
if (!haveinfo)
return -EINVAL;
- if (info->attrs[NL80211_ATTR_IE]) {
- bcn->beacon_ies = nla_data(info->attrs[NL80211_ATTR_IE]);
- bcn->beacon_ies_len = nla_len(info->attrs[NL80211_ATTR_IE]);
+ if (attrs[NL80211_ATTR_IE]) {
+ bcn->beacon_ies = nla_data(attrs[NL80211_ATTR_IE]);
+ bcn->beacon_ies_len = nla_len(attrs[NL80211_ATTR_IE]);
}
- if (info->attrs[NL80211_ATTR_IE_PROBE_RESP]) {
+ if (attrs[NL80211_ATTR_IE_PROBE_RESP]) {
bcn->proberesp_ies =
- nla_data(info->attrs[NL80211_ATTR_IE_PROBE_RESP]);
+ nla_data(attrs[NL80211_ATTR_IE_PROBE_RESP]);
bcn->proberesp_ies_len =
- nla_len(info->attrs[NL80211_ATTR_IE_PROBE_RESP]);
+ nla_len(attrs[NL80211_ATTR_IE_PROBE_RESP]);
}
- if (info->attrs[NL80211_ATTR_IE_ASSOC_RESP]) {
+ if (attrs[NL80211_ATTR_IE_ASSOC_RESP]) {
bcn->assocresp_ies =
- nla_data(info->attrs[NL80211_ATTR_IE_ASSOC_RESP]);
+ nla_data(attrs[NL80211_ATTR_IE_ASSOC_RESP]);
bcn->assocresp_ies_len =
- nla_len(info->attrs[NL80211_ATTR_IE_ASSOC_RESP]);
+ nla_len(attrs[NL80211_ATTR_IE_ASSOC_RESP]);
}
- if (info->attrs[NL80211_ATTR_PROBE_RESP]) {
- bcn->probe_resp =
- nla_data(info->attrs[NL80211_ATTR_PROBE_RESP]);
- bcn->probe_resp_len =
- nla_len(info->attrs[NL80211_ATTR_PROBE_RESP]);
+ if (attrs[NL80211_ATTR_PROBE_RESP]) {
+ bcn->probe_resp = nla_data(attrs[NL80211_ATTR_PROBE_RESP]);
+ bcn->probe_resp_len = nla_len(attrs[NL80211_ATTR_PROBE_RESP]);
}
return 0;
@@ -3015,7 +3012,7 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
!info->attrs[NL80211_ATTR_BEACON_HEAD])
return -EINVAL;
- err = nl80211_parse_beacon(info, ¶ms.beacon);
+ err = nl80211_parse_beacon(info->attrs, ¶ms.beacon);
if (err)
return err;
@@ -3167,7 +3164,7 @@ static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info)
if (!wdev->beacon_interval)
return -EINVAL;
- err = nl80211_parse_beacon(info, ¶ms);
+ err = nl80211_parse_beacon(info->attrs, ¶ms);
if (err)
return err;
--
1.7.10.4
Hey Johannes,
I'm skipping the "style" comments (you were right most of the time anyway) and will
only comment on the design stuff, see below:
On Tue, Jun 18, 2013 at 05:00:28PM +0200, Johannes Berg wrote:
> On Fri, 2013-06-14 at 14:15 +0200, Simon Wunderlich wrote:
> > @@ -2818,6 +2830,8 @@ struct ieee80211_ops {
> > struct ieee80211_vif *vif,
> > struct inet6_dev *idev);
> > #endif
> > + void (*channel_switch_beacon)(struct ieee80211_hw *hw,
> > + struct ieee80211_vif *vif);
>
> What about channel contexts? Actually I don't really understand this?
> Shouldn't it say which channel to switch to?
>
My first implementation (ath9k) does rely on mac80211 to complete the
channel switch, so it does not even need to know which channel is switched
to. I can add that as we can assume other drivers will behave differently ...
> > /**
> > + * ieee80211_csa_finish - notify mac80211 about channel switch
> > + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> > + *
> > + * After a channel switch announcement was scheduled and the counter in this
> > + * announcement hit zero, this function must be called by the driver to
> > + * notify mac80211 that the channel can be changed.
> > + */
> > +void ieee80211_csa_finish(struct ieee80211_vif *vif);
>
> If there are multiple interfaces, should it be called multiple times?
> etc. Maybe it should be on a channel context instead?
>
Multiple interfaces are not supported - and I don't know how this should be
handled anyway. CSAs are triggered on a per-interface base from userspace,
and multiple CSAs would clash with each other (could be different channels,
different counters, etc ...).
Or would you have a suggestion how to handle this differently?
> > + netif_carrier_off(sdata->dev);
> > + err = ieee80211_vif_use_channel(sdata, &local->csa_chandef,
> > + IEEE80211_CHANCTX_SHARED);
> > + netif_carrier_on(sdata->dev);
>
> That seems like a really bad idea, deleting a channel context might tear
> down all kinds of device state and might require deleting the interface
> first ... I think the chan context API needs to be extended to switch
> instead.
>
Hm, yeah I can do that.
> > + if (WARN_ON(err < 0))
> > + return;
>
> This can fail _easily_ too, e.g. if some other vif stays on the channel
> and you're now using too many channel contexts.
>
> > + /* don't handle if chanctx is used */
> > + if (local->use_chanctx)
> > + return -EBUSY;
>
> Still don't really like the way you've implemented it :-)
>
Why not? :)
> > + vif->csa_active = 0;
>
> is that a counter, or should it be a bool?
It should be bool.
> > +static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
> > + struct beacon_data *beacon)
> > +{
> > + struct probe_resp *resp;
> > + int counter_beacon = sdata->csa_counter_offset_beacon;
> > + int counter_presp = sdata->csa_counter_offset_presp;
> > +
> > + if (WARN_ON(counter_beacon > beacon->tail_len))
> > + return;
> > +
> > + if (WARN_ON(((u8 *)beacon->tail)[counter_beacon] == 0))
> > + return;
>
> How can these happen?
>
Maybe when the beacon is re-assigned - although add a check for that
and remove these warnings ...
> > + ((u8 *)beacon->tail)[counter_beacon]--;
> > +
> > + if (counter_presp && sdata->vif.type == NL80211_IFTYPE_AP) {
> > + resp = rcu_dereference(sdata->u.ap.probe_resp);
>
> Who guarantees RCU protection?
>
Hmm ... should add that.
> > + if (WARN_ON(!resp))
> > + return;
>
> That can legimitately happen, no? At least userspace is allowed to not
> set probe_resp now, if you want to change that ...
>
If there is no presp then also counter_presp should not be set.
Cheers,
Simon