* RE: [PATCH] mac80211: always update the PM state of a peer on MGMT / DATA frames
From: Grumbach, Emmanuel @ 2017-09-06 12:26 UTC (permalink / raw)
To: johannes@sipsolutions.net; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <20170906122523.17333-1-emmanuel.grumbach@intel.com>
>
> The 2016 version of the spec is more generic about when the AP should
> update the power management state of the peer:
> the AP shall update the state based on any management or data frames. This
> means that even non-bufferable management frames should be looked at to
> update to maintain the power management state of the peer.
>
> This can avoid problematic cases for example if a station disappears while
> being asleep and then re-appears. The AP would remember it as in power
> save, but the Authentication frame couldn't be used to set the peer as
> awake again.
> Note that this issues wasn't really critical since at some point (after the
> association) we would have removed the station and created another one
> with all the states cleared.
>
> type=feature
> ticket=none
>
> Change-Id: Iae1fbbd502d64f0c31db3e0f2d81224b29acb5af
Looks like I forgot to remove a few things here :)
And to put this as an RFC..
> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> ---
> net/mac80211/rx.c | 17 +++++------------
> 1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index
> fdf616811865..89db6b11af8a 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -1749,23 +1749,16 @@ ieee80211_rx_h_sta_process(struct
> ieee80211_rx_data *rx)
>
> /*
> * Change STA power saving mode only at the end of a frame
> - * exchange sequence.
> + * exchange sequence, and only for a data or management
> + * frame as specified in ieee80211-2016 11.2.3.2
> */
> if (!ieee80211_hw_check(&sta->local->hw, AP_LINK_PS) &&
> !ieee80211_has_morefrags(hdr->frame_control) &&
> - !ieee80211_is_back_req(hdr->frame_control) &&
> + (ieee80211_is_mgmt(hdr->frame_control) ||
> + ieee80211_is_data(hdr->frame_control)) &&
> !(status->rx_flags & IEEE80211_RX_DEFERRED_RELEASE) &&
> (rx->sdata->vif.type == NL80211_IFTYPE_AP ||
> - rx->sdata->vif.type == NL80211_IFTYPE_AP_VLAN) &&
> - /*
> - * PM bit is only checked in frames where it isn't reserved,
> - * in AP mode it's reserved in non-bufferable management frames
> - * (cf. IEEE 802.11-2012 8.2.4.1.7 Power Management field)
> - * BAR frames should be ignored as specified in
> - * IEEE 802.11-2012 10.2.1.2.
> - */
> - (!ieee80211_is_mgmt(hdr->frame_control) ||
> - ieee80211_is_bufferable_mmpdu(hdr->frame_control))) {
> + rx->sdata->vif.type == NL80211_IFTYPE_AP_VLAN)) {
> if (test_sta_flag(sta, WLAN_STA_PS_STA)) {
> if (!ieee80211_has_pm(hdr->frame_control))
> sta_ps_end(sta);
> --
> 2.9.3
^ permalink raw reply
* [PATCH] mac80211: Simplify locking in ieee80211_sta_tear_down_BA_sessions()
From: Ilan Peer @ 2017-09-06 14:18 UTC (permalink / raw)
To: linux-wireless; +Cc: Ilan Peer
Simplify the locking in ieee80211_sta_tear_down_BA_sessions() and
lock sta->ampdu_mlme.mtx over the entire function instead of
locking/unlocking it for each TID etc.
Signed-off-by: Ilan Peer <ilan.peer@intel.com>
---
net/mac80211/ht.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index 11d691c..3169862 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -290,13 +290,15 @@ void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta,
{
int i;
+ mutex_lock(&sta->ampdu_mlme.mtx);
for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
- __ieee80211_stop_tx_ba_session(sta, i, reason);
- __ieee80211_stop_rx_ba_session(sta, i, WLAN_BACK_RECIPIENT,
- WLAN_REASON_QSTA_LEAVE_QBSS,
- reason != AGG_STOP_DESTROY_STA &&
- reason != AGG_STOP_PEER_REQUEST);
+ ___ieee80211_stop_tx_ba_session(sta, i, reason);
+ ___ieee80211_stop_rx_ba_session(sta, i, WLAN_BACK_RECIPIENT,
+ WLAN_REASON_QSTA_LEAVE_QBSS,
+ reason != AGG_STOP_DESTROY_STA &&
+ reason != AGG_STOP_PEER_REQUEST);
}
+ mutex_unlock(&sta->ampdu_mlme.mtx);
/* stopping might queue the work again - so cancel only afterwards */
cancel_work_sync(&sta->ampdu_mlme.work);
--
1.9.1
^ permalink raw reply related
* [PATCH] mac80211: always update the PM state of a peer on MGMT / DATA frames
From: Emmanuel Grumbach @ 2017-09-06 12:25 UTC (permalink / raw)
To: johannes; +Cc: linux-wireless, Emmanuel Grumbach
The 2016 version of the spec is more generic about when the
AP should update the power management state of the peer:
the AP shall update the state based on any management or
data frames. This means that even non-bufferable management
frames should be looked at to update to maintain the power
management state of the peer.
This can avoid problematic cases for example if a station
disappears while being asleep and then re-appears. The AP
would remember it as in power save, but the Authentication
frame couldn't be used to set the peer as awake again.
Note that this issues wasn't really critical since at some
point (after the association) we would have removed the
station and created another one with all the states cleared.
type=feature
ticket=none
Change-Id: Iae1fbbd502d64f0c31db3e0f2d81224b29acb5af
Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
---
net/mac80211/rx.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index fdf616811865..89db6b11af8a 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1749,23 +1749,16 @@ ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
/*
* Change STA power saving mode only at the end of a frame
- * exchange sequence.
+ * exchange sequence, and only for a data or management
+ * frame as specified in ieee80211-2016 11.2.3.2
*/
if (!ieee80211_hw_check(&sta->local->hw, AP_LINK_PS) &&
!ieee80211_has_morefrags(hdr->frame_control) &&
- !ieee80211_is_back_req(hdr->frame_control) &&
+ (ieee80211_is_mgmt(hdr->frame_control) ||
+ ieee80211_is_data(hdr->frame_control)) &&
!(status->rx_flags & IEEE80211_RX_DEFERRED_RELEASE) &&
(rx->sdata->vif.type == NL80211_IFTYPE_AP ||
- rx->sdata->vif.type == NL80211_IFTYPE_AP_VLAN) &&
- /*
- * PM bit is only checked in frames where it isn't reserved,
- * in AP mode it's reserved in non-bufferable management frames
- * (cf. IEEE 802.11-2012 8.2.4.1.7 Power Management field)
- * BAR frames should be ignored as specified in
- * IEEE 802.11-2012 10.2.1.2.
- */
- (!ieee80211_is_mgmt(hdr->frame_control) ||
- ieee80211_is_bufferable_mmpdu(hdr->frame_control))) {
+ rx->sdata->vif.type == NL80211_IFTYPE_AP_VLAN)) {
if (test_sta_flag(sta, WLAN_STA_PS_STA)) {
if (!ieee80211_has_pm(hdr->frame_control))
sta_ps_end(sta);
--
2.9.3
^ permalink raw reply related
* [PATCH v2] mac80211: Complete ampdu work schedule during session tear down
From: Ilan Peer @ 2017-09-06 14:17 UTC (permalink / raw)
To: linux-wireless; +Cc: Ilan Peer
Commit 7a7c0a6438b8 ("mac80211: fix TX aggregation start/stop callback race")
added a cancellation of the ampdu work after the loop that stopped the
Tx and Rx BA sessions. However, in some cases, e.g., during HW reconfig,
the low level driver might call mac80211 APIs to complete the stopping
of the BA sessions, which would queue the ampdu work to handle the actual
completion. This work needs to be performed as otherwise mac80211 data
structures would not be properly synced.
Fix this by checking if BA session STOP_CB bit is set after the BA session
cancellation and properly clean the session.
type=bugfix
ticket=jira:WIFILNX-1267
fixes=Ia3e7824a35ea15e7d36db45c6ada65f9c059c0d6
Change-Id: Ib872bb24e3921fdd44787bface7f2ecbf0cf215d
Signed-off-by: Ilan Peer <ilan.peer@intel.com>
Reviewed-on: https://git-amr-3.devtools.intel.com/gerrit/133919
Tested-by: ec ger unix iil jenkins <EC.GER.UNIX.IIL.JENKINS@INTEL.COM>
Reviewed-by: Coelho, Luciano <luciano.coelho@intel.com>
---
net/mac80211/ht.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df49..11d691c 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -300,6 +300,23 @@ void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta,
/* stopping might queue the work again - so cancel only afterwards */
cancel_work_sync(&sta->ampdu_mlme.work);
+
+ /* In case the tear down is part of a reconfigure due to HW restart
+ * request, it is possible that the low level driver requested to stop
+ * the BA session, so handle it to properly clean tid_tx data.
+ */
+ mutex_lock(&sta->ampdu_mlme.mtx);
+ for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
+ struct tid_ampdu_tx *tid_tx =
+ rcu_dereference_protected_tid_tx(sta, i);
+
+ if (!tid_tx)
+ continue;
+
+ if (test_and_clear_bit(HT_AGG_STATE_STOP_CB, &tid_tx->state))
+ ieee80211_stop_tx_ba_cb(sta, i, tid_tx);
+ }
+ mutex_unlock(&sta->ampdu_mlme.mtx);
}
void ieee80211_ba_session_work(struct work_struct *work)
--
1.9.1
^ permalink raw reply related
* hung task in mac80211
From: Matteo Croce @ 2017-09-06 11:57 UTC (permalink / raw)
To: linux-wireless, netdev, linux-kernel
Hi,
I have an hung task on vanilla 4.13 kernel which I haven't on 4.12.
The problem is present both on my AP and on my notebook,
so it seems it affects AP and STA mode as well.
The generated messages are:
INFO: task kworker/u16:6:120 blocked for more than 120 seconds.
Not tainted 4.13.0 #57
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u16:6 D 0 120 2 0x00000000
Workqueue: phy0 ieee80211_ba_session_work [mac80211]
Call Trace:
? __schedule+0x174/0x5b0
? schedule+0x31/0x80
? schedule_preempt_disabled+0x9/0x10
? __mutex_lock.isra.2+0x163/0x480
? select_task_rq_fair+0xb9f/0xc60
? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
? try_to_wake_up+0x1f1/0x340
? update_curr+0x88/0xd0
? ieee80211_ba_session_work+0x148/0x230 [mac80211]
? process_one_work+0x1a5/0x330
? worker_thread+0x42/0x3c0
? create_worker+0x170/0x170
? kthread+0x10d/0x130
? kthread_create_on_node+0x40/0x40
? ret_from_fork+0x22/0x30
I did a bisect and the offending commit is:
commit 699cb58c8a52ff39bf659bff7971893ebe111bf2
Author: Johannes Berg <johannes.berg@intel.com>
Date: Tue May 30 16:34:46 2017 +0200
mac80211: manage RX BA session offload without SKB queue
Instead of using the SKB queue with the fake pkt_type for the
offloaded RX BA session management, also handle this with the
normal aggregation state machine worker. This also makes the
use of this more reliable since it gets rid of the allocation
of the fake skb.
Combined with the previous patch, this finally allows us to
get rid of the pkt_type hack entirely, so do that as well.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Regards,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* [PATCH v2] cfg80211: honor NL80211_RRF_NO_HT40{MINUS,PLUS}
From: Emmanuel Grumbach @ 2017-09-06 10:45 UTC (permalink / raw)
To: johannes; +Cc: linux-wireless, Emmanuel Grumbach
In-Reply-To: <20170805084438.12550-7-luca@coelho.fi>
Honor the NL80211_RRF_NO_HT40{MINUS,PLUS} flags in
reg_process_ht_flags_channel. Not doing so leads can lead
to a firmware assert in iwlwifi for example.
Fixes: b0d7aa59592b ("cfg80211: allow wiphy specific regdomain management")
Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
---
v2: check that regd is not ERR_PTR
---
net/wireless/reg.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 405e14366b86..dda576f5cba6 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -4,6 +4,7 @@
* Copyright 2007 Johannes Berg <johannes@sipsolutions.net>
* Copyright 2008-2011 Luis R. Rodriguez <mcgrof@qca.qualcomm.com>
* Copyright 2013-2014 Intel Mobile Communications GmbH
+ * Copyright 2017 Intel Deutschland GmbH
*
* Permission to use, copy, modify, and/or distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
@@ -1483,7 +1484,9 @@ static void reg_process_ht_flags_channel(struct wiphy *wiphy,
{
struct ieee80211_supported_band *sband = wiphy->bands[channel->band];
struct ieee80211_channel *channel_before = NULL, *channel_after = NULL;
+ const struct ieee80211_regdomain *regd;
unsigned int i;
+ u32 flags;
if (!is_ht40_allowed(channel)) {
channel->flags |= IEEE80211_CHAN_NO_HT40;
@@ -1503,17 +1506,30 @@ static void reg_process_ht_flags_channel(struct wiphy *wiphy,
channel_after = c;
}
+ flags = 0;
+ regd = get_wiphy_regdom(wiphy);
+ if (regd) {
+ const struct ieee80211_reg_rule *reg_rule =
+ freq_reg_info_regd(MHZ_TO_KHZ(channel->center_freq),
+ regd, MHZ_TO_KHZ(20));
+
+ if (!IS_ERR(reg_rule))
+ flags = reg_rule->flags;
+ }
+
/*
* Please note that this assumes target bandwidth is 20 MHz,
* if that ever changes we also need to change the below logic
* to include that as well.
*/
- if (!is_ht40_allowed(channel_before))
+ if (!is_ht40_allowed(channel_before) ||
+ flags & NL80211_RRF_NO_HT40MINUS)
channel->flags |= IEEE80211_CHAN_NO_HT40MINUS;
else
channel->flags &= ~IEEE80211_CHAN_NO_HT40MINUS;
- if (!is_ht40_allowed(channel_after))
+ if (!is_ht40_allowed(channel_after) ||
+ flags & NL80211_RRF_NO_HT40PLUS)
channel->flags |= IEEE80211_CHAN_NO_HT40PLUS;
else
channel->flags &= ~IEEE80211_CHAN_NO_HT40PLUS;
--
2.9.3
^ permalink raw reply related
* Re: [PATCH] iw: Null check before accessing n_mcs variable
From: Johannes Berg @ 2017-09-06 10:39 UTC (permalink / raw)
To: amit.khatri; +Cc: linux-wireless@vger.kernel.org, Nitin Jhanwar
In-Reply-To: <20170901061008epcms5p5fdd787910f9698c511e01a134afe90ea@epcms5p5>
On Fri, 2017-09-01 at 06:10 +0000, Amit Khatri wrote:
> From: Amit Khatri <amit.khatri@samsung.com>
> Date: Thu, 24 Aug 2017 00:16:26 -0700
> Subject: [PATCH] iw: Null check before accessing n_mcs variable
>
> If all cases will fail n_mcs variable fail to get any assignement
> and remain NULL. so better to check NULL before dereference.
>
This is wrong - you can't get into S_HT state unless n_mcs was also
assigned, and if you can't get into S_HT state then you can't
dereference n_mcs either.
johannes
^ permalink raw reply
* Re: [PATCH] Print NL80211_BSS_LAST_SEEN_BOOTTIME and beacon tsf
From: Johannes Berg @ 2017-09-06 10:36 UTC (permalink / raw)
To: Ningyuan Wang; +Cc: linux-wireless
In-Reply-To: <20170726000118.77660-1-nywang@google.com>
Hi,
> nl80211.h | 4 ++++
Please don't include nl80211.h changes, I'll update that as needed.
> + if (bss[NL80211_BSS_BEACON_TSF]) {
> + unsigned long long tsf;
> + tsf = (unsigned long
> long)nla_get_u64(bss[NL80211_BSS_BEACON_TSF]);
> + printf("\tBeacon TSF: %llu usec (%llud,
> %.2lld:%.2llu:%.2llu)\n",
> + tsf, tsf/1000/1000/60/60/24,
> (tsf/1000/1000/60/60) % 24,
> + (tsf/1000/1000/60) % 60, (tsf/1000/1000) %
> 60);
> + }
This seems fine, but we already print the BSS_TSF - is it really
needed? Anyway, I think it's OK, just not sure there's much point.
> + if (bss[NL80211_BSS_LAST_SEEN_BOOTTIME]) {
> + unsigned long long last_seen_since_boot;
> + last_seen_since_boot =
> + (unsigned long
> long)nla_get_u64(bss[NL80211_BSS_LAST_SEEN_BOOTTIME]);
> + printf("\tlast seen since boot time: %llu usec
> (%llud, %.2lld:%.2llu:%.2llu)\n",
> + last_seen_since_boot, last_seen_since_boot
> /1000/1000/1000/60/60/24,
> + (last_seen_since_boot /1000/1000/1000/60/60)
> % 24,
> + (last_seen_since_boot/1000/1000/1000/60) %
> 60,
> + (last_seen_since_boot/1000/1000/1000) % 60);
> + }
This is wrong - the value isn't "last seen since boot", it's "last seen
*at* boottime X" - but "boottime" maybe some kind of strange concept,
so I don't think printing this to a user makes a lot of sense at all?
johannes
^ permalink raw reply
* Re: [PATCH 09/10] mac80211_hwsim: Add command to control rx status RSSI
From: Johannes Berg @ 2017-09-06 10:33 UTC (permalink / raw)
To: Luca Coelho; +Cc: linux-wireless, Beni Lev, Luca Coelho
In-Reply-To: <20170805084438.12550-10-luca@coelho.fi>
On Sat, 2017-08-05 at 11:44 +0300, Luca Coelho wrote:
>
> +static int hwsim_fops_rx_rssi_write(void *dat, u64 val)
> +{
> + struct mac80211_hwsim_data *data = dat;
> + int rssi = (int)val;
That cast is wrong - you should go to s64, do the range check, and only
then restrict to int. Basically, just do
s64 rssi = val;
in this line instead.
> + debugfs_create_file("rx_rssi", 0666, data->debugfs, data,
> + &hwsim_fops_rx_rssi);
I also can't say I'm really happy with this, since there's no such
thing as an "RX RSSI".
Also, wmediumd already allows changing this - I'd rather people start
using the infrastructure better than papering over more.
IOW - do we really need this? Why can't you do the (simple) wmediumd
setup? There's even a hwsim test in wpa_s with wmediumd, it's not all
that difficult.
johannes
^ permalink raw reply
* Re: [PATCH 05/10] ieee80211: Add WFA TPC report element OUI type
From: Johannes Berg @ 2017-09-06 10:29 UTC (permalink / raw)
To: Luca Coelho; +Cc: linux-wireless, Avraham Stern, Luca Coelho
In-Reply-To: <20170805084438.12550-6-luca@coelho.fi>
On Sat, 2017-08-05 at 11:44 +0300, Luca Coelho wrote:
> From: Avraham Stern <avraham.stern@intel.com>
>
> Signed-off-by: Avraham Stern <avraham.stern@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
A commit log would be good :)
johannes
^ permalink raw reply
* Re: [PATCH] wireless: allow send peer mac in rssi cqm notify
From: Johannes Berg @ 2017-09-06 10:27 UTC (permalink / raw)
To: Pradeep Kumar Chitrapu; +Cc: linux-wireless, pradeep718
In-Reply-To: <1503435112-26204-1-git-send-email-pradeepc@codeaurora.org>
On Tue, 2017-08-22 at 13:51 -0700, Pradeep Kumar Chitrapu wrote:
> Extend cqm rssi notifier apis to include peer mac address. Currently
> netlink cqm msg already accepts the mac. With this change just
> passing the mac address as an argument to cfg and mac80211 apis.
This probably somehow interacts with the other patches you sent in this
area, like enabling the CQM stuff for AP mode, but that *really* needs
to be more than three patches without any substantial commit log and
documentation updates.
I'm not convinced that this makes sense at all.
johannes
^ permalink raw reply
* Re: [PATCH] mac80211: Complete ampdu work schedule during session tear down
From: Johannes Berg @ 2017-09-06 10:22 UTC (permalink / raw)
To: Luca Coelho; +Cc: linux-wireless, Ilan Peer, Luca Coelho
In-Reply-To: <20170825081533.27916-1-luca@coelho.fi>
I was going to apply this, but running with lockdep enabled tells me
that this patch is broken.
In the callers of ieee80211_sta_tear_down_BA_sessions(), we only hold
the &local->sta_mtx.
However,
> + rcu_dereference_protected_tid_tx(sta, i);
requires (and checks, if you have lockdep) that you hold either
&sta->ampdu_mlme.mtx
or
&sta->lock.
Additionally, ieee80211_remove_tid_tx(), called via
ieee80211_stop_tx_ba_cb(), requires holding both - and
ieee80211_stop_tx_ba_cb() only requires the spinlock.
johannes
^ permalink raw reply
* RE: [PATCH 06/10] cfg80211: honor NL80211_RRF_NO_HT40{MINUS,PLUS}
From: Grumbach, Emmanuel @ 2017-09-06 7:51 UTC (permalink / raw)
To: Johannes Berg, luca@coelho.fi
Cc: linux-wireless@vger.kernel.org, Coelho, Luciano
In-Reply-To: <1504682981.12380.28.camel@sipsolutions.net>
PiANCj4gT24gVHVlLCAyMDE3LTA5LTA1IGF0IDE2OjQ5ICswMDAwLCBHcnVtYmFjaCwgRW1tYW51
ZWwgd3JvdGU6DQo+ID4gT24gVHVlLCAyMDE3LTA5LTA1IGF0IDE2OjMwICswMjAwLCBKb2hhbm5l
cyBCZXJnIHdyb3RlOg0KPiA+ID4gT24gU2F0LCAyMDE3LTA4LTA1IGF0IDExOjQ0ICswMzAwLCBM
dWNhIENvZWxobyB3cm90ZToNCj4gPiA+DQo+ID4gPiA+ICsJcmVnZCA9IGdldF93aXBoeV9yZWdk
b20od2lwaHkpOw0KPiA+ID4gPiArCWlmIChyZWdkKSB7DQo+ID4gPiA+ICsJCWNvbnN0IHN0cnVj
dCBpZWVlODAyMTFfcmVnX3J1bGUgKnJlZ19ydWxlID0NCj4gPiA+ID4gKwkJCWZyZXFfcmVnX2lu
Zm9fcmVnZChNSFpfVE9fS0haKGNoYW5uZWwtDQo+ID4gPiA+ID4gY2VudGVyX2ZyZXEpLA0KPiA+
ID4gPg0KPiA+ID4gPiArCQkJCQnCoMKgwqByZWdkLA0KPiA+ID4gPiBNSFpfVE9fS0haKDIwKSk7
DQo+ID4gPiA+DQo+ID4gPg0KPiA+ID4gVGhpcyBjb3VsZCByZXR1cm4gYW4gZXJyb3IsIGhvdyBj
YW4geW91IGJlIHN1cmUgaXQgZG9lc24ndD8NCj4gPiA+DQo+ID4NCj4gPiBIbS4uLiBzbyBJIGd1
ZXNzIEkgY291bGQgY2hlY2sgdGhhdCBpdCBkaWRuJ3QgcmV0dXJuIGFueSBlcnJvciBhbmQgaWYN
Cj4gPiBpdCBkaWQsIHRoZW4gZmxhZ3MgPSAwPw0KPiA+DQo+ID4gU29tZXRoaW5nIGxpa2UgdGhp
cz8gKG9uIHRvcCBvZiB0aGlzIHBhdGNoKToNCj4gPiBbc25pcF0NCj4gDQo+IHllYWggdGhhdCdz
IG9idmlvdXNseSB0aGUgZWFzeSB0aGluZyB0byBkbyAtIEkganVzdCB3YXNuJ3Qgc3VyZSB0aGF0
IHlvdSBkaWRuJ3QNCj4gaGF2ZSBhIHJlYXNvbiB0byBiZWxpZXZlIGl0IGNvdWxkIG5ldmVyIGJl
IGFuIEVSUl9QVFIgOikNCj4NClRCSCwgSSBkb24ndCByZWFsbHkga25vdy4uLiBJIGRvbid0IGtu
b3cgaWYgKmFsbCogdGhlIGNoYW5uZWxzIGFyZSBjb3ZlcmVkIGJ5IHJ1bGVzLg0KRm9yIHN1cmUs
IGlmIHRoZXJlIGlzIG5vIHJ1bGUsIEkgY2FuIGFzc3VtZSB0aGF0IHRoZSBIVDQweyssLX0gYXJl
IG5vdCBzZXQuLi4gQnV0IEkgZG9uJ3Qga25vdyBpZiB0aGF0J3MgZXZlbiBwb3NzaWJsZS4NCg0K
^ permalink raw reply
* Re: [PATCH 06/10] cfg80211: honor NL80211_RRF_NO_HT40{MINUS,PLUS}
From: Johannes Berg @ 2017-09-06 7:29 UTC (permalink / raw)
To: Grumbach, Emmanuel, luca@coelho.fi
Cc: linux-wireless@vger.kernel.org, Coelho, Luciano
In-Reply-To: <1504630175.23076.5.camel@intel.com>
On Tue, 2017-09-05 at 16:49 +0000, Grumbach, Emmanuel wrote:
> On Tue, 2017-09-05 at 16:30 +0200, Johannes Berg wrote:
> > On Sat, 2017-08-05 at 11:44 +0300, Luca Coelho wrote:
> >
> > > + regd = get_wiphy_regdom(wiphy);
> > > + if (regd) {
> > > + const struct ieee80211_reg_rule *reg_rule =
> > > + freq_reg_info_regd(MHZ_TO_KHZ(channel-
> > > > center_freq),
> > >
> > > + regd,
> > > MHZ_TO_KHZ(20));
> > >
> >
> > This could return an error, how can you be sure it doesn't?
> >
>
> Hm... so I guess I could check that it didn't return any error and if
> it did, then flags = 0?
>
> Something like this? (on top of this patch):
> [snip]
yeah that's obviously the easy thing to do - I just wasn't sure that
you didn't have a reason to believe it could never be an ERR_PTR :)
johannes
^ permalink raw reply
* Re: [PATCH 30/30] brcmfmac: Reduce the noise from repeatedly dereferencing common pointers
From: Arend van Spriel @ 2017-09-05 21:46 UTC (permalink / raw)
To: Ian Molton, linux-wireless
In-Reply-To: <20170822112550.60311-31-ian@mnementh.co.uk>
On 22-08-17 13:25, Ian Molton wrote:
> This introduces no functional changes, but makes the code drastically more
> readable, reducing the amount of dereferencing performed inside functions
> throughout the SDIO core.
>
> For example, reduce:
> sdio_release_host(bus->sdiodev->func1);
> to:
> sdio_release_host(func1);
I still have problems with this change. The example above does not
convince me. I explained my rule of thumb regarding using dereferences
and will stick to that.
> Fixup a few inconsistently named pointers whilst we are at it ie.
>
> sdiod -> sdiodev
>
> Signed-off-by: Ian Molton <ian@mnementh.co.uk>
> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> ---
> .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 140 +++----
> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 406 +++++++++++----------
> 2 files changed, 293 insertions(+), 253 deletions(-)
All I see is 40 more lines to read.
Regards,
Arend
^ permalink raw reply
* Re: [PATCH 27/30] brcmfmac: Replace function index with function pointer
From: Arend van Spriel @ 2017-09-05 21:21 UTC (permalink / raw)
To: Ian Molton, linux-wireless
In-Reply-To: <20170822112550.60311-28-ian@mnementh.co.uk>
On 22-08-17 13:25, Ian Molton wrote:
> In preparation for removing the function array, remove all code that
> refers to function by index and replace with pointers to the function
> itself.
Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Ian Molton <ian@mnementh.co.uk>
> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>
> # Conflicts:
> # drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> ---
> .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 83 ++++++++++++----------
> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 15 ++--
> .../wireless/broadcom/brcm80211/brcmfmac/sdio.h | 6 +-
> 3 files changed, 54 insertions(+), 50 deletions(-)
^ permalink raw reply
* Re: [PATCH 26/30] brcmfmac: More efficient and slightly easier to read fixup for 4339 chips
From: Arend van Spriel @ 2017-09-05 21:21 UTC (permalink / raw)
To: Ian Molton, linux-wireless
In-Reply-To: <20170822112550.60311-27-ian@mnementh.co.uk>
On 22-08-17 13:25, Ian Molton wrote:
> Its more efficient to test the register we're interested in first,
> potentially avoiding two more comparisons, and therefore always avoiding
> one comparison per call on all other chips.
Efficiency is really not a big issue. The buscore callbacks are really
only used during chip recognition so this feels like change for sake of
change. Not my favorite.
> Signed-off-by: Ian Molton <ian@mnementh.co.uk>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index 7ebe6460cb5c..8a730133db77 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -3766,15 +3766,18 @@ static u32 brcmf_sdio_buscore_read32(void *ctx, u32 addr)
> /* Force 4339 chips over rev2 to use the same ID */
> /* This is borderline tolerable whilst there is only two exceptions */
> /* But could be handled better */
This is not correct way to do multi-line comment and the story is not
quite correct or maybe I do not understand what is written here.
> - if ((sdiodev->func[1]->device == SDIO_DEVICE_ID_BROADCOM_4335_4339 ||
> - sdiodev->func[1]->device == SDIO_DEVICE_ID_BROADCOM_4339) &&
> - addr == CORE_CC_REG(SI_ENUM_BASE, chipid)) {
> + if (addr == CORE_CC_REG(SI_ENUM_BASE, chipid) &&
> + (sdiodev->func[1]->device == SDIO_DEVICE_ID_BROADCOM_4339 ||
> + sdiodev->func[1]->device == SDIO_DEVICE_ID_BROADCOM_4335_4339)) {
> +
> rev = (val & CID_REV_MASK) >> CID_REV_SHIFT;
> +
> if (rev >= 2) {
> val &= ~CID_ID_MASK;
> val |= BRCM_CC_4339_CHIP_ID;
> }
> }
> +
> return val;
> }
>
>
^ permalink raw reply
* Re: [PATCH 25/30] brcmfmac: Remove func0 from function array
From: Arend van Spriel @ 2017-09-05 21:21 UTC (permalink / raw)
To: Ian Molton, linux-wireless
In-Reply-To: <20170822112550.60311-26-ian@mnementh.co.uk>
On 22-08-17 13:25, Ian Molton wrote:
> Linux doesnt pass you func0 as a function when probing - instead
> providing specific access functions to read/write it.
>
> This prepares for a patch to remove the actual array entry itself.
Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Ian Molton <ian@mnementh.co.uk>
>
> # Conflicts:
> # drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 5 +----
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 10 +++++++---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h | 13 ++++++-------
> 3 files changed, 14 insertions(+), 14 deletions(-)
^ permalink raw reply
* Re: [PATCH 24/30] brcmfmac: Correctly handle accesses to SDIO func0
From: Arend van Spriel @ 2017-09-05 21:21 UTC (permalink / raw)
To: Ian Molton, linux-wireless
In-Reply-To: <20170822112550.60311-25-ian@mnementh.co.uk>
On 22-08-17 13:25, Ian Molton wrote:
> Rather than workaround the restrictions on func0 addressing in the
> driver, set MMC_QUIRK_LENIENT_FN0
Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Ian Molton <ian@mnementh.co.uk>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 ++++
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h | 4 ++--
> 2 files changed, 6 insertions(+), 2 deletions(-)
^ permalink raw reply
* Re: [PATCH 23/30] brcmfmac: stabilise the value of ->sbwad in use for some xfer routines.
From: Arend van Spriel @ 2017-09-05 21:21 UTC (permalink / raw)
To: Ian Molton, linux-wireless
In-Reply-To: <20170822112550.60311-24-ian@mnementh.co.uk>
On 22-08-17 13:25, Ian Molton wrote:
> The IO functions operate within the Chipcommon IO window. Explicitly
> set this, rather than relying on the last initialisation IO access to
> leave it set to the right value by chance.
Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Ian Molton <ian@mnementh.co.uk>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 8 ++++----
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 5 +++++
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h | 1 +
> 3 files changed, 10 insertions(+), 4 deletions(-)
^ permalink raw reply
* Re: [PATCH 22/30] brcmfmac: Rename buscore->core for consistency
From: Arend van Spriel @ 2017-09-05 21:20 UTC (permalink / raw)
To: Ian Molton, linux-wireless
In-Reply-To: <20170822112550.60311-23-ian@mnementh.co.uk>
On 22-08-17 13:25, Ian Molton wrote:
> Avoid confusion with unrelated _buscore labels.
Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Ian Molton <ian@mnementh.co.uk>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
^ permalink raw reply
* Re: [PATCH 21/30] brcmfmac: Remove {r,w}_sdreg32
From: Arend van Spriel @ 2017-09-05 21:20 UTC (permalink / raw)
To: Ian Molton, linux-wireless
In-Reply-To: <20170822112550.60311-22-ian@mnementh.co.uk>
On 22-08-17 13:25, Ian Molton wrote:
> Remove yet another IO function from the code and replace with one
> that already exists.
Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Ian Molton <ian@mnementh.co.uk>
> ---
> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 91 +++++++++++-----------
> 1 file changed, 45 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index 289ff61b7bf8..c1f3366871e5 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
[...]
> @@ -1075,6 +1051,8 @@ static void brcmf_sdio_get_console_addr(struct brcmf_sdio *bus)
>
> static u32 brcmf_sdio_hostmail(struct brcmf_sdio *bus)
> {
> + struct brcmf_sdio_dev *sdiod = bus->sdiodev;
> + struct brcmf_core *core = bus->sdio_core;
> u32 intstatus = 0;
> u32 hmb_data;
> u8 fcbits;
> @@ -1083,10 +1061,13 @@ static u32 brcmf_sdio_hostmail(struct brcmf_sdio *bus)
> brcmf_dbg(SDIO, "Enter\n");
>
> /* Read mailbox data and ack that we did so */
> - ret = r_sdreg32(bus, &hmb_data, __sd_reg(tohostmailboxdata));
> + hmb_data = brcmf_sdiod_readl(sdiod, core->base +
> + __sd_reg(tohostmailboxdata), &ret);
Please keep the address calculation (base + offset) in one line.
> +
> + if (!ret)
> + brcmf_sdiod_writel(sdiod, core->base + __sd_reg(tosbmailbox),
> + SMB_INT_ACK, &ret);
>
> - if (ret == 0)
> - w_sdreg32(bus, SMB_INT_ACK, __sd_reg(tosbmailbox));
> bus->sdcnt.f1regdata += 2;
>
> /* Dongle recomposed rx frames, accept them again */
[...]
> @@ -4044,7 +4041,9 @@ static void brcmf_sdio_firmware_callback(struct device *dev, int err,
> if (!err) {
> /* Set up the interrupt mask and enable interrupts */
> bus->hostintmask = HOSTINTMASK;
> - w_sdreg32(bus, bus->hostintmask, __sd_reg(hostintmask));
> + brcmf_sdiod_writel(sdiod, core->base + __sd_reg(hostintmask),
> + bus->hostintmask, NULL);
> +
>
> brcmf_sdiod_writeb(sdiodev, SBSDIO_WATERMARK, 8, &err);
It seems this function has two pointer variables, ie. sdiod and sdiodev,
pointing to the same instance.
> } else {
>
^ permalink raw reply
* Re: [PATCH 28/30] brcmfmac: Clean up interrupt macros
From: Arend van Spriel @ 2017-09-05 21:19 UTC (permalink / raw)
To: Ian Molton, linux-wireless
In-Reply-To: <20170822112550.60311-29-ian@mnementh.co.uk>
On 22-08-17 13:25, Ian Molton wrote:
> Make it more obvious that this code acually enables interrupts, and
> provide nice definitions for the bits in the register.
Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Ian Molton <ian@mnementh.co.uk>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 3 ++-
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h | 8 +++++---
> 2 files changed, 7 insertions(+), 4 deletions(-)
^ permalink raw reply
* Re: [PATCH 2/2] b43legacy: fix unitialized reads of ret by initializing the array to zero
From: Michael Büsch @ 2017-09-05 20:18 UTC (permalink / raw)
To: Colin King
Cc: Larry Finger, Kalle Valo, linux-wireless, b43-dev, netdev,
kernel-janitors, linux-kernel
In-Reply-To: <20170905181658.23893-1-colin.king@canonical.com>
[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]
On Tue, 5 Sep 2017 19:16:58 +0100
Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The u8 char array ret is not being initialized and elements outside
> the range start to end contain just garbage values from the stack.
> This results in a later scan of the array to read potentially
> uninitialized values. Fix this by initializing the array to zero.
> This seems to have been an issue since the very first commit.
>
> Detected by CoverityScan CID#139653 ("Uninitialized scalar variable")
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/wireless/broadcom/b43legacy/radio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/b43legacy/radio.c b/drivers/net/wireless/broadcom/b43legacy/radio.c
> index 9501420340a9..eab1c9387846 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/radio.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/radio.c
> @@ -280,7 +280,7 @@ u8 b43legacy_radio_aci_detect(struct b43legacy_wldev *dev, u8 channel)
> u8 b43legacy_radio_aci_scan(struct b43legacy_wldev *dev)
> {
> struct b43legacy_phy *phy = &dev->phy;
> - u8 ret[13];
> + u8 ret[13] = { 0 };
> unsigned int channel = phy->channel;
> unsigned int i;
> unsigned int j;
This fix seems to be correct.
Thanks for finding and fixing the issue.
Reviewed-by: Michael Buesch <m@bues.ch>
--
Michael
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] b43: fix unitialized reads of ret by initializing the array to zero
From: Michael Büsch @ 2017-09-05 20:18 UTC (permalink / raw)
To: Colin King
Cc: Kalle Valo, linux-wireless, b43-dev, netdev, kernel-janitors,
linux-kernel
In-Reply-To: <20170905181550.23839-1-colin.king@canonical.com>
[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]
On Tue, 5 Sep 2017 19:15:50 +0100
Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The u8 char array ret is not being initialized and elements outside
> the range start to end contain just garbage values from the stack.
> This results in a later scan of the array to read potentially
> uninitialized values. Fix this by initializing the array to zero.
> This seems to have been an issue since the very first commit.
>
> Detected by CoverityScan CID#139652 ("Uninitialized scalar variable")
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/wireless/broadcom/b43/phy_g.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/b43/phy_g.c b/drivers/net/wireless/broadcom/b43/phy_g.c
> index 822dcaa8ace6..f59c02166462 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_g.c
> +++ b/drivers/net/wireless/broadcom/b43/phy_g.c
> @@ -2297,7 +2297,7 @@ static u8 b43_gphy_aci_detect(struct b43_wldev *dev, u8 channel)
> static u8 b43_gphy_aci_scan(struct b43_wldev *dev)
> {
> struct b43_phy *phy = &dev->phy;
> - u8 ret[13];
> + u8 ret[13] = { 0 };
> unsigned int channel = phy->channel;
> unsigned int i, j, start, end;
>
This fix seems to be correct.
Thanks for finding and fixing the issue.
Reviewed-by: Michael Buesch <m@bues.ch>
--
Michael
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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