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

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

[William Hansen-Baird]

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.

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.

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.

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]

...

[ 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@arm.com/
Signed-off-by: William Hansen-Baird <williamhb+k@fastmail.com>
---
 drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
index e1f811218894..bf7b5a32adaa 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
@@ -1333,11 +1333,6 @@ int rtl8723be_hw_init(struct ieee80211_hw *hw)
 	bool rtstatus = true;
 	int err;
 	u8 tmp_u1b;
-	unsigned long flags;
-
-	/* reenable interrupts to not interfere with other devices */
-	local_save_flags(flags);
-	local_irq_enable();
 
 	rtlhal->fw_ready = false;
 	rtlpriv->rtlhal.being_init_adapter = true;
@@ -1443,7 +1438,6 @@ int rtl8723be_hw_init(struct ieee80211_hw *hw)
 
 	rtl8723be_dm_init(hw);
 exit:
-	local_irq_restore(flags);
 	rtlpriv->rtlhal.being_init_adapter = false;
 	return err;
 }
-- 
2.54.0

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

[Ping-Ke Shih]

William Hansen-Baird <william.hansen.baird@gmail.com> 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@arm.com/

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@arm.com/

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

> Signed-off-by: William Hansen-Baird <williamhb+k@fastmail.com>

My comments are minor, so

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

Please revise commit message and take my ack to v2.


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

Ping-Ke

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

[William Hansen-Baird]

Thanks for the review. I'll fix the commit message and send a v2 with your ack included.

[PATCH rtw-next v2] wifi: rtlwifi: rtl8723be: Remove unnecessary irq save/restore in hw_init()

[William Hansen-Baird]

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.
The warning was introduced to detect this class of bug in [1].

With testing I found that all paths which call hw_init() have irqs
already enabled for rtl8723be.

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.

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.

Tested on a Razer Blade 14 2017.

Example of output from Lockdep prior to fix:

raw_local_irq_restore() called with IRQs enabled

...

Call Trace:
<TASK>
rtl8723be_hw_init+0x5992/0x7220 [rtl8723be]
? static_obj+0x61/0xa0
rtl_pci_start+0x222/0x5c0 [rtl_pci]
rtl_op_start+0x128/0x1a0 [rtlwifi]
? __kasan_check_read+0x11/0x20
drv_start+0x16c/0x550 [mac80211]

...

irq event stamp: 887679
hardirqs last  enabled at (887689): [<ffffffff96511170>] __up_console_sem+0x90/0xa0
hardirqs last disabled at (887698): [<ffffffff96511155>] __up_console_sem+0x75/0xa0
softirqs last  enabled at (887670): [<ffffffff962f4675>] __irq_exit_rcu+0x175/0x2f0
softirqs last disabled at (887649): [<ffffffff962f4675>] __irq_exit_rcu+0x175/0x2f0
---[ end trace 0000000000000000 ]---

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

Signed-off-by: William Hansen-Baird <william.hansen.baird@gmail.com>
Acked-by: Ping-Ke Shih <pkshih@realtek.com>
---
Changes since v1:
- Remove timestamps from dmesg output
- Use [1] footnote style for link
- Fix subject prefix to rtw-next

 drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
index e1f811218894..bf7b5a32adaa 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
@@ -1333,11 +1333,6 @@ int rtl8723be_hw_init(struct ieee80211_hw *hw)
 	bool rtstatus = true;
 	int err;
 	u8 tmp_u1b;
-	unsigned long flags;
-
-	/* reenable interrupts to not interfere with other devices */
-	local_save_flags(flags);
-	local_irq_enable();
 
 	rtlhal->fw_ready = false;
 	rtlpriv->rtlhal.being_init_adapter = true;
@@ -1443,7 +1438,6 @@ int rtl8723be_hw_init(struct ieee80211_hw *hw)
 
 	rtl8723be_dm_init(hw);
 exit:
-	local_irq_restore(flags);
 	rtlpriv->rtlhal.being_init_adapter = false;
 	return err;
 }
-- 
2.54.0