ath10k.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] ath10k fixes for warns
@ 2021-02-10  0:42 Shuah Khan
  2021-02-10  0:42 ` [PATCH 1/5] ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request() Shuah Khan
                   ` (4 more replies)
  0 siblings, 5 replies; 22+ messages in thread
From: Shuah Khan @ 2021-02-10  0:42 UTC (permalink / raw)
  To: kvalo, davem, kuba
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan

I have been seeing lockdep asserts for a couple of months and finally
found time to debug and fix the problems. The dmesg looks clean with
these fixes.

Enabling LOCKDEP and ATH10K_DEBUGFS triggers the lockdep assert and
RCU warns.

The first two patches in this series are fixes to lockdep assert and
RCU usage bugs.

The last patch (5/5) is a fix to reduce invalid ht params rate message
noise. Patch 3/4 changes a message from debug to warn. Patch 4 adds
detect to assert not calling ath10k_drain_tx() holding conf_mutex.

Shuah Khan (5):
  ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request()
  ath10k: fix WARNING: suspicious RCU usage
  ath10k: change ath10k_offchan_tx_work() peer present msg to a warn
  ath10k: detect conf_mutex held ath10k_drain_tx() calls
  ath10k: reduce invalid ht params rate message noise

 drivers/net/wireless/ath/ath10k/mac.c     | 13 ++++++++-----
 drivers/net/wireless/ath/ath10k/wmi-tlv.c | 15 +++++++++++----
 2 files changed, 19 insertions(+), 9 deletions(-)

-- 
2.27.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/5] ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request()
  2021-02-10  0:42 [PATCH 0/5] ath10k fixes for warns Shuah Khan
@ 2021-02-10  0:42 ` Shuah Khan
  2021-02-10  8:09   ` Kalle Valo
       [not found]   ` <20210210080915.8A81AC433C6@smtp.codeaurora.org>
  2021-02-10  0:42 ` [PATCH 2/5] ath10k: fix WARNING: suspicious RCU usage Shuah Khan
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 22+ messages in thread
From: Shuah Khan @ 2021-02-10  0:42 UTC (permalink / raw)
  To: kvalo, davem, kuba
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan

ath10k_debug_fw_stats_request() is called ath10k_sta_statistics()
without holding conf_mutex. ath10k_debug_fw_stats_request() simply
returns when CONFIG_ATH10K_DEBUGFS is disabled.

When CONFIG_ATH10K_DEBUGFS is enabled, ath10k_debug_fw_stats_request()
code path isn't protected. This assert is triggered when CONFIG_LOCKDEP
and CONFIG_ATH10K_DEBUGFS are enabled.

All other ath10k_debug_fw_stats_request() callers hold conf_mutex.
Fix ath10k_sta_statistics() to do the same.

WARNING: CPU: 5 PID: 696 at drivers/net/wireless/ath/ath10k/debug.c:357 ath10k_debug_fw_stats_request+0x29a/0x2d0 [ath10k_core]
Modules linked in: rfcomm ccm fuse cmac algif_hash algif_skcipher af_alg bnep binfmt_misc nls_iso8859_1 intel_rapl_msr intel_rapl_common snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio snd_hda_codec_hdmi snd_hda_intel snd_intel_dspcfg snd_hda_codec snd_hda_core snd_hwdep snd_pcm amdgpu snd_seq_midi snd_seq_midi_event snd_rawmidi edac_mce_amd snd_seq ath10k_pci ath10k_core aesni_intel gpu_sched drm_ttm_helper btusb ttm glue_helper crypto_simd btrtl ath cryptd drm_kms_helper snd_seq_device btbcm snd_timer rapl btintel cec i2c_algo_bit mac80211 bluetooth fb_sys_fops input_leds ecdh_generic snd wmi_bmof syscopyarea ecc serio_raw efi_pstore ccp k10temp sysfillrect soundcore sysimgblt snd_pci_acp3x cfg80211 ipmi_devintf libarc4 ipmi_msghandler mac_hid sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 hid_generic usbhid hid crc32_pclmul psmouse ahci nvme libahci i2c_piix4 nvme_core r8169 realtek wmi video
CPU: 5 PID: 696 Comm: NetworkManager Tainted: G        W         5.11.0-rc7+ #20
Hardware name: LENOVO 10VGCTO1WW/3130, BIOS M1XKT45A 08/21/2019
RIP: 0010:ath10k_debug_fw_stats_request+0x29a/0x2d0 [ath10k_core]
Code: 83 c4 10 44 89 f8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 48 8d bf e8 20 00 00 be ff ff ff ff e8 de 2d 47 fa 85 c0 0f 85 8d fd ff ff <0f> 0b e9 86 fd ff ff 41 bf a1 ff ff ff 44 89 fa 48 c7 c6 2c 71 c4
RSP: 0018:ffffaffbc124b7d0 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff93d02e4fec70 RCX: 0000000000000001
RDX: 0000000000000000 RSI: ffff93d00cba5248 RDI: ffff93d00ab309a0
RBP: ffffaffbc124b808 R08: 0000000000000000 R09: ffff93d02e4fec70
R10: 0000000000000001 R11: 0000000000000246 R12: ffff93d00cba3160
R13: ffff93d00cba3160 R14: ffff93d02e4fe4f0 R15: 0000000000000001
FS:  00007f7ce8d50bc0(0000) GS:ffff93d137d40000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fc3595ad160 CR3: 000000010d492000 CR4: 00000000003506e0
Call Trace:
 ? sta_info_get_bss+0xeb/0x1f0 [mac80211]
 ath10k_sta_statistics+0x4f/0x280 [ath10k_core]
 sta_set_sinfo+0xda/0xd20 [mac80211]
 ieee80211_get_station+0x58/0x80 [mac80211]
 nl80211_get_station+0xbd/0x340 [cfg80211]
 genl_family_rcv_msg_doit+0xe7/0x150
 genl_rcv_msg+0xe2/0x1e0
 ? nl80211_dump_station+0x3a0/0x3a0 [cfg80211]
 ? nl80211_send_station+0xef0/0xef0 [cfg80211]
 ? genl_get_cmd+0xd0/0xd0
 netlink_rcv_skb+0x55/0x100
 genl_rcv+0x29/0x40
 netlink_unicast+0x1a8/0x270
 netlink_sendmsg+0x253/0x480
 sock_sendmsg+0x65/0x70
 ____sys_sendmsg+0x219/0x260
 ? __import_iovec+0x32/0x170
 ___sys_sendmsg+0xb7/0x100
 ? end_opal_session+0x39/0xd0
 ? __fget_files+0xe0/0x1d0
 ? find_held_lock+0x31/0x90
 ? __fget_files+0xe0/0x1d0
 ? __fget_files+0x103/0x1d0
 ? __fget_light+0x32/0x80
 __sys_sendmsg+0x5a/0xa0
 ? syscall_enter_from_user_mode+0x21/0x60
 __x64_sys_sendmsg+0x1f/0x30
 do_syscall_64+0x38/0x50
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f7cea2c791d
Code: 28 89 54 24 1c 48 89 74 24 10 89 7c 24 08 e8 4a ee ff ff 8b 54 24 1c 48 8b 74 24 10 41 89 c0 8b 7c 24 08 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 33 44 89 c7 48 89 44 24 08 e8 9e ee ff ff 48
RSP: 002b:00007ffedf612a30 EFLAGS: 00000293 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00005618c4cfec00 RCX: 00007f7cea2c791d
RDX: 0000000000000000 RSI: 00007ffedf612a80 RDI: 000000000000000b
RBP: 00007ffedf612a80 R08: 0000000000000000 R09: 00005618c4e74000
R10: 00005618c4da0590 R11: 0000000000000293 R12: 00005618c4cfec00
R13: 00005618c4cfe2c0 R14: 00007f7cea32ef80 R15: 00005618c4cff340
irq event stamp: 520897
hardirqs last  enabled at (520903): [<ffffffffba501cc5>] console_unlock+0x4e5/0x5d0
hardirqs last disabled at (520908): [<ffffffffba501c38>] console_unlock+0x458/0x5d0
softirqs last  enabled at (520722): [<ffffffffbb201002>] asm_call_irq_on_stack+0x12/0x20
softirqs last disabled at (520717): [<ffffffffbb201002>] asm_call_irq_on_stack+0x12/0x20

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/net/wireless/ath/ath10k/mac.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 7d98250380ec..e815aab412d7 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -9117,7 +9117,9 @@ static void ath10k_sta_statistics(struct ieee80211_hw *hw,
 	if (!ath10k_peer_stats_enabled(ar))
 		return;
 
+	mutex_lock(&ar->conf_mutex);
 	ath10k_debug_fw_stats_request(ar);
+	mutex_unlock(&ar->conf_mutex);
 
 	sinfo->rx_duration = arsta->rx_duration;
 	sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_DURATION);
-- 
2.27.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 2/5] ath10k: fix WARNING: suspicious RCU usage
  2021-02-10  0:42 [PATCH 0/5] ath10k fixes for warns Shuah Khan
  2021-02-10  0:42 ` [PATCH 1/5] ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request() Shuah Khan
@ 2021-02-10  0:42 ` Shuah Khan
  2021-02-10  8:13   ` Kalle Valo
       [not found]   ` <20210210081320.2FBE5C433CA@smtp.codeaurora.org>
  2021-02-10  0:42 ` [PATCH 3/5] ath10k: change ath10k_offchan_tx_work() peer present msg to a warn Shuah Khan
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 22+ messages in thread
From: Shuah Khan @ 2021-02-10  0:42 UTC (permalink / raw)
  To: kvalo, davem, kuba
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
the resulting pointer is only valid under RCU lock as well.

Fix ath10k_wmi_tlv_parse_peer_stats_info() to hold RCU lock before it
calls ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed. The log below shows the problem.

While at it, fix ath10k_wmi_tlv_op_pull_peer_stats_info() to do the same.

=============================
WARNING: suspicious RCU usage
5.11.0-rc7+ #20 Tainted: G        W
-----------------------------
include/linux/rhashtable.h:594 suspicious rcu_dereference_check() usage!
other info that might help us debug this:
               rcu_scheduler_active = 2, debug_locks = 1
no locks held by ksoftirqd/5/44.

stack backtrace:
CPU: 5 PID: 44 Comm: ksoftirqd/5 Tainted: G        W         5.11.0-rc7+ #20
Hardware name: LENOVO 10VGCTO1WW/3130, BIOS M1XKT45A 08/21/2019
Call Trace:
 dump_stack+0x7d/0x9f
 lockdep_rcu_suspicious+0xdb/0xe5
 __rhashtable_lookup+0x1eb/0x260 [mac80211]
 ieee80211_find_sta_by_ifaddr+0x5b/0xc0 [mac80211]
 ath10k_wmi_tlv_parse_peer_stats_info+0x3e/0x90 [ath10k_core]
 ath10k_wmi_tlv_iter+0x6a/0xc0 [ath10k_core]
 ? ath10k_wmi_tlv_op_pull_mgmt_tx_bundle_compl_ev+0xe0/0xe0 [ath10k_core]
 ath10k_wmi_tlv_op_rx+0x5da/0xda0 [ath10k_core]
 ? trace_hardirqs_on+0x54/0xf0
 ? ath10k_ce_completed_recv_next+0x4e/0x60 [ath10k_core]
 ath10k_wmi_process_rx+0x1d/0x40 [ath10k_core]
 ath10k_htc_rx_completion_handler+0x115/0x180 [ath10k_core]
 ath10k_pci_process_rx_cb+0x149/0x1b0 [ath10k_pci]
 ? ath10k_htc_process_trailer+0x2d0/0x2d0 [ath10k_core]
 ? ath10k_pci_sleep.part.0+0x6a/0x80 [ath10k_pci]
 ath10k_pci_htc_rx_cb+0x15/0x20 [ath10k_pci]
 ath10k_ce_per_engine_service+0x61/0x80 [ath10k_core]
 ath10k_ce_per_engine_service_any+0x7d/0xa0 [ath10k_core]
 ath10k_pci_napi_poll+0x48/0x120 [ath10k_pci]
 net_rx_action+0x136/0x500
 __do_softirq+0xc6/0x459
 ? smpboot_thread_fn+0x2b/0x1f0
 run_ksoftirqd+0x2b/0x60
 smpboot_thread_fn+0x116/0x1f0
 kthread+0x14b/0x170
 ? smpboot_register_percpu_thread+0xe0/0xe0
 ? __kthread_bind_mask+0x70/0x70
 ret_from_fork+0x22/0x30

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/net/wireless/ath/ath10k/wmi-tlv.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index 7b5834157fe5..615157dd6866 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -225,6 +225,7 @@ static int ath10k_wmi_tlv_parse_peer_stats_info(struct ath10k *ar, u16 tag, u16
 	const struct wmi_tlv_peer_stats_info *stat = ptr;
 	struct ieee80211_sta *sta;
 	struct ath10k_sta *arsta;
+	int ret = 0;
 
 	if (tag != WMI_TLV_TAG_STRUCT_PEER_STATS_INFO)
 		return -EPROTO;
@@ -240,10 +241,12 @@ static int ath10k_wmi_tlv_parse_peer_stats_info(struct ath10k *ar, u16 tag, u16
 		   __le32_to_cpu(stat->last_tx_rate_code),
 		   __le32_to_cpu(stat->last_tx_bitrate_kbps));
 
+	rcu_read_lock();
 	sta = ieee80211_find_sta_by_ifaddr(ar->hw, stat->peer_macaddr.addr, NULL);
 	if (!sta) {
 		ath10k_warn(ar, "not found station for peer stats\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto exit;
 	}
 
 	arsta = (struct ath10k_sta *)sta->drv_priv;
@@ -252,7 +255,9 @@ static int ath10k_wmi_tlv_parse_peer_stats_info(struct ath10k *ar, u16 tag, u16
 	arsta->tx_rate_code = __le32_to_cpu(stat->last_tx_rate_code);
 	arsta->tx_bitrate_kbps = __le32_to_cpu(stat->last_tx_bitrate_kbps);
 
-	return 0;
+exit:
+	rcu_read_unlock();
+	return ret;
 }
 
 static int ath10k_wmi_tlv_op_pull_peer_stats_info(struct ath10k *ar,
@@ -573,13 +578,13 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar, struct sk_buff *skb)
 	case WMI_TDLS_TEARDOWN_REASON_TX:
 	case WMI_TDLS_TEARDOWN_REASON_RSSI:
 	case WMI_TDLS_TEARDOWN_REASON_PTR_TIMEOUT:
+		rcu_read_lock();
 		station = ieee80211_find_sta_by_ifaddr(ar->hw,
 						       ev->peer_macaddr.addr,
 						       NULL);
 		if (!station) {
 			ath10k_warn(ar, "did not find station from tdls peer event");
-			kfree(tb);
-			return;
+			goto exit;
 		}
 		arvif = ath10k_get_arvif(ar, __le32_to_cpu(ev->vdev_id));
 		ieee80211_tdls_oper_request(
@@ -590,6 +595,8 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar, struct sk_buff *skb)
 					);
 		break;
 	}
+exit:
+	rcu_read_unlock();
 	kfree(tb);
 }
 
-- 
2.27.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 3/5] ath10k: change ath10k_offchan_tx_work() peer present msg to a warn
  2021-02-10  0:42 [PATCH 0/5] ath10k fixes for warns Shuah Khan
  2021-02-10  0:42 ` [PATCH 1/5] ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request() Shuah Khan
  2021-02-10  0:42 ` [PATCH 2/5] ath10k: fix WARNING: suspicious RCU usage Shuah Khan
@ 2021-02-10  0:42 ` Shuah Khan
  2021-02-11  6:50   ` Kalle Valo
  2021-02-10  0:42 ` [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls Shuah Khan
  2021-02-10  0:42 ` [PATCH 5/5] ath10k: reduce invalid ht params rate message noise Shuah Khan
  4 siblings, 1 reply; 22+ messages in thread
From: Shuah Khan @ 2021-02-10  0:42 UTC (permalink / raw)
  To: kvalo, davem, kuba
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan

Based on the comment block in this function and the FIXME for this, peer
being present for the offchannel tx is unlikely. Peer is deleted once tx
is complete. Change peer present msg to a warn to detect this condition.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/net/wireless/ath/ath10k/mac.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index e815aab412d7..53f92945006f 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3954,9 +3954,8 @@ void ath10k_offchan_tx_work(struct work_struct *work)
 		spin_unlock_bh(&ar->data_lock);
 
 		if (peer)
-			/* FIXME: should this use ath10k_warn()? */
-			ath10k_dbg(ar, ATH10K_DBG_MAC, "peer %pM on vdev %d already present\n",
-				   peer_addr, vdev_id);
+			ath10k_warn(ar, "peer %pM on vdev %d already present\n",
+				    peer_addr, vdev_id);
 
 		if (!peer) {
 			ret = ath10k_peer_create(ar, NULL, NULL, vdev_id,
-- 
2.27.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls
  2021-02-10  0:42 [PATCH 0/5] ath10k fixes for warns Shuah Khan
                   ` (2 preceding siblings ...)
  2021-02-10  0:42 ` [PATCH 3/5] ath10k: change ath10k_offchan_tx_work() peer present msg to a warn Shuah Khan
@ 2021-02-10  0:42 ` Shuah Khan
  2021-02-10  8:03   ` kernel test robot
                     ` (2 more replies)
  2021-02-10  0:42 ` [PATCH 5/5] ath10k: reduce invalid ht params rate message noise Shuah Khan
  4 siblings, 3 replies; 22+ messages in thread
From: Shuah Khan @ 2021-02-10  0:42 UTC (permalink / raw)
  To: kvalo, davem, kuba
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan

ath10k_drain_tx() must not be called with conf_mutex held as workers can
use that also. Add check to detect conf_mutex held calls.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/net/wireless/ath/ath10k/mac.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 53f92945006f..3545ce7dce0a 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4566,6 +4566,7 @@ static void ath10k_mac_op_wake_tx_queue(struct ieee80211_hw *hw,
 /* Must not be called with conf_mutex held as workers can use that also. */
 void ath10k_drain_tx(struct ath10k *ar)
 {
+	WARN_ON(lockdep_is_held(&ar->conf_mutex));
 	/* make sure rcu-protected mac80211 tx path itself is drained */
 	synchronize_net();
 
-- 
2.27.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 5/5] ath10k: reduce invalid ht params rate message noise
  2021-02-10  0:42 [PATCH 0/5] ath10k fixes for warns Shuah Khan
                   ` (3 preceding siblings ...)
  2021-02-10  0:42 ` [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls Shuah Khan
@ 2021-02-10  0:42 ` Shuah Khan
  2021-02-10  2:36   ` Wen Gong
  4 siblings, 1 reply; 22+ messages in thread
From: Shuah Khan @ 2021-02-10  0:42 UTC (permalink / raw)
  To: kvalo, davem, kuba
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan

ath10k_mac_get_rate_flags_ht() floods dmesg with the following messages,
when it fails to find a match for mcs=7 and rate=1440.

supported_ht_mcs_rate_nss2:
{7,  {1300, 2700, 1444, 3000} }

ath10k_pci 0000:02:00.0: invalid ht params rate 1440 100kbps nss 2 mcs 7

dev_warn_ratelimited() isn't helping the noise. Use dev_warn_once()
instead.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/net/wireless/ath/ath10k/mac.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 3545ce7dce0a..276321f0cfdd 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -8970,8 +8970,9 @@ static void ath10k_mac_get_rate_flags_ht(struct ath10k *ar, u32 rate, u8 nss, u8
 		*bw |= RATE_INFO_BW_40;
 		*flags |= RATE_INFO_FLAGS_SHORT_GI;
 	} else {
-		ath10k_warn(ar, "invalid ht params rate %d 100kbps nss %d mcs %d",
-			    rate, nss, mcs);
+		dev_warn_once(ar->dev,
+			      "invalid ht params rate %d 100kbps nss %d mcs %d",
+			      rate, nss, mcs);
 	}
 }
 
-- 
2.27.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise
  2021-02-10  0:42 ` [PATCH 5/5] ath10k: reduce invalid ht params rate message noise Shuah Khan
@ 2021-02-10  2:36   ` Wen Gong
  2021-02-10  8:28     ` Kalle Valo
  0 siblings, 1 reply; 22+ messages in thread
From: Wen Gong @ 2021-02-10  2:36 UTC (permalink / raw)
  To: Shuah Khan
  Cc: netdev, linux-wireless, linux-kernel, ath10k, kuba, davem, kvalo

On 2021-02-10 08:42, Shuah Khan wrote:
> ath10k_mac_get_rate_flags_ht() floods dmesg with the following 
> messages,
> when it fails to find a match for mcs=7 and rate=1440.
> 
> supported_ht_mcs_rate_nss2:
> {7,  {1300, 2700, 1444, 3000} }
> 
> ath10k_pci 0000:02:00.0: invalid ht params rate 1440 100kbps nss 2 mcs 
> 7
> 
> dev_warn_ratelimited() isn't helping the noise. Use dev_warn_once()
> instead.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  drivers/net/wireless/ath/ath10k/mac.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
> b/drivers/net/wireless/ath/ath10k/mac.c
> index 3545ce7dce0a..276321f0cfdd 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -8970,8 +8970,9 @@ static void ath10k_mac_get_rate_flags_ht(struct
> ath10k *ar, u32 rate, u8 nss, u8
>  		*bw |= RATE_INFO_BW_40;
>  		*flags |= RATE_INFO_FLAGS_SHORT_GI;
>  	} else {
> -		ath10k_warn(ar, "invalid ht params rate %d 100kbps nss %d mcs %d",
> -			    rate, nss, mcs);
> +		dev_warn_once(ar->dev,
> +			      "invalid ht params rate %d 100kbps nss %d mcs %d",
> +			      rate, nss, mcs);
>  	}
>  }
The {7,  {1300, 2700, 1444, 3000} } is a correct value.
The 1440 is report from firmware, its a wrong value, it has fixed in 
firmware.
If change it to dev_warn_once, then it will have no chance to find the 
other wrong values which report by firmware, and it indicate
a wrong value to mac80211/cfg80211 and lead "iw wlan0 station dump" get 
a wrong bitrate.

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls
  2021-02-10  0:42 ` [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls Shuah Khan
@ 2021-02-10  8:03   ` kernel test robot
  2021-02-10  8:25   ` Kalle Valo
  2021-02-11  0:13   ` kernel test robot
  2 siblings, 0 replies; 22+ messages in thread
From: kernel test robot @ 2021-02-10  8:03 UTC (permalink / raw)
  To: Shuah Khan, kvalo, davem, kuba
  Cc: kbuild-all, netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan

[-- Attachment #1: Type: text/plain, Size: 1700 bytes --]

Hi Shuah,

I love your patch! Yet something to improve:

[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on wireless-drivers/master v5.11-rc7 next-20210125]
[cannot apply to ath6kl/ath-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Shuah-Khan/ath10k-fixes-for-warns/20210210-085259
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/5a96c0c9dfec2bd59e680b7ec8ade9378b654c4c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Shuah-Khan/ath10k-fixes-for-warns/20210210-085259
        git checkout 5a96c0c9dfec2bd59e680b7ec8ade9378b654c4c
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "lockdep_is_held" [drivers/net/wireless/ath/ath10k/ath10k_core.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 53406 bytes --]

[-- Attachment #3: Type: text/plain, Size: 146 bytes --]

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request()
  2021-02-10  0:42 ` [PATCH 1/5] ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request() Shuah Khan
@ 2021-02-10  8:09   ` Kalle Valo
       [not found]   ` <20210210080915.8A81AC433C6@smtp.codeaurora.org>
  1 sibling, 0 replies; 22+ messages in thread
From: Kalle Valo @ 2021-02-10  8:09 UTC (permalink / raw)
  To: Shuah Khan
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan, kuba, davem

Shuah Khan <skhan@linuxfoundation.org> wrote:

> ath10k_debug_fw_stats_request() is called ath10k_sta_statistics()
> without holding conf_mutex. ath10k_debug_fw_stats_request() simply
> returns when CONFIG_ATH10K_DEBUGFS is disabled.
> 
> When CONFIG_ATH10K_DEBUGFS is enabled, ath10k_debug_fw_stats_request()
> code path isn't protected. This assert is triggered when CONFIG_LOCKDEP
> and CONFIG_ATH10K_DEBUGFS are enabled.
> 
> All other ath10k_debug_fw_stats_request() callers hold conf_mutex.
> Fix ath10k_sta_statistics() to do the same.
> 
> WARNING: CPU: 5 PID: 696 at drivers/net/wireless/ath/ath10k/debug.c:357 ath10k_debug_fw_stats_request+0x29a/0x2d0 [ath10k_core]
> Modules linked in: rfcomm ccm fuse cmac algif_hash algif_skcipher af_alg bnep binfmt_misc nls_iso8859_1 intel_rapl_msr intel_rapl_common snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio snd_hda_codec_hdmi snd_hda_intel snd_intel_dspcfg snd_hda_codec snd_hda_core snd_hwdep snd_pcm amdgpu snd_seq_midi snd_seq_midi_event snd_rawmidi edac_mce_amd snd_seq ath10k_pci ath10k_core aesni_intel gpu_sched drm_ttm_helper btusb ttm glue_helper crypto_simd btrtl ath cryptd drm_kms_helper snd_seq_device btbcm snd_timer rapl btintel cec i2c_algo_bit mac80211 bluetooth fb_sys_fops input_leds ecdh_generic snd wmi_bmof syscopyarea ecc serio_raw efi_pstore ccp k10temp sysfillrect soundcore sysimgblt snd_pci_acp3x cfg80211 ipmi_devintf libarc4 ipmi_msghandler mac_hid sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 hid_generic usbhid hid crc32_pclmul psmouse ahci nvme libahci i2c_piix4 nvme_core r8169 realtek wmi video
> CPU: 5 PID: 696 Comm: NetworkManager Tainted: G        W         5.11.0-rc7+ #20
> Hardware name: LENOVO 10VGCTO1WW/3130, BIOS M1XKT45A 08/21/2019
> RIP: 0010:ath10k_debug_fw_stats_request+0x29a/0x2d0 [ath10k_core]
> Code: 83 c4 10 44 89 f8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 48 8d bf e8 20 00 00 be ff ff ff ff e8 de 2d 47 fa 85 c0 0f 85 8d fd ff ff <0f> 0b e9 86 fd ff ff 41 bf a1 ff ff ff 44 89 fa 48 c7 c6 2c 71 c4
> RSP: 0018:ffffaffbc124b7d0 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffff93d02e4fec70 RCX: 0000000000000001
> RDX: 0000000000000000 RSI: ffff93d00cba5248 RDI: ffff93d00ab309a0
> RBP: ffffaffbc124b808 R08: 0000000000000000 R09: ffff93d02e4fec70
> R10: 0000000000000001 R11: 0000000000000246 R12: ffff93d00cba3160
> R13: ffff93d00cba3160 R14: ffff93d02e4fe4f0 R15: 0000000000000001
> FS:  00007f7ce8d50bc0(0000) GS:ffff93d137d40000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fc3595ad160 CR3: 000000010d492000 CR4: 00000000003506e0
> Call Trace:
>  ? sta_info_get_bss+0xeb/0x1f0 [mac80211]
>  ath10k_sta_statistics+0x4f/0x280 [ath10k_core]
>  sta_set_sinfo+0xda/0xd20 [mac80211]
>  ieee80211_get_station+0x58/0x80 [mac80211]
>  nl80211_get_station+0xbd/0x340 [cfg80211]
>  genl_family_rcv_msg_doit+0xe7/0x150
>  genl_rcv_msg+0xe2/0x1e0
>  ? nl80211_dump_station+0x3a0/0x3a0 [cfg80211]
>  ? nl80211_send_station+0xef0/0xef0 [cfg80211]
>  ? genl_get_cmd+0xd0/0xd0
>  netlink_rcv_skb+0x55/0x100
>  genl_rcv+0x29/0x40
>  netlink_unicast+0x1a8/0x270
>  netlink_sendmsg+0x253/0x480
>  sock_sendmsg+0x65/0x70
>  ____sys_sendmsg+0x219/0x260
>  ? __import_iovec+0x32/0x170
>  ___sys_sendmsg+0xb7/0x100
>  ? end_opal_session+0x39/0xd0
>  ? __fget_files+0xe0/0x1d0
>  ? find_held_lock+0x31/0x90
>  ? __fget_files+0xe0/0x1d0
>  ? __fget_files+0x103/0x1d0
>  ? __fget_light+0x32/0x80
>  __sys_sendmsg+0x5a/0xa0
>  ? syscall_enter_from_user_mode+0x21/0x60
>  __x64_sys_sendmsg+0x1f/0x30
>  do_syscall_64+0x38/0x50
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7f7cea2c791d
> Code: 28 89 54 24 1c 48 89 74 24 10 89 7c 24 08 e8 4a ee ff ff 8b 54 24 1c 48 8b 74 24 10 41 89 c0 8b 7c 24 08 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 33 44 89 c7 48 89 44 24 08 e8 9e ee ff ff 48
> RSP: 002b:00007ffedf612a30 EFLAGS: 00000293 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 00005618c4cfec00 RCX: 00007f7cea2c791d
> RDX: 0000000000000000 RSI: 00007ffedf612a80 RDI: 000000000000000b
> RBP: 00007ffedf612a80 R08: 0000000000000000 R09: 00005618c4e74000
> R10: 00005618c4da0590 R11: 0000000000000293 R12: 00005618c4cfec00
> R13: 00005618c4cfe2c0 R14: 00007f7cea32ef80 R15: 00005618c4cff340
> irq event stamp: 520897
> hardirqs last  enabled at (520903): [<ffffffffba501cc5>] console_unlock+0x4e5/0x5d0
> hardirqs last disabled at (520908): [<ffffffffba501c38>] console_unlock+0x458/0x5d0
> softirqs last  enabled at (520722): [<ffffffffbb201002>] asm_call_irq_on_stack+0x12/0x20
> softirqs last disabled at (520717): [<ffffffffbb201002>] asm_call_irq_on_stack+0x12/0x20
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Bad timing, just yesterday I applied an identical patch: 

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=ath-next&id=7df28718928d08034b36168200d67b558ce36f3d

Patch set to Superseded.

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/1c38ef6d39ed89a564bc817d964d923ff0676c53.1612915444.git.skhan@linuxfoundation.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] ath10k: fix WARNING: suspicious RCU usage
  2021-02-10  0:42 ` [PATCH 2/5] ath10k: fix WARNING: suspicious RCU usage Shuah Khan
@ 2021-02-10  8:13   ` Kalle Valo
       [not found]   ` <20210210081320.2FBE5C433CA@smtp.codeaurora.org>
  1 sibling, 0 replies; 22+ messages in thread
From: Kalle Valo @ 2021-02-10  8:13 UTC (permalink / raw)
  To: Shuah Khan
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan, kuba, davem

Shuah Khan <skhan@linuxfoundation.org> wrote:

> ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
> the resulting pointer is only valid under RCU lock as well.
> 
> Fix ath10k_wmi_tlv_parse_peer_stats_info() to hold RCU lock before it
> calls ieee80211_find_sta_by_ifaddr() and release it when the resulting
> pointer is no longer needed. The log below shows the problem.
> 
> While at it, fix ath10k_wmi_tlv_op_pull_peer_stats_info() to do the same.
> 
> =============================
> WARNING: suspicious RCU usage
> 5.11.0-rc7+ #20 Tainted: G        W
> -----------------------------
> include/linux/rhashtable.h:594 suspicious rcu_dereference_check() usage!
> other info that might help us debug this:
>                rcu_scheduler_active = 2, debug_locks = 1
> no locks held by ksoftirqd/5/44.
> 
> stack backtrace:
> CPU: 5 PID: 44 Comm: ksoftirqd/5 Tainted: G        W         5.11.0-rc7+ #20
> Hardware name: LENOVO 10VGCTO1WW/3130, BIOS M1XKT45A 08/21/2019
> Call Trace:
>  dump_stack+0x7d/0x9f
>  lockdep_rcu_suspicious+0xdb/0xe5
>  __rhashtable_lookup+0x1eb/0x260 [mac80211]
>  ieee80211_find_sta_by_ifaddr+0x5b/0xc0 [mac80211]
>  ath10k_wmi_tlv_parse_peer_stats_info+0x3e/0x90 [ath10k_core]
>  ath10k_wmi_tlv_iter+0x6a/0xc0 [ath10k_core]
>  ? ath10k_wmi_tlv_op_pull_mgmt_tx_bundle_compl_ev+0xe0/0xe0 [ath10k_core]
>  ath10k_wmi_tlv_op_rx+0x5da/0xda0 [ath10k_core]
>  ? trace_hardirqs_on+0x54/0xf0
>  ? ath10k_ce_completed_recv_next+0x4e/0x60 [ath10k_core]
>  ath10k_wmi_process_rx+0x1d/0x40 [ath10k_core]
>  ath10k_htc_rx_completion_handler+0x115/0x180 [ath10k_core]
>  ath10k_pci_process_rx_cb+0x149/0x1b0 [ath10k_pci]
>  ? ath10k_htc_process_trailer+0x2d0/0x2d0 [ath10k_core]
>  ? ath10k_pci_sleep.part.0+0x6a/0x80 [ath10k_pci]
>  ath10k_pci_htc_rx_cb+0x15/0x20 [ath10k_pci]
>  ath10k_ce_per_engine_service+0x61/0x80 [ath10k_core]
>  ath10k_ce_per_engine_service_any+0x7d/0xa0 [ath10k_core]
>  ath10k_pci_napi_poll+0x48/0x120 [ath10k_pci]
>  net_rx_action+0x136/0x500
>  __do_softirq+0xc6/0x459
>  ? smpboot_thread_fn+0x2b/0x1f0
>  run_ksoftirqd+0x2b/0x60
>  smpboot_thread_fn+0x116/0x1f0
>  kthread+0x14b/0x170
>  ? smpboot_register_percpu_thread+0xe0/0xe0
>  ? __kthread_bind_mask+0x70/0x70
>  ret_from_fork+0x22/0x30
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Unlucky timing also on this one, it conflicts with a patch I applied yesterday:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=ath-next&id=2615e3cdbd9c0e864f5906279c952a309871d225

Can you redo the patch to only change ath10k_wmi_event_tdls_peer()?

error: patch failed: drivers/net/wireless/ath/ath10k/wmi-tlv.c:240
error: drivers/net/wireless/ath/ath10k/wmi-tlv.c: patch does not apply
stg import: Diff does not apply cleanly

Patch set to Changes Requested.

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/23a1333dfb0367cc69e7177a2e373df0b6d42980.1612915444.git.skhan@linuxfoundation.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls
  2021-02-10  0:42 ` [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls Shuah Khan
  2021-02-10  8:03   ` kernel test robot
@ 2021-02-10  8:25   ` Kalle Valo
  2021-02-10 15:57     ` Shuah Khan
  2021-02-11  0:13   ` kernel test robot
  2 siblings, 1 reply; 22+ messages in thread
From: Kalle Valo @ 2021-02-10  8:25 UTC (permalink / raw)
  To: Shuah Khan; +Cc: netdev, linux-wireless, linux-kernel, ath10k, kuba, davem

Shuah Khan <skhan@linuxfoundation.org> writes:

> ath10k_drain_tx() must not be called with conf_mutex held as workers can
> use that also. Add check to detect conf_mutex held calls.
>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

The commit log does not answer to "Why?". How did you find this? What
actual problem are you trying to solve?

> ---
>  drivers/net/wireless/ath/ath10k/mac.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 53f92945006f..3545ce7dce0a 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -4566,6 +4566,7 @@ static void ath10k_mac_op_wake_tx_queue(struct ieee80211_hw *hw,
>  /* Must not be called with conf_mutex held as workers can use that also. */
>  void ath10k_drain_tx(struct ath10k *ar)
>  {
> +	WARN_ON(lockdep_is_held(&ar->conf_mutex));

Empty line after WARN_ON().

Shouldn't this check debug_locks similarly lockdep_assert_held() does?

#define lockdep_assert_held(l)	do {				\
		WARN_ON(debug_locks && !lockdep_is_held(l));	\
	} while (0)

And I suspect you need #ifdef CONFIG_LOCKDEP which should fix the kbuild
bot error.

But honestly I would prefer to have lockdep_assert_not_held() in
include/linux/lockdep.h, much cleaner that way. Also
i915_gem_object_lookup_rcu() could then use the same macro.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise
  2021-02-10  2:36   ` Wen Gong
@ 2021-02-10  8:28     ` Kalle Valo
  2021-02-10 16:13       ` Shuah Khan
  0 siblings, 1 reply; 22+ messages in thread
From: Kalle Valo @ 2021-02-10  8:28 UTC (permalink / raw)
  To: Wen Gong
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan, kuba, davem

Wen Gong <wgong@codeaurora.org> writes:

> On 2021-02-10 08:42, Shuah Khan wrote:
>> ath10k_mac_get_rate_flags_ht() floods dmesg with the following
>> messages,
>> when it fails to find a match for mcs=7 and rate=1440.
>>
>> supported_ht_mcs_rate_nss2:
>> {7,  {1300, 2700, 1444, 3000} }
>>
>> ath10k_pci 0000:02:00.0: invalid ht params rate 1440 100kbps nss 2
>> mcs 7
>>
>> dev_warn_ratelimited() isn't helping the noise. Use dev_warn_once()
>> instead.
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>> ---
>>  drivers/net/wireless/ath/ath10k/mac.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>> b/drivers/net/wireless/ath/ath10k/mac.c
>> index 3545ce7dce0a..276321f0cfdd 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -8970,8 +8970,9 @@ static void ath10k_mac_get_rate_flags_ht(struct
>> ath10k *ar, u32 rate, u8 nss, u8
>>  		*bw |= RATE_INFO_BW_40;
>>  		*flags |= RATE_INFO_FLAGS_SHORT_GI;
>>  	} else {
>> -		ath10k_warn(ar, "invalid ht params rate %d 100kbps nss %d mcs %d",
>> -			    rate, nss, mcs);
>> +		dev_warn_once(ar->dev,
>> +			      "invalid ht params rate %d 100kbps nss %d mcs %d",
>> +			      rate, nss, mcs);
>>  	}
>>  }
>
> The {7,  {1300, 2700, 1444, 3000} } is a correct value.
> The 1440 is report from firmware, its a wrong value, it has fixed in
> firmware.

In what version?

> If change it to dev_warn_once, then it will have no chance to find the
> other wrong values which report by firmware, and it indicate
> a wrong value to mac80211/cfg80211 and lead "iw wlan0 station dump"
> get a wrong bitrate.

I agree, we should keep this warning. If the firmware still keeps
sending invalid rates we should add a specific check to ignore the known
invalid values, but not all of them.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls
  2021-02-10  8:25   ` Kalle Valo
@ 2021-02-10 15:57     ` Shuah Khan
  2021-02-11 11:20       ` Kalle Valo
  0 siblings, 1 reply; 22+ messages in thread
From: Shuah Khan @ 2021-02-10 15:57 UTC (permalink / raw)
  To: Kalle Valo
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan, kuba, davem

On 2/10/21 1:25 AM, Kalle Valo wrote:
> Shuah Khan <skhan@linuxfoundation.org> writes:
> 
>> ath10k_drain_tx() must not be called with conf_mutex held as workers can
>> use that also. Add check to detect conf_mutex held calls.
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> The commit log does not answer to "Why?". How did you find this? What
> actual problem are you trying to solve?
> 

I came across the comment block above the ath10k_drain_tx() as I was
reviewing at conf_mutex holds while I was debugging the conf_mutex
lock assert in ath10k_debug_fw_stats_request().

My reasoning is that having this will help detect incorrect usages
of ath10k_drain_tx() while holding conf_mutex which could lead to
locking problems when async worker routines try to call this routine.

>> ---
>>   drivers/net/wireless/ath/ath10k/mac.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>> index 53f92945006f..3545ce7dce0a 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -4566,6 +4566,7 @@ static void ath10k_mac_op_wake_tx_queue(struct ieee80211_hw *hw,
>>   /* Must not be called with conf_mutex held as workers can use that also. */
>>   void ath10k_drain_tx(struct ath10k *ar)
>>   {
>> +	WARN_ON(lockdep_is_held(&ar->conf_mutex));
> 
> Empty line after WARN_ON().
> 

Will do.

> Shouldn't this check debug_locks similarly lockdep_assert_held() does?
> 
> #define lockdep_assert_held(l)	do {				\
> 		WARN_ON(debug_locks && !lockdep_is_held(l));	\
> 	} while (0)
> 
> And I suspect you need #ifdef CONFIG_LOCKDEP which should fix the kbuild
> bot error.
> 

Yes.

> But honestly I would prefer to have lockdep_assert_not_held() in
> include/linux/lockdep.h, much cleaner that way. Also
> i915_gem_object_lookup_rcu() could then use the same macro.
> 

Right. This is the right way to go. That was first instinct and
decided to have the discussion evolve in that direction. Now that
it has, I will combine this change with
include/linux/lockdep.h and add lockdep_assert_not_held()

I think we might have other places in the kernel that could use
lockdep_assert_not_held() in addition to i915_gem_object_lookup_rcu()

thanks,
-- Shuah

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise
  2021-02-10  8:28     ` Kalle Valo
@ 2021-02-10 16:13       ` Shuah Khan
  2021-02-11 11:24         ` Kalle Valo
  0 siblings, 1 reply; 22+ messages in thread
From: Shuah Khan @ 2021-02-10 16:13 UTC (permalink / raw)
  To: Kalle Valo, Wen Gong
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan, kuba, davem

On 2/10/21 1:28 AM, Kalle Valo wrote:
> Wen Gong <wgong@codeaurora.org> writes:
> 
>> On 2021-02-10 08:42, Shuah Khan wrote:
>>> ath10k_mac_get_rate_flags_ht() floods dmesg with the following
>>> messages,
>>> when it fails to find a match for mcs=7 and rate=1440.
>>>
>>> supported_ht_mcs_rate_nss2:
>>> {7,  {1300, 2700, 1444, 3000} }
>>>
>>> ath10k_pci 0000:02:00.0: invalid ht params rate 1440 100kbps nss 2
>>> mcs 7
>>>
>>> dev_warn_ratelimited() isn't helping the noise. Use dev_warn_once()
>>> instead.
>>>
>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>> ---
>>>   drivers/net/wireless/ath/ath10k/mac.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>>> b/drivers/net/wireless/ath/ath10k/mac.c
>>> index 3545ce7dce0a..276321f0cfdd 100644
>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>> @@ -8970,8 +8970,9 @@ static void ath10k_mac_get_rate_flags_ht(struct
>>> ath10k *ar, u32 rate, u8 nss, u8
>>>   		*bw |= RATE_INFO_BW_40;
>>>   		*flags |= RATE_INFO_FLAGS_SHORT_GI;
>>>   	} else {
>>> -		ath10k_warn(ar, "invalid ht params rate %d 100kbps nss %d mcs %d",
>>> -			    rate, nss, mcs);
>>> +		dev_warn_once(ar->dev,
>>> +			      "invalid ht params rate %d 100kbps nss %d mcs %d",
>>> +			      rate, nss, mcs);
>>>   	}
>>>   }
>>
>> The {7,  {1300, 2700, 1444, 3000} } is a correct value.
>> The 1440 is report from firmware, its a wrong value, it has fixed in
>> firmware.
> 
> In what version?
> 

Here is the info:

ath10k_pci 0000:02:00.0: qca6174 hw3.2 target 0x05030000 chip_id 
0x00340aff sub 17aa:0827

ath10k_pci 0000:02:00.0: firmware ver WLAN.RM.4.4.1-00140-QCARMSWPZ-1 
api 6 features wowlan,ignore-otp,mfp crc32 29eb8ca1

ath10k_pci 0000:02:00.0: board_file api 2 bmi_id N/A crc32 4ac0889b

ath10k_pci 0000:02:00.0: htt-ver 3.60 wmi-op 4 htt-op 3 cal otp max-sta 
32 raw 0 hwcrypto 1

>> If change it to dev_warn_once, then it will have no chance to find the
>> other wrong values which report by firmware, and it indicate
>> a wrong value to mac80211/cfg80211 and lead "iw wlan0 station dump"
>> get a wrong bitrate.
> 

Agreed.

> I agree, we should keep this warning. If the firmware still keeps
> sending invalid rates we should add a specific check to ignore the known
> invalid values, but not all of them.
> 

Would it be helpful to adjust the default rate limits and set the to
a higher value instead. It might be difficult to account all possible
invalid values?

Something like, ath10k_warn_ratelimited() to adjust the

DEFAULT_RATELIMIT_INTERVAL and DEFAULT_RATELIMIT_BURST using
DEFINE_RATELIMIT_STATE

Let me know if you like this idea. I can send a patch in to do this.
I will hang on to this firmware version for a little but longer, so
we have a test case. :)

thanks,
-- Shuah



_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request()
       [not found]   ` <20210210080915.8A81AC433C6@smtp.codeaurora.org>
@ 2021-02-10 16:21     ` Shuah Khan
  0 siblings, 0 replies; 22+ messages in thread
From: Shuah Khan @ 2021-02-10 16:21 UTC (permalink / raw)
  To: Kalle Valo
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan, kuba, davem

On 2/10/21 1:09 AM, Kalle Valo wrote:
> Shuah Khan <skhan@linuxfoundation.org> wrote:
> 
>> ath10k_debug_fw_stats_request() is called ath10k_sta_statistics()
>> without holding conf_mutex. ath10k_debug_fw_stats_request() simply
>> returns when CONFIG_ATH10K_DEBUGFS is disabled.
>>
>> When CONFIG_ATH10K_DEBUGFS is enabled, ath10k_debug_fw_stats_request()
>> code path isn't protected. This assert is triggered when CONFIG_LOCKDEP
>> and CONFIG_ATH10K_DEBUGFS are enabled.
>>
>> All other ath10k_debug_fw_stats_request() callers hold conf_mutex.
>> Fix ath10k_sta_statistics() to do the same.
>>
>> WARNING: CPU: 5 PID: 696 at drivers/net/wireless/ath/ath10k/debug.c:357 ath10k_debug_fw_stats_request+0x29a/0x2d0 [ath10k_core]
>> Modules linked in: rfcomm ccm fuse cmac algif_hash algif_skcipher af_alg bnep binfmt_misc nls_iso8859_1 intel_rapl_msr intel_rapl_common snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio snd_hda_codec_hdmi snd_hda_intel snd_intel_dspcfg snd_hda_codec snd_hda_core snd_hwdep snd_pcm amdgpu snd_seq_midi snd_seq_midi_event snd_rawmidi edac_mce_amd snd_seq ath10k_pci ath10k_core aesni_intel gpu_sched drm_ttm_helper btusb ttm glue_helper crypto_simd btrtl ath cryptd drm_kms_helper snd_seq_device btbcm snd_timer rapl btintel cec i2c_algo_bit mac80211 bluetooth fb_sys_fops input_leds ecdh_generic snd wmi_bmof syscopyarea ecc serio_raw efi_pstore ccp k10temp sysfillrect soundcore sysimgblt snd_pci_acp3x cfg80211 ipmi_devintf libarc4 ipmi_msghandler mac_hid sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 hid_generic usbhid hid crc32_pclmul psmouse ahci nvme libahci i2c_piix4 nvme_core r8169 realtek wmi video
>> CPU: 5 PID: 696 Comm: NetworkManager Tainted: G        W         5.11.0-rc7+ #20
>> Hardware name: LENOVO 10VGCTO1WW/3130, BIOS M1XKT45A 08/21/2019
>> RIP: 0010:ath10k_debug_fw_stats_request+0x29a/0x2d0 [ath10k_core]
>> Code: 83 c4 10 44 89 f8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 48 8d bf e8 20 00 00 be ff ff ff ff e8 de 2d 47 fa 85 c0 0f 85 8d fd ff ff <0f> 0b e9 86 fd ff ff 41 bf a1 ff ff ff 44 89 fa 48 c7 c6 2c 71 c4
>> RSP: 0018:ffffaffbc124b7d0 EFLAGS: 00010246
>> RAX: 0000000000000000 RBX: ffff93d02e4fec70 RCX: 0000000000000001
>> RDX: 0000000000000000 RSI: ffff93d00cba5248 RDI: ffff93d00ab309a0
>> RBP: ffffaffbc124b808 R08: 0000000000000000 R09: ffff93d02e4fec70
>> R10: 0000000000000001 R11: 0000000000000246 R12: ffff93d00cba3160
>> R13: ffff93d00cba3160 R14: ffff93d02e4fe4f0 R15: 0000000000000001
>> FS:  00007f7ce8d50bc0(0000) GS:ffff93d137d40000(0000) knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00007fc3595ad160 CR3: 000000010d492000 CR4: 00000000003506e0
>> Call Trace:
>>   ? sta_info_get_bss+0xeb/0x1f0 [mac80211]
>>   ath10k_sta_statistics+0x4f/0x280 [ath10k_core]
>>   sta_set_sinfo+0xda/0xd20 [mac80211]
>>   ieee80211_get_station+0x58/0x80 [mac80211]
>>   nl80211_get_station+0xbd/0x340 [cfg80211]
>>   genl_family_rcv_msg_doit+0xe7/0x150
>>   genl_rcv_msg+0xe2/0x1e0
>>   ? nl80211_dump_station+0x3a0/0x3a0 [cfg80211]
>>   ? nl80211_send_station+0xef0/0xef0 [cfg80211]
>>   ? genl_get_cmd+0xd0/0xd0
>>   netlink_rcv_skb+0x55/0x100
>>   genl_rcv+0x29/0x40
>>   netlink_unicast+0x1a8/0x270
>>   netlink_sendmsg+0x253/0x480
>>   sock_sendmsg+0x65/0x70
>>   ____sys_sendmsg+0x219/0x260
>>   ? __import_iovec+0x32/0x170
>>   ___sys_sendmsg+0xb7/0x100
>>   ? end_opal_session+0x39/0xd0
>>   ? __fget_files+0xe0/0x1d0
>>   ? find_held_lock+0x31/0x90
>>   ? __fget_files+0xe0/0x1d0
>>   ? __fget_files+0x103/0x1d0
>>   ? __fget_light+0x32/0x80
>>   __sys_sendmsg+0x5a/0xa0
>>   ? syscall_enter_from_user_mode+0x21/0x60
>>   __x64_sys_sendmsg+0x1f/0x30
>>   do_syscall_64+0x38/0x50
>>   entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> RIP: 0033:0x7f7cea2c791d
>> Code: 28 89 54 24 1c 48 89 74 24 10 89 7c 24 08 e8 4a ee ff ff 8b 54 24 1c 48 8b 74 24 10 41 89 c0 8b 7c 24 08 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 33 44 89 c7 48 89 44 24 08 e8 9e ee ff ff 48
>> RSP: 002b:00007ffedf612a30 EFLAGS: 00000293 ORIG_RAX: 000000000000002e
>> RAX: ffffffffffffffda RBX: 00005618c4cfec00 RCX: 00007f7cea2c791d
>> RDX: 0000000000000000 RSI: 00007ffedf612a80 RDI: 000000000000000b
>> RBP: 00007ffedf612a80 R08: 0000000000000000 R09: 00005618c4e74000
>> R10: 00005618c4da0590 R11: 0000000000000293 R12: 00005618c4cfec00
>> R13: 00005618c4cfe2c0 R14: 00007f7cea32ef80 R15: 00005618c4cff340
>> irq event stamp: 520897
>> hardirqs last  enabled at (520903): [<ffffffffba501cc5>] console_unlock+0x4e5/0x5d0
>> hardirqs last disabled at (520908): [<ffffffffba501c38>] console_unlock+0x458/0x5d0
>> softirqs last  enabled at (520722): [<ffffffffbb201002>] asm_call_irq_on_stack+0x12/0x20
>> softirqs last disabled at (520717): [<ffffffffbb201002>] asm_call_irq_on_stack+0x12/0x20
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> Bad timing, just yesterday I applied an identical patch:
> 

Yes it is.

> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=ath-next&id=7df28718928d08034b36168200d67b558ce36f3d
> 

No worries. I will apply yours to my repo

thanks,
-- Shuah


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] ath10k: fix WARNING: suspicious RCU usage
       [not found]   ` <20210210081320.2FBE5C433CA@smtp.codeaurora.org>
@ 2021-02-10 16:22     ` Shuah Khan
  0 siblings, 0 replies; 22+ messages in thread
From: Shuah Khan @ 2021-02-10 16:22 UTC (permalink / raw)
  To: Kalle Valo
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan, kuba, davem

On 2/10/21 1:13 AM, Kalle Valo wrote:
> Shuah Khan <skhan@linuxfoundation.org> wrote:
> 
>> ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
>> the resulting pointer is only valid under RCU lock as well.
>>
>> Fix ath10k_wmi_tlv_parse_peer_stats_info() to hold RCU lock before it
>> calls ieee80211_find_sta_by_ifaddr() and release it when the resulting
>> pointer is no longer needed. The log below shows the problem.
>>
>> While at it, fix ath10k_wmi_tlv_op_pull_peer_stats_info() to do the same.
>>
>> =============================
>> WARNING: suspicious RCU usage
>> 5.11.0-rc7+ #20 Tainted: G        W
>> -----------------------------
>> include/linux/rhashtable.h:594 suspicious rcu_dereference_check() usage!
>> other info that might help us debug this:
>>                 rcu_scheduler_active = 2, debug_locks = 1
>> no locks held by ksoftirqd/5/44.
>>
>> stack backtrace:
>> CPU: 5 PID: 44 Comm: ksoftirqd/5 Tainted: G        W         5.11.0-rc7+ #20
>> Hardware name: LENOVO 10VGCTO1WW/3130, BIOS M1XKT45A 08/21/2019
>> Call Trace:
>>   dump_stack+0x7d/0x9f
>>   lockdep_rcu_suspicious+0xdb/0xe5
>>   __rhashtable_lookup+0x1eb/0x260 [mac80211]
>>   ieee80211_find_sta_by_ifaddr+0x5b/0xc0 [mac80211]
>>   ath10k_wmi_tlv_parse_peer_stats_info+0x3e/0x90 [ath10k_core]
>>   ath10k_wmi_tlv_iter+0x6a/0xc0 [ath10k_core]
>>   ? ath10k_wmi_tlv_op_pull_mgmt_tx_bundle_compl_ev+0xe0/0xe0 [ath10k_core]
>>   ath10k_wmi_tlv_op_rx+0x5da/0xda0 [ath10k_core]
>>   ? trace_hardirqs_on+0x54/0xf0
>>   ? ath10k_ce_completed_recv_next+0x4e/0x60 [ath10k_core]
>>   ath10k_wmi_process_rx+0x1d/0x40 [ath10k_core]
>>   ath10k_htc_rx_completion_handler+0x115/0x180 [ath10k_core]
>>   ath10k_pci_process_rx_cb+0x149/0x1b0 [ath10k_pci]
>>   ? ath10k_htc_process_trailer+0x2d0/0x2d0 [ath10k_core]
>>   ? ath10k_pci_sleep.part.0+0x6a/0x80 [ath10k_pci]
>>   ath10k_pci_htc_rx_cb+0x15/0x20 [ath10k_pci]
>>   ath10k_ce_per_engine_service+0x61/0x80 [ath10k_core]
>>   ath10k_ce_per_engine_service_any+0x7d/0xa0 [ath10k_core]
>>   ath10k_pci_napi_poll+0x48/0x120 [ath10k_pci]
>>   net_rx_action+0x136/0x500
>>   __do_softirq+0xc6/0x459
>>   ? smpboot_thread_fn+0x2b/0x1f0
>>   run_ksoftirqd+0x2b/0x60
>>   smpboot_thread_fn+0x116/0x1f0
>>   kthread+0x14b/0x170
>>   ? smpboot_register_percpu_thread+0xe0/0xe0
>>   ? __kthread_bind_mask+0x70/0x70
>>   ret_from_fork+0x22/0x30
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> Unlucky timing also on this one, it conflicts with a patch I applied yesterday:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=ath-next&id=2615e3cdbd9c0e864f5906279c952a309871d225
> 
> Can you redo the patch to only change ath10k_wmi_event_tdls_peer()?
> 

Yes. I will send the patch just for ath10k_wmi_event_tdls_peer()
on top of your patch.

> error: patch failed: drivers/net/wireless/ath/ath10k/wmi-tlv.c:240
> error: drivers/net/wireless/ath/ath10k/wmi-tlv.c: patch does not apply
> stg import: Diff does not apply cleanly
> 
> Patch set to Changes Requested.
> 

thanks,
-- Shuah

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls
  2021-02-10  0:42 ` [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls Shuah Khan
  2021-02-10  8:03   ` kernel test robot
  2021-02-10  8:25   ` Kalle Valo
@ 2021-02-11  0:13   ` kernel test robot
  2 siblings, 0 replies; 22+ messages in thread
From: kernel test robot @ 2021-02-11  0:13 UTC (permalink / raw)
  To: Shuah Khan, kvalo, davem, kuba
  Cc: kbuild-all, netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

Hi Shuah,

I love your patch! Yet something to improve:

[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on wireless-drivers/master v5.11-rc7 next-20210125]
[cannot apply to ath6kl/ath-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Shuah-Khan/ath10k-fixes-for-warns/20210210-085259
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: m68k-allyesconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/5a96c0c9dfec2bd59e680b7ec8ade9378b654c4c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Shuah-Khan/ath10k-fixes-for-warns/20210210-085259
        git checkout 5a96c0c9dfec2bd59e680b7ec8ade9378b654c4c
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   m68k-linux-ld: drivers/net/wireless/ath/ath10k/mac.o: in function `ath10k_drain_tx':
>> (.text+0xc8b2): undefined reference to `lockdep_is_held'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59846 bytes --]

[-- Attachment #3: Type: text/plain, Size: 146 bytes --]

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/5] ath10k: change ath10k_offchan_tx_work() peer present msg to a warn
  2021-02-10  0:42 ` [PATCH 3/5] ath10k: change ath10k_offchan_tx_work() peer present msg to a warn Shuah Khan
@ 2021-02-11  6:50   ` Kalle Valo
  0 siblings, 0 replies; 22+ messages in thread
From: Kalle Valo @ 2021-02-11  6:50 UTC (permalink / raw)
  To: Shuah Khan
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan, kuba, davem

Shuah Khan <skhan@linuxfoundation.org> wrote:

> Based on the comment block in this function and the FIXME for this, peer
> being present for the offchannel tx is unlikely. Peer is deleted once tx
> is complete. Change peer present msg to a warn to detect this condition.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

83bae26532ca ath10k: change ath10k_offchan_tx_work() peer present msg to a warn

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/3b1f71272d56ee1d7f567fbce13bdb56cc06d342.1612915444.git.skhan@linuxfoundation.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls
  2021-02-10 15:57     ` Shuah Khan
@ 2021-02-11 11:20       ` Kalle Valo
  2021-02-11 20:53         ` Shuah Khan
  0 siblings, 1 reply; 22+ messages in thread
From: Kalle Valo @ 2021-02-11 11:20 UTC (permalink / raw)
  To: Shuah Khan; +Cc: netdev, linux-wireless, linux-kernel, ath10k, kuba, davem

Shuah Khan <skhan@linuxfoundation.org> writes:

> On 2/10/21 1:25 AM, Kalle Valo wrote:
>> Shuah Khan <skhan@linuxfoundation.org> writes:
>>
>>> ath10k_drain_tx() must not be called with conf_mutex held as workers can
>>> use that also. Add check to detect conf_mutex held calls.
>>>
>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>
>> The commit log does not answer to "Why?". How did you find this? What
>> actual problem are you trying to solve?
>>
>
> I came across the comment block above the ath10k_drain_tx() as I was
> reviewing at conf_mutex holds while I was debugging the conf_mutex
> lock assert in ath10k_debug_fw_stats_request().
>
> My reasoning is that having this will help detect incorrect usages
> of ath10k_drain_tx() while holding conf_mutex which could lead to
> locking problems when async worker routines try to call this routine.

Ok, makes sense. I prefer having this background info in the commit log,
for example "found by code review" or something like that. Or just copy
what you wrote above :)

>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>> @@ -4566,6 +4566,7 @@ static void
>>> ath10k_mac_op_wake_tx_queue(struct ieee80211_hw *hw,
>>>   /* Must not be called with conf_mutex held as workers can use that also. */
>>>   void ath10k_drain_tx(struct ath10k *ar)
>>>   {
>>> +	WARN_ON(lockdep_is_held(&ar->conf_mutex));
>>
>> Empty line after WARN_ON().
>>
>
> Will do.
>
>> Shouldn't this check debug_locks similarly lockdep_assert_held() does?
>>
>> #define lockdep_assert_held(l)	do {				\
>> 		WARN_ON(debug_locks && !lockdep_is_held(l));	\
>> 	} while (0)
>>
>> And I suspect you need #ifdef CONFIG_LOCKDEP which should fix the kbuild
>> bot error.
>>
>
> Yes.
>
>> But honestly I would prefer to have lockdep_assert_not_held() in
>> include/linux/lockdep.h, much cleaner that way. Also
>> i915_gem_object_lookup_rcu() could then use the same macro.
>>
>
> Right. This is the right way to go. That was first instinct and
> decided to have the discussion evolve in that direction. Now that
> it has, I will combine this change with
> include/linux/lockdep.h and add lockdep_assert_not_held()
>
> I think we might have other places in the kernel that could use
> lockdep_assert_not_held() in addition to i915_gem_object_lookup_rcu()

Great, thank you. The only problem is that lockdep.h changes have to go
via some other tree, I just don't know which :) I think it would be
easiest if also the ath10k patch goes via that other tree, I can ack the
ath10k changes.

Another option is that I'll apply the ath10k patch after the lockdep.h
change has trickled down to my tree, but that usually happens only after
the merge window and means weeks of waiting. Either is fine for me.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise
  2021-02-10 16:13       ` Shuah Khan
@ 2021-02-11 11:24         ` Kalle Valo
  2021-02-26 18:01           ` Shuah Khan
  0 siblings, 1 reply; 22+ messages in thread
From: Kalle Valo @ 2021-02-11 11:24 UTC (permalink / raw)
  To: Shuah Khan
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Wen Gong, kuba, davem

Shuah Khan <skhan@linuxfoundation.org> writes:

> On 2/10/21 1:28 AM, Kalle Valo wrote:
>> Wen Gong <wgong@codeaurora.org> writes:
>>
>>> On 2021-02-10 08:42, Shuah Khan wrote:
>>>> ath10k_mac_get_rate_flags_ht() floods dmesg with the following
>>>> messages,
>>>> when it fails to find a match for mcs=7 and rate=1440.
>>>>
>>>> supported_ht_mcs_rate_nss2:
>>>> {7,  {1300, 2700, 1444, 3000} }
>>>>
>>>> ath10k_pci 0000:02:00.0: invalid ht params rate 1440 100kbps nss 2
>>>> mcs 7
>>>>
>>>> dev_warn_ratelimited() isn't helping the noise. Use dev_warn_once()
>>>> instead.
>>>>
>>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>>> ---
>>>>   drivers/net/wireless/ath/ath10k/mac.c | 5 +++--
>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>>>> b/drivers/net/wireless/ath/ath10k/mac.c
>>>> index 3545ce7dce0a..276321f0cfdd 100644
>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>> @@ -8970,8 +8970,9 @@ static void ath10k_mac_get_rate_flags_ht(struct
>>>> ath10k *ar, u32 rate, u8 nss, u8
>>>>   		*bw |= RATE_INFO_BW_40;
>>>>   		*flags |= RATE_INFO_FLAGS_SHORT_GI;
>>>>   	} else {
>>>> -		ath10k_warn(ar, "invalid ht params rate %d 100kbps nss %d mcs %d",
>>>> -			    rate, nss, mcs);
>>>> +		dev_warn_once(ar->dev,
>>>> +			      "invalid ht params rate %d 100kbps nss %d mcs %d",
>>>> +			      rate, nss, mcs);
>>>>   	}
>>>>   }
>>>
>>> The {7,  {1300, 2700, 1444, 3000} } is a correct value.
>>> The 1440 is report from firmware, its a wrong value, it has fixed in
>>> firmware.
>>
>> In what version?
>>
>
> Here is the info:
>
> ath10k_pci 0000:02:00.0: qca6174 hw3.2 target 0x05030000 chip_id
> 0x00340aff sub 17aa:0827
>
> ath10k_pci 0000:02:00.0: firmware ver WLAN.RM.4.4.1-00140-QCARMSWPZ-1 
> api 6 features wowlan,ignore-otp,mfp crc32 29eb8ca1
>
> ath10k_pci 0000:02:00.0: board_file api 2 bmi_id N/A crc32 4ac0889b
>
> ath10k_pci 0000:02:00.0: htt-ver 3.60 wmi-op 4 htt-op 3 cal otp
> max-sta 32 raw 0 hwcrypto 1
>
>>> If change it to dev_warn_once, then it will have no chance to find the
>>> other wrong values which report by firmware, and it indicate
>>> a wrong value to mac80211/cfg80211 and lead "iw wlan0 station dump"
>>> get a wrong bitrate.
>>
>
> Agreed.
>
>> I agree, we should keep this warning. If the firmware still keeps
>> sending invalid rates we should add a specific check to ignore the known
>> invalid values, but not all of them.
>>
>
> Would it be helpful to adjust the default rate limits and set the to
> a higher value instead. It might be difficult to account all possible
> invalid values?
>
> Something like, ath10k_warn_ratelimited() to adjust the
>
> DEFAULT_RATELIMIT_INTERVAL and DEFAULT_RATELIMIT_BURST using
> DEFINE_RATELIMIT_STATE
>
> Let me know if you like this idea. I can send a patch in to do this.
> I will hang on to this firmware version for a little but longer, so
> we have a test case. :)

I would rather first try to fix the root cause, which is the firmware
sending invalid rates. Wen, you mentioned there's a fix in firmware. Do
you know which firmware version (and branch) has the fix?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls
  2021-02-11 11:20       ` Kalle Valo
@ 2021-02-11 20:53         ` Shuah Khan
  0 siblings, 0 replies; 22+ messages in thread
From: Shuah Khan @ 2021-02-11 20:53 UTC (permalink / raw)
  To: Kalle Valo
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan, kuba, davem

On 2/11/21 4:20 AM, Kalle Valo wrote:
> Shuah Khan <skhan@linuxfoundation.org> writes:
> 
>> On 2/10/21 1:25 AM, Kalle Valo wrote:
>>> Shuah Khan <skhan@linuxfoundation.org> writes:
>>>
>>>> ath10k_drain_tx() must not be called with conf_mutex held as workers can
>>>> use that also. Add check to detect conf_mutex held calls.
>>>>
>>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>>
>>> The commit log does not answer to "Why?". How did you find this? What
>>> actual problem are you trying to solve?
>>>
>>
>> I came across the comment block above the ath10k_drain_tx() as I was
>> reviewing at conf_mutex holds while I was debugging the conf_mutex
>> lock assert in ath10k_debug_fw_stats_request().
>>
>> My reasoning is that having this will help detect incorrect usages
>> of ath10k_drain_tx() while holding conf_mutex which could lead to
>> locking problems when async worker routines try to call this routine.
> 
> Ok, makes sense. I prefer having this background info in the commit log,
> for example "found by code review" or something like that. Or just copy
> what you wrote above :)
> 

Thanks. I will do that.

>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>> @@ -4566,6 +4566,7 @@ static void
>>>> ath10k_mac_op_wake_tx_queue(struct ieee80211_hw *hw,
>>>>    /* Must not be called with conf_mutex held as workers can use that also. */
>>>>    void ath10k_drain_tx(struct ath10k *ar)
>>>>    {
>>>> +	WARN_ON(lockdep_is_held(&ar->conf_mutex));
>>>
>>> Empty line after WARN_ON().
>>>
>>
>> Will do.
>>
>>> Shouldn't this check debug_locks similarly lockdep_assert_held() does?
>>>
>>> #define lockdep_assert_held(l)	do {				\
>>> 		WARN_ON(debug_locks && !lockdep_is_held(l));	\
>>> 	} while (0)
>>>
>>> And I suspect you need #ifdef CONFIG_LOCKDEP which should fix the kbuild
>>> bot error.
>>>
>>
>> Yes.
>>
>>> But honestly I would prefer to have lockdep_assert_not_held() in
>>> include/linux/lockdep.h, much cleaner that way. Also
>>> i915_gem_object_lookup_rcu() could then use the same macro.
>>>
>>
>> Right. This is the right way to go. That was first instinct and
>> decided to have the discussion evolve in that direction. Now that
>> it has, I will combine this change with
>> include/linux/lockdep.h and add lockdep_assert_not_held()
>>
>> I think we might have other places in the kernel that could use
>> lockdep_assert_not_held() in addition to i915_gem_object_lookup_rcu()
> 

I looked at i915_gem_object_lookup_rcu(). The following can be replaced
by lockdep_assert_held().

#ifdef CONFIG_LOCKDEP
         WARN_ON(debug_locks && !lock_is_held(&rcu_lock_map));
#endif

> Great, thank you. The only problem is that lockdep.h changes have to go
> via some other tree, I just don't know which :) I think it would be
> easiest if also the ath10k patch goes via that other tree, I can ack the
> ath10k changes.
> 
> Another option is that I'll apply the ath10k patch after the lockdep.h
> change has trickled down to my tree, but that usually happens only after
> the merge window and means weeks of waiting. Either is fine for me.
> 

I will send the include/linux/lockdep.h and ath10k patch together and
we will take it from there.

thanks,
-- Shuah

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise
  2021-02-11 11:24         ` Kalle Valo
@ 2021-02-26 18:01           ` Shuah Khan
  0 siblings, 0 replies; 22+ messages in thread
From: Shuah Khan @ 2021-02-26 18:01 UTC (permalink / raw)
  To: Kalle Valo, Wen Gong
  Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan, kuba, davem

On 2/11/21 4:24 AM, Kalle Valo wrote:
> Shuah Khan <skhan@linuxfoundation.org> writes:
> 
>> On 2/10/21 1:28 AM, Kalle Valo wrote:
>>> Wen Gong <wgong@codeaurora.org> writes:
>>>
>>>> On 2021-02-10 08:42, Shuah Khan wrote:
>>>>> ath10k_mac_get_rate_flags_ht() floods dmesg with the following
>>>>> messages,
>>>>> when it fails to find a match for mcs=7 and rate=1440.
>>>>>
>>>>> supported_ht_mcs_rate_nss2:
>>>>> {7,  {1300, 2700, 1444, 3000} }
>>>>>
>>>>> ath10k_pci 0000:02:00.0: invalid ht params rate 1440 100kbps nss 2
>>>>> mcs 7
>>>>>
>>>>> dev_warn_ratelimited() isn't helping the noise. Use dev_warn_once()
>>>>> instead.
>>>>>
>>>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>>>> ---
>>>>>    drivers/net/wireless/ath/ath10k/mac.c | 5 +++--
>>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c
>>>>> b/drivers/net/wireless/ath/ath10k/mac.c
>>>>> index 3545ce7dce0a..276321f0cfdd 100644
>>>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>>>> @@ -8970,8 +8970,9 @@ static void ath10k_mac_get_rate_flags_ht(struct
>>>>> ath10k *ar, u32 rate, u8 nss, u8
>>>>>    		*bw |= RATE_INFO_BW_40;
>>>>>    		*flags |= RATE_INFO_FLAGS_SHORT_GI;
>>>>>    	} else {
>>>>> -		ath10k_warn(ar, "invalid ht params rate %d 100kbps nss %d mcs %d",
>>>>> -			    rate, nss, mcs);
>>>>> +		dev_warn_once(ar->dev,
>>>>> +			      "invalid ht params rate %d 100kbps nss %d mcs %d",
>>>>> +			      rate, nss, mcs);
>>>>>    	}
>>>>>    }
>>>>
>>>> The {7,  {1300, 2700, 1444, 3000} } is a correct value.
>>>> The 1440 is report from firmware, its a wrong value, it has fixed in
>>>> firmware.
>>>
>>> In what version?
>>>
>>
>> Here is the info:
>>
>> ath10k_pci 0000:02:00.0: qca6174 hw3.2 target 0x05030000 chip_id
>> 0x00340aff sub 17aa:0827
>>
>> ath10k_pci 0000:02:00.0: firmware ver WLAN.RM.4.4.1-00140-QCARMSWPZ-1
>> api 6 features wowlan,ignore-otp,mfp crc32 29eb8ca1
>>
>> ath10k_pci 0000:02:00.0: board_file api 2 bmi_id N/A crc32 4ac0889b
>>
>> ath10k_pci 0000:02:00.0: htt-ver 3.60 wmi-op 4 htt-op 3 cal otp
>> max-sta 32 raw 0 hwcrypto 1
>>
>>>> If change it to dev_warn_once, then it will have no chance to find the
>>>> other wrong values which report by firmware, and it indicate
>>>> a wrong value to mac80211/cfg80211 and lead "iw wlan0 station dump"
>>>> get a wrong bitrate.
>>>
>>
>> Agreed.
>>
>>> I agree, we should keep this warning. If the firmware still keeps
>>> sending invalid rates we should add a specific check to ignore the known
>>> invalid values, but not all of them.
>>>
>>
>> Would it be helpful to adjust the default rate limits and set the to
>> a higher value instead. It might be difficult to account all possible
>> invalid values?
>>
>> Something like, ath10k_warn_ratelimited() to adjust the
>>
>> DEFAULT_RATELIMIT_INTERVAL and DEFAULT_RATELIMIT_BURST using
>> DEFINE_RATELIMIT_STATE
>>
>> Let me know if you like this idea. I can send a patch in to do this.
>> I will hang on to this firmware version for a little but longer, so
>> we have a test case. :)
> 
> I would rather first try to fix the root cause, which is the firmware
> sending invalid rates. Wen, you mentioned there's a fix in firmware. Do
> you know which firmware version (and branch) has the fix?
> 

Picking this back up. Wen, which firmware version has this fix? I can
test this on my system and get rid of the noisy messages. :)

thanks,
-- Shuah

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2021-02-26 18:02 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-10  0:42 [PATCH 0/5] ath10k fixes for warns Shuah Khan
2021-02-10  0:42 ` [PATCH 1/5] ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request() Shuah Khan
2021-02-10  8:09   ` Kalle Valo
     [not found]   ` <20210210080915.8A81AC433C6@smtp.codeaurora.org>
2021-02-10 16:21     ` Shuah Khan
2021-02-10  0:42 ` [PATCH 2/5] ath10k: fix WARNING: suspicious RCU usage Shuah Khan
2021-02-10  8:13   ` Kalle Valo
     [not found]   ` <20210210081320.2FBE5C433CA@smtp.codeaurora.org>
2021-02-10 16:22     ` Shuah Khan
2021-02-10  0:42 ` [PATCH 3/5] ath10k: change ath10k_offchan_tx_work() peer present msg to a warn Shuah Khan
2021-02-11  6:50   ` Kalle Valo
2021-02-10  0:42 ` [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls Shuah Khan
2021-02-10  8:03   ` kernel test robot
2021-02-10  8:25   ` Kalle Valo
2021-02-10 15:57     ` Shuah Khan
2021-02-11 11:20       ` Kalle Valo
2021-02-11 20:53         ` Shuah Khan
2021-02-11  0:13   ` kernel test robot
2021-02-10  0:42 ` [PATCH 5/5] ath10k: reduce invalid ht params rate message noise Shuah Khan
2021-02-10  2:36   ` Wen Gong
2021-02-10  8:28     ` Kalle Valo
2021-02-10 16:13       ` Shuah Khan
2021-02-11 11:24         ` Kalle Valo
2021-02-26 18:01           ` Shuah Khan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).