2020-08-26 13:22:21

by Shay Bar

[permalink] [raw]
Subject: [PATCH] nl80211: Trigger channel switch from driver

This patch add channel switch event from driver to hostap.
same as can be triggered from US via hostapd_cli chan_switch.
using the already existing NL80211_CMD_CHANNEL_SWITCH.

Signed-off-by: Aviad Brikman <[email protected]>
Signed-off-by: Shay Bar <[email protected]>
---
include/net/cfg80211.h | 11 ++++++
net/wireless/nl80211.c | 83 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 94 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index d9e6b9fbd95b..ae02d96eb8ec 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -7416,6 +7416,17 @@ void cfg80211_ch_switch_started_notify(struct net_device *dev,
struct cfg80211_chan_def *chandef,
u8 count);

+/*
+ * cfg80211_ch_switch - trigger channel switch from driver
+ * same as is can be triggered from hostapd_cli chan_switch
+ * @dev: the device which switched channels
+ * @chandef: the new channel definition
+ * @csa_count: the number of TBTTs until the channel switch happens
+ */
+bool cfg80211_ch_switch(struct net_device *dev,
+ struct cfg80211_chan_def *chandef,
+ u8 csa_count);
+
/**
* ieee80211_operating_class_to_band - convert operating class to band
*
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 19dc0ee807f6..c9ed073e5ab3 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -16854,6 +16854,89 @@ void cfg80211_ch_switch_started_notify(struct net_device *dev,
}
EXPORT_SYMBOL(cfg80211_ch_switch_started_notify);

+static void nl80211_ch_switch(struct cfg80211_registered_device *rdev,
+ struct net_device *netdev,
+ struct cfg80211_chan_def *chandef,
+ u8 csa_count,
+ gfp_t gfp)
+{
+ struct sk_buff *msg;
+ void *hdr;
+
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
+ if (!msg)
+ return;
+
+ hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_CHANNEL_SWITCH);
+ if (!hdr) {
+ nlmsg_free(msg);
+ return;
+ }
+
+ if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx))
+ goto nla_put_failure;
+
+ if (nla_put_u32(msg, NL80211_ATTR_IFINDEX, netdev->ifindex))
+ goto nla_put_failure;
+
+ if (nla_put_u32(msg, NL80211_ATTR_WIPHY_FREQ,
+ chandef->chan->center_freq))
+ goto nla_put_failure;
+
+ if (nla_put_u32(msg, NL80211_ATTR_CENTER_FREQ1, chandef->center_freq1))
+ goto nla_put_failure;
+
+ if (nla_put_u32(msg, NL80211_ATTR_CHANNEL_WIDTH, chandef->width))
+ goto nla_put_failure;
+
+ if (chandef->width == NL80211_CHAN_WIDTH_40) {
+ enum nl80211_channel_type chan_type = NL80211_CHAN_HT40MINUS;
+
+ if (chandef->center_freq1 > chandef->chan->center_freq)
+ chan_type = NL80211_CHAN_HT40PLUS;
+
+ if (nla_put_u32(msg, NL80211_ATTR_WIPHY_CHANNEL_TYPE,
+ chan_type))
+ goto nla_put_failure;
+ }
+
+ if (nla_put_u32(msg, NL80211_ATTR_CH_SWITCH_COUNT, csa_count))
+ goto nla_put_failure;
+
+ genlmsg_end(msg, hdr);
+
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_MLME, gfp);
+ return;
+
+ nla_put_failure:
+ genlmsg_cancel(msg, hdr);
+ nlmsg_free(msg);
+}
+
+bool cfg80211_ch_switch(struct net_device *dev,
+ struct cfg80211_chan_def *chandef,
+ u8 csa_count)
+{
+ struct wireless_dev *wdev = dev->ieee80211_ptr;
+ struct wiphy *wiphy = wdev->wiphy;
+ struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
+
+ if (WARN_ON(wdev->iftype != NL80211_IFTYPE_AP &&
+ wdev->iftype != NL80211_IFTYPE_P2P_GO &&
+ wdev->iftype != NL80211_IFTYPE_ADHOC &&
+ wdev->iftype != NL80211_IFTYPE_MESH_POINT))
+ return false;
+
+ if (WARN_ON(!chandef || !chandef->chan))
+ return false;
+
+ nl80211_ch_switch(rdev, dev, chandef, csa_count, GFP_ATOMIC);
+
+ return true;
+}
+EXPORT_SYMBOL(cfg80211_ch_switch);
+
void
nl80211_radar_notify(struct cfg80211_registered_device *rdev,
const struct cfg80211_chan_def *chandef,
--
2.17.1

________________________________
The information transmitted is intended only for the person or entity to which it is addressed and may contain confidential and/or privileged material. Any retransmission, dissemination, copying or other use of, or taking of any action in reliance upon this information is prohibited. If you received this in error, please contact the sender and delete the material from any computer. Nothing contained herein shall be deemed as a representation, warranty or a commitment by Celeno. No warranties are expressed or implied, including, but not limited to, any implied warranties of non-infringement, merchantability and fitness for a particular purpose.
________________________________


2020-09-28 11:33:13

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] nl80211: Trigger channel switch from driver

Hi Aviad, Shay,

> This patch add channel switch event from driver to hostap.
> same as can be triggered from US via hostapd_cli chan_switch.

(had to think about "US" here for a bit, "channel switch" totally sent
my brain on the regulatory road ... heh)

> using the already existing NL80211_CMD_CHANNEL_SWITCH.

I'm a little confused as to how this would work - did the driver just
*do* a channel switch? Or does it want to do a channel switch, and then
hostapd has to deal with it? Is it able to? What happens if it doesn't
do anything when this happens, e.g. an older version?

> Signed-off-by: Aviad Brikman <[email protected]>
> Signed-off-by: Shay Bar <[email protected]>
> ---
> include/net/cfg80211.h | 11 ++++++
> net/wireless/nl80211.c | 83 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 94 insertions(+)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index d9e6b9fbd95b..ae02d96eb8ec 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -7416,6 +7416,17 @@ void cfg80211_ch_switch_started_notify(struct net_device *dev,
> struct cfg80211_chan_def *chandef,
> u8 count);
>
> +/*
> + * cfg80211_ch_switch - trigger channel switch from driver
> + * same as is can be triggered from hostapd_cli chan_switch

Not sure "same as ... hostapd_cli chan_switch" makes a lot of sense here
in the kernel-doc, tbh.

Also, you say "trigger" here, but ...

> + * @dev: the device which switched channels

"switched" here, which didn't help my above question at all.

> + * @chandef: the new channel definition
> + * @csa_count: the number of TBTTs until the channel switch happens

though I guess this makes it a bit clearer, unless it can be zero? :)

> + */
> +bool cfg80211_ch_switch(struct net_device *dev,
> + struct cfg80211_chan_def *chandef,
> + u8 csa_count);

nit: indentation

> + if (chandef->width == NL80211_CHAN_WIDTH_40) {
> + enum nl80211_channel_type chan_type = NL80211_CHAN_HT40MINUS;
> +
> + if (chandef->center_freq1 > chandef->chan->center_freq)
> + chan_type = NL80211_CHAN_HT40PLUS;
> +
> + if (nla_put_u32(msg, NL80211_ATTR_WIPHY_CHANNEL_TYPE,
> + chan_type))
> + goto nla_put_failure;
> + }

This a bit ties in with the "compatibility" question above - does older
hostapd even understand this? I'd suspect not, and then I'm not sure why
you'd include these attributes?

> The information transmitted is intended only for the person or entity
> to which it is addressed and may contain confidential and/or
> privileged material. Any retransmission, dissemination, copying or
> other use of, or taking of any action in reliance upon this
> information is prohibited. If you received this in error, please
> contact the sender and delete the material from any computer. Nothing
> contained herein shall be deemed as a representation, warranty or a
> commitment by Celeno. No warranties are expressed or implied,
> including, but not limited to, any implied warranties of non-
> infringement, merchantability and fitness for a particular purpose.

Hm. I guess I'm the "intended [...] person or entity" but I'm still a
bit wary about applying patches with that, I guess.

Thanks,
johannes

2020-10-12 13:34:26

by Shay Bar

[permalink] [raw]
Subject: Re: [PATCH] nl80211: Trigger channel switch from driver


On 28/09/2020 14:30, Johannes Berg wrote
>> using the already existing NL80211_CMD_CHANNEL_SWITCH.
> I'm a little confused as to how this would work - did the driver just
> *do* a channel switch? Or does it want to do a channel switch, and then
> hostapd has to deal with it? Is it able to? What happens if it doesn't
> do anything when this happens, e.g. an older version?

The driver "wants" to do channel switch and hostapd, once receiving the
above command, will take the same action as if it received a
"hostapd_cli chan_switch.

The driver will not assume channel was switched upon sending this
command (it will have to wait until hostapd will handle the entire
channel switch "loop").

There are many reasons why kernel driver will want to trigger a channel
switch (e.g. dynamic channel selection).

With older version of hostapd, it will not be able to handle this
command and nothing will happen.

>> +/*
>> + * cfg80211_ch_switch - trigger channel switch from driver
>> + * same as is can be triggered from hostapd_cli chan_switch
> Not sure "same as ... hostapd_cli chan_switch" makes a lot of sense here
> in the kernel-doc, tbh.
can remove the "hostapd_cli chan_switch" reference.
> Also, you say "trigger" here, but ...
>
>> + * @dev: the device which switched channels
> "switched" here, which didn't help my above question at all.
Agree "wants to switch" is clearer
>> + * @chandef: the new channel definition
>> + * @csa_count: the number of TBTTs until the channel switch happens
> though I guess this makes it a bit clearer, unless it can be zero? :)
csa_count of 0/1 is valid, but it means the channel switch will happen
at the current TBTT (see ieee80211_set_csa_beacon())
>> + */
>> +bool cfg80211_ch_switch(struct net_device *dev,
>> + struct cfg80211_chan_def *chandef,
>> + u8 csa_count);
> nit: indentation
will fix
>> + if (chandef->width == NL80211_CHAN_WIDTH_40) {
>> + enum nl80211_channel_type chan_type = NL80211_CHAN_HT40MINUS;
>> +
>> + if (chandef->center_freq1 > chandef->chan->center_freq)
>> + chan_type = NL80211_CHAN_HT40PLUS;
>> +
>> + if (nla_put_u32(msg, NL80211_ATTR_WIPHY_CHANNEL_TYPE,
>> + chan_type))
>> + goto nla_put_failure;
>> + }
> This a bit ties in with the "compatibility" question above - does older
> hostapd even understand this? I'd suspect not, and then I'm not sure why
> you'd include these attributes?
older hostapd will not handle NL80211_CMD_CHANNEL_SWITCH anyway.
>> The information transmitted is intended only for the person or entity
>> to which it is addressed and may contain confidential and/or
>> privileged material. Any retransmission, dissemination, copying or
>> other use of, or taking of any action in reliance upon this
>> information is prohibited. If you received this in error, please
>> contact the sender and delete the material from any computer. Nothing
>> contained herein shall be deemed as a representation, warranty or a
>> commitment by Celeno. No warranties are expressed or implied,
>> including, but not limited to, any implied warranties of non-
>> infringement, merchantability and fitness for a particular purpose.
> Hm. I guess I'm the "intended [...] person or entity" but I'm still a
> bit wary about applying patches with that, I guess.
:) the disclaimer will be removed from now on.