[PATCH V6 1/3] dt-bindings: document common IEEE 802.11 frequency limit property

[PATCH V6 1/3] dt-bindings: document common IEEE 802.11 frequency limit property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This new file should be used for properties that apply to all wireless
devices.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Switch to a single ieee80211-freq-limit property that allows specifying
    *multiple* ranges. This resolves problem with more complex rules as pointed
    by Felx.
    Make description implementation agnostic as pointed by Arend.
    Rename node to wifi as suggested by Martin.
V3: Use more real-life frequencies in the example.
V5: Describe hardware design as possible use for this property
V6: Even better property description, thanks Rob for your help!
---
 .../devicetree/bindings/net/wireless/ieee80211.txt | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/wireless/ieee80211.txt

diff --git a/Documentation/devicetree/bindings/net/wireless/ieee80211.txt b/Documentation/devicetree/bindings/net/wireless/ieee80211.txt
new file mode 100644
index 0000000..f6442b1
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/wireless/ieee80211.txt
@@ -0,0 +1,24 @@
+Common IEEE 802.11 properties
+
+This provides documentation of common properties that are valid for all wireless
+devices.
+
+Optional properties:
+ - ieee80211-freq-limit : list of supported frequency ranges in KHz. This can be
+	used for devices that in a given config support less channels than
+	normally. It may happen chipset supports a wide wireless band but it is
+	limited to some part of it due to used antennas or power amplifier.
+	An example case for this can be tri-band wireless router with two
+	identical chipsets used for two different 5 GHz subbands. Using them
+	incorrectly could not work or decrease performance noticeably.
+
+Example:
+
+pcie@0,0 {
+	reg = <0x0000 0 0 0 0>;
+	wifi@0,0 {
+		reg = <0x0000 0 0 0 0>;
+		ieee80211-freq-limit = <2402000 2482000>,
+				       <5170000 5250000>;
+	};
+};
-- 
2.10.1

[PATCH V6 2/3] cfg80211: move function checking range fit to util.c

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

It is needed for another cfg80211 helper that will be out of reg.c so
move it to common util.c file and make it non-static.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V5: Add this patch as suggested by Arend
---
 net/wireless/core.h |  3 +++
 net/wireless/reg.c  | 27 +++++++--------------------
 net/wireless/util.c | 15 +++++++++++++++
 3 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/net/wireless/core.h b/net/wireless/core.h
index af6e023..bc8ba6e 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -430,6 +430,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);
 
+bool cfg80211_does_bw_fit_range(const struct ieee80211_freq_range *freq_range,
+				u32 center_freq_khz, u32 bw_khz);
+
 /**
  * cfg80211_chandef_dfs_usable - checks if chandef is DFS usable
  * @wiphy: the wiphy to validate against
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 5dbac37..753efcd 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -748,21 +748,6 @@ static bool is_valid_rd(const struct ieee80211_regdomain *rd)
 	return true;
 }
 
-static bool reg_does_bw_fit(const struct ieee80211_freq_range *freq_range,
-			    u32 center_freq_khz, u32 bw_khz)
-{
-	u32 start_freq_khz, end_freq_khz;
-
-	start_freq_khz = center_freq_khz - (bw_khz/2);
-	end_freq_khz = center_freq_khz + (bw_khz/2);
-
-	if (start_freq_khz >= freq_range->start_freq_khz &&
-	    end_freq_khz <= freq_range->end_freq_khz)
-		return true;
-
-	return false;
-}
-
 /**
  * freq_in_rule_band - tells us if a frequency is in a frequency band
  * @freq_range: frequency rule we want to query
@@ -1070,7 +1055,7 @@ freq_reg_info_regd(u32 center_freq,
 		if (!band_rule_found)
 			band_rule_found = freq_in_rule_band(fr, center_freq);
 
-		bw_fits = reg_does_bw_fit(fr, center_freq, bw);
+		bw_fits = cfg80211_does_bw_fit_range(fr, center_freq, bw);
 
 		if (band_rule_found && bw_fits)
 			return rr;
@@ -1138,11 +1123,13 @@ static uint32_t reg_rule_to_chan_bw_flags(const struct ieee80211_regdomain *regd
 		max_bandwidth_khz = reg_get_max_bandwidth(regd, reg_rule);
 
 	/* If we get a reg_rule we can assume that at least 5Mhz fit */
-	if (!reg_does_bw_fit(freq_range, MHZ_TO_KHZ(chan->center_freq),
-			     MHZ_TO_KHZ(10)))
+	if (!cfg80211_does_bw_fit_range(freq_range,
+					MHZ_TO_KHZ(chan->center_freq),
+					MHZ_TO_KHZ(10)))
 		bw_flags |= IEEE80211_CHAN_NO_10MHZ;
-	if (!reg_does_bw_fit(freq_range, MHZ_TO_KHZ(chan->center_freq),
-			     MHZ_TO_KHZ(20)))
+	if (!cfg80211_does_bw_fit_range(freq_range,
+					MHZ_TO_KHZ(chan->center_freq),
+					MHZ_TO_KHZ(20)))
 		bw_flags |= IEEE80211_CHAN_NO_20MHZ;
 
 	if (max_bandwidth_khz < MHZ_TO_KHZ(10))
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 2cf7df8..62dc214 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1847,6 +1847,21 @@ void cfg80211_free_nan_func(struct cfg80211_nan_func *f)
 }
 EXPORT_SYMBOL(cfg80211_free_nan_func);
 
+bool cfg80211_does_bw_fit_range(const struct ieee80211_freq_range *freq_range,
+				u32 center_freq_khz, u32 bw_khz)
+{
+	u32 start_freq_khz, end_freq_khz;
+
+	start_freq_khz = center_freq_khz - (bw_khz / 2);
+	end_freq_khz = center_freq_khz + (bw_khz / 2);
+
+	if (start_freq_khz >= freq_range->start_freq_khz &&
+	    end_freq_khz <= freq_range->end_freq_khz)
+		return true;
+
+	return false;
+}
+
 /* See IEEE 802.1H for LLC/SNAP encapsulation/decapsulation */
 /* Ethernet-II snap header (RFC1042 for most EtherTypes) */
 const unsigned char rfc1042_header[] __aligned(2) =
-- 
2.10.1

[PATCH V6 3/3] cfg80211: support ieee80211-freq-limit DT property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This patch adds a helper for reading that new property and applying
limitations of supported channels specified this way.
It is used with devices that normally support a wide wireless band but
in a given config are limited to some part of it (usually due to board
design). For example a dual-band chipset may be able to support one band
only because of used antennas.
It's also common that tri-band routers have separated radios for lower
and higher part of 5 GHz band and it may be impossible to say which is
which without a DT info.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Put main code in core.c as it isn't strictly part of regulatory - pointed
    by Arend.
    Update to support ieee80211-freq-limit (new property).
V3: Introduce separated wiphy_read_of_freq_limits function.
    Add extra sanity checks for DT data.
    Move code back to reg.c as suggested by Johannes.
V4: Move code to of.c
    Use one helper called at init time (no runtime hooks)
    Modify orig_flags
V5: Make wiphy_read_of_freq_limits return void as there isn't much point of
    handling errors.
V6: Set IEEE80211_CHAN_DISABLED in "flags" instead of "orig_flags" as they will
    be copied during wiphy_register. Also improve description & note that helper
    has to be called before wiphy_register.
---
 include/net/cfg80211.h |  28 ++++++++++
 net/wireless/Makefile  |   1 +
 net/wireless/of.c      | 138 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 net/wireless/of.c

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ca2ac1c..30841a9 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -311,6 +311,34 @@ struct ieee80211_supported_band {
 	struct ieee80211_sta_vht_cap vht_cap;
 };
 
+/**
+ * wiphy_read_of_freq_limits - read frequency limits from device tree
+ *
+ * @wiphy: the wireless device to get extra limits for
+ *
+ * Some devices may have extra limitations specified in DT. This may be useful
+ * for chipsets that normally support more bands but are limited due to board
+ * design (e.g. by antennas or external power amplifier).
+ *
+ * This function reads info from DT and uses it to *modify* channels (disable
+ * unavailable ones). It's usually a *bad* idea to use it in drivers with
+ * shared channel data as DT limitations are device specific. You should make
+ * sure to call it only if channels in wiphy are copied and can be modified
+ * without affecting other devices.
+ *
+ * As this function access device node it has to be called after set_wiphy_dev.
+ * It also modifies channels so they have to be set first.
+ * If using this helper, call it before wiphy_register.
+ */
+#ifdef CONFIG_OF
+void wiphy_read_of_freq_limits(struct wiphy *wiphy);
+#else /* CONFIG_OF */
+static inline void wiphy_read_of_freq_limits(struct wiphy *wiphy)
+{
+}
+#endif /* !CONFIG_OF */
+
+
 /*
  * Wireless hardware/device configuration structures and methods
  */
diff --git a/net/wireless/Makefile b/net/wireless/Makefile
index 4c9e39f..95b4c09 100644
--- a/net/wireless/Makefile
+++ b/net/wireless/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_WEXT_PRIV) += wext-priv.o
 
 cfg80211-y += core.o sysfs.o radiotap.o util.o reg.o scan.o nl80211.o
 cfg80211-y += mlme.o ibss.o sme.o chan.o ethtool.o mesh.o ap.o trace.o ocb.o
+cfg80211-$(CONFIG_OF) += of.o
 cfg80211-$(CONFIG_CFG80211_DEBUGFS) += debugfs.o
 cfg80211-$(CONFIG_CFG80211_WEXT) += wext-compat.o wext-sme.o
 cfg80211-$(CONFIG_CFG80211_INTERNAL_REGDB) += regdb.o
diff --git a/net/wireless/of.c b/net/wireless/of.c
new file mode 100644
index 0000000..de221f0
--- /dev/null
+++ b/net/wireless/of.c
@@ -0,0 +1,138 @@
+/*
+ * Copyright (C) 2017 Rafał Miłecki <rafal@milecki.pl>
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <linux/of.h>
+#include <net/cfg80211.h>
+#include "core.h"
+
+static bool wiphy_freq_limits_valid_chan(struct wiphy *wiphy,
+					 struct ieee80211_freq_range *freq_limits,
+					 unsigned int n_freq_limits,
+					 struct ieee80211_channel *chan)
+{
+	u32 bw = MHZ_TO_KHZ(20);
+	int i;
+
+	for (i = 0; i < n_freq_limits; i++) {
+		struct ieee80211_freq_range *limit = &freq_limits[i];
+
+		if (cfg80211_does_bw_fit_range(limit,
+					       MHZ_TO_KHZ(chan->center_freq),
+					       bw))
+			return true;
+	}
+
+	return false;
+}
+
+static void wiphy_freq_limits_apply(struct wiphy *wiphy,
+				    struct ieee80211_freq_range *freq_limits,
+				    unsigned int n_freq_limits)
+{
+	enum nl80211_band band;
+	int i;
+
+	if (WARN_ON(!n_freq_limits))
+		return;
+
+	for (band = 0; band < NUM_NL80211_BANDS; band++) {
+		struct ieee80211_supported_band *sband = wiphy->bands[band];
+
+		if (!sband)
+			continue;
+
+		for (i = 0; i < sband->n_channels; i++) {
+			struct ieee80211_channel *chan = &sband->channels[i];
+
+			if (chan->flags & IEEE80211_CHAN_DISABLED)
+				continue;
+
+			if (!wiphy_freq_limits_valid_chan(wiphy, freq_limits,
+							  n_freq_limits,
+							  chan)) {
+				pr_debug("Disabling freq %d MHz as it's out of OF limits\n",
+					 chan->center_freq);
+				chan->flags |= IEEE80211_CHAN_DISABLED;
+			}
+		}
+	}
+}
+
+void wiphy_read_of_freq_limits(struct wiphy *wiphy)
+{
+	struct device *dev = wiphy_dev(wiphy);
+	struct device_node *np;
+	struct property *prop;
+	struct ieee80211_freq_range *freq_limits;
+	unsigned int n_freq_limits;
+	const __be32 *p;
+	int len, i;
+	int err = 0;
+
+	if (!dev)
+		return;
+	np = dev_of_node(dev);
+	if (!np)
+		return;
+
+	prop = of_find_property(np, "ieee80211-freq-limit", &len);
+	if (!prop)
+		return;
+
+	if (!len || len % sizeof(u32) || len / sizeof(u32) % 2) {
+		dev_err(dev, "ieee80211-freq-limit wrong format");
+		return;
+	}
+	n_freq_limits = len / sizeof(u32) / 2;
+
+	freq_limits = kcalloc(n_freq_limits, sizeof(*freq_limits), GFP_KERNEL);
+	if (!freq_limits) {
+		err = -ENOMEM;
+		goto out_kfree;
+	}
+
+	p = NULL;
+	for (i = 0; i < n_freq_limits; i++) {
+		struct ieee80211_freq_range *limit = &freq_limits[i];
+
+		p = of_prop_next_u32(prop, p, &limit->start_freq_khz);
+		if (!p) {
+			err = -EINVAL;
+			goto out_kfree;
+		}
+
+		p = of_prop_next_u32(prop, p, &limit->end_freq_khz);
+		if (!p) {
+			err = -EINVAL;
+			goto out_kfree;
+		}
+
+		if (!limit->start_freq_khz ||
+		    !limit->end_freq_khz ||
+		    limit->start_freq_khz >= limit->end_freq_khz) {
+			err = -EINVAL;
+			goto out_kfree;
+		}
+	}
+
+	wiphy_freq_limits_apply(wiphy, freq_limits, n_freq_limits);
+
+out_kfree:
+	kfree(freq_limits);
+	if (err)
+		dev_err(dev, "Failed to get limits: %d\n", err);
+}
+EXPORT_SYMBOL(wiphy_read_of_freq_limits);
-- 
2.10.1

[PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

There are some devices (e.g. Netgear R8000 home router) with one chipset
model used for different radios, some of them limited to subbands. NVRAM
entries don't contain any extra info on such limitations and firmware
reports full list of channels to us. We need to store extra limitation
info in DT to support such devices properly.

Now there is a cfg80211 helper for reading such info use it in brcmfmac.
This patch adds check for channel being disabled with orig_flags which
is how this wiphy helper and wiphy_register work.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
This patch should probably go through wireless-driver-next which is why
it got weird number 4/3. I'm sending it just as a proof of concept.
It was succesfully tested on SmartRG SR400ac with BCM43602.

V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
V5: Update commit message
V6: Call wiphy_read_of_freq_limits after brcmf_setup_wiphybands to make it work
    with helper setting "flags" instead of "orig_flags".
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index ccae3bb..a008ba5 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5886,6 +5886,9 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
 						       band->band);
 		channel[index].hw_value = ch.control_ch_num;
 
+		if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
+			continue;
+
 		/* assuming the chanspecs order is HT20,
 		 * HT40 upper, HT40 lower, and VHT80.
 		 */
@@ -6478,7 +6481,11 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp)
 		}
 	}
 	err = brcmf_setup_wiphybands(wiphy);
-	return err;
+	if (err)
+		return err;
+	wiphy_read_of_freq_limits(wiphy);
+
+	return 0;
 }
 
 static s32 brcmf_config_dongle(struct brcmf_cfg80211_info *cfg)
-- 
2.10.1

Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits

[Arend Van Spriel]

On 4-1-2017 18:58, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> There are some devices (e.g. Netgear R8000 home router) with one chipset
> model used for different radios, some of them limited to subbands. NVRAM
> entries don't contain any extra info on such limitations and firmware
> reports full list of channels to us. We need to store extra limitation
> info in DT to support such devices properly.
> 
> Now there is a cfg80211 helper for reading such info use it in brcmfmac.
> This patch adds check for channel being disabled with orig_flags which
> is how this wiphy helper and wiphy_register work.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> This patch should probably go through wireless-driver-next which is why
> it got weird number 4/3. I'm sending it just as a proof of concept.
> It was succesfully tested on SmartRG SR400ac with BCM43602.
> 
> V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
> V5: Update commit message
> V6: Call wiphy_read_of_freq_limits after brcmf_setup_wiphybands to make it work
>     with helper setting "flags" instead of "orig_flags".
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index ccae3bb..a008ba5 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -5886,6 +5886,9 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
>  						       band->band);
>  		channel[index].hw_value = ch.control_ch_num;
>  
> +		if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
> +			continue;
> +

So to be clear this is still needed for subsequent calls to
brcmf_setup_wiphybands(). The subsequent calls are done from the
regulatory notifier. So I think we have an issue with this approach. Say
the device comes up with US. That would set DISABLED flags for channels
12 to 14. With a country update to PL we would want to enable channels
12 and 13, right? The orig_flags are copied from the initial flags
during wiphy_register() so it seems we will skip enabling 12 and 13. I
think we should remove the check above and call
wiphy_read_of_freq_limits() as a last step within
brcmf_setup_wiphybands(). It means it is called every time, but it
safeguards that the limits in DT are always applied.

Regards,
Arend

>  		/* assuming the chanspecs order is HT20,
>  		 * HT40 upper, HT40 lower, and VHT80.
>  		 */
> @@ -6478,7 +6481,11 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp)
>  		}
>  	}
>  	err = brcmf_setup_wiphybands(wiphy);
> -	return err;
> +	if (err)
> +		return err;
> +	wiphy_read_of_freq_limits(wiphy);
> +
> +	return 0;
>  }
>  
>  static s32 brcmf_config_dongle(struct brcmf_cfg80211_info *cfg)
> 

Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits

[Rafał Miłecki]

On 4 January 2017 at 21:07, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 4-1-2017 18:58, Rafa=C5=82 Mi=C5=82ecki wrote:
>> From: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>>
>> There are some devices (e.g. Netgear R8000 home router) with one chipset
>> model used for different radios, some of them limited to subbands. NVRAM
>> entries don't contain any extra info on such limitations and firmware
>> reports full list of channels to us. We need to store extra limitation
>> info in DT to support such devices properly.
>>
>> Now there is a cfg80211 helper for reading such info use it in brcmfmac.
>> This patch adds check for channel being disabled with orig_flags which
>> is how this wiphy helper and wiphy_register work.
>>
>> Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>> ---
>> This patch should probably go through wireless-driver-next which is why
>> it got weird number 4/3. I'm sending it just as a proof of concept.
>> It was succesfully tested on SmartRG SR400ac with BCM43602.
>>
>> V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
>> V5: Update commit message
>> V6: Call wiphy_read_of_freq_limits after brcmf_setup_wiphybands to make =
it work
>>     with helper setting "flags" instead of "orig_flags".
>> ---
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 +++++++=
+-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c=
 b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> index ccae3bb..a008ba5 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> @@ -5886,6 +5886,9 @@ static int brcmf_construct_chaninfo(struct brcmf_c=
fg80211_info *cfg,
>>                                                      band->band);
>>               channel[index].hw_value =3D ch.control_ch_num;
>>
>> +             if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
>> +                     continue;
>> +
>
> So to be clear this is still needed for subsequent calls to
> brcmf_setup_wiphybands(). The subsequent calls are done from the
> regulatory notifier. So I think we have an issue with this approach. Say
> the device comes up with US. That would set DISABLED flags for channels
> 12 to 14. With a country update to PL we would want to enable channels
> 12 and 13, right? The orig_flags are copied from the initial flags
> during wiphy_register() so it seems we will skip enabling 12 and 13. I
> think we should remove the check above and call
> wiphy_read_of_freq_limits() as a last step within
> brcmf_setup_wiphybands(). It means it is called every time, but it
> safeguards that the limits in DT are always applied.

I'm not exactly happy with channels management in brcmfmac. Before
calling wiphy_register it already disables channels unavailable for
current country. This results in setting IEEE80211_CHAN_DISABLED in
orig_flags of channels that may become available later, after country
change. Please note it happens even right now, without this patch.
Maybe you can workaround this by ignoring orig_flags in custom
regulatory code, but I'd just prefer to have it nicely handled in the
first place.

This is the next feature I'm going to work on. AFAIU this patch won't
be applied for now (it's for wireless-drivers-next and we first need
to get wiphy_read_of_freq_limits in that tree). By the time that
happens I may have another patchset cleaning brcmfmac ready. And FWIW
this patch wouldn't make things worse *at this point* as we don't
really support country switching for any device yet.

So I hope problem with channels in brcmfmac doesn't mean we need to
postpone patches 1-3.

--=20
Rafa=C5=82

Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits

[Arend Van Spriel]

On 4-1-2017 22:19, Rafał Miłecki wrote:
> On 4 January 2017 at 21:07, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 4-1-2017 18:58, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> There are some devices (e.g. Netgear R8000 home router) with one chipset
>>> model used for different radios, some of them limited to subbands. NVRAM
>>> entries don't contain any extra info on such limitations and firmware
>>> reports full list of channels to us. We need to store extra limitation
>>> info in DT to support such devices properly.
>>>
>>> Now there is a cfg80211 helper for reading such info use it in brcmfmac.
>>> This patch adds check for channel being disabled with orig_flags which
>>> is how this wiphy helper and wiphy_register work.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>> This patch should probably go through wireless-driver-next which is why
>>> it got weird number 4/3. I'm sending it just as a proof of concept.
>>> It was succesfully tested on SmartRG SR400ac with BCM43602.
>>>
>>> V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
>>> V5: Update commit message
>>> V6: Call wiphy_read_of_freq_limits after brcmf_setup_wiphybands to make it work
>>>     with helper setting "flags" instead of "orig_flags".
>>> ---
>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> index ccae3bb..a008ba5 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> @@ -5886,6 +5886,9 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
>>>                                                      band->band);
>>>               channel[index].hw_value = ch.control_ch_num;
>>>
>>> +             if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
>>> +                     continue;
>>> +
>>
>> So to be clear this is still needed for subsequent calls to
>> brcmf_setup_wiphybands(). The subsequent calls are done from the
>> regulatory notifier. So I think we have an issue with this approach. Say
>> the device comes up with US. That would set DISABLED flags for channels
>> 12 to 14. With a country update to PL we would want to enable channels
>> 12 and 13, right? The orig_flags are copied from the initial flags
>> during wiphy_register() so it seems we will skip enabling 12 and 13. I
>> think we should remove the check above and call
>> wiphy_read_of_freq_limits() as a last step within
>> brcmf_setup_wiphybands(). It means it is called every time, but it
>> safeguards that the limits in DT are always applied.
> 
> I'm not exactly happy with channels management in brcmfmac. Before
> calling wiphy_register it already disables channels unavailable for
> current country. This results in setting IEEE80211_CHAN_DISABLED in

What do you mean by current country. There is none that we are aware off
in the driver. So we obtain the channels for the current
country/revision in the firmware and enable those before
wiphy_register(). This all is within the probe/init sequence so I do not
really see an issue. As the wiphy object is not yet registered there is
no user-space awareness

> orig_flags of channels that may become available later, after country
> change. Please note it happens even right now, without this patch.

Nope. As stated earlier the country setting in firmware is not updated
unless you provide a *proper* mapping of user-space country code to
firmware country code/revision. That is the reason, ie. firmware simply
returns the same list of channels as nothing changed from its
perspective. We may actually drop 11d support.

> Maybe you can workaround this by ignoring orig_flags in custom
> regulatory code, but I'd just prefer to have it nicely handled in the
> first place.

Please care to explain your ideas before putting any effort in this
"feature". As the author of the code that makes you unhappy and as
driver maintainer I would like to get a clearer picture of your point of
view. What exactly is the issue that makes you unhappy.

> This is the next feature I'm going to work on. AFAIU this patch won't
> be applied for now (it's for wireless-drivers-next and we first need
> to get wiphy_read_of_freq_limits in that tree). By the time that
> happens I may have another patchset cleaning brcmfmac ready. And FWIW
> this patch wouldn't make things worse *at this point* as we don't
> really support country switching for any device yet.

Now who is *we*? We as Broadcom can, because we know how to map the ISO
3166-1 country code to firmware country code/revision for a specific
firmware release. Firmware uses its own regulatory rules which may
differ from what regdb has. Now I know Intel submitted a mechanism to
export firmware rules to regdb so maybe we should consider switching to
that api if that has been upstreamed. Need to check.

> So I hope problem with channels in brcmfmac doesn't mean we need to
> postpone patches 1-3.

I do not see any reason to postpone.

Regards,
Arend

Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits

[Rafał Miłecki]

On 5 January 2017 at 10:31, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 4-1-2017 22:19, Rafa=C5=82 Mi=C5=82ecki wrote:
>> On 4 January 2017 at 21:07, Arend Van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>> On 4-1-2017 18:58, Rafa=C5=82 Mi=C5=82ecki wrote:
>>>> From: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>>>>
>>>> There are some devices (e.g. Netgear R8000 home router) with one chips=
et
>>>> model used for different radios, some of them limited to subbands. NVR=
AM
>>>> entries don't contain any extra info on such limitations and firmware
>>>> reports full list of channels to us. We need to store extra limitation
>>>> info in DT to support such devices properly.
>>>>
>>>> Now there is a cfg80211 helper for reading such info use it in brcmfma=
c.
>>>> This patch adds check for channel being disabled with orig_flags which
>>>> is how this wiphy helper and wiphy_register work.
>>>>
>>>> Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>>>> ---
>>>> This patch should probably go through wireless-driver-next which is wh=
y
>>>> it got weird number 4/3. I'm sending it just as a proof of concept.
>>>> It was succesfully tested on SmartRG SR400ac with BCM43602.
>>>>
>>>> V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
>>>> V5: Update commit message
>>>> V6: Call wiphy_read_of_freq_limits after brcmf_setup_wiphybands to mak=
e it work
>>>>     with helper setting "flags" instead of "orig_flags".
>>>> ---
>>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 +++++=
+++-
>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211=
.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>> index ccae3bb..a008ba5 100644
>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>> @@ -5886,6 +5886,9 @@ static int brcmf_construct_chaninfo(struct brcmf=
_cfg80211_info *cfg,
>>>>                                                      band->band);
>>>>               channel[index].hw_value =3D ch.control_ch_num;
>>>>
>>>> +             if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
>>>> +                     continue;
>>>> +
>>>
>>> So to be clear this is still needed for subsequent calls to
>>> brcmf_setup_wiphybands(). The subsequent calls are done from the
>>> regulatory notifier. So I think we have an issue with this approach. Sa=
y
>>> the device comes up with US. That would set DISABLED flags for channels
>>> 12 to 14. With a country update to PL we would want to enable channels
>>> 12 and 13, right? The orig_flags are copied from the initial flags
>>> during wiphy_register() so it seems we will skip enabling 12 and 13. I
>>> think we should remove the check above and call
>>> wiphy_read_of_freq_limits() as a last step within
>>> brcmf_setup_wiphybands(). It means it is called every time, but it
>>> safeguards that the limits in DT are always applied.
>>
>> I'm not exactly happy with channels management in brcmfmac. Before
>> calling wiphy_register it already disables channels unavailable for
>> current country. This results in setting IEEE80211_CHAN_DISABLED in
>
> What do you mean by current country. There is none that we are aware off
> in the driver. So we obtain the channels for the current
> country/revision in the firmware and enable those before
> wiphy_register(). This all is within the probe/init sequence so I do not
> really see an issue. As the wiphy object is not yet registered there is
> no user-space awareness

It seems I'm terrible as describing my patches/problems/ideas :( Here
I used 1 inaccurate word and you couldn't understand my point.

By "current country" I meant current region (and so a set of
regulatory rules) used by the firmware. I believe firmware describes
it using "ccode" and "regrev".

Now, about the issue I see:

AFAIU if you set IEEE80211_CHAN_DISABLED in "orig_flags" it's meant to
be there for good. Some reference code that makes me believe so
(reg.c):
pr_debug("Disabling freq %d MHz for good\n", chan->center_freq);
chan->orig_flags |=3D IEEE80211_CHAN_DISABLED;

This is what happens with brcmfmac right now. If firmware doesn't
report some channels, you set "flags" to IEEE80211_CHAN_DISABLED for
them. Then you call wiphy_register which copies that "flags" to the
"orig_flags". I read it as: we are never going to use these channels.

Now, consider you support regdom change (I do with my local patches).
You translate alpha2 to a proper firmware request (board specific!),
you execute it and then firmware may allow you to use channels that
you marked as disabled for good. You would need to mess with
orig_flags to recover from this issue.

Does my explanation make more sense of this issue now?


>> orig_flags of channels that may become available later, after country
>> change. Please note it happens even right now, without this patch.
>
> Nope. As stated earlier the country setting in firmware is not updated
> unless you provide a *proper* mapping of user-space country code to
> firmware country code/revision. That is the reason, ie. firmware simply
> returns the same list of channels as nothing changed from its
> perspective. We may actually drop 11d support.

I implemented mapping support locally, this is the feature I'm talking abou=
t.


>> Maybe you can workaround this by ignoring orig_flags in custom
>> regulatory code, but I'd just prefer to have it nicely handled in the
>> first place.
>
> Please care to explain your ideas before putting any effort in this
> "feature". As the author of the code that makes you unhappy and as
> driver maintainer I would like to get a clearer picture of your point of
> view. What exactly is the issue that makes you unhappy.

I meant that problem with "orig_flags" I described in the first
paragraph. I wasn't trying to hide whatever issue I'm seeing, I swear
;)


>> This is the next feature I'm going to work on. AFAIU this patch won't
>> be applied for now (it's for wireless-drivers-next and we first need
>> to get wiphy_read_of_freq_limits in that tree). By the time that
>> happens I may have another patchset cleaning brcmfmac ready. And FWIW
>> this patch wouldn't make things worse *at this point* as we don't
>> really support country switching for any device yet.
>
> Now who is *we*? We as Broadcom can, because we know how to map the ISO
> 3166-1 country code to firmware country code/revision for a specific
> firmware release. Firmware uses its own regulatory rules which may
> differ from what regdb has. Now I know Intel submitted a mechanism to
> export firmware rules to regdb so maybe we should consider switching to
> that api if that has been upstreamed. Need to check.

We as a driver developers. Please read
"we don't really support country switching for any device yet"
as
"brcmfmac doesn't really support country switching for any device yet"

Does it help to get the context?

--=20
Rafa=C5=82

Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits

[Arend Van Spriel]

On 5-1-2017 11:02, Rafał Miłecki wrote:
> On 5 January 2017 at 10:31, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 4-1-2017 22:19, Rafał Miłecki wrote:
>>> On 4 January 2017 at 21:07, Arend Van Spriel
>>> <arend.vanspriel@broadcom.com> wrote:
>>>> On 4-1-2017 18:58, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>
>>>>> There are some devices (e.g. Netgear R8000 home router) with one chipset
>>>>> model used for different radios, some of them limited to subbands. NVRAM
>>>>> entries don't contain any extra info on such limitations and firmware
>>>>> reports full list of channels to us. We need to store extra limitation
>>>>> info in DT to support such devices properly.
>>>>>
>>>>> Now there is a cfg80211 helper for reading such info use it in brcmfmac.
>>>>> This patch adds check for channel being disabled with orig_flags which
>>>>> is how this wiphy helper and wiphy_register work.
>>>>>
>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>> ---
>>>>> This patch should probably go through wireless-driver-next which is why
>>>>> it got weird number 4/3. I'm sending it just as a proof of concept.
>>>>> It was succesfully tested on SmartRG SR400ac with BCM43602.
>>>>>
>>>>> V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
>>>>> V5: Update commit message
>>>>> V6: Call wiphy_read_of_freq_limits after brcmf_setup_wiphybands to make it work
>>>>>     with helper setting "flags" instead of "orig_flags".
>>>>> ---
>>>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 ++++++++-
>>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> index ccae3bb..a008ba5 100644
>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> @@ -5886,6 +5886,9 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
>>>>>                                                      band->band);
>>>>>               channel[index].hw_value = ch.control_ch_num;
>>>>>
>>>>> +             if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
>>>>> +                     continue;
>>>>> +
>>>>
>>>> So to be clear this is still needed for subsequent calls to
>>>> brcmf_setup_wiphybands(). The subsequent calls are done from the
>>>> regulatory notifier. So I think we have an issue with this approach. Say
>>>> the device comes up with US. That would set DISABLED flags for channels
>>>> 12 to 14. With a country update to PL we would want to enable channels
>>>> 12 and 13, right? The orig_flags are copied from the initial flags
>>>> during wiphy_register() so it seems we will skip enabling 12 and 13. I
>>>> think we should remove the check above and call
>>>> wiphy_read_of_freq_limits() as a last step within
>>>> brcmf_setup_wiphybands(). It means it is called every time, but it
>>>> safeguards that the limits in DT are always applied.
>>>
>>> I'm not exactly happy with channels management in brcmfmac. Before
>>> calling wiphy_register it already disables channels unavailable for
>>> current country. This results in setting IEEE80211_CHAN_DISABLED in
>>
>> What do you mean by current country. There is none that we are aware off
>> in the driver. So we obtain the channels for the current
>> country/revision in the firmware and enable those before
>> wiphy_register(). This all is within the probe/init sequence so I do not
>> really see an issue. As the wiphy object is not yet registered there is
>> no user-space awareness
> 
> It seems I'm terrible as describing my patches/problems/ideas :( Here
> I used 1 inaccurate word and you couldn't understand my point.

Well. Because of our track record of miscommunication and other
annoyances I want to be sure this time :-p

> By "current country" I meant current region (and so a set of
> regulatory rules) used by the firmware. I believe firmware describes
> it using "ccode" and "regrev".
> 
> Now, about the issue I see:
> 
> AFAIU if you set IEEE80211_CHAN_DISABLED in "orig_flags" it's meant to
> be there for good. Some reference code that makes me believe so

Indeed. Admittedly, it is the first time I became aware of it when
Johannes brought it up.

> (reg.c):
> pr_debug("Disabling freq %d MHz for good\n", chan->center_freq);
> chan->orig_flags |= IEEE80211_CHAN_DISABLED;
> 
> This is what happens with brcmfmac right now. If firmware doesn't
> report some channels, you set "flags" to IEEE80211_CHAN_DISABLED for
> them. Then you call wiphy_register which copies that "flags" to the
> "orig_flags". I read it as: we are never going to use these channels.
> 
> Now, consider you support regdom change (I do with my local patches).
> You translate alpha2 to a proper firmware request (board specific!),
> you execute it and then firmware may allow you to use channels that
> you marked as disabled for good. You would need to mess with
> orig_flags to recover from this issue.
> 
> Does my explanation make more sense of this issue now?

Sure. It seems we should leave all channels enabled and disable them
after wiphy_register() based on firmware information. Or did you have
something else in mind. DFS channels may need to pass a feature check in
firmware and always be disabled otherwise.

>>> orig_flags of channels that may become available later, after country
>>> change. Please note it happens even right now, without this patch.
>>
>> Nope. As stated earlier the country setting in firmware is not updated
>> unless you provide a *proper* mapping of user-space country code to
>> firmware country code/revision. That is the reason, ie. firmware simply
>> returns the same list of channels as nothing changed from its
>> perspective. We may actually drop 11d support.
> 
> I implemented mapping support locally, this is the feature I'm talking about.

Ok. So this is not an upstream feature. Or are you getting the mapping
from DT?

>>> Maybe you can workaround this by ignoring orig_flags in custom
>>> regulatory code, but I'd just prefer to have it nicely handled in the
>>> first place.
>>
>> Please care to explain your ideas before putting any effort in this
>> "feature". As the author of the code that makes you unhappy and as
>> driver maintainer I would like to get a clearer picture of your point of
>> view. What exactly is the issue that makes you unhappy.
> 
> I meant that problem with "orig_flags" I described in the first
> paragraph. I wasn't trying to hide whatever issue I'm seeing, I swear
> ;)
> 
> 
>>> This is the next feature I'm going to work on. AFAIU this patch won't
>>> be applied for now (it's for wireless-drivers-next and we first need
>>> to get wiphy_read_of_freq_limits in that tree). By the time that
>>> happens I may have another patchset cleaning brcmfmac ready. And FWIW
>>> this patch wouldn't make things worse *at this point* as we don't
>>> really support country switching for any device yet.
>>
>> Now who is *we*? We as Broadcom can, because we know how to map the ISO
>> 3166-1 country code to firmware country code/revision for a specific
>> firmware release. Firmware uses its own regulatory rules which may
>> differ from what regdb has. Now I know Intel submitted a mechanism to
>> export firmware rules to regdb so maybe we should consider switching to
>> that api if that has been upstreamed. Need to check.
> 
> We as a driver developers. Please read
> "we don't really support country switching for any device yet"
> as
> "brcmfmac doesn't really support country switching for any device yet"
> 
> Does it help to get the context?

I indeed prefer to talk about the driver instead of we. Indeed it is
true due to the orig_flags behavior although that only seems to involve
regulatory code. Could it be that brcmfmac undo that through the notifier?

Regards,
Arend

Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits

[Rafał Miłecki]

On 7 January 2017 at 13:52, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 5-1-2017 11:02, Rafa=C5=82 Mi=C5=82ecki wrote:
>> On 5 January 2017 at 10:31, Arend Van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>> On 4-1-2017 22:19, Rafa=C5=82 Mi=C5=82ecki wrote:
>>>> On 4 January 2017 at 21:07, Arend Van Spriel
>>>> <arend.vanspriel@broadcom.com> wrote:
>>>>> On 4-1-2017 18:58, Rafa=C5=82 Mi=C5=82ecki wrote:
>>>>>> From: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>>>>>>
>>>>>> There are some devices (e.g. Netgear R8000 home router) with one chi=
pset
>>>>>> model used for different radios, some of them limited to subbands. N=
VRAM
>>>>>> entries don't contain any extra info on such limitations and firmwar=
e
>>>>>> reports full list of channels to us. We need to store extra limitati=
on
>>>>>> info in DT to support such devices properly.
>>>>>>
>>>>>> Now there is a cfg80211 helper for reading such info use it in brcmf=
mac.
>>>>>> This patch adds check for channel being disabled with orig_flags whi=
ch
>>>>>> is how this wiphy helper and wiphy_register work.
>>>>>>
>>>>>> Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>>>>>> ---
>>>>>> This patch should probably go through wireless-driver-next which is =
why
>>>>>> it got weird number 4/3. I'm sending it just as a proof of concept.
>>>>>> It was succesfully tested on SmartRG SR400ac with BCM43602.
>>>>>>
>>>>>> V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
>>>>>> V5: Update commit message
>>>>>> V6: Call wiphy_read_of_freq_limits after brcmf_setup_wiphybands to m=
ake it work
>>>>>>     with helper setting "flags" instead of "orig_flags".
>>>>>> ---
>>>>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 +++=
+++++-
>>>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg802=
11.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>> index ccae3bb..a008ba5 100644
>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>> @@ -5886,6 +5886,9 @@ static int brcmf_construct_chaninfo(struct brc=
mf_cfg80211_info *cfg,
>>>>>>                                                      band->band);
>>>>>>               channel[index].hw_value =3D ch.control_ch_num;
>>>>>>
>>>>>> +             if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
>>>>>> +                     continue;
>>>>>> +
>>>>>
>>>>> So to be clear this is still needed for subsequent calls to
>>>>> brcmf_setup_wiphybands(). The subsequent calls are done from the
>>>>> regulatory notifier. So I think we have an issue with this approach. =
Say
>>>>> the device comes up with US. That would set DISABLED flags for channe=
ls
>>>>> 12 to 14. With a country update to PL we would want to enable channel=
s
>>>>> 12 and 13, right? The orig_flags are copied from the initial flags
>>>>> during wiphy_register() so it seems we will skip enabling 12 and 13. =
I
>>>>> think we should remove the check above and call
>>>>> wiphy_read_of_freq_limits() as a last step within
>>>>> brcmf_setup_wiphybands(). It means it is called every time, but it
>>>>> safeguards that the limits in DT are always applied.
>>>>
>>>> I'm not exactly happy with channels management in brcmfmac. Before
>>>> calling wiphy_register it already disables channels unavailable for
>>>> current country. This results in setting IEEE80211_CHAN_DISABLED in
>>>
>>> What do you mean by current country. There is none that we are aware of=
f
>>> in the driver. So we obtain the channels for the current
>>> country/revision in the firmware and enable those before
>>> wiphy_register(). This all is within the probe/init sequence so I do no=
t
>>> really see an issue. As the wiphy object is not yet registered there is
>>> no user-space awareness
>>
>> It seems I'm terrible as describing my patches/problems/ideas :( Here
>> I used 1 inaccurate word and you couldn't understand my point.
>
> Well. Because of our track record of miscommunication and other
> annoyances I want to be sure this time :-p
>
>> By "current country" I meant current region (and so a set of
>> regulatory rules) used by the firmware. I believe firmware describes
>> it using "ccode" and "regrev".
>>
>> Now, about the issue I see:
>>
>> AFAIU if you set IEEE80211_CHAN_DISABLED in "orig_flags" it's meant to
>> be there for good. Some reference code that makes me believe so
>
> Indeed. Admittedly, it is the first time I became aware of it when
> Johannes brought it up.
>
>> (reg.c):
>> pr_debug("Disabling freq %d MHz for good\n", chan->center_freq);
>> chan->orig_flags |=3D IEEE80211_CHAN_DISABLED;
>>
>> This is what happens with brcmfmac right now. If firmware doesn't
>> report some channels, you set "flags" to IEEE80211_CHAN_DISABLED for
>> them. Then you call wiphy_register which copies that "flags" to the
>> "orig_flags". I read it as: we are never going to use these channels.
>>
>> Now, consider you support regdom change (I do with my local patches).
>> You translate alpha2 to a proper firmware request (board specific!),
>> you execute it and then firmware may allow you to use channels that
>> you marked as disabled for good. You would need to mess with
>> orig_flags to recover from this issue.
>>
>> Does my explanation make more sense of this issue now?
>
> Sure. It seems we should leave all channels enabled and disable them
> after wiphy_register() based on firmware information. Or did you have
> something else in mind. DFS channels may need to pass a feature check in
> firmware and always be disabled otherwise.

I got in mind exactly what you described. I didn't look at DFS channels yet=
.


>>>> orig_flags of channels that may become available later, after country
>>>> change. Please note it happens even right now, without this patch.
>>>
>>> Nope. As stated earlier the country setting in firmware is not updated
>>> unless you provide a *proper* mapping of user-space country code to
>>> firmware country code/revision. That is the reason, ie. firmware simply
>>> returns the same list of channels as nothing changed from its
>>> perspective. We may actually drop 11d support.
>>
>> I implemented mapping support locally, this is the feature I'm talking a=
bout.
>
> Ok. So this is not an upstream feature. Or are you getting the mapping
> from DT?

I'll send patch next week. So far I put mapping table directly in a
driver, but I think it's better to have this in DT.


>>>> Maybe you can workaround this by ignoring orig_flags in custom
>>>> regulatory code, but I'd just prefer to have it nicely handled in the
>>>> first place.
>>>
>>> Please care to explain your ideas before putting any effort in this
>>> "feature". As the author of the code that makes you unhappy and as
>>> driver maintainer I would like to get a clearer picture of your point o=
f
>>> view. What exactly is the issue that makes you unhappy.
>>
>> I meant that problem with "orig_flags" I described in the first
>> paragraph. I wasn't trying to hide whatever issue I'm seeing, I swear
>> ;)
>>
>>
>>>> This is the next feature I'm going to work on. AFAIU this patch won't
>>>> be applied for now (it's for wireless-drivers-next and we first need
>>>> to get wiphy_read_of_freq_limits in that tree). By the time that
>>>> happens I may have another patchset cleaning brcmfmac ready. And FWIW
>>>> this patch wouldn't make things worse *at this point* as we don't
>>>> really support country switching for any device yet.
>>>
>>> Now who is *we*? We as Broadcom can, because we know how to map the ISO
>>> 3166-1 country code to firmware country code/revision for a specific
>>> firmware release. Firmware uses its own regulatory rules which may
>>> differ from what regdb has. Now I know Intel submitted a mechanism to
>>> export firmware rules to regdb so maybe we should consider switching to
>>> that api if that has been upstreamed. Need to check.
>>
>> We as a driver developers. Please read
>> "we don't really support country switching for any device yet"
>> as
>> "brcmfmac doesn't really support country switching for any device yet"
>>
>> Does it help to get the context?
>
> I indeed prefer to talk about the driver instead of we. Indeed it is
> true due to the orig_flags behavior although that only seems to involve
> regulatory code. Could it be that brcmfmac undo that through the notifier=
?

I guess you could touch orig_flags, but I don't know if it's preferred
way. This is probably question to Johannes & cfg80211 guys.

--=20
Rafa=C5=82

Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits

[Johannes Berg]

On Sat, 2017-01-07 at 13:58 +0100, Rafał Miłecki wrote:

> > I indeed prefer to talk about the driver instead of we. Indeed it
> > is true due to the orig_flags behavior although that only seems to
> > involve regulatory code. Could it be that brcmfmac undo that
> > through the notifier?
> 
> I guess you could touch orig_flags, but I don't know if it's
> preferred way. This is probably question to Johannes & cfg80211 guys.

Right now - before the OF patch - there can't really be any orig_flags
with DISABLED since the driver doesn't set flags to DISABLED before
registering, does it? While registering, flags are copied to orig_flags
so the driver can register with flags like DFS or NO_IR already enabled
- say the firmware requires that - and they will never be overwritten
by cfg80211.

Arguably, what the driver does today - before OF - isn't incorrect
either, since it simply doesn't care about anything it registered with
at all.

However, with the OF, I argued (succesfully it seems :P) that the
sensible thing to do was to register with the DISABLED flag and thereby
"permanently" disable the channels that OF didn't think were usable,
but in this case now the driver has to adhere to the cfg80211 logic of
preserving orig_flags forever.

johannes

Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits

[Arend Van Spriel]

On 9-1-2017 9:58, Johannes Berg wrote:
> On Sat, 2017-01-07 at 13:58 +0100, Rafał Miłecki wrote:
> 
>>> I indeed prefer to talk about the driver instead of we. Indeed it
>>> is true due to the orig_flags behavior although that only seems to
>>> involve regulatory code. Could it be that brcmfmac undo that
>>> through the notifier?
>>
>> I guess you could touch orig_flags, but I don't know if it's
>> preferred way. This is probably question to Johannes & cfg80211 guys.
> 
> Right now - before the OF patch - there can't really be any orig_flags
> with DISABLED since the driver doesn't set flags to DISABLED before
> registering, does it? While registering, flags are copied to orig_flags
> so the driver can register with flags like DFS or NO_IR already enabled
> - say the firmware requires that - and they will never be overwritten
> by cfg80211.

Actually, in brcmfmac we do set channels to DISABLED before registering.
I was blissfully unaware of the orig_flags when I added the channel
setup in our probe sequence.

> Arguably, what the driver does today - before OF - isn't incorrect
> either, since it simply doesn't care about anything it registered with
> at all.

Given the statement above I think brcmfmac is incorrect.

> However, with the OF, I argued (succesfully it seems :P) that the
> sensible thing to do was to register with the DISABLED flag and thereby
> "permanently" disable the channels that OF didn't think were usable,
> but in this case now the driver has to adhere to the cfg80211 logic of
> preserving orig_flags forever.

By adhere you mean we should not enable channes for which orig_flags
indicate DISABLED?

Regards,
Arend

Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits

[Johannes Berg]

On Mon, 2017-01-09 at 12:02 +0100, Arend Van Spriel wrote:

> > However, with the OF, I argued (succesfully it seems :P) that the
> > sensible thing to do was to register with the DISABLED flag and
> > thereby
> > "permanently" disable the channels that OF didn't think were
> > usable,
> > but in this case now the driver has to adhere to the cfg80211 logic
> > of
> > preserving orig_flags forever.
> 
> By adhere you mean we should not enable channes for which orig_flags
> indicate DISABLED?

Well, the regulatory code will "OR" in all the orig_flags after
modifications. If you don't use any of the others before registering,
and you don't use any other helpers other than the OF one that we know
sets only DISABLED, then keeping only the DISABLED flag would be OK -
however, it should *also* be OK to always keep *all* of the orig_flags,
just like the normal regulatory code would?

johannes

Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits

[Rafał Miłecki]

On 5 January 2017 at 11:02, Rafa=C5=82 Mi=C5=82ecki <zajec5@gmail.com> wrot=
e:
> On 5 January 2017 at 10:31, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 4-1-2017 22:19, Rafa=C5=82 Mi=C5=82ecki wrote:
>>> I'm not exactly happy with channels management in brcmfmac. Before
>>> calling wiphy_register it already disables channels unavailable for
>>> current country. This results in setting IEEE80211_CHAN_DISABLED in
>>
>> What do you mean by current country. There is none that we are aware off
>> in the driver. So we obtain the channels for the current
>> country/revision in the firmware and enable those before
>> wiphy_register(). This all is within the probe/init sequence so I do not
>> really see an issue. As the wiphy object is not yet registered there is
>> no user-space awareness
>
> It seems I'm terrible as describing my patches/problems/ideas :( Here
> I used 1 inaccurate word and you couldn't understand my point.
>
> By "current country" I meant current region (and so a set of
> regulatory rules) used by the firmware. I believe firmware describes
> it using "ccode" and "regrev".
>
> Now, about the issue I see:
>
> AFAIU if you set IEEE80211_CHAN_DISABLED in "orig_flags" it's meant to
> be there for good. Some reference code that makes me believe so
> (reg.c):
> pr_debug("Disabling freq %d MHz for good\n", chan->center_freq);
> chan->orig_flags |=3D IEEE80211_CHAN_DISABLED;
>
> This is what happens with brcmfmac right now. If firmware doesn't
> report some channels, you set "flags" to IEEE80211_CHAN_DISABLED for
> them. Then you call wiphy_register which copies that "flags" to the
> "orig_flags". I read it as: we are never going to use these channels.
>
> Now, consider you support regdom change (I do with my local patches).
> You translate alpha2 to a proper firmware request (board specific!),
> you execute it and then firmware may allow you to use channels that
> you marked as disabled for good. You would need to mess with
> orig_flags to recover from this issue.

I was wrong about this. Current notifier implementation in brcmfmac
doesn't care about "orig_flags" and it just sets "flags" as it wants.
This way it can enable even these channels that were believed to be
disabled for good (DISABLED in "orig_flags").

For my test I booted SR400ac with ccode & regrev set to values that
didn't allow me to use channels 149 - 165 (5735 - 5835). It matches my
current country:
country PL: DFS-ETSI
        (2402 - 2482 @ 40), (20)
        (5170 - 5250 @ 80), (20), AUTO-BW
        (5250 - 5330 @ 80), (20), DFS, AUTO-BW
        (5490 - 5710 @ 160), (27), DFS
        # 60 GHz band channels 1-4, ref: Etsi En 302 567
        (57000 - 66000 @ 2160), (40)

Then I used "iw reg set ??" to set country that allows these channels.
My locally modified brcmfmac driver sent proper "country" iovar to the
firmware and queried it for the updated "chanspecs". See my debugging
messages below:
brcmfmac: [brcmf_construct_chaninfo] hw_value:34 orig_flags:0x101 flags:0x0=
01
brcmfmac: [brcmf_construct_chaninfo] hw_value:36 orig_flags:0x1a0 flags:0x0=
a0
brcmfmac: [brcmf_construct_chaninfo] hw_value:38 orig_flags:0x101 flags:0x0=
01
brcmfmac: [brcmf_construct_chaninfo] hw_value:40 orig_flags:0x190 flags:0x0=
90
brcmfmac: [brcmf_construct_chaninfo] hw_value:42 orig_flags:0x101 flags:0x0=
01
brcmfmac: [brcmf_construct_chaninfo] hw_value:44 orig_flags:0x1a0 flags:0x0=
a0
brcmfmac: [brcmf_construct_chaninfo] hw_value:46 orig_flags:0x101 flags:0x0=
01
brcmfmac: [brcmf_construct_chaninfo] hw_value:48 orig_flags:0x190 flags:0x0=
90
brcmfmac: [brcmf_construct_chaninfo] hw_value:52 orig_flags:0x1aa flags:0x0=
aa
brcmfmac: [brcmf_construct_chaninfo] hw_value:56 orig_flags:0x19a flags:0x0=
9a
brcmfmac: [brcmf_construct_chaninfo] hw_value:60 orig_flags:0x1aa flags:0x0=
aa
brcmfmac: [brcmf_construct_chaninfo] hw_value:64 orig_flags:0x19a flags:0x0=
9a
brcmfmac: [brcmf_construct_chaninfo] hw_value:100 orig_flags:0x1aa flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:104 orig_flags:0x19a flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:108 orig_flags:0x1aa flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:112 orig_flags:0x19a flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:116 orig_flags:0x1aa flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:120 orig_flags:0x19a flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:124 orig_flags:0x1aa flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:128 orig_flags:0x19a flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:132 orig_flags:0x1aa flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:136 orig_flags:0x19a flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:140 orig_flags:0x1ba flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:144 orig_flags:0x101 flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:149 orig_flags:0x101 flags:0x=
0a0
brcmfmac: [brcmf_construct_chaninfo] hw_value:153 orig_flags:0x101 flags:0x=
090
brcmfmac: [brcmf_construct_chaninfo] hw_value:157 orig_flags:0x101 flags:0x=
0a0
brcmfmac: [brcmf_construct_chaninfo] hw_value:161 orig_flags:0x101 flags:0x=
090
brcmfmac: [brcmf_construct_chaninfo] hw_value:165 orig_flags:0x101 flags:0x=
0b0

As you can see brcmfmac dropped IEEE80211_CHAN_DISABLED (hint: 0x1)
for channels 149 - 165 even though they were disabled according to the
"orig_flags".

So this patch with its
if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
        continue;
could be considered some kind of regression. My change wouldn't allow
enabling such channels anymore.

Of course noone would notice this as noone uses country_codes table
yet I guess (except for me locally). Anyway, this patch should be
reworked to make sure it still allows implementing support for
country_codes tables in the future.

Re: [PATCH V6 1/3] dt-bindings: document common IEEE 802.11 frequency limit property

[Rob Herring]

On Wed, Jan 4, 2017 at 11:58 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> This new file should be used for properties that apply to all wireless
> devices.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Switch to a single ieee80211-freq-limit property that allows specifying
>     *multiple* ranges. This resolves problem with more complex rules as pointed
>     by Felx.
>     Make description implementation agnostic as pointed by Arend.
>     Rename node to wifi as suggested by Martin.
> V3: Use more real-life frequencies in the example.
> V5: Describe hardware design as possible use for this property
> V6: Even better property description, thanks Rob for your help!
> ---
>  .../devicetree/bindings/net/wireless/ieee80211.txt | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/wireless/ieee80211.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH V6 1/3] dt-bindings: document common IEEE 802.11 frequency limit property

[Johannes Berg]

> Acked-by: Rob Herring <robh@kernel.org>

Do I take that to mean that we'll merge it through the subsystem tree,
and not go through some common DT tree?

Thanks,
johannes

Re: [PATCH V6 1/3] dt-bindings: document common IEEE 802.11 frequency limit property

[Rob Herring]

On Thu, Jan 5, 2017 at 5:51 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
>
>> Acked-by: Rob Herring <robh@kernel.org>
>
> Do I take that to mean that we'll merge it through the subsystem tree,
> and not go through some common DT tree?

Yes, that's generally the case when bindings are in a series with
driver changes.

Rob

Re: [PATCH V6 1/3] dt-bindings: document common IEEE 802.11 frequency limit property

[Johannes Berg]

On Thu, 2017-01-05 at 10:34 -0600, Rob Herring wrote:
> On Thu, Jan 5, 2017 at 5:51 AM, Johannes Berg <johannes@sipsolutions.
> net> wrote:
> > 
> > > Acked-by: Rob Herring <robh@kernel.org>
> > 
> > Do I take that to mean that we'll merge it through the subsystem
> > tree,
> > and not go through some common DT tree?
> 
> Yes, that's generally the case when bindings are in a series with
> driver changes.

Alright, thanks.

I've applied patches 1-3, patch 4 obviously still needs work (and
probably won't go through my tree anyway.)

Note that I made some documentation fixes in patch 3, Rafał, please
check.

johannes

Re: [PATCH V6 1/3] dt-bindings: document common IEEE 802.11 frequency limit property

[Rafał Miłecki]

On 6 January 2017 at 13:59, Johannes Berg <johannes@sipsolutions.net> wrote=
:
> On Thu, 2017-01-05 at 10:34 -0600, Rob Herring wrote:
>> On Thu, Jan 5, 2017 at 5:51 AM, Johannes Berg <johannes@sipsolutions.
>> net> wrote:
>> >
>> > > Acked-by: Rob Herring <robh@kernel.org>
>> >
>> > Do I take that to mean that we'll merge it through the subsystem
>> > tree,
>> > and not go through some common DT tree?
>>
>> Yes, that's generally the case when bindings are in a series with
>> driver changes.
>
> Alright, thanks.
>
> I've applied patches 1-3, patch 4 obviously still needs work (and
> probably won't go through my tree anyway.)
>
> Note that I made some documentation fixes in patch 3, Rafa=C5=82, please
> check.

Looks great, thanks everyone for your help on getting this patchset in shap=
e!

--=20
Rafa=C5=82