* Re: [PATCH V3 2/2] cfg80211: support ieee80211-freq-limit DT property
From: Johannes Berg @ 2017-01-03 7:06 UTC (permalink / raw)
To: Rafał Miłecki
Cc: linux-wireless@vger.kernel.org, Martin Blumenstingl,
Felix Fietkau, Arend van Spriel, Arnd Bergmann,
devicetree@vger.kernel.org, Rafał Miłecki
In-Reply-To: <CACna6ryrYY5G=Fvmsrq_sD3iX5dWreyz7R8mGEd6ogUck=twnQ@mail.gmail.com>
> 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
^ permalink raw reply
* [PATCH] brcmfmac: avoid writing channel out of allocated array
From: Rafał Miłecki @ 2017-01-03 8:38 UTC (permalink / raw)
To: Kalle Valo
Cc: Arend van Spriel, Franky Lin, Hante Meuleman,
Pieter-Paul Giesberts, Franky Lin, linux-wireless,
brcm80211-dev-list.pdl, Rafał Miłecki
From: Rafał Miłecki <rafal@milecki.pl>
Our code was assigning number of channels to the index variable by
default. If firmware reported channel we didn't predict this would
result in using that initial index value and writing out of array.
Fix this by detecting unexpected channel and ignoring it.
Fixes: 58de92d2f95e ("brcmfmac: use static superset of channels for wiphy bands")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
I'm not sure what kind of material it is. It fixes possible memory corruption
(serious thing?) but this bug was there since Apr 2015, so is it worth fixing
in 4.10? Or maybe I should even cc stable?
I don't think any released firmware reports any unexpected channel, so I guess
noone ever hit this problem. I just noticed this possible problem when working
on another feature.
---
.../broadcom/brcm80211/brcmfmac/cfg80211.c | 29 +++++++++++-----------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 13ca3eb..0babfc7 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5825,7 +5825,6 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
u32 i, j;
u32 total;
u32 chaninfo;
- u32 index;
pbuf = kzalloc(BRCMF_DCMD_MEDLEN, GFP_KERNEL);
@@ -5873,33 +5872,33 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
ch.bw == BRCMU_CHAN_BW_80)
continue;
- channel = band->channels;
- index = band->n_channels;
+ channel = NULL;
for (j = 0; j < band->n_channels; j++) {
- if (channel[j].hw_value == ch.control_ch_num) {
- index = j;
+ if (band->channels[j].hw_value == ch.control_ch_num) {
+ channel = &band->channels[j];
break;
}
}
- channel[index].center_freq =
- ieee80211_channel_to_frequency(ch.control_ch_num,
- band->band);
- channel[index].hw_value = ch.control_ch_num;
+ if (!channel) {
+ brcmf_err("Firmware reported unexpected channel %d\n",
+ ch.control_ch_num);
+ continue;
+ }
/* assuming the chanspecs order is HT20,
* HT40 upper, HT40 lower, and VHT80.
*/
if (ch.bw == BRCMU_CHAN_BW_80) {
- channel[index].flags &= ~IEEE80211_CHAN_NO_80MHZ;
+ channel->flags &= ~IEEE80211_CHAN_NO_80MHZ;
} else if (ch.bw == BRCMU_CHAN_BW_40) {
- brcmf_update_bw40_channel_flag(&channel[index], &ch);
+ brcmf_update_bw40_channel_flag(channel, &ch);
} else {
/* enable the channel and disable other bandwidths
* for now as mentioned order assure they are enabled
* for subsequent chanspecs.
*/
- channel[index].flags = IEEE80211_CHAN_NO_HT40 |
- IEEE80211_CHAN_NO_80MHZ;
+ channel->flags = IEEE80211_CHAN_NO_HT40 |
+ IEEE80211_CHAN_NO_80MHZ;
ch.bw = BRCMU_CHAN_BW_20;
cfg->d11inf.encchspec(&ch);
chaninfo = ch.chspec;
@@ -5907,11 +5906,11 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
&chaninfo);
if (!err) {
if (chaninfo & WL_CHAN_RADAR)
- channel[index].flags |=
+ channel->flags |=
(IEEE80211_CHAN_RADAR |
IEEE80211_CHAN_NO_IR);
if (chaninfo & WL_CHAN_PASSIVE)
- channel[index].flags |=
+ channel->flags |=
IEEE80211_CHAN_NO_IR;
}
}
--
2.10.1
^ permalink raw reply related
* mac80211 / ath9k / Monitor Mode
From: Thomas Graf @ 2017-01-03 9:39 UTC (permalink / raw)
To: linux-wireless
Hello Everyone,
Since I updated my Kernel to the current version 4.9 I don't get my tx
packets dumped in monitor mode. That worked at least with the old 4.1
Kernel.
That problem seems to have to do with IEEE80211_TX_CTL_REQ_TX_STATUS. In
the xmit.c of the ath9k driver only packets with that flag are passed on
to ieee80211_tx_status and otherwise to ieee80211_tx_status_noskb.
So if that flag is not set that packet won't get to the monitor
interface in mac80211.
My solution was to add that flag in ieee80211_xmit:
if (local->monitors)
{
info->flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
}
I'm not sure if that is the right position or if I'm missing something else.
Thanks,
Thomas
^ permalink raw reply
* Re: [PATCH] brcmfmac: avoid writing channel out of allocated array
From: Arend Van Spriel @ 2017-01-03 11:02 UTC (permalink / raw)
To: Rafał Miłecki, Kalle Valo
Cc: Franky Lin, Hante Meuleman, Pieter-Paul Giesberts, Franky Lin,
linux-wireless, brcm80211-dev-list.pdl, Rafał Miłecki
In-Reply-To: <20170103083858.6981-1-zajec5@gmail.com>
On 3-1-2017 9:38, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> Our code was assigning number of channels to the index variable by
> default. If firmware reported channel we didn't predict this would
> result in using that initial index value and writing out of array.
>
> Fix this by detecting unexpected channel and ignoring it.
>
> Fixes: 58de92d2f95e ("brcmfmac: use static superset of channels for wiphy bands")
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> I'm not sure what kind of material it is. It fixes possible memory corruption
> (serious thing?) but this bug was there since Apr 2015, so is it worth fixing
> in 4.10? Or maybe I should even cc stable?
> I don't think any released firmware reports any unexpected channel, so I guess
> noone ever hit this problem. I just noticed this possible problem when working
> on another feature.
Looking at the change I was going to ask if you actually hit the issue
you are addressing here. The channels in __wl_2ghz_channels and
__wl_5ghz_channels are complete list of channels for the particular band
so it would mean firmware behaves out-of-spec or firmware api was
changed. For robustness a change is acceptable I guess.
My general policy is to submit fixes to wireless-drivers (and stable)
only if it resolves a critical issue found during testing or a reported
issue.
> ---
> .../broadcom/brcm80211/brcmfmac/cfg80211.c | 29 +++++++++++-----------
> 1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 13ca3eb..0babfc7 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -5825,7 +5825,6 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
> u32 i, j;
> u32 total;
> u32 chaninfo;
> - u32 index;
>
> pbuf = kzalloc(BRCMF_DCMD_MEDLEN, GFP_KERNEL);
>
> @@ -5873,33 +5872,33 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
> ch.bw == BRCMU_CHAN_BW_80)
> continue;
>
> - channel = band->channels;
> - index = band->n_channels;
> + channel = NULL;
> for (j = 0; j < band->n_channels; j++) {
> - if (channel[j].hw_value == ch.control_ch_num) {
> - index = j;
> + if (band->channels[j].hw_value == ch.control_ch_num) {
> + channel = &band->channels[j];
> break;
> }
> }
You could have kept the index construct and simply check if j ==
band->n_channels here to determine something is wrong.
> - channel[index].center_freq =
> - ieee80211_channel_to_frequency(ch.control_ch_num,
> - band->band);
> - channel[index].hw_value = ch.control_ch_num;
So you are also removing these assignments which indeed seem redundant.
Maybe make note of that in the commit message?
> + if (!channel) {
> + brcmf_err("Firmware reported unexpected channel %d\n",
> + ch.control_ch_num);
> + continue;
> + }
As stated above something is really off when this happens so should we
continue and try to make sense of what firmware provides or simply fail.
Regards,
Arend
^ permalink raw reply
* [PATCH V4 1/2] dt-bindings: document common IEEE 802.11 frequency limit property
From: Rafał Miłecki @ 2017-01-03 11:03 UTC (permalink / raw)
To: Johannes Berg, linux-wireless
Cc: Martin Blumenstingl, Felix Fietkau, Arend van Spriel,
Arnd Bergmann, devicetree, 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.
---
.../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
^ permalink raw reply related
* [PATCH V4 2/2] cfg80211: support ieee80211-freq-limit DT property
From: Rafał Miłecki @ 2017-01-03 11:03 UTC (permalink / raw)
To: Johannes Berg, linux-wireless
Cc: Martin Blumenstingl, Felix Fietkau, Arend van Spriel,
Arnd Bergmann, devicetree, Rafał Miłecki
In-Reply-To: <20170103110340.23249-1-zajec5@gmail.com>
From: Rafał Miłecki <rafal@milecki.pl>
This patch adds a helper for reading that new property and applying
limitations or supported channels specified this way.
It may be useful for specifying single band devices or devices that
support only some part of the whole band. It's common that tri-band
routers have separated radios for lower and higher part of 5 GHz band.
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
---
include/net/cfg80211.h | 26 ++++++++++
net/wireless/Makefile | 1 +
net/wireless/of.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++
net/wireless/reg.c | 4 +-
net/wireless/reg.h | 2 +
5 files changed, 168 insertions(+), 2 deletions(-)
create mode 100644 net/wireless/of.c
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ca2ac1c..d7723a8 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -311,6 +311,32 @@ 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 extermal 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.
+ *
+ * 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.
+ */
+#ifdef CONFIG_OF
+int wiphy_read_of_freq_limits(struct wiphy *wiphy);
+#else /* CONFIG_OF */
+static inline int wiphy_read_of_freq_limits(struct wiphy *wiphy)
+{
+ return 0;
+}
+#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..d5791c8
--- /dev/null
+++ b/net/wireless/of.c
@@ -0,0 +1,137 @@
+/*
+ * 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 "reg.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 (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,
+ 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->orig_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->orig_flags |= IEEE80211_CHAN_DISABLED;
+ }
+ }
+ }
+}
+
+int 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, err;
+
+ 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;
+ }
+ 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;
+ }
+
+ 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;
+ }
+
+ 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;
+ }
+ }
+
+ wiphy_freq_limits_apply(wiphy, freq_limits, n_freq_limits);
+
+ return 0;
+
+out:
+ dev_err(dev, "Failed to get limits: %d\n", err);
+ kfree(freq_limits);
+ return err;
+}
+EXPORT_SYMBOL(wiphy_read_of_freq_limits);
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 5dbac37..bda0e9e 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -748,8 +748,8 @@ 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)
+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;
diff --git a/net/wireless/reg.h b/net/wireless/reg.h
index f6ced31..b8e44ef 100644
--- a/net/wireless/reg.h
+++ b/net/wireless/reg.h
@@ -54,6 +54,8 @@ void regulatory_exit(void);
int set_regdom(const struct ieee80211_regdomain *rd,
enum ieee80211_regd_source regd_src);
+bool reg_does_bw_fit(const struct ieee80211_freq_range *freq_range,
+ u32 center_freq_khz, u32 bw_khz);
unsigned int reg_get_max_bandwidth(const struct ieee80211_regdomain *rd,
const struct ieee80211_reg_rule *rule);
--
2.10.1
^ permalink raw reply related
* [PATCH V4 3/2] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits
From: Rafał Miłecki @ 2017-01-03 11:03 UTC (permalink / raw)
To: Johannes Berg, linux-wireless
Cc: Martin Blumenstingl, Felix Fietkau, Arend van Spriel,
Arnd Bergmann, devicetree, Rafał Miłecki
In-Reply-To: <20170103110340.23249-1-zajec5@gmail.com>
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 on DT to support such devices properly.
This patch adds check for channel being disabled with orig_flags which
is how this wiphy helper works.
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
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.
V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index ccae3bb..f95e316 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.
*/
@@ -6477,6 +6480,7 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp)
wiphy->bands[NL80211_BAND_5GHZ] = band;
}
}
+ wiphy_read_of_freq_limits(wiphy);
err = brcmf_setup_wiphybands(wiphy);
return err;
}
--
2.10.1
^ permalink raw reply related
* Re: [PATCH] brcmfmac: avoid writing channel out of allocated array
From: Rafał Miłecki @ 2017-01-03 11:31 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Kalle Valo, Franky Lin, Hante Meuleman, Pieter-Paul Giesberts,
Franky Lin, linux-wireless@vger.kernel.org,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
Rafał Miłecki
In-Reply-To: <f74a07d7-94a6-f9c6-ba12-adec6f708c3a@broadcom.com>
On 3 January 2017 at 12:02, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 3-1-2017 9:38, Rafa=C5=82 Mi=C5=82ecki wrote:
>> From: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>>
>> Our code was assigning number of channels to the index variable by
>> default. If firmware reported channel we didn't predict this would
>> result in using that initial index value and writing out of array.
>>
>> Fix this by detecting unexpected channel and ignoring it.
>>
>> Fixes: 58de92d2f95e ("brcmfmac: use static superset of channels for wiph=
y bands")
>> Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>> ---
>> I'm not sure what kind of material it is. It fixes possible memory corru=
ption
>> (serious thing?) but this bug was there since Apr 2015, so is it worth f=
ixing
>> in 4.10? Or maybe I should even cc stable?
>> I don't think any released firmware reports any unexpected channel, so I=
guess
>> noone ever hit this problem. I just noticed this possible problem when w=
orking
>> on another feature.
>
> Looking at the change I was going to ask if you actually hit the issue
> you are addressing here. The channels in __wl_2ghz_channels and
> __wl_5ghz_channels are complete list of channels for the particular band
> so it would mean firmware behaves out-of-spec or firmware api was
> changed. For robustness a change is acceptable I guess.
I guess that point of view depends on one's trust to the firmware. I
think it's wrong if a wrong/bugged/hacked firmware can result in
memory corruption in the kernel. Even if it's only about sizeof(struct
ieee80211_channel).
> My general policy is to submit fixes to wireless-drivers (and stable)
> only if it resolves a critical issue found during testing or a reported
> issue.
I'm OK with that.
>> ---
>> .../broadcom/brcm80211/brcmfmac/cfg80211.c | 29 +++++++++++----=
-------
>> 1 file changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c=
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> index 13ca3eb..0babfc7 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> @@ -5825,7 +5825,6 @@ static int brcmf_construct_chaninfo(struct brcmf_c=
fg80211_info *cfg,
>> u32 i, j;
>> u32 total;
>> u32 chaninfo;
>> - u32 index;
>>
>> pbuf =3D kzalloc(BRCMF_DCMD_MEDLEN, GFP_KERNEL);
>>
>> @@ -5873,33 +5872,33 @@ static int brcmf_construct_chaninfo(struct brcmf=
_cfg80211_info *cfg,
>> ch.bw =3D=3D BRCMU_CHAN_BW_80)
>> continue;
>>
>> - channel =3D band->channels;
>> - index =3D band->n_channels;
>> + channel =3D NULL;
>> for (j =3D 0; j < band->n_channels; j++) {
>> - if (channel[j].hw_value =3D=3D ch.control_ch_num) =
{
>> - index =3D j;
>> + if (band->channels[j].hw_value =3D=3D ch.control_c=
h_num) {
>> + channel =3D &band->channels[j];
>> break;
>> }
>> }
>
> You could have kept the index construct and simply check if j =3D=3D
> band->n_channels here to determine something is wrong.
I wanted to simplify code at the same time. Having channel[index]
repeated 7 times was a hint for me it could be handled better. I
should have made that clear, I'll fix improve this in V2.
>> - channel[index].center_freq =3D
>> - ieee80211_channel_to_frequency(ch.control_ch_num,
>> - band->band);
>> - channel[index].hw_value =3D ch.control_ch_num;
>
> So you are also removing these assignments which indeed seem redundant.
> Maybe make note of that in the commit message?
Good idea.
>> + if (!channel) {
>> + brcmf_err("Firmware reported unexpected channel %d=
\n",
>> + ch.control_ch_num);
>> + continue;
>> + }
>
> As stated above something is really off when this happens so should we
> continue and try to make sense of what firmware provides or simply fail.
Well, I could image something like this happening and not being critical.
The simplest case: Broadcom team releases a new firmware which
supports extra 5 GHz channels (e.g. due to the IEEE standard change).
Why should we refuse to run & support all "old" channel just because of tha=
t?
What do you mean by "make sense of what firmware provides"? Would kind
of solution would you suggest?
--=20
Rafa=C5=82
^ permalink raw reply
* Re: [PATCH net-next] bridge: multicast to unicast
From: Nikolay Aleksandrov @ 2017-01-03 11:58 UTC (permalink / raw)
To: Linus Lüssing, netdev
Cc: David S . Miller, Stephen Hemminger, bridge, linux-kernel,
linux-wireless, Felix Fietkau
In-Reply-To: <20170102193214.31723-1-linus.luessing@c0d3.blue>
On 02/01/17 20:32, Linus Lüssing wrote:
> Implements an optional, per bridge port flag and feature to deliver
> multicast packets to any host on the according port via unicast
> individually. This is done by copying the packet per host and
> changing the multicast destination MAC to a unicast one accordingly.
>
> multicast-to-unicast works on top of the multicast snooping feature of
> the bridge. Which means unicast copies are only delivered to hosts which
> are interested in it and signalized this via IGMP/MLD reports
> previously.
>
> This feature is intended for interface types which have a more reliable
> and/or efficient way to deliver unicast packets than broadcast ones
> (e.g. wifi).
>
> However, it should only be enabled on interfaces where no IGMPv2/MLDv1
> report suppression takes place. This feature is disabled by default.
>
> The initial patch and idea is from Felix Fietkau.
>
> Cc: Felix Fietkau <nbd@nbd.name>
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
>
> ---
>
Hi Linus,
A few comments below, in general I have 2 concerns: the new mcast fast-path
tests and cache line ref, and adding netlink support for the new flag.
> This feature is used and enabled by default in OpenWRT and LEDE for AP
> interfaces for more than a year now to allow both a more robust multicast
> delivery and multicast at higher rates (e.g. multicast streaming).
>
> In OpenWRT/LEDE the IGMP/MLD report suppression issue is overcome by
> the network daemon enabling AP isolation and by that separating all STAs.
> Delivery of STA-to-STA IP mulitcast is made possible again by
> enabling and utilizing the bridge hairpin mode, which considers the
> incoming port as a potential outgoing port, too.
>
> Hairpin-mode is performed after multicast snooping, therefore leading to
> only deliver reports to STAs running a multicast router.
> ---
> include/linux/if_bridge.h | 1 +
> net/bridge/br_forward.c | 44 +++++++++++++++++++++--
> net/bridge/br_mdb.c | 2 +-
> net/bridge/br_multicast.c | 92 ++++++++++++++++++++++++++++++++++-------------
> net/bridge/br_private.h | 4 ++-
> net/bridge/br_sysfs_if.c | 2 ++
> 6 files changed, 115 insertions(+), 30 deletions(-)
>
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index c6587c0..f1b0d78 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -46,6 +46,7 @@ struct br_ip_list {
> #define BR_LEARNING_SYNC BIT(9)
> #define BR_PROXYARP_WIFI BIT(10)
> #define BR_MCAST_FLOOD BIT(11)
> +#define BR_MULTICAST_TO_UCAST BIT(12)
>
> #define BR_DEFAULT_AGEING_TIME (300 * HZ)
>
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index 7cb41ae..49d742d 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -174,6 +174,33 @@ static struct net_bridge_port *maybe_deliver(
> return p;
> }
>
> +static struct net_bridge_port *maybe_deliver_addr(
> + struct net_bridge_port *prev, struct net_bridge_port *p,
> + struct sk_buff *skb, const unsigned char *addr,
> + bool local_orig)
> +{
> + struct net_device *dev = BR_INPUT_SKB_CB(skb)->brdev;
> + const unsigned char *src = eth_hdr(skb)->h_source;
> +
> + if (!should_deliver(p, skb))
> + return prev;
> +
> + /* Even with hairpin, no soliloquies - prevent breaking IPv6 DAD */
> + if (skb->dev == p->dev && ether_addr_equal(src, addr))
> + return prev;
> +
> + skb = skb_copy(skb, GFP_ATOMIC);
> + if (!skb) {
> + dev->stats.tx_dropped++;
> + return prev;
> + }
> +
> + memcpy(eth_hdr(skb)->h_dest, addr, ETH_ALEN);
> + __br_forward(p, skb, local_orig);
> +
> + return prev;
> +}
> +
> /* called under rcu_read_lock */
> void br_flood(struct net_bridge *br, struct sk_buff *skb,
> enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
> @@ -231,6 +258,7 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
> struct net_bridge_port *prev = NULL;
> struct net_bridge_port_group *p;
> struct hlist_node *rp;
> + const unsigned char *addr;
nit: please arrange these into reverse christmas tree
>
> rp = rcu_dereference(hlist_first_rcu(&br->router_list));
> p = mdst ? rcu_dereference(mdst->ports) : NULL;
> @@ -241,10 +269,20 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
> rport = rp ? hlist_entry(rp, struct net_bridge_port, rlist) :
> NULL;
>
> - port = (unsigned long)lport > (unsigned long)rport ?
> - lport : rport;
> + if ((unsigned long)lport > (unsigned long)rport) {
> + port = lport;
> + addr = p->unicast ? p->eth_addr : NULL;
> + } else {
> + port = rport;
> + addr = NULL;
> + }
> +
> + if (addr)
> + prev = maybe_deliver_addr(prev, port, skb, addr,
> + local_orig);
> + else
> + prev = maybe_deliver(prev, port, skb, local_orig);
This hunk adds 2 new tests and an additional cache line ref to all mcast forwarding,
regardless if the new (special case) flag is set or not.
Also are you intentionally sending the original skb through the last port ?
>
> - prev = maybe_deliver(prev, port, skb, local_orig);
> if (IS_ERR(prev))
> goto out;
> if (prev == port)
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index 7dbc80d..056e6ac 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -531,7 +531,7 @@ static int br_mdb_add_group(struct net_bridge *br, struct net_bridge_port *port,
> break;
> }
>
> - p = br_multicast_new_port_group(port, group, *pp, state);
> + p = br_multicast_new_port_group(port, group, *pp, state, NULL);
> if (unlikely(!p))
> return -ENOMEM;
> rcu_assign_pointer(*pp, p);
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index b30e77e..470a2409 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -43,12 +43,14 @@ static void br_multicast_add_router(struct net_bridge *br,
> static void br_ip4_multicast_leave_group(struct net_bridge *br,
> struct net_bridge_port *port,
> __be32 group,
> - __u16 vid);
> + __u16 vid,
> + const unsigned char *src);
> +
> #if IS_ENABLED(CONFIG_IPV6)
> static void br_ip6_multicast_leave_group(struct net_bridge *br,
> struct net_bridge_port *port,
> const struct in6_addr *group,
> - __u16 vid);
> + __u16 vid, const unsigned char *src);
> #endif
> unsigned int br_mdb_rehash_seq;
>
> @@ -711,7 +713,8 @@ struct net_bridge_port_group *br_multicast_new_port_group(
> struct net_bridge_port *port,
> struct br_ip *group,
> struct net_bridge_port_group __rcu *next,
> - unsigned char flags)
> + unsigned char flags,
> + const unsigned char *src)
> {
> struct net_bridge_port_group *p;
>
> @@ -726,12 +729,35 @@ struct net_bridge_port_group *br_multicast_new_port_group(
> hlist_add_head(&p->mglist, &port->mglist);
> setup_timer(&p->timer, br_multicast_port_group_expired,
> (unsigned long)p);
> +
> + if ((port->flags & BR_MULTICAST_TO_UCAST) && src) {
> + memcpy(p->eth_addr, src, ETH_ALEN);
> + p->unicast = true;
> + }
> +
> return p;
> }
>
> +static bool br_port_group_equal(struct net_bridge_port_group *p,
> + struct net_bridge_port *port,
> + const unsigned char *src)
> +{
> + if (p->port != port)
> + return false;
> +
> + if (!p->unicast)
> + return true;
> +
> + if (!src)
> + return false;
> +
> + return ether_addr_equal(src, p->eth_addr);
> +}
> +
> static int br_multicast_add_group(struct net_bridge *br,
> struct net_bridge_port *port,
> - struct br_ip *group)
> + struct br_ip *group,
> + const unsigned char *src)
> {
> struct net_bridge_port_group __rcu **pp;
> struct net_bridge_port_group *p;
> @@ -758,13 +784,13 @@ static int br_multicast_add_group(struct net_bridge *br,
> for (pp = &mp->ports;
> (p = mlock_dereference(*pp, br)) != NULL;
> pp = &p->next) {
> - if (p->port == port)
> + if (br_port_group_equal(p, port, src))
> goto found;
> if ((unsigned long)p->port < (unsigned long)port)
> break;
> }
>
> - p = br_multicast_new_port_group(port, group, *pp, 0);
> + p = br_multicast_new_port_group(port, group, *pp, 0, src);
> if (unlikely(!p))
> goto err;
> rcu_assign_pointer(*pp, p);
> @@ -783,7 +809,8 @@ static int br_multicast_add_group(struct net_bridge *br,
> static int br_ip4_multicast_add_group(struct net_bridge *br,
> struct net_bridge_port *port,
> __be32 group,
> - __u16 vid)
> + __u16 vid,
> + const unsigned char *src)
> {
> struct br_ip br_group;
>
> @@ -794,14 +821,15 @@ static int br_ip4_multicast_add_group(struct net_bridge *br,
> br_group.proto = htons(ETH_P_IP);
> br_group.vid = vid;
>
> - return br_multicast_add_group(br, port, &br_group);
> + return br_multicast_add_group(br, port, &br_group, src);
> }
>
> #if IS_ENABLED(CONFIG_IPV6)
> static int br_ip6_multicast_add_group(struct net_bridge *br,
> struct net_bridge_port *port,
> const struct in6_addr *group,
> - __u16 vid)
> + __u16 vid,
> + const unsigned char *src)
> {
> struct br_ip br_group;
>
> @@ -812,7 +840,7 @@ static int br_ip6_multicast_add_group(struct net_bridge *br,
> br_group.proto = htons(ETH_P_IPV6);
> br_group.vid = vid;
>
> - return br_multicast_add_group(br, port, &br_group);
> + return br_multicast_add_group(br, port, &br_group, src);
> }
> #endif
>
> @@ -1081,6 +1109,7 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
> struct sk_buff *skb,
> u16 vid)
> {
> + const unsigned char *src;
> struct igmpv3_report *ih;
> struct igmpv3_grec *grec;
> int i;
> @@ -1121,12 +1150,14 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
> continue;
> }
>
> + src = eth_hdr(skb)->h_source;
> if ((type == IGMPV3_CHANGE_TO_INCLUDE ||
> type == IGMPV3_MODE_IS_INCLUDE) &&
> ntohs(grec->grec_nsrcs) == 0) {
> - br_ip4_multicast_leave_group(br, port, group, vid);
> + br_ip4_multicast_leave_group(br, port, group, vid, src);
> } else {
> - err = br_ip4_multicast_add_group(br, port, group, vid);
> + err = br_ip4_multicast_add_group(br, port, group, vid,
> + src);
> if (err)
> break;
> }
> @@ -1141,6 +1172,7 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
> struct sk_buff *skb,
> u16 vid)
> {
> + const unsigned char *src = eth_hdr(skb)->h_source;
> struct icmp6hdr *icmp6h;
> struct mld2_grec *grec;
> int i;
> @@ -1192,10 +1224,11 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
> grec->grec_type == MLD2_MODE_IS_INCLUDE) &&
> ntohs(*nsrcs) == 0) {
> br_ip6_multicast_leave_group(br, port, &grec->grec_mca,
> - vid);
> + vid, src);
> } else {
> err = br_ip6_multicast_add_group(br, port,
> - &grec->grec_mca, vid);
> + &grec->grec_mca, vid,
> + src);
> if (err)
> break;
> }
> @@ -1511,7 +1544,8 @@ br_multicast_leave_group(struct net_bridge *br,
> struct net_bridge_port *port,
> struct br_ip *group,
> struct bridge_mcast_other_query *other_query,
> - struct bridge_mcast_own_query *own_query)
> + struct bridge_mcast_own_query *own_query,
> + const unsigned char *src)
> {
> struct net_bridge_mdb_htable *mdb;
> struct net_bridge_mdb_entry *mp;
> @@ -1535,7 +1569,7 @@ br_multicast_leave_group(struct net_bridge *br,
> for (pp = &mp->ports;
> (p = mlock_dereference(*pp, br)) != NULL;
> pp = &p->next) {
> - if (p->port != port)
> + if (!br_port_group_equal(p, port, src))
> continue;
>
> rcu_assign_pointer(*pp, p->next);
> @@ -1566,7 +1600,7 @@ br_multicast_leave_group(struct net_bridge *br,
> for (p = mlock_dereference(mp->ports, br);
> p != NULL;
> p = mlock_dereference(p->next, br)) {
> - if (p->port != port)
> + if (!br_port_group_equal(p, port, src))
> continue;
>
> if (!hlist_unhashed(&p->mglist) &&
> @@ -1617,7 +1651,8 @@ br_multicast_leave_group(struct net_bridge *br,
> static void br_ip4_multicast_leave_group(struct net_bridge *br,
> struct net_bridge_port *port,
> __be32 group,
> - __u16 vid)
> + __u16 vid,
> + const unsigned char *src)
> {
> struct br_ip br_group;
> struct bridge_mcast_own_query *own_query;
> @@ -1632,14 +1667,15 @@ static void br_ip4_multicast_leave_group(struct net_bridge *br,
> br_group.vid = vid;
>
> br_multicast_leave_group(br, port, &br_group, &br->ip4_other_query,
> - own_query);
> + own_query, src);
> }
>
> #if IS_ENABLED(CONFIG_IPV6)
> static void br_ip6_multicast_leave_group(struct net_bridge *br,
> struct net_bridge_port *port,
> const struct in6_addr *group,
> - __u16 vid)
> + __u16 vid,
> + const unsigned char *src)
> {
> struct br_ip br_group;
> struct bridge_mcast_own_query *own_query;
> @@ -1654,7 +1690,7 @@ static void br_ip6_multicast_leave_group(struct net_bridge *br,
> br_group.vid = vid;
>
> br_multicast_leave_group(br, port, &br_group, &br->ip6_other_query,
> - own_query);
> + own_query, src);
> }
> #endif
>
> @@ -1711,6 +1747,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
> struct sk_buff *skb,
> u16 vid)
> {
> + const unsigned char *src;
nit: please arrange these in reverse christmas tree
> struct sk_buff *skb_trimmed = NULL;
> struct igmphdr *ih;
> int err;
> @@ -1731,13 +1768,14 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
> }
>
> ih = igmp_hdr(skb);
> + src = eth_hdr(skb)->h_source;
> BR_INPUT_SKB_CB(skb)->igmp = ih->type;
>
> switch (ih->type) {
> case IGMP_HOST_MEMBERSHIP_REPORT:
> case IGMPV2_HOST_MEMBERSHIP_REPORT:
> BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
> - err = br_ip4_multicast_add_group(br, port, ih->group, vid);
> + err = br_ip4_multicast_add_group(br, port, ih->group, vid, src);
> break;
> case IGMPV3_HOST_MEMBERSHIP_REPORT:
> err = br_ip4_multicast_igmp3_report(br, port, skb_trimmed, vid);
> @@ -1746,7 +1784,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
> err = br_ip4_multicast_query(br, port, skb_trimmed, vid);
> break;
> case IGMP_HOST_LEAVE_MESSAGE:
> - br_ip4_multicast_leave_group(br, port, ih->group, vid);
> + br_ip4_multicast_leave_group(br, port, ih->group, vid, src);
> break;
> }
>
> @@ -1765,6 +1803,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
> struct sk_buff *skb,
> u16 vid)
> {
> + const unsigned char *src;
nit: same about arrangement
> struct sk_buff *skb_trimmed = NULL;
> struct mld_msg *mld;
> int err;
> @@ -1785,8 +1824,10 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
>
> switch (mld->mld_type) {
> case ICMPV6_MGM_REPORT:
> + src = eth_hdr(skb)->h_source;
> BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
> - err = br_ip6_multicast_add_group(br, port, &mld->mld_mca, vid);
> + err = br_ip6_multicast_add_group(br, port, &mld->mld_mca, vid,
> + src);
> break;
> case ICMPV6_MLD2_REPORT:
> err = br_ip6_multicast_mld2_report(br, port, skb_trimmed, vid);
> @@ -1795,7 +1836,8 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
> err = br_ip6_multicast_query(br, port, skb_trimmed, vid);
> break;
> case ICMPV6_MGM_REDUCTION:
> - br_ip6_multicast_leave_group(br, port, &mld->mld_mca, vid);
> + src = eth_hdr(skb)->h_source;
> + br_ip6_multicast_leave_group(br, port, &mld->mld_mca, vid, src);
> break;
> }
>
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 8ce621e..cc55100 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -177,6 +177,8 @@ struct net_bridge_port_group {
> struct timer_list timer;
> struct br_ip addr;
> unsigned char flags;
> + unsigned char eth_addr[ETH_ALEN];
> + bool unicast;
I think you can remove the boolean unicast here and either use the "flags" or
the eth_addr itself.
This structure needs a serious re-arrangement.
> };
>
> struct net_bridge_mdb_entry
> @@ -599,7 +601,7 @@ void br_multicast_free_pg(struct rcu_head *head);
> struct net_bridge_port_group *
> br_multicast_new_port_group(struct net_bridge_port *port, struct br_ip *group,
> struct net_bridge_port_group __rcu *next,
> - unsigned char flags);
> + unsigned char flags, const unsigned char *src);
> void br_mdb_init(void);
> void br_mdb_uninit(void);
> void br_mdb_notify(struct net_device *dev, struct net_bridge_port *port,
> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> index 8bd5696..1730278 100644
> --- a/net/bridge/br_sysfs_if.c
> +++ b/net/bridge/br_sysfs_if.c
> @@ -188,6 +188,7 @@ static BRPORT_ATTR(multicast_router, S_IRUGO | S_IWUSR, show_multicast_router,
> store_multicast_router);
>
> BRPORT_ATTR_FLAG(multicast_fast_leave, BR_MULTICAST_FAST_LEAVE);
> +BRPORT_ATTR_FLAG(multicast_to_unicast, BR_MULTICAST_TO_UCAST);
> #endif
>
> static const struct brport_attribute *brport_attrs[] = {
> @@ -214,6 +215,7 @@ static const struct brport_attribute *brport_attrs[] = {
> #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> &brport_attr_multicast_router,
> &brport_attr_multicast_fast_leave,
> + &brport_attr_multicast_to_unicast,
> #endif
> &brport_attr_proxyarp,
> &brport_attr_proxyarp_wifi,
>
Please also add netlink support, we've been working hard at adding support for all bridge
options via netlink.
Thanks,
Nik
^ permalink raw reply
* Re: [PATCH V4 2/2] cfg80211: support ieee80211-freq-limit DT property
From: Arend Van Spriel @ 2017-01-03 12:10 UTC (permalink / raw)
To: Rafał Miłecki, Johannes Berg, linux-wireless
Cc: Martin Blumenstingl, Felix Fietkau, Arend van Spriel,
Arnd Bergmann, devicetree, Rafał Miłecki
In-Reply-To: <20170103110340.23249-2-zajec5@gmail.com>
On 3-1-2017 12:03, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> This patch adds a helper for reading that new property and applying
> limitations or supported channels specified this way.
> It may be useful for specifying single band devices or devices that
> support only some part of the whole band. It's common that tri-band
> routers have separated radios for lower and higher part of 5 GHz band.
>
> 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
> ---
> include/net/cfg80211.h | 26 ++++++++++
> net/wireless/Makefile | 1 +
> net/wireless/of.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++
> net/wireless/reg.c | 4 +-
> net/wireless/reg.h | 2 +
> 5 files changed, 168 insertions(+), 2 deletions(-)
> create mode 100644 net/wireless/of.c
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index ca2ac1c..d7723a8 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -311,6 +311,32 @@ 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 extermal 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.
> + *
> + * As this function access device node it has to be called after set_wiphy_dev.
You are aware that you need to modify this description with earlier
patch "cfg80211: allow passing struct device in the wiphy_new call",
right? :-p
> + * It also modifies channels so they have to be set first.
> + */
> +#ifdef CONFIG_OF
> +int wiphy_read_of_freq_limits(struct wiphy *wiphy);
> +#else /* CONFIG_OF */
> +static inline int wiphy_read_of_freq_limits(struct wiphy *wiphy)
> +{
> + return 0;
> +}
> +#endif /* !CONFIG_OF */
> +
> +
[...]
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index 5dbac37..bda0e9e 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -748,8 +748,8 @@ 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)
> +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;
would it be more appropriate to move this function to util.c?
Regards,
Arend
^ permalink raw reply
* Re: [RFC] nl80211: allow multiple active scheduled scan requests
From: Arend Van Spriel @ 2017-01-03 12:25 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Dmitry Shmidt
In-Reply-To: <1483353841.4596.2.camel@sipsolutions.net>
On 2-1-2017 11:44, Johannes Berg wrote:
>
>> + /*
>> + * allow only one legacy scheduled scan if user-space
>> + * does not indicate multiple scheduled scan support.
>> + */
>> + if (!info->attrs[NL80211_ATTR_SCHED_SCAN_MULTI] &&
>> + cfg80211_legacy_sched_scan_active(rdev))
>> return -EINPROGRESS;
>
> That probably doesn't go far enough - if legacy one is active then we
> probably shouldn't allow a new MULTI one either (or abandon the legacy
> one) so that older userspace doesn't get confused with multiple
> notifications from sched scans it didn't start.
I considered that although not taking the notifications into account.
Will change it. Abandoning the legacy one would be a behavioral change
so probably not acceptable, right?
>> + if (rdev->sched_scan_req_count == rdev->wiphy.max_sched_scan_reqs)
>> + return -ENOSPC;
>
> Do we really want to do the double-accounting, just to avoid counting
> the list length here?
Ok. I have no strong preference.
>> + /* leave request id zero for legacy request */
>
> why? The ID would be ignored, so why special-case it?
It makes the function cfg80211_legacy_sched_scan_active() easier, ie.
not needing a is_legacy flag in struct cfg80211_sched_scan_request.
>> +static void cfg80211_del_sched_scan_req(struct
>> cfg80211_registered_device *rdev,
>> + struct
>> cfg80211_sched_scan_request *req)
>> +{
>> + list_del_rcu(&req->list);
>> + kfree_rcu(req, rcu_head);
>> + synchronize_rcu();
>> + rdev->sched_scan_req_count--;
>> +}
>
> That's bogus - either you use kfree_rcu() or synchronize_rcu() (the
> former is much better); combining both makes no sense.
Thanks. Both functions mentioned the rcu grace period so I was doubtful.
Will change it.
>> +bool cfg80211_legacy_sched_scan_active(struct
>> cfg80211_registered_device *rdev)
>> +{
>> + struct cfg80211_sched_scan_request *req;
>> +
>> + req = list_first_or_null_rcu(&rdev->sched_scan_req_list,
>> + struct
>> cfg80211_sched_scan_request, list);
>> + /* request id 0 indicates legacy request in progress */
>> + return req && !req->reqid;
>> +}
>
> Ok, fair enough.
I guess your remark means this clarifies your earlier question about the
request id, right?
Regards,
Arend
^ permalink raw reply
* Re: [PATCH net-next] bridge: multicast to unicast
From: Felix Fietkau @ 2017-01-03 13:15 UTC (permalink / raw)
To: Linus Lüssing, netdev
Cc: David S . Miller, Stephen Hemminger, bridge, linux-kernel,
linux-wireless
In-Reply-To: <20170102193214.31723-1-linus.luessing@c0d3.blue>
On 2017-01-02 20:32, Linus Lüssing wrote:
> Implements an optional, per bridge port flag and feature to deliver
> multicast packets to any host on the according port via unicast
> individually. This is done by copying the packet per host and
> changing the multicast destination MAC to a unicast one accordingly.
>
> multicast-to-unicast works on top of the multicast snooping feature of
> the bridge. Which means unicast copies are only delivered to hosts which
> are interested in it and signalized this via IGMP/MLD reports
> previously.
>
> This feature is intended for interface types which have a more reliable
> and/or efficient way to deliver unicast packets than broadcast ones
> (e.g. wifi).
>
> However, it should only be enabled on interfaces where no IGMPv2/MLDv1
> report suppression takes place. This feature is disabled by default.
>
> The initial patch and idea is from Felix Fietkau.
>
> Cc: Felix Fietkau <nbd@nbd.name>
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
Please add Signed-off-by: Felix Fietkau <nbd@nbd.name>
in the next version, and maybe also From:
Thanks,
- Felix
^ permalink raw reply
* Re: [PATCH] brcmfmac: avoid writing channel out of allocated array
From: Arend Van Spriel @ 2017-01-03 13:19 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Kalle Valo, Franky Lin, Hante Meuleman, Pieter-Paul Giesberts,
Franky Lin, linux-wireless@vger.kernel.org,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
Rafał Miłecki
In-Reply-To: <CACna6rzGRHLCdzxS_+MHXeZwF=BW4S52pDb9g9MuanNGyToEOw@mail.gmail.com>
On 3-1-2017 12:31, Rafał Miłecki wrote:
>>> + if (!channel) {
>>> + brcmf_err("Firmware reported unexpected channel %d\n",
>>> + ch.control_ch_num);
>>> + continue;
>>> + }
>> As stated above something is really off when this happens so should we
>> continue and try to make sense of what firmware provides or simply fail.
> Well, I could image something like this happening and not being critical.
> The simplest case: Broadcom team releases a new firmware which
> supports extra 5 GHz channels (e.g. due to the IEEE standard change).
> Why should we refuse to run & support all "old" channel just because of that?
Fair enough. I was assuming we keep __wl_{2,5}ghz_channels up to date
with IEEE standard.
> What do you mean by "make sense of what firmware provides"? Would kind
> of solution would you suggest?
When the above assumption can be assured (by us) the only other scenario
would be a change in the firmware API where we wrongly interpret the
information retrieved. In this case all subsequent channels will likely
result in bogus or accidental matches hence it seems better to bail out
early.
Regards,
Arend
^ permalink raw reply
* Re: [PATCH V4 3/2] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits
From: Arend Van Spriel @ 2017-01-03 13:29 UTC (permalink / raw)
To: Rafał Miłecki, Johannes Berg, linux-wireless
Cc: Martin Blumenstingl, Felix Fietkau, Arend van Spriel,
Arnd Bergmann, devicetree, Rafał Miłecki
In-Reply-To: <20170103110340.23249-3-zajec5@gmail.com>
What is with the patch numbering, ie. 3/2?
On 3-1-2017 12:03, 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 on DT to support such devices properly.
>
> This patch adds check for channel being disabled with orig_flags which
> is how this wiphy helper works.
this is the first mention about the wiphy helper. Probably need
statement here that call to wiphy_read_of_freq_limits() was added in
this patch which applies the extra limitation info read from DT.
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> 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.
>
> V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index ccae3bb..f95e316 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.
> */
> @@ -6477,6 +6480,7 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp)
> wiphy->bands[NL80211_BAND_5GHZ] = band;
> }
> }
> + wiphy_read_of_freq_limits(wiphy);
The return value is ignored, which I suppose is fine. So does the
function need a return value at all? Is there a scenario where the DT
info *must* be supplied?
Regards,
Arend
^ permalink raw reply
* Re: [PATCH] brcmfmac: avoid writing channel out of allocated array
From: Rafał Miłecki @ 2017-01-03 14:14 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Kalle Valo, Franky Lin, Hante Meuleman, Pieter-Paul Giesberts,
Franky Lin, linux-wireless@vger.kernel.org,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
Rafał Miłecki
In-Reply-To: <0c0c9680-2cc8-ad0a-3aa0-ba406a838ab8@broadcom.com>
On 3 January 2017 at 14:19, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 3-1-2017 12:31, Rafa=C5=82 Mi=C5=82ecki wrote:
>>>> + if (!channel) {
>>>> + brcmf_err("Firmware reported unexpected channel =
%d\n",
>>>> + ch.control_ch_num);
>>>> + continue;
>>>> + }
>>> As stated above something is really off when this happens so should we
>>> continue and try to make sense of what firmware provides or simply fail=
.
>> Well, I could image something like this happening and not being critical=
.
>> The simplest case: Broadcom team releases a new firmware which
>> supports extra 5 GHz channels (e.g. due to the IEEE standard change).
>> Why should we refuse to run & support all "old" channel just because of =
that?
>
> Fair enough. I was assuming we keep __wl_{2,5}ghz_channels up to date
> with IEEE standard.
>
>> What do you mean by "make sense of what firmware provides"? Would kind
>> of solution would you suggest?
>
> When the above assumption can be assured (by us) the only other scenario
> would be a change in the firmware API where we wrongly interpret the
> information retrieved. In this case all subsequent channels will likely
> result in bogus or accidental matches hence it seems better to bail out
> early.
Good point, this actually gave me an idea for a small nice
improvement. I remember I saw something like WL_VER in wl ioctl
protocol, it was already bumped at some point by Broadcom, when
chanspec format has changed. We should probably read this number from
firmware and maybe refuse to run if version is newer than the one we
know.
--=20
Rafa=C5=82
^ permalink raw reply
* Re: [PATCH V4 2/2] cfg80211: support ieee80211-freq-limit DT property
From: Rafał Miłecki @ 2017-01-03 14:17 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Johannes Berg, linux-wireless@vger.kernel.org,
Martin Blumenstingl, Felix Fietkau, Arend van Spriel,
Arnd Bergmann, devicetree@vger.kernel.org,
Rafał Miłecki
In-Reply-To: <a67789ba-3346-6e10-89c2-2df419a80910@broadcom.com>
On 3 January 2017 at 13:10, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 3-1-2017 12:03, Rafa=C5=82 Mi=C5=82ecki wrote:
>> From: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>>
>> This patch adds a helper for reading that new property and applying
>> limitations or supported channels specified this way.
>> It may be useful for specifying single band devices or devices that
>> support only some part of the whole band. It's common that tri-band
>> routers have separated radios for lower and higher part of 5 GHz band.
>>
>> Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>> ---
>> V2: Put main code in core.c as it isn't strictly part of regulatory - po=
inted
>> 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
>> ---
>> include/net/cfg80211.h | 26 ++++++++++
>> net/wireless/Makefile | 1 +
>> net/wireless/of.c | 137 ++++++++++++++++++++++++++++++++++++++++++=
+++++++
>> net/wireless/reg.c | 4 +-
>> net/wireless/reg.h | 2 +
>> 5 files changed, 168 insertions(+), 2 deletions(-)
>> create mode 100644 net/wireless/of.c
>>
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index ca2ac1c..d7723a8 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -311,6 +311,32 @@ 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 extermal power amplifier).
>> + *
>> + * This function reads info from DT and uses it to *modify* channels (d=
isable
>> + * unavailable ones). It's usually a *bad* idea to use it in drivers wi=
th
>> + * shared channel data as DT limitations are device specific.
>> + *
>> + * As this function access device node it has to be called after set_wi=
phy_dev.
>
> You are aware that you need to modify this description with earlier
> patch "cfg80211: allow passing struct device in the wiphy_new call",
> right? :-p
I dropped that earlier patch for now as it's no longer a requirement
for this change. If someone find is useful though, I'll be happy to
resume my work on it later. And update this documentation as you
pointed out ;)
>> + * It also modifies channels so they have to be set first.
>> + */
>> +#ifdef CONFIG_OF
>> +int wiphy_read_of_freq_limits(struct wiphy *wiphy);
>> +#else /* CONFIG_OF */
>> +static inline int wiphy_read_of_freq_limits(struct wiphy *wiphy)
>> +{
>> + return 0;
>> +}
>> +#endif /* !CONFIG_OF */
>> +
>> +
>
> [...]
>
>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> index 5dbac37..bda0e9e 100644
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -748,8 +748,8 @@ static bool is_valid_rd(const struct ieee80211_regdo=
main *rd)
>> return true;
>> }
>>
>> -static bool reg_does_bw_fit(const struct ieee80211_freq_range *freq_ran=
ge,
>> - u32 center_freq_khz, u32 bw_khz)
>> +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;
>
> would it be more appropriate to move this function to util.c?
I'm OK with moving this function (and maybe struct
ieee80211_freq_range as well). Any objections?
--=20
Rafa=C5=82
^ permalink raw reply
* Re: [PATCH V4 3/2] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits
From: Rafał Miłecki @ 2017-01-03 14:24 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Johannes Berg, linux-wireless@vger.kernel.org,
Martin Blumenstingl, Felix Fietkau, Arend van Spriel,
Arnd Bergmann, devicetree@vger.kernel.org,
Rafał Miłecki
In-Reply-To: <d306c928-3725-f9d3-e3c5-e75a1849bd8b@broadcom.com>
On 3 January 2017 at 14:29, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> What is with the patch numbering, ie. 3/2?
It's my small trick related to the "This patch should probably go
through wireless-driver-next" ;) I wanted to make it clear that only 2
patches are strictly targeted for the mac80211-next tree.
> On 3-1-2017 12:03, 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 on DT to support such devices properly.
>>
>> This patch adds check for channel being disabled with orig_flags which
>> is how this wiphy helper works.
>
> this is the first mention about the wiphy helper. Probably need
> statement here that call to wiphy_read_of_freq_limits() was added in
> this patch which applies the extra limitation info read from DT.
OK, I'll improve this description.
>> Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>> ---
>> 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.
>>
>> V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c=
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> index ccae3bb..f95e316 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;
>> +
>> /* assuming the chanspecs order is HT20,
>> * HT40 upper, HT40 lower, and VHT80.
>> */
>> @@ -6477,6 +6480,7 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, =
struct brcmf_if *ifp)
>> wiphy->bands[NL80211_BAND_5GHZ] =3D band;
>> }
>> }
>> + wiphy_read_of_freq_limits(wiphy);
>
> The return value is ignored, which I suppose is fine. So does the
> function need a return value at all? Is there a scenario where the DT
> info *must* be supplied?
To be honest, I can't decide. Right now I don't see a point of
checking that function result (as you noticed, it should never be
required). If no one objects, I'll try switching that function to
void.
--=20
Rafa=C5=82
^ permalink raw reply
* Re: kernel 4.9 iwlwifi startup error
From: Alexander Morozov @ 2017-01-03 15:41 UTC (permalink / raw)
To: Andrew Donnellan; +Cc: Fabio Coatti, LKML, linux-wireless@vger.kernel.org
In-Reply-To: <942b4775-6027-2b99-84c7-37b7cde124b9@au1.ibm.com>
I have a similar problem on Gentoo. But in my case, it just can't load
firmware: "no suitable firmware found". I've tried to reinstall
firmware with no luck. Everything is ok with 4.8.6.
On Mon, Jan 2, 2017 at 6:42 PM, Andrew Donnellan
<andrew.donnellan@au1.ibm.com> wrote:
> On 02/01/17 21:12, Fabio Coatti wrote:
>>
>> Hi all,
>> I'm using kernel 4.9 and maybe half of the times I boot my laptop I get
>> the
>> error reported below, and the wifi does not work. I have to remove iwlwifi
>> (like
>> modprobe -r iwldvm iwlwifi) and insert it again to get things workig
>> again.
>> This seems a bit random, it does not happens all the times so it could be
>> a
>> timing issue or even a flaky hardware (unlikely, as I see only this issue
>> with
>> wifi, once it starts it works just fine)
>> I'm pretty sure to have seen the same behaviour at some point in 4.8.X
>> release, but right now I lost the related notes.
>> Environment:
>> Distro: gentoo
>> gcc 5.4.0
>> HW: Hewlett-Packard HP EliteBook Folio 9470m/18DF, BIOS 68IBD Ver. F.63
>> 04/26/2016
>> Intel(R) Core(TM) i5-3427U CPU @ 1.80GHz
>> Network controller: Intel Corporation Centrino Advanced-N 6235 (rev 24)
>>
>> Of course if more info is needed, just drop me a note.
>>
>> I'm not subscribed to mailing lists, so please keep me in CC: for any
>> information request.
>>
>> Many thanks.
>
>
> I've so far seen this once on my laptop, a Samsung NP540U3C (don't have it
> with me right now, so I'm not sure what the wifi chipset is), running with a
> Debian 4.9 kernel.
>
> --
> Andrew Donnellan OzLabs, ADL Canberra
> andrew.donnellan@au1.ibm.com IBM Australia Limited
>
^ permalink raw reply
* Re: [PATCH] brcmfmac: avoid writing channel out of allocated array
From: Rafał Miłecki @ 2017-01-03 15:49 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Kalle Valo, Franky Lin, Hante Meuleman, Pieter-Paul Giesberts,
Franky Lin, linux-wireless@vger.kernel.org,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
Rafał Miłecki
In-Reply-To: <CACna6rzBr0Co2o4Ym-7coVk1v5g1nvgDv6099oJVFr9drgU_Rw@mail.gmail.com>
On 3 January 2017 at 15:14, Rafa=C5=82 Mi=C5=82ecki <zajec5@gmail.com> wrot=
e:
> On 3 January 2017 at 14:19, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 3-1-2017 12:31, Rafa=C5=82 Mi=C5=82ecki wrote:
>>>>> + if (!channel) {
>>>>> + brcmf_err("Firmware reported unexpected channel=
%d\n",
>>>>> + ch.control_ch_num);
>>>>> + continue;
>>>>> + }
>>>> As stated above something is really off when this happens so should we
>>>> continue and try to make sense of what firmware provides or simply fai=
l.
>>> Well, I could image something like this happening and not being critica=
l.
>>> The simplest case: Broadcom team releases a new firmware which
>>> supports extra 5 GHz channels (e.g. due to the IEEE standard change).
>>> Why should we refuse to run & support all "old" channel just because of=
that?
>>
>> Fair enough. I was assuming we keep __wl_{2,5}ghz_channels up to date
>> with IEEE standard.
>>
>>> What do you mean by "make sense of what firmware provides"? Would kind
>>> of solution would you suggest?
>>
>> When the above assumption can be assured (by us) the only other scenario
>> would be a change in the firmware API where we wrongly interpret the
>> information retrieved. In this case all subsequent channels will likely
>> result in bogus or accidental matches hence it seems better to bail out
>> early.
>
> Good point, this actually gave me an idea for a small nice
> improvement. I remember I saw something like WL_VER in wl ioctl
> protocol, it was already bumped at some point by Broadcom, when
> chanspec format has changed. We should probably read this number from
> firmware and maybe refuse to run if version is newer than the one we
> know.
I was thinking about WLC_GET_VERSION and you seem to already have it
in brcmfmac under nam BRCMF_C_GET_VERSION. If you wish to be prepared
for firmware API change, I guess you should check version. It seems
brcmfmac supports 1 and 2.
On the other hand if adding firmware with incompatible API you may
want to have different directory or file names. I think this is what
Intel does. This allows one to have multiple firmwares in
/lib/firmware/ and switching between kernels freely.
--=20
Rafa=C5=82
^ permalink raw reply
* Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel
From: Mark Greer @ 2017-01-03 16:33 UTC (permalink / raw)
To: Geoff Lansberry
Cc: linux-wireless, Lauro Ramos Venancio, Aloisio Almeida Jr,
Samuel Ortiz, robh+dt, mark.rutland, netdev, devicetree,
linux-kernel, Justin Bronder, Jaret Cantu
In-Reply-To: <CAO7Z3WJa0goJ-VXc7dvyz8imZtqby6QsC0QNH+uRAE8LhxqU2w@mail.gmail.com>
[Please stop top-posting. Bottom-post only to these lists.]
Hi Geoff & happy new year.
On Tue, Dec 27, 2016 at 09:18:32AM -0500, Geoff Lansberry wrote:
> Mark - I will split this off soon.
OK
> In the meantime - here is some more info about how we use it.
>
> We do use NFC structures. I did find an interesting clue in that
> there are certain bottles that cause neard to segfault, I'm not sure
> what is different about them. We write a string, like
> "coppola_chardonnay_2015" to the bottles.
Off the top of my head, it could be the length of the text.
It would be useful to compare the data that works to the data
that doesn't work. Can you install NXP's 'TagInfo' app on a
smartphone and scan tags with working & non-working data?
You can email the data from the app to yourself, edit out
the cruft, and share here.
> Come to think of it, I
> haven't done anything special to make that an ndef record, just
> assumed that it would happen by default, I'll look into this further.
If you wrote the data using neard, it will be NDEF formatted.
Since it is working this well, it is virtually guaranteed that
the data is NDEF formatted.
> Also, I've been running neard with --plugin nfctype2. Just in case
> the problem was happening due to cycling through other tag types. It
> didn't seem to make any difference, but I have not gone back to
> default.
Good to know, thanks.
Mark
--
^ permalink raw reply
* [PATCH] brcmfmac: check if we can support used firmware API version
From: Rafał Miłecki @ 2017-01-03 16:11 UTC (permalink / raw)
To: Kalle Valo
Cc: Arend van Spriel, Franky Lin, Hante Meuleman,
Pieter-Paul Giesberts, Franky Lin, linux-wireless,
brcm80211-dev-list.pdl, Rafał Miłecki
From: Rafał Miłecki <rafal@milecki.pl>
Every new firmware API will most likely require changes in our code to
support it. Right now we support 2 versions only. Refuse to init if we
detect newer version.
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
Hi Arend,
I think you were concerned about possible firmware API changes. Please
review this patch, I hope it's a proper check for running unsupported
firmware version which could result in broken communication between host
driver and a device.
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 0babfc7..c69ae84 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -6816,6 +6816,11 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
brcmf_err("Failed to get D11 version (%d)\n", err);
goto priv_out;
}
+ if (io_type > BRCMU_D11AC_IOTYPE) {
+ brcmf_err("Unsupported IO version %d\n", io_type);
+ goto priv_out;
+ }
+
cfg->d11inf.io_type = (u8)io_type;
brcmu_d11_attach(&cfg->d11inf);
--
2.10.1
^ permalink raw reply related
* [PATCH next V2] brcmfmac: avoid writing channel out of allocated array
From: Rafał Miłecki @ 2017-01-03 16:49 UTC (permalink / raw)
To: Kalle Valo
Cc: Arend van Spriel, Franky Lin, Hante Meuleman,
Pieter-Paul Giesberts, Franky Lin, linux-wireless,
brcm80211-dev-list.pdl, Rafał Miłecki
In-Reply-To: <20170103083858.6981-1-zajec5@gmail.com>
From: Rafał Miłecki <rafal@milecki.pl>
Our code was assigning number of channels to the index variable by
default. If firmware reported channel we didn't predict this would
result in using that initial index value and writing out of array. This
never happened so far (we got a complete list of supported channels) but
it means possible memory corruption so we should handle it anyway.
This patch simply detects unexpected channel and ignores it.
As we don't try to create new entry now, it's also safe to drop hw_value
and center_freq assignment. For known channels we have these set anyway.
I decided to fix this issue by assigning NULL or a target channel to the
channel variable. This was one of possible ways, I prefefred this one as
it also avoids using channel[index] over and over.
Fixes: 58de92d2f95e ("brcmfmac: use static superset of channels for wiphy bands")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Add extra comment in code for not-found channel.
Make it clear this problem have never been seen so far
Explain why it's safe to drop extra assignments
Note & reason changing channel variable usage
---
.../broadcom/brcm80211/brcmfmac/cfg80211.c | 32 ++++++++++++----------
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 9c2c128..a16dd7b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5825,7 +5825,6 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
u32 i, j;
u32 total;
u32 chaninfo;
- u32 index;
pbuf = kzalloc(BRCMF_DCMD_MEDLEN, GFP_KERNEL);
@@ -5873,33 +5872,36 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
ch.bw == BRCMU_CHAN_BW_80)
continue;
- channel = band->channels;
- index = band->n_channels;
+ channel = NULL;
for (j = 0; j < band->n_channels; j++) {
- if (channel[j].hw_value == ch.control_ch_num) {
- index = j;
+ if (band->channels[j].hw_value == ch.control_ch_num) {
+ channel = &band->channels[j];
break;
}
}
- channel[index].center_freq =
- ieee80211_channel_to_frequency(ch.control_ch_num,
- band->band);
- channel[index].hw_value = ch.control_ch_num;
+ if (!channel) {
+ /* It seems firmware supports some channel we never
+ * considered. Something new in IEEE standard?
+ */
+ brcmf_err("Firmware reported unexpected channel %d\n",
+ ch.control_ch_num);
+ continue;
+ }
/* assuming the chanspecs order is HT20,
* HT40 upper, HT40 lower, and VHT80.
*/
if (ch.bw == BRCMU_CHAN_BW_80) {
- channel[index].flags &= ~IEEE80211_CHAN_NO_80MHZ;
+ channel->flags &= ~IEEE80211_CHAN_NO_80MHZ;
} else if (ch.bw == BRCMU_CHAN_BW_40) {
- brcmf_update_bw40_channel_flag(&channel[index], &ch);
+ brcmf_update_bw40_channel_flag(channel, &ch);
} else {
/* enable the channel and disable other bandwidths
* for now as mentioned order assure they are enabled
* for subsequent chanspecs.
*/
- channel[index].flags = IEEE80211_CHAN_NO_HT40 |
- IEEE80211_CHAN_NO_80MHZ;
+ channel->flags = IEEE80211_CHAN_NO_HT40 |
+ IEEE80211_CHAN_NO_80MHZ;
ch.bw = BRCMU_CHAN_BW_20;
cfg->d11inf.encchspec(&ch);
chaninfo = ch.chspec;
@@ -5907,11 +5909,11 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
&chaninfo);
if (!err) {
if (chaninfo & WL_CHAN_RADAR)
- channel[index].flags |=
+ channel->flags |=
(IEEE80211_CHAN_RADAR |
IEEE80211_CHAN_NO_IR);
if (chaninfo & WL_CHAN_PASSIVE)
- channel[index].flags |=
+ channel->flags |=
IEEE80211_CHAN_NO_IR;
}
}
--
2.10.1
^ permalink raw reply related
* Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
From: Luis R. Rodriguez @ 2017-01-03 17:59 UTC (permalink / raw)
To: Pavel Machek, Daniel Wagner, Tom Gundersen
Cc: Arend Van Spriel, Pali Rohár, Ming Lei, Luis R. Rodriguez,
Greg Kroah-Hartman, Kalle Valo, David Gnedt, Michal Kazior,
Daniel Wagner, Tony Lindgren, Sebastian Reichel, Ivaylo Dimitrov,
Aaro Koskinen, Takashi Iwai, David Woodhouse, Bjorn Andersson,
Grazvydas Ignotas, linux-kernel, linux-wireless, netdev
In-Reply-To: <20161226163559.GB27087@amd>
On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote:
>
> Right question is "should we solve it without user-space help"?
>
> Answer is no, too. Way too complex. Yes, it would be nice if hardware
> was designed in such a way that getting calibration data from kernel
> is easy, and if you design hardware, please design it like that. But
> N900 is not designed like that and getting the calibration through
> userspace looks like only reasonable solution.
Arend seems to have a better alternative in mind possible for other
devices which *can* probably pull of doing this easily and nicely,
given the nasty history of the usermode helper crap we should not
in any way discourage such efforts.
Arend -- please look at the firmware cache, it not a hash but a hash
table for an O(1) lookups would be a welcomed change, then it could
be repurposed for what you describe, I think the only difference is
you'd perhaps want a custom driver hook to fetch the calibration data
so the driver does whatever it needs.
> Now... how exactly to do that is other question. (But this is looks
> very reasonable. Maybe I'd add request_firmware_with_flags(, ...int
> flags), but.. that's a tiny detail.). But userspace needs to be
> involved.
No, no, we keep adding yet-another-exported symbol for requesting firmware,
instead of just adding a set of parameters possible and easily extending
functionality. Please review the patches posted on my last set which adds
a flexible API with only 2 calls, sync and async, and lets us customize
our requests using a parameter:
https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20161216-drvdata-v3-try3
This also documents the "usermode helper" properly and explains some of
the issues and limitations you will need to consider if you use it, its
one reason I'd highly encourage to consider an alternative as what Arend
is considering. *Iff* you insist on using the (now using the proper term,
as per the documentation update I am providing) "custom fallback mechanism"
I welcome such a change but I ask we *really* think this through well so
we avoid the stupid issues which have historically made the custom fallback
mechanism more of a nuisance for Linux distributions, users and developers.
To this end -- I ask you check out Daniel Wagner and Tom Gundersen's firmwared
work [0] which I referred you to in December. Although the drvdata API does
not yet use a custom fallback mechanism, after and its merged the goal here
would be to *only* support a clean custom fallback mechanism which aligns
itself *well* with firmwared or solutions like it. Your patch set then could
just become a patch set to add the custom fallback mechaism support to drvdata
API with the new options/prefernce you are looking for to be specified in
the new parameters, not a new exported symbol.
One of the cruxes we should consider addressing before the drvdata API gets a
custom fallback mechanism support added is that the usermode helper lock should
be replaced with a generic solution for the races it was intended to address:
use of the API on suspend/resume and implicitly later avoid a race on init. To
this end we should consider the same race for *other* real kernel "user mode
helpers", I've documented this on a wiki [1] which documents the *real*
kernel usermode helpers users, one of which was the kobject uevent which is
one of the fallback mechanisms.
I should also note that the idea of fallback mechanism using kobject uevents
should really suffice, in review with Johannes Berg at least, he seemed
convinced just letting either the upstream firmwared, a custom firmwared or
a custom userspace solution should be able to just monitor for uevents for
drvdata and do the right thing, this whole "custom fallback mechanism"
in retrospect seems not really needed as far as I can tell.
[0] https://github.com/teg/firmwared
[1] https://kernelnewbies.org/KernelProjects/usermode-helper-enhancements
Luis
^ permalink raw reply
* Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel
From: Geoff Lansberry @ 2017-01-03 18:35 UTC (permalink / raw)
To: Mark Greer
Cc: linux-wireless, Lauro Ramos Venancio, Aloisio Almeida Jr,
Samuel Ortiz, robh+dt, mark.rutland, netdev, devicetree,
linux-kernel, Justin Bronder, Jaret Cantu
In-Reply-To: <20170103163324.GA3184@animalcreek.com>
On Tue, Jan 3, 2017 at 11:33 AM, Mark Greer <mgreer@animalcreek.com> wrote:
> [Please stop top-posting. Bottom-post only to these lists.]
Sorry; gmail keeps baiting me to do it...
>
> Hi Geoff & happy new year.
>
> On Tue, Dec 27, 2016 at 09:18:32AM -0500, Geoff Lansberry wrote:
>> Mark - I will split this off soon.
>
> OK
Thanks for the reminder!
>
>> In the meantime - here is some more info about how we use it.
>>
>> We do use NFC structures. I did find an interesting clue in that
>> there are certain bottles that cause neard to segfault, I'm not sure
>> what is different about them. We write a string, like
>> "coppola_chardonnay_2015" to the bottles.
>
> Off the top of my head, it could be the length of the text.
> It would be useful to compare the data that works to the data
> that doesn't work. Can you install NXP's 'TagInfo' app on a
> smartphone and scan tags with working & non-working data?
> You can email the data from the app to yourself, edit out
> the cruft, and share here.
The data is always the same - and the tags are all the same. Only
difference is that the tag is physically different, and perhaps
orientation; distance from antenna to tag is fixed. I can't even
write the tags at all, so reading them will show blank. Also a minor
but significant detail, is that the tags are embedded in such a way
that the phone cannot get close enough to them to connect.
>
>> Come to think of it, I
>> haven't done anything special to make that an ndef record, just
>> assumed that it would happen by default, I'll look into this further.
>
> If you wrote the data using neard, it will be NDEF formatted.
> Since it is working this well, it is virtually guaranteed that
> the data is NDEF formatted.
OK, good.
>
>> Also, I've been running neard with --plugin nfctype2. Just in case
>> the problem was happening due to cycling through other tag types. It
>> didn't seem to make any difference, but I have not gone back to
>> default.
>
> Good to know, thanks.
>
> Mark
> --
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: document common IEEE 802.11 frequency properties
From: Rob Herring @ 2017-01-03 19:55 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Kalle Valo, linux-wireless, Martin Blumenstingl, Felix Fietkau,
Arnd Bergmann, devicetree, Rafał Miłecki
In-Reply-To: <20161228155955.25518-1-zajec5@gmail.com>
On Wed, Dec 28, 2016 at 04:59:54PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> This new file should be used for properties handled at higher level and
> so usable with all drivers.
Why is this needed? Where would this data normally come from?
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> .../devicetree/bindings/net/wireless/ieee80211.txt | 16 ++++++++++++++++
> 1 file changed, 16 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..c762769
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wireless/ieee80211.txt
> @@ -0,0 +1,16 @@
> +Common IEEE 802.11 properties
> +
> +This provides documentation of common properties that are handled by a proper
> +net layer and don't require extra driver code.
Not relavent to a binding. Bindings describe h/w.
> +
> +Optional properties:
> + - ieee80211-min-center-freq : minimal supported frequency in KHz
> + - ieee80211-max-center-freq : maximal supported frequency in KHz
> +
> +Example:
> +
> +pcie@0,0 {
> + reg = <0x0000 0 0 0 0>;
> + ieee80211-min-center-freq = <2437000>;
> + ieee80211-max-center-freq = <2457000>;
> +};
> --
> 2.10.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox