[PATCH 00/10] Ben's grab bag of mac80211 patches

[PATCH 00/10] Ben's grab bag of mac80211 patches

[greearb]

From: Ben Greear <greearb@candelatech.com>

Here are some patches from my tree, freshly applied and compile tested on top of
upstream 5.4.0-rc6.  One incorporates a fix suggested back in 2013 :)

There should not be much dependencies between these, so just
skip any you don't want.

Ben Greear (10):
  mac80211: Add comment about tx on monitor devs.
  mac80211: Change warn-on to warn-on-once
  mac80211: Don't spam kernel with sdata-in-driver failures.
  mac80211: Don't spam so loud about warned-sdata-in-driver.
  mac80211: Improve connection-loss probing.
  mac80211: Make max-auth-tries configurable as module option
  mac80211: Revert some of e8e4f5, fixes setting single rate in ath10k.
  mac80211: Support decrypting action frames for reinsertion into the
    driver.
  mac80211: Use warn-on-once for queue mis-configuration.
  mlme: Don't unlink bss on assoc timeout and similar.

 include/net/mac80211.h     | 10 +++++
 net/mac80211/cfg.c         |  6 ++-
 net/mac80211/debug.h       |  3 ++
 net/mac80211/driver-ops.c  |  9 ++++
 net/mac80211/driver-ops.h  | 36 ++++++++++++++--
 net/mac80211/ieee80211_i.h |  3 +-
 net/mac80211/iface.c       |  7 ++++
 net/mac80211/mlme.c        | 84 +++++++++++++++++---------------------
 net/mac80211/rx.c          |  2 +-
 net/mac80211/tx.c          |  6 +++
 net/mac80211/util.c        |  5 ++-
 11 files changed, 115 insertions(+), 56 deletions(-)

-- 
2.20.1

[PATCH 01/10] mac80211: Add comment about tx on monitor devs.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This tries to encapsulate email conversation with Johannes on
this topic for posterity's sake.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 net/mac80211/tx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1fa422782905..05982538c3cf 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2277,6 +2277,12 @@ netdev_tx_t ieee80211_monitor_start_xmit(struct sk_buff *skb,
 	 * isn't always enough to find the interface to use; for proper
 	 * VLAN/WDS support we will need a different mechanism (which
 	 * likely isn't going to be monitor interfaces).
+	 *
+	 * I had a question about why we need to do this, and the answer
+	 * is that old hostap used this API and expects it to work like this,
+	 * and also monitor vdevs are not directly mapped into the driver
+	 * (and have no chantx in my case, at least), so you cannot directly
+	 * transmit on a monitor port anyway.
 	 */
 	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
-- 
2.20.1

[PATCH 02/10] mac80211: Change warn-on to warn-on-once

[greearb]

From: Ben Greear <greearb@candelatech.com>

I saw a bunch of splats while reloading ath10k module, one splat is plenty.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 net/mac80211/rx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 0e05ff037672..670ae0cecc33 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -4553,7 +4553,7 @@ void ieee80211_rx_napi(struct ieee80211_hw *hw, struct ieee80211_sta *pubsta,
 	 * The same happens when we're not even started,
 	 * but that's worth a warning.
 	 */
-	if (WARN_ON(!local->started))
+	if (WARN_ON_ONCE(!local->started))
 		goto drop;
 
 	if (likely(!(status->flag & RX_FLAG_FAILED_PLCP_CRC))) {
-- 
2.20.1

[PATCH 03/10] mac80211: Don't spam kernel with sdata-in-driver failures.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Using 400 hwsim radios with 1 station on each, we see lots and
lots of kernel warnings about sdata-in-driver check failures:

Feb 19 17:08:22 kernel: WARNING: CPU: 0 PID: 24673 at /home/greearb/git/linux-4.2.dev.y/net/mac80211/driver-ops.h:12 ieee80211_bss_info_change_notify+0xe7/0x166 [mac80211]()
Feb 19 17:08:22 kernel: S170657910:  Failed check-sdata-in-driver check, flags: 0x0
Feb 19 17:08:22 kernel: Modules linked in: nf_conntrack_netlink nf_conntrack nfnetlink wanlink(O) mac80211_hwsim nf_defrag_ipv4 mac80211 cfg80211 8021q mrp garp stp llc macvlan pktgen iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi vfat fat ipv6 ppdev pvpanic parport_pc parport 8250_fintek pcspkr joydev i2c_piix4 floppy cirrus ttm drm_kms_helper drm i2c_core [last unloaded: nfnetlink]
Feb 19 17:08:22 kernel: CPU: 0 PID: 24673 Comm: iwconfig Tainted: G        W  O    4.2.8+ #53
Feb 19 17:08:22 kernel: Hardware name: OpenStack Foundation OpenStack Nova, BIOS Bochs 01/01/2011
Feb 19 17:08:22 kernel: 0000000000000009 ffff8800bb233b68 ffffffff81696197 0000000000000006
Feb 19 17:08:22 kernel: ffff8800bb233bb8 ffff8800bb233ba8 ffffffff810e1fb4 ffff8800bb233c68
Feb 19 17:08:22 kernel: ffffffffa02ec8f2 ffff88037424a800 0000000000040000 ffff88037424b018
Feb 19 17:08:22 kernel: Call Trace:
Feb 19 17:08:22 kernel: [<ffffffff81696197>] dump_stack+0x4c/0x6e
Feb 19 17:08:22 kernel: [<ffffffff810e1fb4>] warn_slowpath_common+0x96/0xb0
Feb 19 17:08:22 kernel: [<ffffffffa02ec8f2>] ? ieee80211_bss_info_change_notify+0xe7/0x166 [mac80211]
Feb 19 17:08:22 kernel: [<ffffffff810e200f>] warn_slowpath_fmt+0x41/0x43
Feb 19 17:08:22 kernel: [<ffffffffa02ec8f2>] ieee80211_bss_info_change_notify+0xe7/0x166 [mac80211]
Feb 19 17:08:22 kernel: [<ffffffffa02fda00>] ieee80211_recalc_txpower+0x28/0x2d [mac80211]
Feb 19 17:08:22 kernel: [<ffffffffa0304155>] ieee80211_set_tx_power+0x8a/0x152 [mac80211]
Feb 19 17:08:22 kernel: [<ffffffffa027e4e1>] cfg80211_wext_siwtxpower+0xdc/0x12b [cfg80211]
Feb 19 17:08:22 kernel: [<ffffffff816837dc>] ioctl_standard_call+0x4a/0xa3
Feb 19 17:08:22 kernel: [<ffffffff816840f5>] ? iw_handler_get_private+0x49/0x49
Feb 19 17:08:22 kernel: [<ffffffff81683792>] ? call_commit_handler+0x2c/0x2c
Feb 19 17:08:22 kernel: [<ffffffff81682fba>] wireless_process_ioctl+0x6b/0x124
Feb 19 17:08:22 kernel: [<ffffffff816840f5>] ? iw_handler_get_private+0x49/0x49
Feb 19 17:08:22 kernel: [<ffffffff81683915>] wext_handle_ioctl+0x64/0xa1
Feb 19 17:08:22 kernel: [<ffffffff811f5af6>] ? inode_init_always+0x109/0x1b0
Feb 19 17:08:22 kernel: [<ffffffff815ffd30>] dev_ioctl+0x5a5/0x5d6
Feb 19 17:08:22 kernel: [<ffffffff811b3650>] ? handle_mm_fault+0xe6c/0xeb5
Feb 19 17:08:22 kernel: [<ffffffff815d4f12>] sock_ioctl+0x46/0x208
Feb 19 17:08:22 kernel: [<ffffffff811efb3c>] do_vfs_ioctl+0x372/0x420
Feb 19 17:08:22 kernel: [<ffffffff811365d2>] ? current_kernel_time+0x9/0x2d
Feb 19 17:08:22 kernel: [<ffffffff8115c765>] ? __audit_syscall_entry+0xbc/0xde
Feb 19 17:08:22 kernel: [<ffffffff811f7a59>] ? __fget_light+0x28/0x4a
Feb 19 17:08:22 kernel: [<ffffffff811efc3f>] SyS_ioctl+0x55/0x7a
Feb 19 17:08:22 kernel: [<ffffffff8169bcf2>] entry_SYSCALL_64_fastpath+0x16/0x75
Feb 19 17:08:22 kernel: ---[ end trace d4e525588dd9d9fc ]---
Feb 19 17:08:22 kernel: cfg80211: Setting DFS Master region in update_regulatory, was: 00 unset, new: 00 unset  lr: ffff8803ca63b300  regdom: ffffffffa0280f50

Instead, warn once, and then be silent after that.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 net/mac80211/driver-ops.h | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 2c9b3eb8b652..d8967cd461fe 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -19,6 +19,17 @@ static inline bool check_sdata_in_driver(struct ieee80211_sub_if_data *sdata)
 		     sdata->dev ? sdata->dev->name : sdata->name, sdata->flags);
 }
 
+static inline bool _check_sdata_in_driver(struct ieee80211_sub_if_data *sdata,
+					  bool can_warn)
+{
+	if (can_warn)
+		return !WARN(!(sdata->flags & IEEE80211_SDATA_IN_DRIVER),
+			     "%s:  Failed check-sdata-in-driver check, flags: 0x%x\n",
+			     sdata->dev ? sdata->dev->name : sdata->name, sdata->flags);
+
+	return (sdata->flags & IEEE80211_SDATA_IN_DRIVER);
+}
+
 static inline struct ieee80211_sub_if_data *
 get_bss_sdata(struct ieee80211_sub_if_data *sdata)
 {
@@ -153,6 +164,7 @@ static inline void drv_bss_info_changed(struct ieee80211_local *local,
 					struct ieee80211_bss_conf *info,
 					u32 changed)
 {
+	static int warn_once = true;
 	might_sleep();
 
 	if (WARN_ON_ONCE(changed & (BSS_CHANGED_BEACON |
@@ -170,8 +182,10 @@ static inline void drv_bss_info_changed(struct ieee80211_local *local,
 			  !(changed & BSS_CHANGED_TXPOWER))))
 		return;
 
-	if (!check_sdata_in_driver(sdata))
+	if (!_check_sdata_in_driver(sdata, warn_once)) {
+		warn_once = false;
 		return;
+	}
 
 	trace_drv_bss_info_changed(local, sdata, info, changed);
 	if (local->ops->bss_info_changed)
-- 
2.20.1

[PATCH 04/10] mac80211: Don't spam so loud about warned-sdata-in-driver.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Once per sdata is plenty, and possibly still too much.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 net/mac80211/driver-ops.h  | 17 ++++++++++++++---
 net/mac80211/ieee80211_i.h |  2 ++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index d8967cd461fe..e734a85165ad 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -14,9 +14,20 @@
 
 static inline bool check_sdata_in_driver(struct ieee80211_sub_if_data *sdata)
 {
-	return !WARN(!(sdata->flags & IEEE80211_SDATA_IN_DRIVER),
-		     "%s:  Failed check-sdata-in-driver check, flags: 0x%x\n",
-		     sdata->dev ? sdata->dev->name : sdata->name, sdata->flags);
+	if (unlikely(!(sdata->flags & IEEE80211_SDATA_IN_DRIVER))) {
+		if (!sdata->warned_sdata_in_driver) {
+			WARN(1, "%s:  Failed check-sdata-in-driver check, flags: 0x%x\n",
+			     sdata->dev ? sdata->dev->name : sdata->name, sdata->flags);
+			sdata->warned_sdata_in_driver = true;
+		}
+		else {
+			/* just print error instead of full WARN spam */
+			sdata_err(sdata, "Failed check-sdata-in-driver check, flags: 0x%x\n",
+				  sdata->flags);
+		}
+		return false;
+	}
+	return true;
 }
 
 static inline bool _check_sdata_in_driver(struct ieee80211_sub_if_data *sdata,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 05406e9c05b3..5594ab80d9c1 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -932,6 +932,8 @@ struct ieee80211_sub_if_data {
 	bool reserved_radar_required;
 	bool reserved_ready;
 
+	bool warned_sdata_in_driver;
+
 	/* used to reconfigure hardware SM PS */
 	struct work_struct recalc_smps;
 
-- 
2.20.1

[PATCH 05/10] mac80211: Improve connection-loss probing.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This makes the mlme code probe multiple times spread out across
the time-out period instead of lots of probes all at once.

It also makes the ap-probe and the nullfunc probe logic
act similarly, which allows us to remove some code.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 net/mac80211/debug.h       |  3 ++
 net/mac80211/ieee80211_i.h |  1 -
 net/mac80211/mlme.c        | 64 +++++++++++++++-----------------------
 3 files changed, 28 insertions(+), 40 deletions(-)

diff --git a/net/mac80211/debug.h b/net/mac80211/debug.h
index d90a8f9cc3fd..6182608cd20f 100644
--- a/net/mac80211/debug.h
+++ b/net/mac80211/debug.h
@@ -194,6 +194,9 @@ do {									\
 	_sdata_dbg(MAC80211_MLME_DEBUG,					\
 		   sdata, fmt, ##__VA_ARGS__)
 
+#define mlme_wrn(sdata, fmt, ...)					\
+	_sdata_err(sdata, fmt, ##__VA_ARGS__)
+
 #define mlme_dbg_ratelimited(sdata, fmt, ...)				\
 	_sdata_dbg(MAC80211_MLME_DEBUG && net_ratelimit(),		\
 		   sdata, fmt, ##__VA_ARGS__)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 5594ab80d9c1..46bc9d3df591 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -444,7 +444,6 @@ struct ieee80211_if_managed {
 	unsigned long beacon_timeout;
 	unsigned long probe_timeout;
 	int probe_send_count;
-	bool nullfunc_failed;
 	bool connection_loss;
 
 	struct cfg80211_bss *associated;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 54dd8849d1cc..6a458aac331d 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -2464,8 +2464,6 @@ void ieee80211_sta_tx_notify(struct ieee80211_sub_if_data *sdata,
 	    sdata->u.mgd.probe_send_count > 0) {
 		if (ack)
 			ieee80211_sta_reset_conn_monitor(sdata);
-		else
-			sdata->u.mgd.nullfunc_failed = true;
 		ieee80211_queue_work(&sdata->local->hw, &sdata->work);
 		return;
 	}
@@ -2495,6 +2493,7 @@ static void ieee80211_mgd_probe_ap_send(struct ieee80211_sub_if_data *sdata)
 	u8 *dst = ifmgd->associated->bssid;
 	u8 unicast_limit = max(1, max_probe_tries - 3);
 	struct sta_info *sta;
+	u32 max_tries;
 
 	/*
 	 * Try sending broadcast probe requests for the last three
@@ -2522,7 +2521,7 @@ static void ieee80211_mgd_probe_ap_send(struct ieee80211_sub_if_data *sdata)
 	}
 
 	if (ieee80211_hw_check(&sdata->local->hw, REPORTS_TX_ACK_STATUS)) {
-		ifmgd->nullfunc_failed = false;
+		max_tries = max_nullfunc_tries;
 		if (!(ifmgd->flags & IEEE80211_STA_DISABLE_HE))
 			ifmgd->probe_send_count--;
 		else
@@ -2530,6 +2529,8 @@ static void ieee80211_mgd_probe_ap_send(struct ieee80211_sub_if_data *sdata)
 	} else {
 		int ssid_len;
 
+		max_tries = max_probe_tries;
+
 		rcu_read_lock();
 		ssid = ieee80211_bss_get_ie(ifmgd->associated, WLAN_EID_SSID);
 		if (WARN_ON_ONCE(ssid == NULL))
@@ -2543,7 +2544,8 @@ static void ieee80211_mgd_probe_ap_send(struct ieee80211_sub_if_data *sdata)
 		rcu_read_unlock();
 	}
 
-	ifmgd->probe_timeout = jiffies + msecs_to_jiffies(probe_wait_ms);
+	ifmgd->probe_timeout = jiffies +
+		msecs_to_jiffies(probe_wait_ms / max_tries);
 	run_again(sdata, ifmgd->probe_timeout);
 }
 
@@ -4388,54 +4390,38 @@ void ieee80211_sta_work(struct ieee80211_sub_if_data *sdata)
 	    ifmgd->associated) {
 		u8 bssid[ETH_ALEN];
 		int max_tries;
+		bool ack_status = ieee80211_hw_check(&local->hw,
+						     REPORTS_TX_ACK_STATUS);
 
 		memcpy(bssid, ifmgd->associated->bssid, ETH_ALEN);
 
-		if (ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS))
+		if (ack_status)
 			max_tries = max_nullfunc_tries;
 		else
 			max_tries = max_probe_tries;
 
 		/* ACK received for nullfunc probing frame */
-		if (!ifmgd->probe_send_count)
+		if (!ifmgd->probe_send_count) {
+			/* probe_send_count of zero means probe succeeded */
 			ieee80211_reset_ap_probe(sdata);
-		else if (ifmgd->nullfunc_failed) {
-			if (ifmgd->probe_send_count < max_tries) {
-				mlme_dbg(sdata,
-					 "No ack for nullfunc frame to AP %pM, try %d/%i\n",
-					 bssid, ifmgd->probe_send_count,
-					 max_tries);
-				ieee80211_mgd_probe_ap_send(sdata);
-			} else {
-				mlme_dbg(sdata,
-					 "No ack for nullfunc frame to AP %pM, disconnecting.\n",
-					 bssid);
-				ieee80211_sta_connection_lost(sdata, bssid,
-					WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
-					false);
-			}
-		} else if (time_is_after_jiffies(ifmgd->probe_timeout))
+		} else if (time_is_after_jiffies(ifmgd->probe_timeout)) {
+			/* probe_timeout is after current jiffies
+			 * Not time to (re)probe yet
+			 */
 			run_again(sdata, ifmgd->probe_timeout);
-		else if (ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS)) {
-			mlme_dbg(sdata,
-				 "Failed to send nullfunc to AP %pM after %dms, disconnecting\n",
-				 bssid, probe_wait_ms);
-			ieee80211_sta_connection_lost(sdata, bssid,
-				WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY, false);
 		} else if (ifmgd->probe_send_count < max_tries) {
-			mlme_dbg(sdata,
-				 "No probe response from AP %pM after %dms, try %d/%i\n",
-				 bssid, probe_wait_ms,
-				 ifmgd->probe_send_count, max_tries);
+			mlme_dbg(sdata, "No %s AP %pM, try %d/%i\n",
+				 ack_status ? "ack for nullfunc frame to" :
+				 "probe response from",
+				 bssid, ifmgd->probe_send_count, max_tries);
 			ieee80211_mgd_probe_ap_send(sdata);
 		} else {
-			/*
-			 * We actually lost the connection ... or did we?
-			 * Let's make sure!
-			 */
-			mlme_dbg(sdata,
-				 "No probe response from AP %pM after %dms, disconnecting.\n",
-				 bssid, probe_wait_ms);
+			mlme_wrn(sdata,
+				 "No %s AP %pM after %dms, tried %d/%i, disconnecting.\n",
+				 ack_status ? "ack for nullfunc frame to" :
+				 "probe response from",
+				  bssid, probe_wait_ms, ifmgd->probe_send_count,
+				  max_tries);
 
 			ieee80211_sta_connection_lost(sdata, bssid,
 				WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY, false);
-- 
2.20.1

[PATCH 06/10] mac80211: Make max-auth-tries configurable as module option

[greearb]

From: Ben Greear <greearb@candelatech.com>

In some cases, one might wish to have more retries before giving up
on auth.  For instance, ath10k 9984 radios take about 270ms to
(re)calibrate when moving channels, and auth will not go out during
that calibration period.  I think that might be part of the issue
I see with roaming stations on that radio.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 net/mac80211/mlme.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 6a458aac331d..64336433925d 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -35,13 +35,17 @@
 #define IEEE80211_AUTH_TIMEOUT_LONG	(HZ / 2)
 #define IEEE80211_AUTH_TIMEOUT_SHORT	(HZ / 10)
 #define IEEE80211_AUTH_TIMEOUT_SAE	(HZ * 2)
-#define IEEE80211_AUTH_MAX_TRIES	3
 #define IEEE80211_AUTH_WAIT_ASSOC	(HZ * 5)
 #define IEEE80211_ASSOC_TIMEOUT		(HZ / 5)
 #define IEEE80211_ASSOC_TIMEOUT_LONG	(HZ / 2)
 #define IEEE80211_ASSOC_TIMEOUT_SHORT	(HZ / 10)
 #define IEEE80211_ASSOC_MAX_TRIES	3
 
+static int max_auth_tries = 3;
+module_param(max_auth_tries, int, 0644);
+MODULE_PARM_DESC(max_auth_tries,
+		 "Maximum auth tries before giving up (default is 3).");
+
 static int max_nullfunc_tries = 2;
 module_param(max_nullfunc_tries, int, 0644);
 MODULE_PARM_DESC(max_nullfunc_tries,
@@ -4189,9 +4193,9 @@ static int ieee80211_auth(struct ieee80211_sub_if_data *sdata)
 
 	auth_data->tries++;
 
-	if (auth_data->tries > IEEE80211_AUTH_MAX_TRIES) {
-		sdata_info(sdata, "authentication with %pM timed out\n",
-			   auth_data->bss->bssid);
+	if (auth_data->tries > max_auth_tries) {
+		sdata_info(sdata, "authentication with %pM timed out after %d tries\n",
+			   auth_data->bss->bssid, max_auth_tries);
 
 		/*
 		 * Most likely AP is not in the range so remove the
@@ -4210,7 +4214,7 @@ static int ieee80211_auth(struct ieee80211_sub_if_data *sdata)
 
 	sdata_info(sdata, "send auth to %pM (try %d/%d)\n",
 		   auth_data->bss->bssid, auth_data->tries,
-		   IEEE80211_AUTH_MAX_TRIES);
+		   max_auth_tries);
 
 	auth_data->expected_transaction = 2;
 
-- 
2.20.1

[PATCH 07/10] mac80211: Revert some of e8e4f5, fixes setting single rate in ath10k.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This lets us successfully set a single rate in ath10k again.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 net/mac80211/cfg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 70739e746c13..41d55bd1b43a 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2877,8 +2877,10 @@ static int ieee80211_set_bitrate_mask(struct wiphy *wiphy,
 		u32 basic_rates = sdata->vif.bss_conf.basic_rates;
 		enum nl80211_band band = sdata->vif.bss_conf.chandef.chan->band;
 
-		if (!(mask->control[band].legacy & basic_rates))
-			return -EINVAL;
+		if (!(mask->control[band].legacy & basic_rates)) {
+			pr_err("%s:  WARNING: no legacy rates for band[%d] in set-bitrate-mask.\n",
+			       sdata->dev->name, band);
+		}
 	}
 
 	if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)) {
-- 
2.20.1

[PATCH 08/10] mac80211: Support decrypting action frames for reinsertion into the driver.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Some drivers, like ath10k-ct support rxswcrypt, but the firmware needs to
handle at least part of the blockack logic internally.  Since firmware cannot
decode the frame in rxswcrypt mode, this patch adds a way for the driver to
request to be delivered the decrypted blockack action frames.  The
driver can then re-insert the decrypted frame for proper handling.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 include/net/mac80211.h    | 10 ++++++++++
 net/mac80211/driver-ops.c |  9 +++++++++
 net/mac80211/driver-ops.h |  3 +++
 net/mac80211/iface.c      |  7 +++++++
 4 files changed, 29 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 523c6a09e1c8..7ea9a17c8cb8 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3752,6 +3752,14 @@ enum ieee80211_reconfig_type {
  *
  * @start_pmsr: start peer measurement (e.g. FTM) (this call can sleep)
  * @abort_pmsr: abort peer measurement (this call can sleep)
+ * @consume_block_ack: Offer block-ack management frames back to driver to see
+ *      if it wishes to consume it.  This can be useful for when firmware wants
+ *      to handle block-ack logic itself, but PMF is used and the firmware
+ *      cannot actually decode the block-ack frames itself.  So, firmware can
+ *      pass the encoded block-ack up the stack, and receive it through this
+ *      callback.  If return value is zero, the mac80211 stack will not further
+ *      process the skb.  skb will be freed by calling code, so driver must
+ *      make a copy of anything it needs in the skb before returning.
  */
 struct ieee80211_ops {
 	void (*tx)(struct ieee80211_hw *hw,
@@ -4043,6 +4051,8 @@ struct ieee80211_ops {
 	void (*del_nan_func)(struct ieee80211_hw *hw,
 			    struct ieee80211_vif *vif,
 			    u8 instance_id);
+	int (*consume_block_ack)(struct ieee80211_hw *hw,
+				 struct ieee80211_vif *vif, struct sk_buff* skb);
 	bool (*can_aggregate_in_amsdu)(struct ieee80211_hw *hw,
 				       struct sk_buff *head,
 				       struct sk_buff *skb);
diff --git a/net/mac80211/driver-ops.c b/net/mac80211/driver-ops.c
index c9a8a2433e8a..aba803bd2718 100644
--- a/net/mac80211/driver-ops.c
+++ b/net/mac80211/driver-ops.c
@@ -49,6 +49,15 @@ void drv_stop(struct ieee80211_local *local)
 	local->started = false;
 }
 
+int drv_consume_block_ack(struct ieee80211_local *local,
+			  struct ieee80211_sub_if_data *sdata, struct sk_buff *skb)
+{
+	/*pr_warn("consume-block-ack: %p\n", local->ops->consume_block_ack);*/
+	if (local->ops->consume_block_ack)
+		return local->ops->consume_block_ack(&local->hw, &sdata->vif, skb);
+	return -EINVAL;
+}
+
 int drv_add_interface(struct ieee80211_local *local,
 		      struct ieee80211_sub_if_data *sdata)
 {
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index e734a85165ad..b3cf5d88f83e 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -148,6 +148,9 @@ static inline void drv_set_wakeup(struct ieee80211_local *local,
 }
 #endif
 
+int drv_consume_block_ack(struct ieee80211_local *local,
+			  struct ieee80211_sub_if_data *sdata, struct sk_buff *skb);
+
 int drv_add_interface(struct ieee80211_local *local,
 		      struct ieee80211_sub_if_data *sdata);
 
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index af8b09214786..f050410d0986 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1249,6 +1249,11 @@ static void ieee80211_iface_work(struct work_struct *work)
 		if (ieee80211_is_action(mgmt->frame_control) &&
 		    mgmt->u.action.category == WLAN_CATEGORY_BACK) {
 			int len = skb->len;
+			int barv = drv_consume_block_ack(local, sdata, skb);
+
+			/*pr_err("called drv_consume_blockack, rv: %d\n", barv);*/
+			if (barv == 0)
+				goto done_skb_free;
 
 			mutex_lock(&local->sta_mtx);
 			sta = sta_info_get_bss(sdata, mgmt->sa);
@@ -1349,6 +1354,8 @@ static void ieee80211_iface_work(struct work_struct *work)
 			break;
 		}
 
+	done_skb_free:
+
 		kfree_skb(skb);
 	}
 
-- 
2.20.1

[PATCH 09/10] mac80211: Use warn-on-once for queue mis-configuration.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Don't spam logs if a user manages to hit this warning.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 net/mac80211/util.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 32a7a53833c0..21c2f439fc6a 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -387,8 +387,11 @@ static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue,
 
 	trace_wake_queue(local, queue, reason);
 
-	if (WARN_ON(queue >= hw->queues))
+	if (WARN_ON_ONCE(queue >= hw->queues)) {
+		pr_err("wake-queue, queue: %d > hw->queues: %d\n",
+		       queue, hw->queues);
 		return;
+	}
 
 	if (!test_bit(reason, &local->queue_stop_reasons[queue]))
 		return;
-- 
2.20.1

Re: [PATCH 00/10] Ben's grab bag of mac80211 patches

[Johannes Berg]

Hi Ben,

Well, for me to consider any of these, you really should come with
commit logs and reasons that actually make sense outside of your
environment :)

I committed a note that replaces patch 1.

I don't like 2-4, that just adds state and one should never hit it. If
you're hitting these you should investigate the root cause, not fix the
symptoms.

5 seems like it might be reasonable, but it's hard to read and
understand, maybe revisit that?

I tend to think the previous module options along the lines of patch 6
were a mistake, rather than add more ...

7 is totally not understandable, but might be legitimate? Unlikely, but
hard to say.

8 I don't like at all. How about you do it in the driver somehow?

9 is like 2-4 really, I guess maybe this one I could get behind if it
came with a commit log that actually explains why one is likely to hit
this multiple times or something?

10 we did to fix other behaviour, so ...

johannes

Re: [PATCH 00/10] Ben's grab bag of mac80211 patches

[Ben Greear]

On 11/22/2019 03:49 AM, Johannes Berg wrote:
> Hi Ben,
>
> Well, for me to consider any of these, you really should come with
> commit logs and reasons that actually make sense outside of your
> environment :)
>
> I committed a note that replaces patch 1.

Well, 1 out of 10 ain't that bad!

> I don't like 2-4, that just adds state and one should never hit it. If
> you're hitting these you should investigate the root cause, not fix the
> symptoms.
>
> 5 seems like it might be reasonable, but it's hard to read and
> understand, maybe revisit that?

This is the patch you previously said you liked but it would not apply upstream.
Now it applies.  That whole code mess is hard to understand, but I have been running
a similar patch for a while and it has worked well.

Instead of trying to understand the patch, try applying it and then read the resulting
code.  It is a lot simpler to understand that way I think.

You can also sniff air with current code w/out this patch and watch the crappy
retry behaviour where the retries are clumped in a few ms of time instead of being
spread out.

>
> I tend to think the previous module options along the lines of patch 6
> were a mistake, rather than add more ...
>
> 7 is totally not understandable, but might be legitimate? Unlikely, but
> hard to say.

Ath10k will always use legacy rates for ctrl frames and such even if
you otherwise restrict the rateset.  So, it is legit to set a single rate even if
that would leave no legacy rates configured as far as mac80211 is concerned.

Your patch broke the ability to set a single rate in ath10k, and my change will
allow it to work again.

>
> 8 I don't like at all. How about you do it in the driver somehow?

I had low hopes for this one anyway.  mac80211 has the software decrypt logic, not the
driver, so it seemed reasonable to have the mac80211 do a callback.  This patch is likely
only useful for drivers that do block-ack in the firmware and support software decrypt,
which may only be my modified ath10k-ct driver/firmware.

>
> 9 is like 2-4 really, I guess maybe this one I could get behind if it
> came with a commit log that actually explains why one is likely to hit
> this multiple times or something?

Basically, it is almost never useful to use WARN_ON instead of WARN_ON_ONCE.  If you ever
do hit the bug, often the logs are full of WARN_ON spam and you cannot even find the real problem
until you change it to WARN_ON_ONCE and reproduce the problem.

I hit this problem due to some coding bug while poking at ath10k, and you get one splat
per tx frame, so you can image the spam.


> 10 we did to fix other behaviour, so ...

This one is especially useful for roaming several virtual stations, but maybe that is only useful test
case.  I included it more as an RFC, but it has worked well enough for us in case you see some worth in
it (and obviously it should be changed to not use // comments in case the functionality is actually
deemed useful).

Thanks,
Ben

>
> johannes
>

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 00/10] Ben's grab bag of mac80211 patches

[Johannes Berg]

On Fri, 2019-11-22 at 08:37 -0800, Ben Greear wrote:

> Well, 1 out of 10 ain't that bad!

Heh.

> > 5 seems like it might be reasonable, but it's hard to read and
> > understand, maybe revisit that?
> 
> This is the patch you previously said you liked but it would not apply upstream.
> Now it applies.  That whole code mess is hard to understand, but I have been running
> a similar patch for a while and it has worked well.
> 
> Instead of trying to understand the patch, try applying it and then read the resulting
> code.  It is a lot simpler to understand that way I think.

OK :)

> You can also sniff air with current code w/out this patch and watch the crappy
> retry behaviour where the retries are clumped in a few ms of time instead of being
> spread out.

Right. I'll have to review this carefully. I think the Intel firmware
will soon spread out the hardware retries a bit too (vs. here the
software retries).

> Ath10k will always use legacy rates for ctrl frames and such even if

*control* frames have a very tightly controlled rate, e.g. ACK must have
the same or next lower mandatory rate, or basic rate, or something. I
don't have it in my head right now.

Do you mean *management* frames?

> you otherwise restrict the rateset.  So, it is legit to set a single rate even if
> that would leave no legacy rates configured as far as mac80211 is concerned.
> 
> Your patch broke the ability to set a single rate in ath10k, and my change will
> allow it to work again.

Either way, it's kind of an ath10k bug in a sense. You don't actually
want to "set this single rate", you want to "set this single rate for
data frames" or so ... which is a different API.

If you look at ath9k or any other driver with software rate control, it
would not be able to pick a legacy rate for mgmt frames at all with your
patch and the warning, and then I'm pretty sure you'll hit a WARN_ON()
in rate.c.

So no, I will certainly not apply this.

Maybe you want to add a different API that affects only data frames (or
a variant of this API or something), instead, but this is not it. We
can't leave the software drivers without any rates to pick from for
management frames.

> > 8 I don't like at all. How about you do it in the driver somehow?
> 
> I had low hopes for this one anyway.  mac80211 has the software decrypt logic, not the
> driver, so it seemed reasonable to have the mac80211 do a callback.  This patch is likely
> only useful for drivers that do block-ack in the firmware and support software decrypt,
> which may only be my modified ath10k-ct driver/firmware.

So far that's the only one, as far as I can tell, yeah ...

> > 9 is like 2-4 really, I guess maybe this one I could get behind if it
> > came with a commit log that actually explains why one is likely to hit
> > this multiple times or something?
> 
> Basically, it is almost never useful to use WARN_ON instead of WARN_ON_ONCE.  If you ever
> do hit the bug, often the logs are full of WARN_ON spam and you cannot even find the real problem
> until you change it to WARN_ON_ONCE and reproduce the problem.
> 
> I hit this problem due to some coding bug while poking at ath10k, and you get one splat
> per tx frame, so you can image the spam.

Yeah, but does it matter? It should really just happen during
development, right? Having the _ONCE makes this cost more space and
code, and I'm not sure I see that much point in that.

> > 10 we did to fix other behaviour, so ...
> 
> This one is especially useful for roaming several virtual stations, but maybe that is only useful test
> case.  I included it more as an RFC, but it has worked well enough for us in case you see some worth in
> it (and obviously it should be changed to not use // comments in case the functionality is actually
> deemed useful).

I guess we can go back and forth on this ... but in most cases you don't
know that the AP didn't go away, so IMHO the current behaviour is
better. It's just in your special case you know it's more likely some
local issues vs. the AP having disappeared.

johannes

Re: [PATCH 00/10] Ben's grab bag of mac80211 patches

[Ben Greear]

On 11/22/19 9:53 AM, Johannes Berg wrote:
> On Fri, 2019-11-22 at 08:37 -0800, Ben Greear wrote:
> 
>> Well, 1 out of 10 ain't that bad!
> 
> Heh.
> 
>>> 5 seems like it might be reasonable, but it's hard to read and
>>> understand, maybe revisit that?
>>
>> This is the patch you previously said you liked but it would not apply upstream.
>> Now it applies.  That whole code mess is hard to understand, but I have been running
>> a similar patch for a while and it has worked well.
>>
>> Instead of trying to understand the patch, try applying it and then read the resulting
>> code.  It is a lot simpler to understand that way I think.
> 
> OK :)
> 
>> You can also sniff air with current code w/out this patch and watch the crappy
>> retry behaviour where the retries are clumped in a few ms of time instead of being
>> spread out.
> 
> Right. I'll have to review this carefully. I think the Intel firmware
> will soon spread out the hardware retries a bit too (vs. here the
> software retries).
> 
>> Ath10k will always use legacy rates for ctrl frames and such even if
> 
> *control* frames have a very tightly controlled rate, e.g. ACK must have
> the same or next lower mandatory rate, or basic rate, or something. I
> don't have it in my head right now.
> 
> Do you mean *management* frames?

Yes, I mean all sorts of frames that should use legacy rates.  When you 'set the rate'
in ath10k, you are really just setting the data-packet's rate, and upstream driver/firmware
has other limitations (you can set single rate, or mcs-8 or mcs-9, but you cannot tell it to use
mcs-5 and mcs-6 only, for instance).

There are other ways to adjust management frame rates, but it is not through normal
mac80211 rate-setting logic.

>> you otherwise restrict the rateset.  So, it is legit to set a single rate even if
>> that would leave no legacy rates configured as far as mac80211 is concerned.
>>
>> Your patch broke the ability to set a single rate in ath10k, and my change will
>> allow it to work again.
> 
> Either way, it's kind of an ath10k bug in a sense. You don't actually
> want to "set this single rate", you want to "set this single rate for
> data frames" or so ... which is a different API. >
> If you look at ath9k or any other driver with software rate control, it
> would not be able to pick a legacy rate for mgmt frames at all with your
> patch and the warning, and then I'm pretty sure you'll hit a WARN_ON()
> in rate.c.
> 
> So no, I will certainly not apply this.
> 
> Maybe you want to add a different API that affects only data frames (or
> a variant of this API or something), instead, but this is not it. We
> can't leave the software drivers without any rates to pick from for
> management frames.

Maybe we can add a driver flag that indicates we can relax this check
and have ath10k set that flag?  Adding a whole top to bottom API to change
how rates are set sounds like a pain, and would probably be specific to
ath10k only in the end anyway?

(Remember the discussion about setting advertised vs used rates?  This would
  be even more complex and less useful than that I think).

> 
>>> 8 I don't like at all. How about you do it in the driver somehow?
>>
>> I had low hopes for this one anyway.  mac80211 has the software decrypt logic, not the
>> driver, so it seemed reasonable to have the mac80211 do a callback.  This patch is likely
>> only useful for drivers that do block-ack in the firmware and support software decrypt,
>> which may only be my modified ath10k-ct driver/firmware.
> 
> So far that's the only one, as far as I can tell, yeah ...
> 
>>> 9 is like 2-4 really, I guess maybe this one I could get behind if it
>>> came with a commit log that actually explains why one is likely to hit
>>> this multiple times or something?
>>
>> Basically, it is almost never useful to use WARN_ON instead of WARN_ON_ONCE.  If you ever
>> do hit the bug, often the logs are full of WARN_ON spam and you cannot even find the real problem
>> until you change it to WARN_ON_ONCE and reproduce the problem.
>>
>> I hit this problem due to some coding bug while poking at ath10k, and you get one splat
>> per tx frame, so you can image the spam.
> 
> Yeah, but does it matter? It should really just happen during
> development, right? Having the _ONCE makes this cost more space and
> code, and I'm not sure I see that much point in that.

Sure, for this one.  The sdata-in-driver warnings happen often-ish in ath10k.  No one can figure
out how to fix it, or really cares to look I think.

But, really, not worth arguing over.  The main patch in this series that I would like to see upstream
is the one that tweaks retry logic.  Others are fairly easy to maintain outside the tree and are
of more limited use to the general user.

> 
>>> 10 we did to fix other behaviour, so ...
>>
>> This one is especially useful for roaming several virtual stations, but maybe that is only useful test
>> case.  I included it more as an RFC, but it has worked well enough for us in case you see some worth in
>> it (and obviously it should be changed to not use // comments in case the functionality is actually
>> deemed useful).
> 
> I guess we can go back and forth on this ... but in most cases you don't
> know that the AP didn't go away, so IMHO the current behaviour is
> better. It's just in your special case you know it's more likely some
> local issues vs. the AP having disappeared.

What I saw is that supplicant would try to do a roam, and kernel would reject it because
it had just deleted the object.  Maybe it is better to have the kernel be more lenient and
let supplicant take care of the policy decisions?

Thanks,
Ben

> 
> johannes
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com