[PATCH 0/5] ath10k fixes for warns

[PATCH 0/5] ath10k fixes for warns

[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

[PATCH 1/5] ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request()

[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

Re: [PATCH 1/5] ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request()

[Kalle Valo]

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

Re: [PATCH 1/5] ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request()

[Shuah Khan]

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

[PATCH 2/5] ath10k: fix WARNING: suspicious RCU usage

[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

Re: [PATCH 2/5] ath10k: fix WARNING: suspicious RCU usage

[Kalle Valo]

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

Re: [PATCH 2/5] ath10k: fix WARNING: suspicious RCU usage

[Shuah Khan]

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

[PATCH 3/5] ath10k: change ath10k_offchan_tx_work() peer present msg to a warn

[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

Re: [PATCH 3/5] ath10k: change ath10k_offchan_tx_work() peer present msg to a warn

[Kalle Valo]

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

[PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls

[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

Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls

[kernel test robot]

[-- 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 --]

Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls

[Kalle Valo]

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

Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls

[Shuah Khan]

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

Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls

[Kalle Valo]

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

Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls

[Shuah Khan]

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

Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls

[kernel test robot]

[-- 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 --]

[PATCH 5/5] ath10k: reduce invalid ht params rate message noise

[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

Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise

[Wen Gong]

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.

Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise

[Kalle Valo]

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

Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise

[Shuah Khan]

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

Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise

[Kalle Valo]

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

Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise

[Shuah Khan]

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

Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise

[James Prestwood]

On 2/26/21 10:01 AM, Shuah Khan wrote:
> 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

I know its been years, but reading through this Wen mentioned there is a 
fix in the firmware? I haven't tried all of the firmware binaries in 
Kalle's tree but the most recent definitely still spam the logs with 
this message. Is there a specific version I can use to get rid of these?

One thing to note is the older "firmware-4.bin" did not have this 
problem, but was met with worse problems like driver/firmware crashes.

Thanks,

James

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

Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise

[Jeff Johnson]

(just a resend with Wen's current e-mail address, no further comments)
On 9/20/2023 11:27 AM, James Prestwood wrote:
> On 2/26/21 10:01 AM, Shuah Khan wrote:
>> 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
> 
> I know its been years, but reading through this Wen mentioned there is a 
> fix in the firmware? I haven't tried all of the firmware binaries in 
> Kalle's tree but the most recent definitely still spam the logs with 
> this message. Is there a specific version I can use to get rid of these?
> 
> One thing to note is the older "firmware-4.bin" did not have this 
> problem, but was met with worse problems like driver/firmware crashes.

Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise

[James Prestwood]

On 9/20/23 12:23 PM, Jeff Johnson wrote:
> (just a resend with Wen's current e-mail address, no further comments)
> On 9/20/2023 11:27 AM, James Prestwood wrote:
>> On 2/26/21 10:01 AM, Shuah Khan wrote:
>>> 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
>>
>> I know its been years, but reading through this Wen mentioned there 
>> is a fix in the firmware? I haven't tried all of the firmware 
>> binaries in Kalle's tree but the most recent definitely still spam 
>> the logs with this message. Is there a specific version I can use to 
>> get rid of these?
>>
>> One thing to note is the older "firmware-4.bin" did not have this 
>> problem, but was met with worse problems like driver/firmware crashes.

I hate to keep bringing this up, and if its a "won't fix" type of issue 
you don't have to tell me twice and I can deal with it out of tree. Any 
answer would be greatly appreciated so I know how to proceed, and if its 
something I can wait for on upstream or handle on my own. Since its 
apparently a firmware bug its not something I can fix, or I would try to 
myself.

(sorry, my last inquiry was mistakenly from a different email address).

Thanks,

James

Re: [PATCH 5/5] ath10k: reduce invalid ht params rate message noise

[Jeff Johnson]

On 11/14/2023 11:31 AM, James Prestwood wrote:
> I hate to keep bringing this up, and if its a "won't fix" type of issue 
> you don't have to tell me twice and I can deal with it out of tree. Any 
> answer would be greatly appreciated so I know how to proceed, and if its 
> something I can wait for on upstream or handle on my own. Since its 
> apparently a firmware bug its not something I can fix, or I would try to 
> myself.

I've asked the development team for an update.

/jeff