* [PATCH] ath9k: Prevent radar detection and spectral scan to be used concurrently
From: Benjamin Berg @ 2016-11-21 14:04 UTC (permalink / raw)
To: Kalle Valo
Cc: ath9k-devel, linux-wireless, Benjamin Berg, Mathias Kretschmer,
Simon Wunderlich
In the case that a spectral scan is enabled the PHY errors sent by the
hardware as part of the scanning might trigger the radar detection and
channels might be marked as 'unusable' incorrectly. This patch fixes
the issue by preventing the spectral scan to be enabled if DFS is used
and only analysing the PHY errors for DFS if radar detection is enabled.
Signed-off-by: Mathias Kretschmer <mathias.kretschmer@fit.fraunhofer.de>
Signed-off-by: Benjamin Berg <benjamin@sipsolutions.net>
Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
---
drivers/net/wireless/ath/ath9k/common-spectral.c | 23 +++++++++++++++++++----
drivers/net/wireless/ath/ath9k/main.c | 6 ++++++
drivers/net/wireless/ath/ath9k/recv.c | 7 +++++--
3 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/common-spectral.c b/drivers/net/wireless/ath/ath9k/common-spectral.c
index e2512d5..8444d6d 100644
--- a/drivers/net/wireless/ath/ath9k/common-spectral.c
+++ b/drivers/net/wireless/ath/ath9k/common-spectral.c
@@ -802,16 +802,27 @@ static ssize_t write_file_spec_scan_ctl(struct file *file,
size_t count, loff_t *ppos)
{
struct ath_spec_scan_priv *spec_priv = file->private_data;
+ struct ath_hw *ah = spec_priv->ah;
+ struct ath_softc *sc = ah->hw->priv;
struct ath_common *common = ath9k_hw_common(spec_priv->ah);
char buf[32];
ssize_t len;
+ ssize_t result = count;
if (IS_ENABLED(CONFIG_ATH9K_TX99))
return -EOPNOTSUPP;
+ mutex_lock(&sc->mutex);
+ if (ah->hw->conf.radar_enabled) {
+ result = -EINVAL;
+ goto unlock;
+ }
+
len = min(count, sizeof(buf) - 1);
- if (copy_from_user(buf, user_buf, len))
- return -EFAULT;
+ if (copy_from_user(buf, user_buf, len)) {
+ result = -EFAULT;
+ goto unlock;
+ }
buf[len] = '\0';
@@ -830,10 +841,14 @@ static ssize_t write_file_spec_scan_ctl(struct file *file,
ath9k_cmn_spectral_scan_config(common, spec_priv, SPECTRAL_DISABLED);
ath_dbg(common, CONFIG, "spectral scan: disabled\n");
} else {
- return -EINVAL;
+ result = -EINVAL;
+ goto unlock;
}
- return count;
+unlock:
+ mutex_unlock(&sc->mutex);
+
+ return result;
}
static const struct file_operations fops_spec_scan_ctl = {
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index e9f32b5..62b86fb 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1459,6 +1459,12 @@ static int ath9k_config(struct ieee80211_hw *hw, u32 changed)
if (!ath9k_is_chanctx_enabled() && (changed & IEEE80211_CONF_CHANGE_CHANNEL)) {
ctx->offchannel = !!(conf->flags & IEEE80211_CONF_OFFCHANNEL);
ath_chanctx_set_channel(sc, ctx, &hw->conf.chandef);
+
+ /* We need to ensure that spectral scan is disabled. */
+ if (conf->radar_enabled &&
+ sc->spec_priv.spectral_mode != SPECTRAL_DISABLED)
+ ath9k_cmn_spectral_scan_config(common, &sc->spec_priv,
+ SPECTRAL_DISABLED);
}
mutex_unlock(&sc->mutex);
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 6697342..ce34add 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -867,8 +867,11 @@ static int ath9k_rx_skb_preprocess(struct ath_softc *sc,
* can be dropped.
*/
if (rx_stats->rs_status & ATH9K_RXERR_PHY) {
- ath9k_dfs_process_phyerr(sc, hdr, rx_stats, rx_status->mactime);
- if (ath_cmn_process_fft(&sc->spec_priv, hdr, rx_stats, rx_status->mactime))
+ if (hw->conf.radar_enabled)
+ ath9k_dfs_process_phyerr(sc, hdr, rx_stats,
+ rx_status->mactime);
+ else if (ath_cmn_process_fft(&sc->spec_priv, hdr, rx_stats,
+ rx_status->mactime))
RX_STAT_INC(rx_spectral);
return -EINVAL;
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] ath9k: Prevent radar detection and spectral scan to be used concurrently
From: Michal Kazior @ 2016-11-21 14:16 UTC (permalink / raw)
To: Benjamin Berg
Cc: Kalle Valo, ath9k-devel@lists.ath9k.org, linux-wireless,
Mathias Kretschmer, Simon Wunderlich
In-Reply-To: <20161121140423.24367-1-benjamin@sipsolutions.net>
On 21 November 2016 at 15:04, Benjamin Berg <benjamin@sipsolutions.net> wro=
te:
> In the case that a spectral scan is enabled the PHY errors sent by the
> hardware as part of the scanning might trigger the radar detection and
> channels might be marked as 'unusable' incorrectly. This patch fixes
> the issue by preventing the spectral scan to be enabled if DFS is used
> and only analysing the PHY errors for DFS if radar detection is enabled.
According to the comment in ath_cmn_process_fft() this doesn't seem to
be necessary for all chips:
515 /* AR9280 and before report via ATH9K_PHYERR_RADAR,
AR93xx and newer
516 * via ATH9K_PHYERR_SPECTRAL. Haven't seen
ATH9K_PHYERR_FALSE_RADAR_EXT
517 * yet, but this is supposed to be possible as well.
518 */
519 if (rs->rs_phyerr !=3D ATH9K_PHYERR_RADAR &&
520 rs->rs_phyerr !=3D ATH9K_PHYERR_FALSE_RADAR_EXT &&
521 rs->rs_phyerr !=3D ATH9K_PHYERR_SPECTRAL)
522 return 0;
Micha=C5=82
^ permalink raw reply
* Re: [PATCH 4.9.0-rc5] AR9300 calibration problems with antenna selected
From: Matthias May @ 2016-11-21 14:11 UTC (permalink / raw)
To: Krzysztof Hałasa
Cc: miaoqing, ath9k-devel, Kalle Valo, linux-wireless, ath9k-devel,
netdev, linux-kernel
In-Reply-To: <m3d1hp2b0z.fsf@t19.piap.pl>
On 21/11/16 14:54, Krzysztof Hałasa wrote:
> miaoqing@codeaurora.org writes:
>
>>> rmmod ath9k
>>> modprobe ath9k
>>> iw dev wlan0 set type ibss
>>> iw phy phyX set antenna 2
>>
>> 2 is a bad mask. We use bitmap, the valid masks are 1, 3, 7.
>
> Thanks for your response.
>
> I have two antenna connections (and a single antenna). Is it possible to
> select the secondary antenna connector only? How?
>
No this is not really possible.
We have been playing around with this two, three years ago with this.
There are just too many things which rely on chain0
Noise calibration, DFS, Temperature measurement, Spectrum measurement
are just a few of the things I remember which don't work realiably anymore.
See [1] for more.
BR
Matthias
[1]
http://ath9k-devel.ath9k.narkive.com/QZcobwy1/ath9k-deaf-qca9558-when-setting-rxchainmask#post1
^ permalink raw reply
* Re: [PATCH] ath9k: Prevent radar detection and spectral scan to be used concurrently
From: Zefir Kurtisi @ 2016-11-21 14:41 UTC (permalink / raw)
To: Benjamin Berg, Kalle Valo
Cc: ath9k-devel, linux-wireless, Mathias Kretschmer, Simon Wunderlich
In-Reply-To: <20161121140423.24367-1-benjamin@sipsolutions.net>
[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]
On 11/21/2016 03:04 PM, Benjamin Berg wrote:
> In the case that a spectral scan is enabled the PHY errors sent by the
> hardware as part of the scanning might trigger the radar detection and
> channels might be marked as 'unusable' incorrectly. This patch fixes
> the issue by preventing the spectral scan to be enabled if DFS is used
> and only analysing the PHY errors for DFS if radar detection is enabled.
>
> [...]
>From the relevant source code portion in channel.c:ath_set_channel()
80 /* Enable radar pulse detection if on a DFS channel. Spectral
81 * scanning and radar detection can not be used concurrently.
82 */
83 if (hw->conf.radar_enabled) {
84 u32 rxfilter;
85
86 rxfilter = ath9k_hw_getrxfilter(ah);
87 rxfilter |= ATH9K_RX_FILTER_PHYRADAR |
88 ATH9K_RX_FILTER_PHYERR;
89 ath9k_hw_setrxfilter(ah, rxfilter);
90 ath_dbg(common, DFS, "DFS enabled at freq %d\n",
91 chan->center_freq);
92 } else {
93 /* perform spectral scan if requested. */
94 if (test_bit(ATH_OP_SCANNING, &common->op_flags) &&
95 sc->spec_priv.spectral_mode == SPECTRAL_CHANSCAN)
96 ath9k_cmn_spectral_scan_trigger(common, &sc->spec_priv);
97 }
it seems that spectral can't ever be activated while operating on a DFS channel.
If you need to catch the opposite case, i.e. prevent feeding pseudo-radar pulses
into the pattern detector, you just need to ensure that they depend on
hw->conf.radar_enabled. A patch like the attached one should be enough.
Cheers,
Zefir
[-- Attachment #2: 0001-ath9k-feed-DFS-detector-only-if-operating-on-radar-c.patch --]
[-- Type: text/x-patch, Size: 986 bytes --]
>From c24edf82e1f509490ba9dd3e34eec3ac3b309321 Mon Sep 17 00:00:00 2001
From: Zefir Kurtisi <zefir.kurtisi@neratec.com>
Date: Mon, 21 Nov 2016 15:33:45 +0100
Subject: [PATCH] ath9k: feed DFS detector only if operating on radar channel
---
drivers/net/wireless/ath/ath9k/recv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 6697342..e4701a7 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -867,7 +867,8 @@ static int ath9k_rx_skb_preprocess(struct ath_softc *sc,
* can be dropped.
*/
if (rx_stats->rs_status & ATH9K_RXERR_PHY) {
- ath9k_dfs_process_phyerr(sc, hdr, rx_stats, rx_status->mactime);
+ if (hw->conf.radar_enabled)
+ ath9k_dfs_process_phyerr(sc, hdr, rx_stats, rx_status->mactime);
if (ath_cmn_process_fft(&sc->spec_priv, hdr, rx_stats, rx_status->mactime))
RX_STAT_INC(rx_spectral);
--
2.7.4
^ permalink raw reply related
* Re: scheduled scan interval
From: Luca Coelho @ 2016-11-21 15:08 UTC (permalink / raw)
To: Arend Van Spriel, Johannes Berg; +Cc: linux-wireless
In-Reply-To: <e176de78-6ab3-ad83-5e9d-082ceb85d786@broadcom.com>
Hi Arend,
On Mon, 2016-11-21 at 13:03 +0100, Arend Van Spriel wrote:
> On 21-11-2016 12:30, Arend Van Spriel wrote:
> > On 21-11-2016 12:19, Arend Van Spriel wrote:
> > > Hi Johannes, Luca,
> > >
> > > The gscan work made me look at scheduled scan and the implementation of
> > > it in brcmfmac. The driver ignored the interval parameter from
> > > user-space. Now I am fixing that. One thing is that our firmware has a
> > > minimum interval which can not be indicated in struct wiphy. The other
> > > issue is how the maximum interval is used in the nl80211.c.
> > >
> > > In nl80211_parse_sched_scan_plans() it is used against value passed in
> > > NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
> > > For the first one it caps the value to the maximum, but for the second
> > > one it returns -EINVAL. I suspect this is done because maximum interval
> > > was introduced with schedule scan plans, but it feels inconsistent.
> >
> > It also maybe simply wrong to cap. At least brcmfmac does not set the
> > maximum so it will always get interval being zero. Maybe better to do:
> >
> > if (wiphy->max_sched_scan_plan_interval &&
> > request->scan_plans[0].interval >
> > wiphy->max_sched_scan_plan_interval)
> > return -EINVAL;
> >
> > > Thoughts?
>
> Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
> struct sched_scan_request::interval was specified in milliseconds! Below
> the drivers that I see having scheduled scan support:
>
> iwlmvm: cap interval, convert to seconds.
> ath6kl: cap to 1sec minimum, no max check, convert to seconds.
> wl12xx: no checking in driver, fw need milliseconds.
> wl18xx: no checking in driver, fw need milliseconds.
>
> The milliseconds conversion seems to be taken care of by multiplying
> with MSEC_PER_SEC in wl{12,18}xx drivers.
>
> It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
> other drivers will get interval of zero which only ath6kl handles.
With the introduction of scheduled scan plans, we sort of deprecated
the "generic" scheduled scan interval. It doesn't make sense to have
both passed at the same time, so nl80211 forbids
NL80211_ATTR_SCHED_SCAN_INTERVAL if we pass
NL80211_ATTR_SCHED_SCAN_PLANS.
The original NL80211_ATTR_SCHED_SCAN_INTERVAL was specified in msecs,
which is silly because we can never get millisecond accuracy in this.
Thus, in the plans API, we decided to use seconds instead (because it
makes much more sense). Additionally, the interval is considered
"advisory", because the FW may not be able guarantee the exact
intervals (for instance, the iwlwifi driver actually starts the
interval timer after scan completion, so if you specify 10 seconds
intervals, in practice they'll be 13-14 seconds).
I'm not sure I'm answering your question, because I'm also not sure I
understood the question. :)
--
Cheers,
Luca.
^ permalink raw reply
* Re: [ath9k-devel] [PATCH] ath9k: Prevent radar detection and spectral scan to be used concurrently
From: Michal Kazior @ 2016-11-21 15:10 UTC (permalink / raw)
To: Zefir Kurtisi
Cc: Benjamin Berg, Kalle Valo, ath9k-devel@lists.ath9k.org,
linux-wireless
In-Reply-To: <7d04126f-0f24-b4a4-6d13-6f52046d7671@neratec.com>
On 21 November 2016 at 15:41, Zefir Kurtisi <zefir.kurtisi@neratec.com> wro=
te:
> On 11/21/2016 03:04 PM, Benjamin Berg wrote:
>> In the case that a spectral scan is enabled the PHY errors sent by the
>> hardware as part of the scanning might trigger the radar detection and
>> channels might be marked as 'unusable' incorrectly. This patch fixes
>> the issue by preventing the spectral scan to be enabled if DFS is used
>> and only analysing the PHY errors for DFS if radar detection is enabled.
>>
>> [...]
>
> From the relevant source code portion in channel.c:ath_set_channel()
>
> 80 /* Enable radar pulse detection if on a DFS channel. Spectral
> 81 * scanning and radar detection can not be used concurrently.
> 82 */
> 83 if (hw->conf.radar_enabled) {
> 84 u32 rxfilter;
> 85
> 86 rxfilter =3D ath9k_hw_getrxfilter(ah);
> 87 rxfilter |=3D ATH9K_RX_FILTER_PHYRADAR |
> 88 ATH9K_RX_FILTER_PHYERR;
> 89 ath9k_hw_setrxfilter(ah, rxfilter);
> 90 ath_dbg(common, DFS, "DFS enabled at freq %d\n",
> 91 chan->center_freq);
> 92 } else {
> 93 /* perform spectral scan if requested. */
> 94 if (test_bit(ATH_OP_SCANNING, &common->op_flags) &&
> 95 sc->spec_priv.spectral_mode =3D=3D SPECTRAL_CH=
ANSCAN)
> 96 ath9k_cmn_spectral_scan_trigger(common, &sc->s=
pec_priv);
> 97 }
>
> it seems that spectral can't ever be activated while operating on a DFS c=
hannel.
>
> If you need to catch the opposite case, i.e. prevent feeding pseudo-radar=
pulses
> into the pattern detector, you just need to ensure that they depend on
> hw->conf.radar_enabled. A patch like the attached one should be enough.
Good point. I guess set_channel could be oversimplified as well. I
mean, it makes sense to consider radar and spectral mutually exclusive
if they use the same phyerr code. However some chips actually seem (as
per the comment I mentioned) to distinguish the two so I don't know if
the "mutually exclusive" is true for all chips per se. Just thinking
out loud.
I also wonder if calling ieee80211_radar_detect() should have any
effect if there are no radar operated interfaces in the first place?
Micha=C5=82
^ permalink raw reply
* Re: wl1251 & mac address & calibration data
From: Pali Rohár @ 2016-11-21 15:51 UTC (permalink / raw)
To: Kalle Valo, Pavel Machek, Ivaylo Dimitrov, Sebastian Reichel,
Aaro Koskinen, Tony Lindgren
Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <201611111820.52072@pali>
On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
> Hi! I will open discussion about mac address and calibration data for
> wl1251 wireless chip again...
>
> Problem: Mac address & calibration data for wl1251 chip on Nokia N900
> are stored on second nand partition (mtd1) in special proprietary format
> which is used only for Nokia N900 (probably on N8x0 and N9 too).
> Wireless driver wl1251.ko cannot work without mac address and
> calibration data.
>
> Absence of mac address cause that driver generates random mac address at
> every kernel boot which has couple of problems (unstable identifier of
> wireless device due to udev permanent storage rules; unpredictable
> behaviour for dhcp mac address assignment, mac address filtering, ...).
>
> Currently there is no way to set (permanent) mac address for network
> interface from userspace. And it does not make sense to implement in
> linux kernel large parser for proprietary format of second nand
> partition where is mac address stored only for one device -- Nokia N900.
>
> Driver wl1251.ko loads calibration data via request_firmware() for file
> wl1251-nvs.bin. There are some "example" calibration file in linux-
> firmware repository, but it is not suitable for normal usage as real
> calibration data are per-device specific.
>
> So questions are:
>
> 1) How to set mac address from userspace for that wl1251 interface? In
> userspace I can write parser for that proprietary format of nand
> partition and extract mac address from it
Proposed solutions for 1)
* Introduce new IOCL for setting that permanent mac address from
userspace. Currently we have IOCL for get request
* Use request_firmware() (with flag from 2)) to ask for mac address from
userspace. This is already used by wl12xx driver (as mac address is
part of calibration data firmware file)
* Allow to set mac address via sysfs file, e.g.
/sys/class/ieee80211/phy0/macaddress
> 2) How to send calibration data to wl1251 driver? Those are again stored
> in proprietary format and I can write userspace parser for it.
Proposed solution for 2)
Introduce new flag for request_firmware(), so it first try to use
userspace helper for loading firmware file with possibility to fallback
to direct VFS access.
So... what do you think about it?
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Break-it testing for wifi
From: Ben Greear @ 2016-11-21 16:10 UTC (permalink / raw)
To: linux-wireless@vger.kernel.org
Hello!
I am thinking about adding some sort of framework to wpa_supplicant and/or the
mac80211 stack to allow purposefully creating bad station behaviour in order to
test robustness of APs.
Some ideas so far:
1) Allow supplicant to do bad state-machine transitions (start 4-way before associating, for instance).
2) Randomly corrupt mgt frames in driver and/or mac80211 stack and/or supplicant.
3) Possibly allow user to make specific corruptions. This would probably be in supplicant
only, and I am not sure how this would be configured. Maybe allow user to over-ride
existing IEs and add bogus ones of their own choosing.
4) Maybe some specific tests like putting in over-flow sized lengths of IEs.
Has anyone done anything similar they would like to share?
Johannes: Any interest in having such a framework in upstream kernels?
Any other ideas for how to improve this feature set?
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [ath9k-devel] [PATCH] ath9k: Prevent radar detection and spectral scan to be used concurrently
From: Zefir Kurtisi @ 2016-11-21 16:16 UTC (permalink / raw)
To: Michal Kazior
Cc: Benjamin Berg, Kalle Valo, ath9k-devel@lists.ath9k.org,
linux-wireless
In-Reply-To: <CA+BoTQm5do0pfMpQ=ua=Q3Uomd2jV6_xbRFK=BMzUi-D5z7C+A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3091 bytes --]
On 11/21/2016 04:10 PM, Michal Kazior wrote:
> On 21 November 2016 at 15:41, Zefir Kurtisi <zefir.kurtisi@neratec.com> wrote:
>> On 11/21/2016 03:04 PM, Benjamin Berg wrote:
>>> In the case that a spectral scan is enabled the PHY errors sent by the
>>> hardware as part of the scanning might trigger the radar detection and
>>> channels might be marked as 'unusable' incorrectly. This patch fixes
>>> the issue by preventing the spectral scan to be enabled if DFS is used
>>> and only analysing the PHY errors for DFS if radar detection is enabled.
>>>
>>> [...]
>>
>> From the relevant source code portion in channel.c:ath_set_channel()
>>
>> 80 /* Enable radar pulse detection if on a DFS channel. Spectral
>> 81 * scanning and radar detection can not be used concurrently.
>> 82 */
>> 83 if (hw->conf.radar_enabled) {
>> 84 u32 rxfilter;
>> 85
>> 86 rxfilter = ath9k_hw_getrxfilter(ah);
>> 87 rxfilter |= ATH9K_RX_FILTER_PHYRADAR |
>> 88 ATH9K_RX_FILTER_PHYERR;
>> 89 ath9k_hw_setrxfilter(ah, rxfilter);
>> 90 ath_dbg(common, DFS, "DFS enabled at freq %d\n",
>> 91 chan->center_freq);
>> 92 } else {
>> 93 /* perform spectral scan if requested. */
>> 94 if (test_bit(ATH_OP_SCANNING, &common->op_flags) &&
>> 95 sc->spec_priv.spectral_mode == SPECTRAL_CHANSCAN)
>> 96 ath9k_cmn_spectral_scan_trigger(common, &sc->spec_priv);
>> 97 }
>>
>> it seems that spectral can't ever be activated while operating on a DFS channel.
>>
>> If you need to catch the opposite case, i.e. prevent feeding pseudo-radar pulses
>> into the pattern detector, you just need to ensure that they depend on
>> hw->conf.radar_enabled. A patch like the attached one should be enough.
>
> Good point. I guess set_channel could be oversimplified as well. I
> mean, it makes sense to consider radar and spectral mutually exclusive
> if they use the same phyerr code. However some chips actually seem (as
> per the comment I mentioned) to distinguish the two so I don't know if
> the "mutually exclusive" is true for all chips per se. Just thinking
> out loud.
>
You're right, blindly feeding PHYERR frames to spectral when spectral is not
enabled is as wrong as feeding them to the dfs-detector when not on a radar
channel. Therefore, the patch I proposed should be extended to make calling
ath_cmn_process_fft() depending on
a) not operating on radar channel, and
b) spectral scan is enabled
Updated patch attached as proposal.
> I also wonder if calling ieee80211_radar_detect() should have any
> effect if there are no radar operated interfaces in the first place?
>
True, calling ieee80211_radar_detect() for a non-DFS channel is (or should be)
ignored by mac. But feeding the dfs-detector with invalid pulses causes quite some
computational overhead - dropping them if no radar detection is required is a good
thing to do.
Cheers,
Zefir
[-- Attachment #2: 0001-ath9k-feed-DFS-detector-only-if-operating-on-radar-c.patch --]
[-- Type: text/x-patch, Size: 1455 bytes --]
>From aaa28dcaf0879c6bfc7be96077c79894bb230dfb Mon Sep 17 00:00:00 2001
From: Zefir Kurtisi <zefir.kurtisi@neratec.com>
Date: Mon, 21 Nov 2016 15:33:45 +0100
Subject: [PATCH] ath9k: feed DFS detector only if operating on radar channel
---
drivers/net/wireless/ath/ath9k/recv.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 6697342..48f8af1 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -867,10 +867,21 @@ static int ath9k_rx_skb_preprocess(struct ath_softc *sc,
* can be dropped.
*/
if (rx_stats->rs_status & ATH9K_RXERR_PHY) {
- ath9k_dfs_process_phyerr(sc, hdr, rx_stats, rx_status->mactime);
- if (ath_cmn_process_fft(&sc->spec_priv, hdr, rx_stats, rx_status->mactime))
+ /*
+ * DFS and spectral are mutually exclusive
+ *
+ * Since some chips use RXERR_PHY as indication for both, we
+ * need to double check which feature is enabled to prevent
+ * feeding spectral or dfs-detector with wrong frames.
+ */
+ if (hw->conf.radar_enabled) {
+ ath9k_dfs_process_phyerr(sc, hdr, rx_stats,
+ rx_status->mactime);
+ } else if (sc->spec_priv.spectral_mode != SPECTRAL_DISABLED &&
+ ath_cmn_process_fft(&sc->spec_priv, hdr, rx_stats,
+ rx_status->mactime)) {
RX_STAT_INC(rx_spectral);
-
+ }
return -EINVAL;
}
--
2.7.4
^ permalink raw reply related
* Re: Break-it testing for wifi
From: Mohammed Shafi Shajakhan @ 2016-11-21 16:28 UTC (permalink / raw)
To: Ben Greear; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <a60dac22-655e-ea93-61c2-73db05a47572@candelatech.com>
Hi Ben,
just googled out 'wifi fuzzy testing' and found something relevant
as below
https://www.blackhat.com/presentations/bh-europe-07/Butti/Presentation/bh-eu-07-Butti.pdf
regards,
shafi
On Mon, Nov 21, 2016 at 08:10:37AM -0800, Ben Greear wrote:
> Hello!
>
> I am thinking about adding some sort of framework to wpa_supplicant and/or the
> mac80211 stack to allow purposefully creating bad station behaviour in order to
> test robustness of APs.
>
> Some ideas so far:
>
> 1) Allow supplicant to do bad state-machine transitions (start 4-way before associating, for instance).
>
> 2) Randomly corrupt mgt frames in driver and/or mac80211 stack and/or supplicant.
>
> 3) Possibly allow user to make specific corruptions. This would probably be in supplicant
> only, and I am not sure how this would be configured. Maybe allow user to over-ride
> existing IEs and add bogus ones of their own choosing.
>
> 4) Maybe some specific tests like putting in over-flow sized lengths of IEs.
>
> Has anyone done anything similar they would like to share?
>
> Johannes: Any interest in having such a framework in upstream kernels?
>
> Any other ideas for how to improve this feature set?
>
> Thanks,
> Ben
>
> --
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc http://www.candelatech.com
>
^ permalink raw reply
* Re: [PATCH] rtl8xxxu: Fix failure to reconnect to AP
From: Jes Sorensen @ 2016-11-21 16:57 UTC (permalink / raw)
To: Barry Day; +Cc: Kalle Valo, linux-wireless
In-Reply-To: <wrfjoa1iyz1e.fsf@redhat.com>
Jes Sorensen <Jes.Sorensen@redhat.com> writes:
> Barry Day <briselec@gmail.com> writes:
>> The rtl8192e and rtl8723 fail to reconnect to an AP after being
>> disconnected. Ths patch fixes that without affecting the rtl8192cu.
>> I don't have a rtl8723 to test but it has been tested on a rtl8192eu.
>> After going through the orginal realtek code for the rtl8723, I am
>> confident the patch is applicable to both.
>>
>> Signed-off-by: Barry Day <briselec@gmail.com>
>> ---
>> rtl8xxxu_core.c | 18 ++++++++++++++----
>> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> Hi Barry,
>
> Thank you for the patch. There are a couple of items which I am not
> 100% sure about the order of.
>
>> diff --git a/rtl8xxxu_core.c b/rtl8xxxu_core.c
>> index 04141e5..6ac10d2 100644
>> --- a/rtl8xxxu_core.c
>> +++ b/rtl8xxxu_core.c
>> @@ -4372,17 +4372,25 @@ void rtl8xxxu_gen1_report_connect(struct rtl8xxxu_priv *priv,
>> void rtl8xxxu_gen2_report_connect(struct rtl8xxxu_priv *priv,
>> u8 macid, bool connect)
>> {
>> + u8 val8;
>> struct h2c_cmd h2c;
>>
>> memset(&h2c, 0, sizeof(struct h2c_cmd));
>>
>> h2c.media_status_rpt.cmd = H2C_8723B_MEDIA_STATUS_RPT;
>> - if (connect)
>> + if (connect) {
>> h2c.media_status_rpt.parm |= BIT(0);
>> - else
>> - h2c.media_status_rpt.parm &= ~BIT(0);
>> + rtl8xxxu_gen2_h2c_cmd(priv, &h2c,
>> + sizeof(h2c.media_status_rpt));
>> + } else {
>> + val8 = rtl8xxxu_read8(priv, REG_BEACON_CTRL);
>> + val8 &= ~BEACON_FUNCTION_ENABLE;
>> +
>> + rtl8xxxu_write8(priv, REG_BEACON_CTRL, val8);
>> + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0x00);
>> + rtl8xxxu_write8(priv, REG_DUAL_TSF_RST, (BIT(0) | BIT(1)));
>> + }
>>
>> - rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.media_status_rpt));
>> }
Barry,
So looking at this again, I am pretty sure you will break monitor mode
with this. By setting REG_RXFLTMAP2 to 0x0000 you stop reception of all
data frames.
The other thing here is that you change removes the part notifying the
firmware that we disconnected since you now only send the
media_start_rpt command on connect, but not on disconnect.
I am curious if the problem goes away if you simply add the BEACON_CTRL
and REG_DUAL_TSF_RST parts?
>
> This only affects 8192eu and not 8192cu - we left RXFLTMAP2 out of here
> on purpose for monitor mode, but you now disable it for 8192eu/8723bu.
>
>> void rtl8xxxu_gen1_init_aggregation(struct rtl8xxxu_priv *priv)
>> @@ -4515,6 +4523,8 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>> sgi = 1;
>> rcu_read_unlock();
>>
>> + rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0xffff);
>> +
>> priv->fops->update_rate_mask(priv, ramask, sgi);
>>
>> rtl8xxxu_write8(priv, REG_BCN_MAX_ERR, 0xff);
I believe this change only matters because you disable RXFLTMAP2
above. If we really have to write to RXFLTMAP2 to make this work, I
suspect we need to keep some sort of state information.
I would also be curious if RXFLTMAP2 gets reset somehow by the firmware,
and we do not account for that.
Cheers,
Jes
^ permalink raw reply
* Re: scheduled scan interval
From: Dave Taht @ 2016-11-21 16:59 UTC (permalink / raw)
To: Luca Coelho; +Cc: Arend Van Spriel, Johannes Berg, linux-wireless
In-Reply-To: <1479740918.2517.28.camel@coelho.fi>
On Mon, Nov 21, 2016 at 7:08 AM, Luca Coelho <luca@coelho.fi> wrote:
> Hi Arend,
>
> On Mon, 2016-11-21 at 13:03 +0100, Arend Van Spriel wrote:
>> On 21-11-2016 12:30, Arend Van Spriel wrote:
>> > On 21-11-2016 12:19, Arend Van Spriel wrote:
>> > > Hi Johannes, Luca,
>> > >
>> > > The gscan work made me look at scheduled scan and the implementation=
of
>> > > it in brcmfmac. The driver ignored the interval parameter from
>> > > user-space. Now I am fixing that. One thing is that our firmware has=
a
>> > > minimum interval which can not be indicated in struct wiphy. The oth=
er
>> > > issue is how the maximum interval is used in the nl80211.c.
>> > >
>> > > In nl80211_parse_sched_scan_plans() it is used against value passed =
in
>> > > NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVA=
L.
>> > > For the first one it caps the value to the maximum, but for the seco=
nd
>> > > one it returns -EINVAL. I suspect this is done because maximum inter=
val
>> > > was introduced with schedule scan plans, but it feels inconsistent.
>> >
>> > It also maybe simply wrong to cap. At least brcmfmac does not set the
>> > maximum so it will always get interval being zero. Maybe better to do:
>> >
>> > if (wiphy->max_sched_scan_plan_interval &&
>> > request->scan_plans[0].interval >
>> > wiphy->max_sched_scan_plan_interval)
>> > return -EINVAL;
>> >
>> > > Thoughts?
>>
>> Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
>> struct sched_scan_request::interval was specified in milliseconds! Below
>> the drivers that I see having scheduled scan support:
>>
>> iwlmvm: cap interval, convert to seconds.
>> ath6kl: cap to 1sec minimum, no max check, convert to seconds.
>> wl12xx: no checking in driver, fw need milliseconds.
>> wl18xx: no checking in driver, fw need milliseconds.
>>
>> The milliseconds conversion seems to be taken care of by multiplying
>> with MSEC_PER_SEC in wl{12,18}xx drivers.
>>
>> It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
>> other drivers will get interval of zero which only ath6kl handles.
>
> With the introduction of scheduled scan plans, we sort of deprecated
> the "generic" scheduled scan interval. It doesn't make sense to have
> both passed at the same time, so nl80211 forbids
> NL80211_ATTR_SCHED_SCAN_INTERVAL if we pass
> NL80211_ATTR_SCHED_SCAN_PLANS.
>
> The original NL80211_ATTR_SCHED_SCAN_INTERVAL was specified in msecs,
> which is silly because we can never get millisecond accuracy in this.
> Thus, in the plans API, we decided to use seconds instead (because it
> makes much more sense). Additionally, the interval is considered
> "advisory", because the FW may not be able guarantee the exact
> intervals (for instance, the iwlwifi driver actually starts the
> interval timer after scan completion, so if you specify 10 seconds
> intervals, in practice they'll be 13-14 seconds).
>
> I'm not sure I'm answering your question, because I'm also not sure I
> understood the question. :)
I'm not sure if I understand the discussion and hooks myself, but
recently fixes for doing channel scans saner from userspace ended up
here, after some discussion.
https://bugzilla.gnome.org/show_bug.cgi?id=3D766482
Anything that can reduce the impact of this behavior, I'm for!
http://www.taht.net/~d/channel_scans_destroying_latency_under_load_for_10s.=
png
>
> --
> Cheers,
> Luca.
--=20
Dave T=C3=A4ht
Let's go make home routers and wifi faster! With better software!
http://blog.cerowrt.org
^ permalink raw reply
* Re: [PATCH 0/7] rtl8xxxu: Pending patches
From: Jes Sorensen @ 2016-11-21 16:59 UTC (permalink / raw)
To: Barry Day; +Cc: linux-wireless, kvalo, Larry.Finger
In-Reply-To: <20161120024257.GA13169@testbox>
Barry Day <briselec@gmail.com> writes:
> On Sat, Nov 19, 2016 at 06:53:42PM -0500, Jes Sorensen wrote:
>> Barry Day <briselec@gmail.com> writes:
>> > On Fri, Nov 18, 2016 at 09:00:10PM -0500, Jes Sorensen wrote:
>> >> Barry Day <briselec@gmail.com> writes:
>> >> > On Fri, Nov 18, 2016 at 04:44:21PM -0500, Jes.Sorensen@redhat.com wrote:
>> >> >> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>> >> >>
>> >> >> Kalle,
>> >> >>
>> >> >> Please find attached a number of patches for the rtl8xxxu
>> >> >> driver.
>> >> >>
>> >> >> The issues reported with wpa_supplicant on 8723bu still needs further
>> >> >> investigation.
>> >> >>
>> >> >
>> >> > The patch I posted that you want tested more will also fix the
>> >> > wpa_supplicant issue. Currently I'm looking at why the tx rate is not
>> >> > what it should be. I feel fixing that first will be beneficial for
>> >> > fixing any other issues.
>> >>
>> >> Interesting, I was thinking that might be the case. I do want to dig
>> >> into this further to understand it better. If we use your solution I
>> >> will want to make sure we cover both gen1 and gen2 parts.
>> >>
>> >> > The recent merge has made my local branch of rtl8xxxu-devel 14
>> >> > commits ahead.
>> >> > Do I need to do a reset and submit a new patch for the DWA-131 dongle?
>> >>
>> >> In general you need to use 'git pull --rebase' on my tree. I rebase it
>> >> to stay in sync with Kalle's tree.
>> >>
>> >> The DWA-131 is the 8192eu? Sorry a bit behind and my mind is losing
>> >> bits. If it's the patch you posted earlier I can dig it out and play
>> >> with it - I am still catching up though, so please be patient.
>> >
>> > yes it's an 8192eu.
>>
>> Gotcha - how do you do your testing to reproduce the problem btw? Most
>> of my testing is using the 8723au as a primary device and the next
>> device as a follow-on, so I may not see as long a run with the device
>> active as you see.
>>
>
> Testing is simple. Connect to an AP in the usual
> manner..disconnect..reconnect. The 8192eu will fail to reconnect and
> John Heenan reported the 8723bu also fails to reconnect. Even though
> he was directly stopping and restarting wpa_supplicant, it's the same
> thing to the driver - connect..disconnect..reconnect.
Thanks for the details - I'll have a look shortly.
> With using a usb_device pointer, each message starts with the usb bus
> address. Plug it into a different port and that address could
> change. By using a pointer to the device associated with the wiphy
> each message will begin with the driver name. Just makes it easier for
> the average user to report what's in the log because he can just grep
> for "rtl8xxxu".
I see - that would be problematic for me as I quite often have 3-4 of
these things plugged in at the same time. Not knowing which port spits
out the message would make it a lot harder to track. In fact my primary
devel box for this (Lenovo Yoga 13) has an rtl8723au soldered on the
motherboard, so the moment I plug in any other dongle I'll have two.
The alternative would be to add a prefer to the individual messages.
Cheers,
Jes
^ permalink raw reply
* Re: Break-it testing for wifi
From: Ben Greear @ 2016-11-21 17:19 UTC (permalink / raw)
To: Steve deRosier; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <CALLGbR+q=QKLgXj6jYJ_jUdvrh2GcOk_3NkndTf2JWZsOqzecQ@mail.gmail.com>
On 11/21/2016 08:55 AM, Steve deRosier wrote:
>
>
> On Mon, Nov 21, 2016 at 8:10 AM, Ben Greear <greearb@candelatech.com <mailto:greearb@candelatech.com>> wrote:
>
> Hello!
>
> I am thinking about adding some sort of framework to wpa_supplicant and/or the
> mac80211 stack to allow purposefully creating bad station behaviour in order to
> test robustness of APs.
>
> Some ideas so far:
>
> 1) Allow supplicant to do bad state-machine transitions (start 4-way before associating, for instance).
>
> 2) Randomly corrupt mgt frames in driver and/or mac80211 stack and/or supplicant.
>
> 3) Possibly allow user to make specific corruptions. This would probably be in supplicant
> only, and I am not sure how this would be configured. Maybe allow user to over-ride
> existing IEs and add bogus ones of their own choosing.
>
> 4) Maybe some specific tests like putting in over-flow sized lengths of IEs.
>
> Has anyone done anything similar they would like to share?
>
> Johannes: Any interest in having such a framework in upstream kernels?
>
> Any other ideas for how to improve this feature set?
>
> Thanks,
> Ben
>
>
> Hi Ben,
>
> I did something related to this many years ago using the radiotap interface and based on Andy Green's work. It's very limited but might be interesting.
>
> https://github.com/derosier/packetvector
>
> I would absolutely welcome such a tool. I'd like it to be useful for testing clients and mesh nodes in addition to APs, but I don't think that's a big stretch.
>
> I guess my only comment is if it's possible for this to be in userspace, that's where'd like to see it. If not, some sort of framework we all could use would be
> nice. All in one place would also be nice, but as you've got multiple independent components that cooperate that might be difficult to achieve.
[re-added linux-wireless to CC]
Some things, like probe requests, are created as far down as the firmware (ath10k, for instance).
Some IEs are made in mac80211 as well.
And, ath10k (and likely other thick-firmware implementations) is crap for packet injection.
I think the closer I can be to 'real' supplicant, stack & driver behaviour, the better chance I have of finding
more interesting bugs.
So, I don't think a pure user-space app is that useful for my desired testing scenario.
My specific goal is testing APs, but I think it should work fine for testing other
connection types.
If I made some generic code to mangle management frames, including parsing IEs and re-writing
them and such (as well as randomly bit-flipping as requested), maybe it could be called from
tx logic in mac80211. Corrupting things in the stack might help harden the drivers (and firmware)
a bit on the sending path, and might work for generic network devices. Possibly I would need to add hooks in the
driver as well if frames were actually generated there. For probe requests, I might have to
really hack all the way down into the firmware to have full control for corrupting that.
Since probe requests don't require much state, possibly that is a place where a relatively
dumb user-space packet injection would be best.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
From: Brian Norris @ 2016-11-21 17:24 UTC (permalink / raw)
To: Kalle Valo
Cc: Amitkumar Karwar, linux-wireless, Cathy Luo, Nishant Sarmukadam,
rajatja, dmitry.torokhov, Shengzhen Li
In-Reply-To: <877f82mg7s.fsf@kamboji.qca.qualcomm.com>
On Thu, Nov 17, 2016 at 02:41:11PM +0200, Kalle Valo wrote:
> Patchwork link for the same:
>
> https://patchwork.kernel.org/project/linux-wireless/list/?state=2&q=mwifiex
>
> Does that look ok?
FWIW, your merges (since you made the above comments) look sane to me.
Thanks!
Brian
^ permalink raw reply
* Re: [PATCH v3 4/5] mwifiex: wait firmware dump complete during card remove process
From: Brian Norris @ 2016-11-21 17:36 UTC (permalink / raw)
To: Amitkumar Karwar
Cc: linux-wireless, Cathy Luo, Nishant Sarmukadam, rajatja,
dmitry.torokhov, Xinming Hu
In-Reply-To: <1479301749-14803-4-git-send-email-akarwar@marvell.com>
Hi,
On Wed, Nov 16, 2016 at 06:39:08PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu <huxm@marvell.com>
>
> Wait for firmware dump complete in card remove function.
> For sdio interface, there are two diffenrent cases,
> card reset trigger sdio_work and firmware dump trigger sdio_work.
> Do code rearrangement for distinguish between these two cases.
On second review of the SDIO card reset code (which I'll repeat is quite
ugly), you seem to be making a bad distinction here. What if there is a
firmware dump happening concurrently with your card-reset handling? You
*do* want to synchronize with the firmware dump before completing the
card reset, or else you might be freeing up internal card resources that
are still in use. See below.
>
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
> v2: 1. Get rid of reset_triggered flag. Instead split the code and use
> __mwifiex_sdio_remove() (Brian Norris/Dmitry Torokhov)
> 2. "v1 4/5 mwifiex: firmware dump code rearrangement.." is dropped. So
> rebased accordingly.
> v3: same as [v2,5/5]. The improvement of 'moving pcie_work to card struct'
> suggested by Brian is taken care in next patch.
> ---
> drivers/net/wireless/marvell/mwifiex/pcie.c | 6 +++++-
> drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++++++++++++---
> 2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index dd8f7aa..c8e69a4 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -51,6 +51,9 @@ static int mwifiex_pcie_probe_of(struct device *dev)
> return 0;
> }
>
> +static void mwifiex_pcie_work(struct work_struct *work);
> +static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
> +
> static int
> mwifiex_map_pci_memory(struct mwifiex_adapter *adapter, struct sk_buff *skb,
> size_t size, int flags)
> @@ -254,6 +257,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
> if (!adapter || !adapter->priv_num)
> return;
>
> + cancel_work_sync(&pcie_work);
> +
> if (user_rmmod && !adapter->mfg_mode) {
> mwifiex_deauthenticate_all(adapter);
>
> @@ -2722,7 +2727,6 @@ static void mwifiex_pcie_work(struct work_struct *work)
> mwifiex_pcie_device_dump_work(save_adapter);
> }
>
> -static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
> /* This function dumps FW information */
> static void mwifiex_pcie_device_dump(struct mwifiex_adapter *adapter)
> {
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 16d1d30..78f2cc9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -46,6 +46,9 @@
> */
> static u8 user_rmmod;
>
> +static void mwifiex_sdio_work(struct work_struct *work);
> +static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
> +
> static struct mwifiex_if_ops sdio_ops;
> static unsigned long iface_work_flags;
>
> @@ -220,7 +223,7 @@ static int mwifiex_sdio_resume(struct device *dev)
> * This function removes the interface and frees up the card structure.
> */
> static void
> -mwifiex_sdio_remove(struct sdio_func *func)
> +__mwifiex_sdio_remove(struct sdio_func *func)
> {
> struct sdio_mmc_card *card;
> struct mwifiex_adapter *adapter;
> @@ -249,6 +252,13 @@ static int mwifiex_sdio_resume(struct device *dev)
> mwifiex_remove_card(adapter);
> }
>
> +static void
> +mwifiex_sdio_remove(struct sdio_func *func)
> +{
> + cancel_work_sync(&sdio_work);
> + __mwifiex_sdio_remove(func);
> +}
> +
> /*
> * SDIO suspend.
> *
> @@ -2227,7 +2237,7 @@ static void mwifiex_recreate_adapter(struct sdio_mmc_card *card)
> * discovered and initializes them from scratch.
> */
>
> - mwifiex_sdio_remove(func);
> + __mwifiex_sdio_remove(func);
^^ So here, you're trying to avoid syncing with the card-reset work
event, except that function will free up all your resources (including
the static save_adapter). Thus, you're explicitly allowing a
use-after-free error here. That seems unwise.
Instead, you should actually retain the invariant that you're doing a
full remove/reinitialize here, which includes doing the *same*
cancel_work_sync() here in mwifiex_recreate_adapter() as you would in
any other remove().
IOW, kill the __mwifiex_sdio_remove() and just call
mwifiex_sdio_remove() as you were.
That also means that you can do the same per-adapter cleanup in the
following patch as you do for PCIe.
Brian
>
> /*
> * Normally, we would let the driver core take care of releasing these.
> @@ -2568,7 +2578,6 @@ static void mwifiex_sdio_work(struct work_struct *work)
> mwifiex_sdio_card_reset_work(save_adapter);
> }
>
> -static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
> /* This function resets the card */
> static void mwifiex_sdio_card_reset(struct mwifiex_adapter *adapter)
> {
> --
> 1.9.1
>
^ permalink raw reply
* Re: scheduled scan interval
From: Arend Van Spriel @ 2016-11-21 19:05 UTC (permalink / raw)
To: Dave Taht, Luca Coelho; +Cc: Johannes Berg, linux-wireless
In-Reply-To: <CAA93jw5hkLWCCKdabEyKWTF_kFs8xWKC-kjUeFNh9os+zEuxJw@mail.gmail.com>
On 21-11-2016 17:59, Dave Taht wrote:
> On Mon, Nov 21, 2016 at 7:08 AM, Luca Coelho <luca@coelho.fi> wrote:
>> Hi Arend,
>>
>> On Mon, 2016-11-21 at 13:03 +0100, Arend Van Spriel wrote:
>>> On 21-11-2016 12:30, Arend Van Spriel wrote:
>>>> On 21-11-2016 12:19, Arend Van Spriel wrote:
>>>>> Hi Johannes, Luca,
>>>>>
>>>>> The gscan work made me look at scheduled scan and the implementation of
>>>>> it in brcmfmac. The driver ignored the interval parameter from
>>>>> user-space. Now I am fixing that. One thing is that our firmware has a
>>>>> minimum interval which can not be indicated in struct wiphy. The other
>>>>> issue is how the maximum interval is used in the nl80211.c.
>>>>>
>>>>> In nl80211_parse_sched_scan_plans() it is used against value passed in
>>>>> NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
>>>>> For the first one it caps the value to the maximum, but for the second
>>>>> one it returns -EINVAL. I suspect this is done because maximum interval
>>>>> was introduced with schedule scan plans, but it feels inconsistent.
>>>>
>>>> It also maybe simply wrong to cap. At least brcmfmac does not set the
>>>> maximum so it will always get interval being zero. Maybe better to do:
>>>>
>>>> if (wiphy->max_sched_scan_plan_interval &&
>>>> request->scan_plans[0].interval >
>>>> wiphy->max_sched_scan_plan_interval)
>>>> return -EINVAL;
>>>>
>>>>> Thoughts?
>>>
>>> Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
>>> struct sched_scan_request::interval was specified in milliseconds! Below
>>> the drivers that I see having scheduled scan support:
>>>
>>> iwlmvm: cap interval, convert to seconds.
>>> ath6kl: cap to 1sec minimum, no max check, convert to seconds.
>>> wl12xx: no checking in driver, fw need milliseconds.
>>> wl18xx: no checking in driver, fw need milliseconds.
>>>
>>> The milliseconds conversion seems to be taken care of by multiplying
>>> with MSEC_PER_SEC in wl{12,18}xx drivers.
>>>
>>> It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
>>> other drivers will get interval of zero which only ath6kl handles.
>>
>> With the introduction of scheduled scan plans, we sort of deprecated
>> the "generic" scheduled scan interval. It doesn't make sense to have
>> both passed at the same time, so nl80211 forbids
>> NL80211_ATTR_SCHED_SCAN_INTERVAL if we pass
>> NL80211_ATTR_SCHED_SCAN_PLANS.
>>
>> The original NL80211_ATTR_SCHED_SCAN_INTERVAL was specified in msecs,
>> which is silly because we can never get millisecond accuracy in this.
>> Thus, in the plans API, we decided to use seconds instead (because it
>> makes much more sense). Additionally, the interval is considered
>> "advisory", because the FW may not be able guarantee the exact
>> intervals (for instance, the iwlwifi driver actually starts the
>> interval timer after scan completion, so if you specify 10 seconds
>> intervals, in practice they'll be 13-14 seconds).
>>
>> I'm not sure I'm answering your question, because I'm also not sure I
>> understood the question. :)
>
> I'm not sure if I understand the discussion and hooks myself, but
> recently fixes for doing channel scans saner from userspace ended up
> here, after some discussion.
scheduled scan is a scan-offload feature in which user-space requests
the firmware on the device to perform a period scan at a requested
interval. As such it is not different from NM or wpa_supplicant
performing a scan, but the host will only get informed about found SSIDs
which user-space has configured, eg. the networks that are configured in
wpa_supplicant. How much time the scan takes is determined by the
channels in the scheduled scan request. Worst case that means all
supported 2G and 5G channels. So it is just about offloading. There is
not necessarily reduced impact.
Regards,
Arend
> https://bugzilla.gnome.org/show_bug.cgi?id=766482
>
> Anything that can reduce the impact of this behavior, I'm for!
>
> http://www.taht.net/~d/channel_scans_destroying_latency_under_load_for_10s.png
>
>
>
>>
>> --
>> Cheers,
>> Luca.
>
>
>
^ permalink raw reply
* Re: scheduled scan interval
From: Arend Van Spriel @ 2016-11-21 19:34 UTC (permalink / raw)
To: Luca Coelho, Johannes Berg; +Cc: linux-wireless
In-Reply-To: <1479740918.2517.28.camel@coelho.fi>
On 21-11-2016 16:08, Luca Coelho wrote:
> Hi Arend,
>
> On Mon, 2016-11-21 at 13:03 +0100, Arend Van Spriel wrote:
>> On 21-11-2016 12:30, Arend Van Spriel wrote:
>>> On 21-11-2016 12:19, Arend Van Spriel wrote:
>>>> Hi Johannes, Luca,
>>>>
>>>> The gscan work made me look at scheduled scan and the implementation of
>>>> it in brcmfmac. The driver ignored the interval parameter from
>>>> user-space. Now I am fixing that. One thing is that our firmware has a
>>>> minimum interval which can not be indicated in struct wiphy. The other
>>>> issue is how the maximum interval is used in the nl80211.c.
>>>>
>>>> In nl80211_parse_sched_scan_plans() it is used against value passed in
>>>> NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
>>>> For the first one it caps the value to the maximum, but for the second
>>>> one it returns -EINVAL. I suspect this is done because maximum interval
>>>> was introduced with schedule scan plans, but it feels inconsistent.
>>>
>>> It also maybe simply wrong to cap. At least brcmfmac does not set the
>>> maximum so it will always get interval being zero. Maybe better to do:
>>>
>>> if (wiphy->max_sched_scan_plan_interval &&
>>> request->scan_plans[0].interval >
>>> wiphy->max_sched_scan_plan_interval)
>>> return -EINVAL;
>>>
>>>> Thoughts?
>>
>> Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
>> struct sched_scan_request::interval was specified in milliseconds! Below
>> the drivers that I see having scheduled scan support:
>>
>> iwlmvm: cap interval, convert to seconds.
>> ath6kl: cap to 1sec minimum, no max check, convert to seconds.
>> wl12xx: no checking in driver, fw need milliseconds.
>> wl18xx: no checking in driver, fw need milliseconds.
>>
>> The milliseconds conversion seems to be taken care of by multiplying
>> with MSEC_PER_SEC in wl{12,18}xx drivers.
>>
>> It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
>> other drivers will get interval of zero which only ath6kl handles.
>
> With the introduction of scheduled scan plans, we sort of deprecated
> the "generic" scheduled scan interval. It doesn't make sense to have
> both passed at the same time, so nl80211 forbids
> NL80211_ATTR_SCHED_SCAN_INTERVAL if we pass
> NL80211_ATTR_SCHED_SCAN_PLANS.
Indeed, but if no plans are passed it is still allowed.
> The original NL80211_ATTR_SCHED_SCAN_INTERVAL was specified in msecs,
> which is silly because we can never get millisecond accuracy in this.
> Thus, in the plans API, we decided to use seconds instead (because it
> makes much more sense). Additionally, the interval is considered
> "advisory", because the FW may not be able guarantee the exact
> intervals (for instance, the iwlwifi driver actually starts the
> interval timer after scan completion, so if you specify 10 seconds
> intervals, in practice they'll be 13-14 seconds).
Agree. Our firmware wants to have it in seconds as well.
> I'm not sure I'm answering your question, because I'm also not sure I
> understood the question. :)
The question is this: Why is the interval capped at
max_sched_scan_plan_interval if it exceeds it and no plans are provided
(so continue to setup the scheduled scan request) whereas when plans are
provided and an interval exceeds max_sched_scan_plan_interval it aborts
with -EINVAL.
And a follow-up question for this snippet of code in
nl80211_parse_sched_scan_plans():
if (!attrs[NL80211_ATTR_SCHED_SCAN_PLANS]) {
u32 interval;
/*
* If scan plans are not specified,
* %NL80211_ATTR_SCHED_SCAN_INTERVAL must be specified. In this
* case one scan plan will be set with the specified scan
* interval and infinite number of iterations.
*/
if (!attrs[NL80211_ATTR_SCHED_SCAN_INTERVAL])
return -EINVAL;
interval = nla_get_u32(attrs[NL80211_ATTR_SCHED_SCAN_INTERVAL]);
if (!interval)
return -EINVAL;
request->scan_plans[0].interval =
DIV_ROUND_UP(interval, MSEC_PER_SEC);
if (!request->scan_plans[0].interval)
return -EINVAL;
if (request->scan_plans[0].interval >
wiphy->max_sched_scan_plan_interval)
request->scan_plans[0].interval =
wiphy->max_sched_scan_plan_interval;
return 0;
}
So in v4.3 the interval was only validated to be non-zero. Now the
interval is validated and capped to wiphy->max_sched_scan_plan_interval
but apart from iwlmvm there are no driver specifying that so
max_sched_scan_plan_interval = 0 for those and interval is unsigned int
not equal to zero. So the last if statement above is true setting the
interval to zero. I think the if statement should be extended to assure
max_sched_scan_interval is non-zero, ie. set explicitly by the driver:
if (wiphy->max_sched_scan_plan_interval &&
request->scan_plans[0].interval >
wiphy->max_sched_scan_plan_interval)
request->scan_plans[0].interval =
wiphy->max_sched_scan_plan_interval;
Regards,
Arend
^ permalink raw reply
* [PATCH] mac80211: Remove unused 'struct ieee80211_rx_status' ptr
From: Kirtika Ruchandani @ 2016-11-22 5:44 UTC (permalink / raw)
To: Johannes Berg; +Cc: Arnd Bergmann, linux-wireless, netdev
Commit 554891e63a29 introduced 'struct ieee80211_rx_status' in
ieee80211_rx_h_defragment but did not use it. Compiling with W=1
gives the following warning, fix it.
net/mac80211/rx.c: In function ‘ieee80211_rx_h_defragment’:
net/mac80211/rx.c:1911:30: warning: variable ‘status’ set but not used [-Wunused-but-set-variable]
Fixes: 554891e63a29 ("mac80211: move packet flags into packet")
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: John W. Linville <linville@tuxdriver.com>
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>
---
net/mac80211/rx.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index eeab725..d2a00f2 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1908,7 +1908,6 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
unsigned int frag, seq;
struct ieee80211_fragment_entry *entry;
struct sk_buff *skb;
- struct ieee80211_rx_status *status;
hdr = (struct ieee80211_hdr *)rx->skb->data;
fc = hdr->frame_control;
@@ -2034,9 +2033,6 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
dev_kfree_skb(skb);
}
- /* Complete frame has been reassembled - process it now */
- status = IEEE80211_SKB_RXCB(rx->skb);
-
out:
ieee80211_led_rx(rx->local);
out_no_led:
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: scheduled scan interval
From: Luca Coelho @ 2016-11-22 5:59 UTC (permalink / raw)
To: Arend Van Spriel, Johannes Berg; +Cc: linux-wireless
In-Reply-To: <a222d5d1-408d-fd96-4cb9-896f6a9dab59@broadcom.com>
Hi Arend,
On Mon, 2016-11-21 at 20:34 +0100, Arend Van Spriel wrote:
> On 21-11-2016 16:08, Luca Coelho wrote:
> > Hi Arend,
> >
> > On Mon, 2016-11-21 at 13:03 +0100, Arend Van Spriel wrote:
> > > On 21-11-2016 12:30, Arend Van Spriel wrote:
> > > > On 21-11-2016 12:19, Arend Van Spriel wrote:
> > > > > Hi Johannes, Luca,
> > > > >
> > > > > The gscan work made me look at scheduled scan and the implementation of
> > > > > it in brcmfmac. The driver ignored the interval parameter from
> > > > > user-space. Now I am fixing that. One thing is that our firmware has a
> > > > > minimum interval which can not be indicated in struct wiphy. The other
> > > > > issue is how the maximum interval is used in the nl80211.c.
> > > > >
> > > > > In nl80211_parse_sched_scan_plans() it is used against value passed in
> > > > > NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
> > > > > For the first one it caps the value to the maximum, but for the second
> > > > > one it returns -EINVAL. I suspect this is done because maximum interval
> > > > > was introduced with schedule scan plans, but it feels inconsistent.
> > > >
> > > > It also maybe simply wrong to cap. At least brcmfmac does not set the
> > > > maximum so it will always get interval being zero. Maybe better to do:
> > > >
> > > > if (wiphy->max_sched_scan_plan_interval &&
> > > > request->scan_plans[0].interval >
> > > > wiphy->max_sched_scan_plan_interval)
> > > > return -EINVAL;
> > > >
> > > > > Thoughts?
> > >
> > > Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
> > > struct sched_scan_request::interval was specified in milliseconds! Below
> > > the drivers that I see having scheduled scan support:
> > >
> > > iwlmvm: cap interval, convert to seconds.
> > > ath6kl: cap to 1sec minimum, no max check, convert to seconds.
> > > wl12xx: no checking in driver, fw need milliseconds.
> > > wl18xx: no checking in driver, fw need milliseconds.
> > >
> > > The milliseconds conversion seems to be taken care of by multiplying
> > > with MSEC_PER_SEC in wl{12,18}xx drivers.
> > >
> > > It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
> > > other drivers will get interval of zero which only ath6kl handles.
> >
> > With the introduction of scheduled scan plans, we sort of deprecated
> > the "generic" scheduled scan interval. It doesn't make sense to have
> > both passed at the same time, so nl80211 forbids
> > NL80211_ATTR_SCHED_SCAN_INTERVAL if we pass
> > NL80211_ATTR_SCHED_SCAN_PLANS.
>
> Indeed, but if no plans are passed it is still allowed.
That's right. But the driver will get it as a single plan.
> > The original NL80211_ATTR_SCHED_SCAN_INTERVAL was specified in msecs,
> > which is silly because we can never get millisecond accuracy in this.
> > Thus, in the plans API, we decided to use seconds instead (because it
> > makes much more sense). Additionally, the interval is considered
> > "advisory", because the FW may not be able guarantee the exact
> > intervals (for instance, the iwlwifi driver actually starts the
> > interval timer after scan completion, so if you specify 10 seconds
> > intervals, in practice they'll be 13-14 seconds).
>
> Agree. Our firmware wants to have it in seconds as well.
It was actually my mistake when I implemented it in my TI days (can't
hide from git blame ;)). TI's firmware used msecs and I just blindly
followed it.
> > I'm not sure I'm answering your question, because I'm also not sure I
> > understood the question. :)
>
> The question is this: Why is the interval capped at
> max_sched_scan_plan_interval if it exceeds it and no plans are provided
> (so continue to setup the scheduled scan request) whereas when plans are
> provided and an interval exceeds max_sched_scan_plan_interval it aborts
> with -EINVAL.
Oh, I see. The problem is that the "max_sched_scan_plan_interval" was
introduced later. If the userspace passes
NL80211__ATTR_SCHED_SCAN_INTERVAL, it probably means that it doesn't
know about NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL (i.e. it's only using an
old API). If it is also, for some reason, passing a very large number,
we shouldn't suddenly make it fail with -EINVALID, because that would
be a break of UABI. And since we know the driver cannot support such a
large number, we cap it because it's the best we can do.
Now, if the userspace uses NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL, it
means that it knows the new API (and was written after the new API was
introduced), so we can be stricter and assume it must have checked
NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL.
Makes sense?
> And a follow-up question for this snippet of code in
> nl80211_parse_sched_scan_plans():
>
> if (!attrs[NL80211_ATTR_SCHED_SCAN_PLANS]) {
> u32 interval;
>
> /*
> * If scan plans are not specified,
> * %NL80211_ATTR_SCHED_SCAN_INTERVAL must be specified. In this
> * case one scan plan will be set with the specified scan
> * interval and infinite number of iterations.
> */
> if (!attrs[NL80211_ATTR_SCHED_SCAN_INTERVAL])
> return -EINVAL;
>
> interval = nla_get_u32(attrs[NL80211_ATTR_SCHED_SCAN_INTERVAL]);
> if (!interval)
> return -EINVAL;
>
> request->scan_plans[0].interval =
> DIV_ROUND_UP(interval, MSEC_PER_SEC);
> if (!request->scan_plans[0].interval)
> return -EINVAL;
>
> if (request->scan_plans[0].interval >
> wiphy->max_sched_scan_plan_interval)
> request->scan_plans[0].interval =
> wiphy->max_sched_scan_plan_interval;
>
> return 0;
> }
>
> So in v4.3 the interval was only validated to be non-zero. Now the
> interval is validated and capped to wiphy->max_sched_scan_plan_interval
> but apart from iwlmvm there are no driver specifying that so
> max_sched_scan_plan_interval = 0 for those and interval is unsigned int
> not equal to zero. So the last if statement above is true setting the
> interval to zero. I think the if statement should be extended to assure
> max_sched_scan_interval is non-zero, ie. set explicitly by the driver:
>
> if (wiphy->max_sched_scan_plan_interval &&
> request->scan_plans[0].interval >
> wiphy->max_sched_scan_plan_interval)
> request->scan_plans[0].interval =
> wiphy->max_sched_scan_plan_interval;
If the driver doesn't set the max_sched_scan_plan_interval, mac80211's
default of MAX_U32 will be used:
struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
const char *requested_name)
{
[...]
rdev->wiphy.max_sched_scan_plans = 1;
rdev->wiphy.max_sched_scan_plan_interval = U32_MAX;
return &rdev->wiphy;
}
EXPORT_SYMBOL(wiphy_new_nm);
...so max_sched_scan_plan_interval will never be zero, unless the
driver explicitly sets it to zero.
--
Cheers,
Luca.
^ permalink raw reply
* [PATCH] mac80211: Remove unused 'rates_idx' variable
From: Kirtika Ruchandani @ 2016-11-22 6:04 UTC (permalink / raw)
To: Johannes Berg
Cc: Arnd Bergmann, linux-wireless, netdev, Felix Fietkau,
Johannes Berg
Commit f027c2aca0cf introduced 'rates_idx' in
ieee80211_tx_status_noskb but did not use it. Compiling with W=1
gives the following warning, fix it.
mac80211/status.c: In function ‘ieee80211_tx_status_noskb’:
mac80211/status.c:636:6: warning: variable ‘rates_idx’ set but not used [-Wunused-but-set-variable]
This is a harmless warning, and is only being fixed to reduce the
noise generated with W=1.
Fixes: f027c2aca0cf ("mac80211: add ieee80211_tx_status_noskb")
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Felix Fietkau <nbd@openwrt.org>
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>
---
net/mac80211/status.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index ddf71c6..f7c5ae5 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -633,10 +633,9 @@ void ieee80211_tx_status_noskb(struct ieee80211_hw *hw,
struct ieee80211_local *local = hw_to_local(hw);
struct ieee80211_supported_band *sband;
int retry_count;
- int rates_idx;
bool acked, noack_success;
- rates_idx = ieee80211_tx_get_rates(hw, info, &retry_count);
+ ieee80211_tx_get_rates(hw, info, &retry_count);
sband = hw->wiphy->bands[info->band];
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH] mac80211: Remove unused 'struct rate_control_ref' variable
From: Kirtika Ruchandani @ 2016-11-22 6:54 UTC (permalink / raw)
To: Johannes Berg
Cc: Arnd Bergmann, linux-wireless, netdev, Maxim Altshul,
Johannes Berg
Commit 3b17fbf87d5d introduced sta_get_expected_throughput()
leaving variable 'struct rate_control_ref* ref' set but unused.
Compiling with W=1 gives the following warning, fix it.
net/mac80211/sta_info.c: In function ‘sta_set_sinfo’:
net/mac80211/sta_info.c:2052:27: warning: variable ‘ref’ set but not used [-Wunused-but-set-variable]
Fixes: 3b17fbf87d5d ("mac80211: mesh: Add support for HW RC implementation")
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Maxim Altshul <maxim.altshul@ti.com>
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>
---
net/mac80211/sta_info.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 1711bae..4ab75a9 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -2049,16 +2049,12 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo)
{
struct ieee80211_sub_if_data *sdata = sta->sdata;
struct ieee80211_local *local = sdata->local;
- struct rate_control_ref *ref = NULL;
u32 thr = 0;
int i, ac, cpu;
struct ieee80211_sta_rx_stats *last_rxstats;
last_rxstats = sta_get_last_rx_stats(sta);
- if (test_sta_flag(sta, WLAN_STA_RATE_CONTROL))
- ref = local->rate_ctrl;
-
sinfo->generation = sdata->local->sta_generation;
/* do before driver, so beacon filtering drivers have a
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH] RFC: Universal scan proposal
From: Luca Coelho @ 2016-11-22 7:24 UTC (permalink / raw)
To: dimitrysh, linux-wireless
In-Reply-To: <94eb2c110db85c2379054172dad0@google.com>
Hi Dmitry,
On Wed, 2016-11-16 at 22:47 +0000, dimitrysh@google.com wrote:
> From 68a9d37a4c7e9dc7a90a6e922cdea52737a98d66 Mon Sep 17 00:00:00 2001
> From: Dmitry Shmidt <dimitrysh@google.com>
> Date: Wed, 16 Nov 2016 14:27:26 -0800
> Subject: [PATCH] RFC: Universal scan proposal
>
> Currently we have sched scan with possibility of various
> intervals. We would like to extend it to support also
> different types of scan.
> In case of powerful wlan CPU, all this functionality
> can be offloaded.
> In general case FW processes additional scan requests
> and puts them into queue based on start time and interval.
> Once current request is fulfilled, FW adds it (if interval != 0)
> again to the queue with proper interval. If requests are
> overlapping, new request can be combined with either one before,
> or one after, assuming that requests are not mutually exclusive.
> Combining requests is done by combining scan channels, ssids,
> bssids and types of scan result. Once combined request was fulfilled
> it will be reinserted as two (or three) different requests based on
> their type and interval.
> Each request has attribute:
> Type: connectivity / location
> Report: none / batch / immediate
> Request may have priority and can be inserted into
> the head of the queue.
> Types of scans:
> - Normal scan
> - Scheduled scan
> - Hotlist (BSSID scan)
> - Roaming
> - AutoJoin
>
> Change-Id: I9f3e4c975784f1c1c5156887144d80fc5a26bffa
> Signed-off-by: Dmitry Shmidt <dimitrysh@google.com>
> ---
I like the initiative and I think this is definitely something that can
improve concurrent scanning instances. But IMHO the most important is
to discuss the semantics of this change, such as which scans can be
combined, who makes the decisions of combining them, how priorities are
sorted out etc. I think the types of scan are not relevant in the
nl80211 API, but the characteristics of the scans are. For instance,
"urgent scan" (for initial connection), best-effort scan for roaming...
and latency requirements, such as low-latency for location and initial
connection and high-latency for scheduled scan. Then we decided, in
the kernel, how to combine and prioritize them according to their
characteristics, instead of having to map scan types to these
characteristics.
What do you think?
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH] mac80211: Remove unused 'struct rate_control_ref' variable
From: Johannes Berg @ 2016-11-22 7:30 UTC (permalink / raw)
To: Kirtika Ruchandani; +Cc: Arnd Bergmann, linux-wireless, netdev, Maxim Altshul
In-Reply-To: <20161122065416.GA3565@google.com>
On Mon, 2016-11-21 at 22:54 -0800, Kirtika Ruchandani wrote:
> Commit 3b17fbf87d5d introduced sta_get_expected_throughput()
> leaving variable 'struct rate_control_ref* ref' set but unused.
> Compiling with W=1 gives the following warning, fix it.
>
> net/mac80211/sta_info.c: In function ‘sta_set_sinfo’:
> net/mac80211/sta_info.c:2052:27: warning: variable ‘ref’ set but not
> used [-Wunused-but-set-variable]
Applied all three of these, thanks Kirtika.
johannes
^ permalink raw reply
* Re: scheduled scan interval
From: Arend Van Spriel @ 2016-11-22 9:10 UTC (permalink / raw)
To: Luca Coelho, Johannes Berg; +Cc: linux-wireless
In-Reply-To: <1479794340.2517.34.camel@coelho.fi>
On 22-11-2016 6:59, Luca Coelho wrote:
> Hi Arend,
> On Mon, 2016-11-21 at 20:34 +0100, Arend Van Spriel wrote:
>> On 21-11-2016 16:08, Luca Coelho wrote:
>>> Hi Arend,
>>>
>>> On Mon, 2016-11-21 at 13:03 +0100, Arend Van Spriel wrote:
>>>> On 21-11-2016 12:30, Arend Van Spriel wrote:
>>>>> On 21-11-2016 12:19, Arend Van Spriel wrote:
>>>>>> Hi Johannes, Luca,
>>>>>>
>>>>>> The gscan work made me look at scheduled scan and the implementation of
>>>>>> it in brcmfmac. The driver ignored the interval parameter from
>>>>>> user-space. Now I am fixing that. One thing is that our firmware has a
>>>>>> minimum interval which can not be indicated in struct wiphy. The other
>>>>>> issue is how the maximum interval is used in the nl80211.c.
>>>>>>
>>>>>> In nl80211_parse_sched_scan_plans() it is used against value passed in
>>>>>> NL80211_ATTR_SCHED_SCAN_INTERVAL and NL80211_SCHED_SCAN_PLAN_INTERVAL.
>>>>>> For the first one it caps the value to the maximum, but for the second
>>>>>> one it returns -EINVAL. I suspect this is done because maximum interval
>>>>>> was introduced with schedule scan plans, but it feels inconsistent.
>>>>>
>>>>> It also maybe simply wrong to cap. At least brcmfmac does not set the
>>>>> maximum so it will always get interval being zero. Maybe better to do:
>>>>>
>>>>> if (wiphy->max_sched_scan_plan_interval &&
>>>>> request->scan_plans[0].interval >
>>>>> wiphy->max_sched_scan_plan_interval)
>>>>> return -EINVAL;
>>>>>
>>>>>> Thoughts?
>>>>
>>>> Digging deeper. Looking at v4.3 before introduction of sched_scan_plans.
>>>> struct sched_scan_request::interval was specified in milliseconds! Below
>>>> the drivers that I see having scheduled scan support:
>>>>
>>>> iwlmvm: cap interval, convert to seconds.
>>>> ath6kl: cap to 1sec minimum, no max check, convert to seconds.
>>>> wl12xx: no checking in driver, fw need milliseconds.
>>>> wl18xx: no checking in driver, fw need milliseconds.
>>>>
>>>> The milliseconds conversion seems to be taken care of by multiplying
>>>> with MSEC_PER_SEC in wl{12,18}xx drivers.
>>>>
>>>> It seems in 4.8 only iwlmvm set wiphy->max_sched_scan_plan_interval so
>>>> other drivers will get interval of zero which only ath6kl handles.
>>>
>>> With the introduction of scheduled scan plans, we sort of deprecated
>>> the "generic" scheduled scan interval. It doesn't make sense to have
>>> both passed at the same time, so nl80211 forbids
>>> NL80211_ATTR_SCHED_SCAN_INTERVAL if we pass
>>> NL80211_ATTR_SCHED_SCAN_PLANS.
>>
>> Indeed, but if no plans are passed it is still allowed.
>
> That's right. But the driver will get it as a single plan.
>
>
>>> The original NL80211_ATTR_SCHED_SCAN_INTERVAL was specified in msecs,
>>> which is silly because we can never get millisecond accuracy in this.
>>> Thus, in the plans API, we decided to use seconds instead (because it
>>> makes much more sense). Additionally, the interval is considered
>>> "advisory", because the FW may not be able guarantee the exact
>>> intervals (for instance, the iwlwifi driver actually starts the
>>> interval timer after scan completion, so if you specify 10 seconds
>>> intervals, in practice they'll be 13-14 seconds).
>>
>> Agree. Our firmware wants to have it in seconds as well.
>
> It was actually my mistake when I implemented it in my TI days (can't
> hide from git blame ;)). TI's firmware used msecs and I just blindly
> followed it.
>
>
>>> I'm not sure I'm answering your question, because I'm also not sure I
>>> understood the question. :)
>>
>> The question is this: Why is the interval capped at
>> max_sched_scan_plan_interval if it exceeds it and no plans are provided
>> (so continue to setup the scheduled scan request) whereas when plans are
>> provided and an interval exceeds max_sched_scan_plan_interval it aborts
>> with -EINVAL.
>
> Oh, I see. The problem is that the "max_sched_scan_plan_interval" was
> introduced later. If the userspace passes
> NL80211__ATTR_SCHED_SCAN_INTERVAL, it probably means that it doesn't
> know about NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL (i.e. it's only using an
> old API). If it is also, for some reason, passing a very large number,
> we shouldn't suddenly make it fail with -EINVALID, because that would
> be a break of UABI. And since we know the driver cannot support such a
> large number, we cap it because it's the best we can do.
>
> Now, if the userspace uses NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL, it
> means that it knows the new API (and was written after the new API was
> introduced), so we can be stricter and assume it must have checked
> NL80211_ATTR_MAX_SCAN_PLAN_INTERVAL.
>
> Makes sense?
Not really. As you say if user-space passes
NL80211_ATTR_SCHED_SCAN_INTERVAL it is using old API. Otherwise it
should pass NL80211_ATTR_SCHED_SCAN_PLANS.
>> And a follow-up question for this snippet of code in
>> nl80211_parse_sched_scan_plans():
>>
>> if (!attrs[NL80211_ATTR_SCHED_SCAN_PLANS]) {
>> u32 interval;
>>
>> /*
>> * If scan plans are not specified,
>> * %NL80211_ATTR_SCHED_SCAN_INTERVAL must be specified. In this
>> * case one scan plan will be set with the specified scan
>> * interval and infinite number of iterations.
>> */
>> if (!attrs[NL80211_ATTR_SCHED_SCAN_INTERVAL])
>> return -EINVAL;
>>
>> interval = nla_get_u32(attrs[NL80211_ATTR_SCHED_SCAN_INTERVAL]);
>> if (!interval)
>> return -EINVAL;
>>
>> request->scan_plans[0].interval =
>> DIV_ROUND_UP(interval, MSEC_PER_SEC);
>> if (!request->scan_plans[0].interval)
>> return -EINVAL;
>>
>> if (request->scan_plans[0].interval >
>> wiphy->max_sched_scan_plan_interval)
>> request->scan_plans[0].interval =
>> wiphy->max_sched_scan_plan_interval;
>>
>> return 0;
>> }
>>
>> So in v4.3 the interval was only validated to be non-zero. Now the
>> interval is validated and capped to wiphy->max_sched_scan_plan_interval
>> but apart from iwlmvm there are no driver specifying that so
>> max_sched_scan_plan_interval = 0 for those and interval is unsigned int
>> not equal to zero. So the last if statement above is true setting the
>> interval to zero. I think the if statement should be extended to assure
>> max_sched_scan_interval is non-zero, ie. set explicitly by the driver:
>>
>> if (wiphy->max_sched_scan_plan_interval &&
>> request->scan_plans[0].interval >
>> wiphy->max_sched_scan_plan_interval)
>> request->scan_plans[0].interval =
>> wiphy->max_sched_scan_plan_interval;
>
> If the driver doesn't set the max_sched_scan_plan_interval, mac80211's
> default of MAX_U32 will be used:
>
> struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
> const char *requested_name)
> {
> [...]
> rdev->wiphy.max_sched_scan_plans = 1;
> rdev->wiphy.max_sched_scan_plan_interval = U32_MAX;
>
> return &rdev->wiphy;
> }
> EXPORT_SYMBOL(wiphy_new_nm);
>
> ...so max_sched_scan_plan_interval will never be zero, unless the
> driver explicitly sets it to zero.
I think you are overlooking the cfg80211-based drivers here. According
to lxr at least brcmfmac, mwifiex, and ath6kl are not specifying it.
"Funny" detail is that scheduled scan support in mwifiex seems to be
introduced after the scan plan API change.
With the old API nl80211 only assured it to be non-zero. The only one
capping the value was iwlmvm. The only one setting the
max_sched_scan_plan_interval (overriding mac80211 value) now is iwlmvm.
So I think checking if it is set before capping it retains the old API
behaviour.
Regards,
Arend
^ 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