RE: [PATCH] wifi: rtlwifi: rtl8723be: Remove unnecessary irq save/restore in hw_init()

[Ping-Ke Shih]

William Hansen-Baird <william.hansen.baird@xxxxxxxxx> wrote:
> rtl8723be hw_init() calls local_save_flags(flags) followed by
> local_irq_enable(). Later, local_irq_restore(flags) is called.
> 
> This causes warnings from Lockdep on boot and modprobe,
> as local_irq_restore(flags) should only be called while irqs are disabled.
> 
> With testing I found that all paths which call hw_init() have irqs
> already enabled for rtl8723be.

By commits you found below, current hw_init() is not in scope of
spin_lock_irqsave(), so irqs is not disabled, right?

> 
> Furthermore, the calls were originally added for the rtl8192ce
> in commit f78bccd79ba3 ("rtlwifi: rtl8192ce: Fix too long disable of IRQs")
> before later being added to most other rtlwifi drivers.

This commit adding the code you are going to remove, is to resolve
"disabling for too long the local interrupts". I think current code
has no this problem, so I think removal is okay.

> 
> Commit d3feae41a347 ("rtlwifi: Update power-save routines for 062814 driver")
> then replaces the call to spin_lock_irqsave() before hw_init(),
> and thus the codepath which caused irqs to be disabled in hw_init and
> prompted the original commit has been removed.

> 
> The same irq save/restore pattern is also present in the hw_init() of
> rtl8192ce, rtl8723ae, rtl8188ee, rtl8192se and rtl8192cu,
> however I don't have the hardware to test them,
> so I did not include them in my changes.

Okay to me.

> 
> Tested on a Razer Blade 14 2017.
> 
> Example of output from Lockdep prior to fix:
> 
> [ 2305.476471] raw_local_irq_restore() called with IRQs enabled
> 
> ...
> 
> [ 2305.478709] Call Trace:
> [ 2305.478731]  <TASK>
> [ 2305.478753]  rtl8723be_hw_init+0x5992/0x7220 [rtl8723be]
> [ 2305.478798]  ? static_obj+0x61/0xa0
> [ 2305.478848]  rtl_pci_start+0x222/0x5c0 [rtl_pci]
> [ 2305.478891]  rtl_op_start+0x128/0x1a0 [rtlwifi]
> [ 2305.478940]  ? __kasan_check_read+0x11/0x20
> [ 2305.480082]  drv_start+0x16c/0x550 [mac80211]

Not prefer timestamp in commit message.

> 
> ...
> 
> [ 2305.570855] irq event stamp: 887679
> [ 2305.571569] hardirqs last  enabled at (887689): [<ffffffff96511170>] __up_console_sem+0x90/0xa0
> [ 2305.572347] hardirqs last disabled at (887698): [<ffffffff96511155>] __up_console_sem+0x75/0xa0
> [ 2305.573076] softirqs last  enabled at (887670): [<ffffffff962f4675>] __irq_exit_rcu+0x175/0x2f0
> [ 2305.573817] softirqs last disabled at (887649): [<ffffffff962f4675>] __irq_exit_rcu+0x175/0x2f0
> [ 2305.574636] ---[ end trace 0000000000000000 ]---
> 
> Link: https://lore.kernel.org/all/20210111153707.10071-1-mark.rutland@xxxxxxx/

I think this is point out the commit which introduce the detection of
"raw_local_irq_restore() called with IRQs enabled". I think this isn't
very highly related to this commit. 

Please use below style, and describe what it is in commit message.
It can be ignored if a reviewer has understood this. 

[1] https://lore.kernel.org/all/20210111153707.10071-1-mark.rutland@xxxxxxx/

(a blank line before s-o-b line)

> Signed-off-by: William Hansen-Baird <williamhb+k@xxxxxxxxxxxx>

My comments are minor, so

Acked-by: Ping-Ke Shih <pkshih@xxxxxxxxxxx>

Please revise commit message and take my ack to v2.


By the way, subject prefix should be "[PATCH rtw-next] wifi: rtlwifi: ..."

Ping-Ke