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

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

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

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

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
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.
---
 .../devicetree/bindings/net/wireless/ieee80211.txt     | 18 ++++++++++++++++++
 1 file changed, 18 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..0cd1219
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/wireless/ieee80211.txt
@@ -0,0 +1,18 @@
+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
+
+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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V3 2/2] cfg80211: support ieee80211-freq-limit DT property

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

It allows specifying hardware limitations of supported channels. This
may be useful for specifying single band devices or devices that support
only some part of the whole band.
This can be useful for some tri-band routers that have separated radios
for lower and higher part of 5 GHz band.

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
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.
---
 include/net/cfg80211.h |  23 ++++++++++
 net/wireless/core.c    |   1 +
 net/wireless/reg.c     | 118 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ca2ac1c..5002625 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3515,6 +3515,9 @@ struct wiphy_iftype_ext_capab {
  *	attribute indices defined in &enum nl80211_bss_select_attr.
  *
  * @cookie_counter: unique generic cookie counter, used to identify objects.
+ *
+ * @n_freq_limits: number of frequency limits
+ * @freq_limits: array of extra frequency limits
  */
 struct wiphy {
 	/* assign these fields before you register the wiphy */
@@ -3646,6 +3649,9 @@ struct wiphy {
 
 	u64 cookie_counter;
 
+	unsigned int n_freq_limits;
+	struct ieee80211_freq_range *freq_limits;
+
 	char priv[0] __aligned(NETDEV_ALIGN);
 };
 
@@ -4278,6 +4284,23 @@ const u8 *cfg80211_find_vendor_ie(unsigned int oui, int oui_type,
  */
 
 /**
+ * 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 extermal power amplifier).
+ *
+ * This function reads info from DT and uses it to *modify* channels (disable
+ * unavailable ones) in a regulary code. It's usually a *bad* idea to use it in
+ * drivers with *shared* channel data as DT limitations are device specific.
+ *
+ * As this function access device node it has to be called after set_wiphy_dev.
+ */
+int wiphy_read_of_freq_limits(struct wiphy *wiphy);
+
+/**
  * regulatory_hint - driver hint to the wireless core a regulatory domain
  * @wiphy: the wireless device giving the hint (used only for reporting
  *	conflicts)
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 158c59e..c1b5fc7 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -940,6 +940,7 @@ void cfg80211_dev_free(struct cfg80211_registered_device *rdev)
 
 void wiphy_free(struct wiphy *wiphy)
 {
+	kfree(wiphy->freq_limits);
 	put_device(&wiphy->dev);
 }
 EXPORT_SYMBOL(wiphy_free);
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 5dbac37..e00fd5b 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1158,6 +1158,120 @@ static uint32_t reg_rule_to_chan_bw_flags(const struct ieee80211_regdomain *regd
 	return bw_flags;
 }
 
+int wiphy_read_of_freq_limits(struct wiphy *wiphy)
+{
+	struct device *dev = wiphy_dev(wiphy);
+	struct device_node *np;
+	struct property *prop;
+	const __be32 *p;
+	int len, i, err;
+
+	if (wiphy->n_freq_limits)
+		return 0;
+
+	if (!dev)
+		return 0;
+	np = dev_of_node(dev);
+	if (!np)
+		return 0;
+
+	prop = of_find_property(np, "ieee80211-freq-limit", &len);
+	if (!prop)
+		return 0;
+
+	if (!len || len % sizeof(u32) || len / sizeof(u32) % 2) {
+		dev_err(dev, "ieee80211-freq-limit wrong format");
+		return -EPROTO;
+	}
+	wiphy->n_freq_limits = len / sizeof(u32) / 2;
+
+	wiphy->freq_limits = kzalloc(wiphy->n_freq_limits * sizeof(*wiphy->freq_limits),
+				     GFP_KERNEL);
+	if (!wiphy->freq_limits) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	p = NULL;
+	for (i = 0; i < wiphy->n_freq_limits; i++) {
+		struct ieee80211_freq_range *limit = &wiphy->freq_limits[i];
+
+		p = of_prop_next_u32(prop, p, &limit->start_freq_khz);
+		if (!p) {
+			err = -EINVAL;
+			goto out;
+		}
+
+		p = of_prop_next_u32(prop, p, &limit->end_freq_khz);
+		if (!p) {
+			err = -EINVAL;
+			goto out;
+		}
+
+		if (!limit->start_freq_khz ||
+		    !limit->end_freq_khz ||
+		    limit->start_freq_khz >= limit->end_freq_khz) {
+			err = -EINVAL;
+			goto out;
+		}
+	}
+
+	return 0;
+
+out:
+	dev_err(dev, "Failed to get limits: %d\n", err);
+	kfree(wiphy->freq_limits);
+	wiphy->n_freq_limits = 0;
+
+	return err;
+}
+EXPORT_SYMBOL(wiphy_read_of_freq_limits);
+
+static bool wiphy_freq_limits_valid_chan(struct wiphy *wiphy,
+					 struct ieee80211_channel *chan)
+{
+	u32 bw = MHZ_TO_KHZ(20);
+	int i;
+
+	for (i = 0; i < wiphy->n_freq_limits; i++) {
+		struct ieee80211_freq_range *limit = &wiphy->freq_limits[i];
+
+		if (reg_does_bw_fit(limit, MHZ_TO_KHZ(chan->center_freq), bw))
+			return true;
+	}
+
+	return false;
+}
+
+static void wiphy_freq_limits_apply(struct wiphy *wiphy)
+{
+	enum nl80211_band band;
+	int i;
+
+	if (!wiphy->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, chan)) {
+				pr_debug("Disabling freq %d MHz as it's out of OF limits\n",
+					 chan->center_freq);
+				chan->flags |= IEEE80211_CHAN_DISABLED;
+			}
+		}
+	}
+}
+
 /*
  * Note that right now we assume the desired channel bandwidth
  * is always 20 MHz for each individual channel (HT40 uses 20 MHz
@@ -1693,6 +1807,8 @@ static void wiphy_update_regulatory(struct wiphy *wiphy,
 	for (band = 0; band < NUM_NL80211_BANDS; band++)
 		handle_band(wiphy, initiator, wiphy->bands[band]);
 
+	wiphy_freq_limits_apply(wiphy);
+
 	reg_process_beacons(wiphy);
 	reg_process_ht_flags(wiphy);
 	reg_call_notifier(wiphy, lr);
@@ -1800,6 +1916,8 @@ void wiphy_apply_custom_regulatory(struct wiphy *wiphy,
 		bands_set++;
 	}
 
+	wiphy_freq_limits_apply(wiphy);
+
 	/*
 	 * no point in calling this if it won't have any effect
 	 * on your device's supported bands.
-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] cfg80211: support ieee80211-freq-limit DT property

[Johannes Berg]

> +static void wiphy_freq_limits_apply(struct wiphy *wiphy)
[...]
> +			if (!wiphy_freq_limits_valid_chan(wiphy,
> chan)) {
> +				pr_debug("Disabling freq %d MHz as
> it's out of OF limits\n",
> +					 chan->center_freq);
> +				chan->flags |=
> IEEE80211_CHAN_DISABLED;

I think you didn't address the problem in the best way now.

The problem with the channel sharing was the way you're applying the
limits - at runtime. This is now OK since the new function shouldn't be
called when the channel structs are shared, but hooking it all into the
regulatory code is now no longer needed.

What you can do now, when reading the OF data, is actually apply it to
the channel flags immediately. If done *before* wiphy_register(), these
flags will be preserved forever, so you no longer need any hooks in
regulatory code at all - you can just set the original channel flags
according to the OF data.

I think this greatly simplifies the flow, since you can also remove
wiphy->freq_limits (and n_freq_limits) completely, since now the only
effect of the function would be to modify the channel list, and later
regulatory updates would always preserve the flags.

johannes
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] cfg80211: support ieee80211-freq-limit DT property

[Arend van Spriel]

On 02-01-17 18:52, Johannes Berg wrote:
>> +static void wiphy_freq_limits_apply(struct wiphy *wiphy)
> [...]
>> +			if (!wiphy_freq_limits_valid_chan(wiphy,
>> chan)) {
>> +				pr_debug("Disabling freq %d MHz as
>> it's out of OF limits\n",
>> +					 chan->center_freq);
>> +				chan->flags |=
>> IEEE80211_CHAN_DISABLED;
> 
> I think you didn't address the problem in the best way now.
> 
> The problem with the channel sharing was the way you're applying the
> limits - at runtime. This is now OK since the new function shouldn't be
> called when the channel structs are shared, but hooking it all into thes 
> regulatory code is now no longer needed.
> 
> What you can do now, when reading the OF data, is actually apply it to
> the channel flags immediately. If done *before* wiphy_register(), these
> flags will be preserved forever, so you no longer need any hooks in
> regulatory code at all - you can just set the original channel flags
> according to the OF data.

I suppose this then can also be done early in the wiphy_register()
function itself, right?

> I think this greatly simplifies the flow, since you can also remove
> wiphy->freq_limits (and n_freq_limits) completely, since now the only
> effect of the function would be to modify the channel list, and later
> regulatory updates would always preserve the flags.

So does it mean the function can go in core.c again :-p If it is likely
there will be other properties being added it might justify adding a new
source file, eg. of.c, and only compile it when CONFIG_OF is set. Just a
thought.

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] cfg80211: support ieee80211-freq-limit DT property

[Rafał Miłecki]

On 2 January 2017 at 21:12, Arend van Spriel
<arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> On 02-01-17 18:52, Johannes Berg wrote:
>>> +static void wiphy_freq_limits_apply(struct wiphy *wiphy)
>> [...]
>>> +                    if (!wiphy_freq_limits_valid_chan(wiphy,
>>> chan)) {
>>> +                            pr_debug("Disabling freq %d MHz as
>>> it's out of OF limits\n",
>>> +                                     chan->center_freq);
>>> +                            chan->flags |=
>>> IEEE80211_CHAN_DISABLED;
>>
>> I think you didn't address the problem in the best way now.
>>
>> The problem with the channel sharing was the way you're applying the
>> limits - at runtime. This is now OK since the new function shouldn't be
>> called when the channel structs are shared, but hooking it all into thes
>> regulatory code is now no longer needed.
>>
>> What you can do now, when reading the OF data, is actually apply it to
>> the channel flags immediately. If done *before* wiphy_register(), these
>> flags will be preserved forever, so you no longer need any hooks in
>> regulatory code at all - you can just set the original channel flags
>> according to the OF data.
>
> I suppose this then can also be done early in the wiphy_register()
> function itself, right?

When driver calls wiphy_apply_custom_regulatory I need to already have
limits read from the DT (at least in my current implementation, it may
change, but I need help understanding flow first). As
wiphy_apply_custom_regulatory has to be called before wiphy_register,
I can't read DT so late (in wiphy_register).

-- 
Rafał

Re: [PATCH V3 2/2] cfg80211: support ieee80211-freq-limit DT property

[Johannes Berg]

> I suppose this then can also be done early in the wiphy_register()
> function itself, right?

No, because of the shared channel data issue. If this is
unconditionally in wiphy_register() then we can no longer guarantee
that sharing it is acceptable - which it is today under certain
circumstances the driver controls. This would move it out of driver
control, making it never possible to share any more.

> So does it mean the function can go in core.c again :-p If it is
> likely there will be other properties being added it might justify
> adding a new source file, eg. of.c, and only compile it when
> CONFIG_OF is set. Just a thought.

Yeah, whatever :)
We can figure that out once we have the mechanisms in place.

johannes
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] cfg80211: support ieee80211-freq-limit DT property

[Rafał Miłecki]

On 2 January 2017 at 18:52, Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org> wrote:
>> +static void wiphy_freq_limits_apply(struct wiphy *wiphy)
> [...]
>> +                     if (!wiphy_freq_limits_valid_chan(wiphy,
>> chan)) {
>> +                             pr_debug("Disabling freq %d MHz as
>> it's out of OF limits\n",
>> +                                      chan->center_freq);
>> +                             chan->flags |=
>> IEEE80211_CHAN_DISABLED;
>
> I think you didn't address the problem in the best way now.
>
> The problem with the channel sharing was the way you're applying the
> limits - at runtime. This is now OK since the new function shouldn't be
> called when the channel structs are shared, but hooking it all into the
> regulatory code is now no longer needed.
>
> What you can do now, when reading the OF data, is actually apply it to
> the channel flags immediately. If done *before* wiphy_register(), these
> flags will be preserved forever, so you no longer need any hooks in
> regulatory code at all - you can just set the original channel flags
> according to the OF data.
>
> I think this greatly simplifies the flow, since you can also remove
> wiphy->freq_limits (and n_freq_limits) completely, since now the only
> effect of the function would be to modify the channel list, and later
> regulatory updates would always preserve the flags.

I need some extra help understanding this :(

When driver uses custom regulatory it registers initial channels at
init but it can also react to regdom changes using reg_notifier. Is
that correct?

So I'm looking at brcmf_cfg80211_reg_notifier which calls
brcmf_setup_wiphybands which calls brcmf_construct_chaninfo.
That last one reworks all channels on every call. It first marks all
existing channels as DISABLED then queries firmware for the list of
supported channels and updates wiphy channels one by one.
So if I understand this correctly, every regdom change can result in
rebuilding channels pretty much from the scratch. That's why I
believed I need to call wiphy_freq_limits_apply on runtime, not just
during the init.

Is there some flow in my understanding?

-- 
Rafał

Re: [PATCH V3 2/2] cfg80211: support ieee80211-freq-limit DT property

[Johannes Berg]

> When driver uses custom regulatory it registers initial channels at
> init but it can also react to regdom changes using reg_notifier. Is
> that correct?

We can treat regulatory and OF data as entirely independent, I think.
At least that's my suggestion:

 * use OF data to populate the original channel list, saying which
   channels are valid (or not)
 * use regulatory later to further restrict settings of the channels

> So I'm looking at brcmf_cfg80211_reg_notifier which calls
> brcmf_setup_wiphybands which calls brcmf_construct_chaninfo.
> That last one reworks all channels on every call. It first marks all
> existing channels as DISABLED then queries firmware for the list of
> supported channels and updates wiphy channels one by one.
> So if I understand this correctly, every regdom change can result in
> rebuilding channels pretty much from the scratch. That's why I
> believed I need to call wiphy_freq_limits_apply on runtime, not just
> during the init.
> 
> Is there some flow in my understanding?

I think maybe there's a problem in my understanding :)

All the regulatory code usually takes into account channel->orig_flags. 
If this code also did, then we could have the original DISABLED flag
taken from OF still be valid here.

johannes

johannes

[PATCH V3 3/2] brcmfmac: use wiphy_read_of_freq_limits to get extra limits

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

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 on DT to support such devices properly.

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
This patch should probably go through wireless-driver-next, I'm sending
it just as a proof of concept. It was succesfully tested on SmartRG
SR400ac with BCM43602.
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index ccae3bb..dab4082 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -6825,6 +6825,7 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
 		goto priv_out;
 
 	brcmf_dbg(INFO, "Registering custom regulatory\n");
+	wiphy_read_of_freq_limits(wiphy);
 	wiphy->reg_notifier = brcmf_cfg80211_reg_notifier;
 	wiphy->regulatory_flags |= REGULATORY_CUSTOM_REG;
 	wiphy_apply_custom_regulatory(wiphy, &brcmf_regdom);
-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html