Routine rtl_addr_delay() uses delay statements in code that can
sleep. To improve system responsiveness, the various delay statements
are changed.
In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are
rewritten to use the code in rtl_addr_delay() for most of their
input values.
Suggested-by: Byeoungwook Kim <[email protected]>
Signed-off-by: Larry Finger <[email protected]>
---
Kalle,
This patch will interfere with a set of 3 patches submitted by Byeoungwook Kim
<[email protected]> on Feb. 3, 2016 that are currently in the deferred list
at patchwork.
Larry
drivers/net/wireless/realtek/rtlwifi/core.c | 44 ++++++++---------------------
1 file changed, 12 insertions(+), 32 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c
index 02eba0e..16ad0d6 100644
--- a/drivers/net/wireless/realtek/rtlwifi/core.c
+++ b/drivers/net/wireless/realtek/rtlwifi/core.c
@@ -54,59 +54,39 @@ EXPORT_SYMBOL(channel5g_80m);
void rtl_addr_delay(u32 addr)
{
if (addr == 0xfe)
- mdelay(50);
+ msleep(50);
else if (addr == 0xfd)
- mdelay(5);
+ msleep(5);
else if (addr == 0xfc)
- mdelay(1);
+ msleep(1);
else if (addr == 0xfb)
- udelay(50);
+ usleep_range(50, 100);
else if (addr == 0xfa)
- udelay(5);
+ usleep_range(5, 10);
else if (addr == 0xf9)
- udelay(1);
+ usleep_range(1, 2);
}
EXPORT_SYMBOL(rtl_addr_delay);
void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32 addr,
u32 mask, u32 data)
{
- if (addr == 0xfe) {
- mdelay(50);
- } else if (addr == 0xfd) {
- mdelay(5);
- } else if (addr == 0xfc) {
- mdelay(1);
- } else if (addr == 0xfb) {
- udelay(50);
- } else if (addr == 0xfa) {
- udelay(5);
- } else if (addr == 0xf9) {
- udelay(1);
+ if (addr >= 0xf9 && addr <= 0xfe) {
+ rtl_addr_delay(addr);
} else {
rtl_set_rfreg(hw, rfpath, addr, mask, data);
- udelay(1);
+ usleep_range(1, 2);
}
}
EXPORT_SYMBOL(rtl_rfreg_delay);
void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data)
{
- if (addr == 0xfe) {
- mdelay(50);
- } else if (addr == 0xfd) {
- mdelay(5);
- } else if (addr == 0xfc) {
- mdelay(1);
- } else if (addr == 0xfb) {
- udelay(50);
- } else if (addr == 0xfa) {
- udelay(5);
- } else if (addr == 0xf9) {
- udelay(1);
+ if (addr >= 0xf9 && addr <= 0xfe) {
+ rtl_addr_delay(addr);
} else {
rtl_set_bbreg(hw, addr, MASKDWORD, data);
- udelay(1);
+ usleep_range(1, 2);
}
}
EXPORT_SYMBOL(rtl_bb_delay);
--
2.1.4
2016-02-20 23:06 GMT+09:00 Larry Finger <[email protected]>:
> On 02/19/2016 11:48 PM, ByeoungWook Kim wrote:
>>
>> 2016-02-16 7:12 GMT+09:00 Larry Finger <[email protected]>:
>>>
>>> Routine rtl_addr_delay() uses delay statements in code that can
>>> sleep. To improve system responsiveness, the various delay statements
>>> are changed.
>>>
>>> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are
>>> rewritten to use the code in rtl_addr_delay() for most of their
>>> input values.
>>>
>>> Suggested-by: Byeoungwook Kim <[email protected]>
>>> Signed-off-by: Larry Finger <[email protected]>
>>> ---
>>
>>
>> Why you are using 'Suggested-by:' of me?
>> I think to that your commit was included a part of my
>> commit(https://patchwork.kernel.org/patch/8197071/).
>
>
> Your commit was placed in the deferred list of patches. It did lead me to
> look at that section and realize that using delays there was a bug, and that
> it should be fixed as quickly as possible. That argues against waiting for
> your patches to be implemented. I did use some of your changes. How do you
> think I should have referenced your material?
>
> Larry
>
>
I'm understood your mail.
Only, I wondered why 'Suggested-by' was used.
Thank for your answer.
Regards,
Byeoungwook
On 02/19/2016 11:48 PM, ByeoungWook Kim wrote:
> 2016-02-16 7:12 GMT+09:00 Larry Finger <[email protected]>:
>> Routine rtl_addr_delay() uses delay statements in code that can
>> sleep. To improve system responsiveness, the various delay statements
>> are changed.
>>
>> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are
>> rewritten to use the code in rtl_addr_delay() for most of their
>> input values.
>>
>> Suggested-by: Byeoungwook Kim <[email protected]>
>> Signed-off-by: Larry Finger <[email protected]>
>> ---
>
> Why you are using 'Suggested-by:' of me?
> I think to that your commit was included a part of my
> commit(https://patchwork.kernel.org/patch/8197071/).
Your commit was placed in the deferred list of patches. It did lead me to look
at that section and realize that using delays there was a bug, and that it
should be fixed as quickly as possible. That argues against waiting for your
patches to be implemented. I did use some of your changes. How do you think I
should have referenced your material?
Larry
2016-02-16 7:12 GMT+09:00 Larry Finger <[email protected]>:
> Routine rtl_addr_delay() uses delay statements in code that can
> sleep. To improve system responsiveness, the various delay statements
> are changed.
>
> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are
> rewritten to use the code in rtl_addr_delay() for most of their
> input values.
>
> Suggested-by: Byeoungwook Kim <[email protected]>
> Signed-off-by: Larry Finger <[email protected]>
> ---
Why you are using 'Suggested-by:' of me?
I think to that your commit was included a part of my
commit(https://patchwork.kernel.org/patch/8197071/).
>
> Kalle,
>
> This patch will interfere with a set of 3 patches submitted by Byeoungwook Kim
> <[email protected]> on Feb. 3, 2016 that are currently in the deferred list
> at patchwork.
>
> Larry
>
> drivers/net/wireless/realtek/rtlwifi/core.c | 44 ++++++++---------------------
> 1 file changed, 12 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c
> index 02eba0e..16ad0d6 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/core.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/core.c
> @@ -54,59 +54,39 @@ EXPORT_SYMBOL(channel5g_80m);
> void rtl_addr_delay(u32 addr)
> {
> if (addr == 0xfe)
> - mdelay(50);
> + msleep(50);
> else if (addr == 0xfd)
> - mdelay(5);
> + msleep(5);
> else if (addr == 0xfc)
> - mdelay(1);
> + msleep(1);
> else if (addr == 0xfb)
> - udelay(50);
> + usleep_range(50, 100);
> else if (addr == 0xfa)
> - udelay(5);
> + usleep_range(5, 10);
> else if (addr == 0xf9)
> - udelay(1);
> + usleep_range(1, 2);
> }
> EXPORT_SYMBOL(rtl_addr_delay);
>
> void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32 addr,
> u32 mask, u32 data)
> {
> - if (addr == 0xfe) {
> - mdelay(50);
> - } else if (addr == 0xfd) {
> - mdelay(5);
> - } else if (addr == 0xfc) {
> - mdelay(1);
> - } else if (addr == 0xfb) {
> - udelay(50);
> - } else if (addr == 0xfa) {
> - udelay(5);
> - } else if (addr == 0xf9) {
> - udelay(1);
> + if (addr >= 0xf9 && addr <= 0xfe) {
> + rtl_addr_delay(addr);
> } else {
> rtl_set_rfreg(hw, rfpath, addr, mask, data);
> - udelay(1);
> + usleep_range(1, 2);
> }
> }
> EXPORT_SYMBOL(rtl_rfreg_delay);
>
> void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data)
> {
> - if (addr == 0xfe) {
> - mdelay(50);
> - } else if (addr == 0xfd) {
> - mdelay(5);
> - } else if (addr == 0xfc) {
> - mdelay(1);
> - } else if (addr == 0xfb) {
> - udelay(50);
> - } else if (addr == 0xfa) {
> - udelay(5);
> - } else if (addr == 0xf9) {
> - udelay(1);
> + if (addr >= 0xf9 && addr <= 0xfe) {
> + rtl_addr_delay(addr);
> } else {
> rtl_set_bbreg(hw, addr, MASKDWORD, data);
> - udelay(1);
> + usleep_range(1, 2);
> }
> }
> EXPORT_SYMBOL(rtl_bb_delay);
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Regards,
Byeoungwook
On Tue, Feb 16, 2016 at 3:42 AM, Larry Finger <[email protected]> wrote:
> Routine rtl_addr_delay() uses delay statements in code that can
> sleep. To improve system responsiveness, the various delay statements
> are changed.
>
> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are
> rewritten to use the code in rtl_addr_delay() for most of their
> input values.
>
> Suggested-by: Byeoungwook Kim <[email protected]>
> Signed-off-by: Larry Finger <[email protected]>
> ---
>
> Kalle,
>
> This patch will interfere with a set of 3 patches submitted by Byeoungwook Kim
> <[email protected]> on Feb. 3, 2016 that are currently in the deferred list
> at patchwork.
>
> Larry
>
> drivers/net/wireless/realtek/rtlwifi/core.c | 44 ++++++++---------------------
> 1 file changed, 12 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c
> index 02eba0e..16ad0d6 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/core.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/core.c
> @@ -54,59 +54,39 @@ EXPORT_SYMBOL(channel5g_80m);
> void rtl_addr_delay(u32 addr)
> {
> if (addr == 0xfe)
> - mdelay(50);
> + msleep(50);
> else if (addr == 0xfd)
> - mdelay(5);
> + msleep(5);
> else if (addr == 0xfc)
> - mdelay(1);
> + msleep(1);
> else if (addr == 0xfb)
> - udelay(50);
> + usleep_range(50, 100);
> else if (addr == 0xfa)
> - udelay(5);
> + usleep_range(5, 10);
> else if (addr == 0xf9)
> - udelay(1);
> + usleep_range(1, 2);
why udelay is replaced by usleep_range?
> }
> EXPORT_SYMBOL(rtl_addr_delay);
>
> void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32 addr,
> u32 mask, u32 data)
> {
> - if (addr == 0xfe) {
> - mdelay(50);
> - } else if (addr == 0xfd) {
> - mdelay(5);
> - } else if (addr == 0xfc) {
> - mdelay(1);
> - } else if (addr == 0xfb) {
> - udelay(50);
> - } else if (addr == 0xfa) {
> - udelay(5);
> - } else if (addr == 0xf9) {
> - udelay(1);
> + if (addr >= 0xf9 && addr <= 0xfe) {
> + rtl_addr_delay(addr);
> } else {
> rtl_set_rfreg(hw, rfpath, addr, mask, data);
> - udelay(1);
> + usleep_range(1, 2);
> }
> }
> EXPORT_SYMBOL(rtl_rfreg_delay);
>
> void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data)
> {
> - if (addr == 0xfe) {
> - mdelay(50);
> - } else if (addr == 0xfd) {
> - mdelay(5);
> - } else if (addr == 0xfc) {
> - mdelay(1);
> - } else if (addr == 0xfb) {
> - udelay(50);
> - } else if (addr == 0xfa) {
> - udelay(5);
> - } else if (addr == 0xf9) {
> - udelay(1);
> + if (addr >= 0xf9 && addr <= 0xfe) {
> + rtl_addr_delay(addr);
> } else {
> rtl_set_bbreg(hw, addr, MASKDWORD, data);
> - udelay(1);
> + usleep_range(1, 2);
> }
> }
> EXPORT_SYMBOL(rtl_bb_delay);
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-Souptick
On 02/16/2016 12:17 AM, Souptick Joarder wrote:
> On Tue, Feb 16, 2016 at 3:42 AM, Larry Finger <[email protected]> wrote:
--snip--
>> else if (addr == 0xf9)
>> - udelay(1);
>> + usleep_range(1, 2);
>
> why udelay is replaced by usleep_range?
I'm not sure of your level of sophistication, but here goes.
All delay statements cause a processor to stay in control and make the system
wait for that amount of time. A sleep statement allows a context switch, and the
processor is able to run some other job. For that reason, sleeps are always
preferred over delays as long as the code is not running in atomic context.
There used to be a usleep() function, but as the system cannot promise to return
from sleep after a specific delay, it was replaced with usleep_range().
It is true that the difference between delaying and sleeping for 1 usec would
not be too destructive to the system, but I decided to convert every branch of
that if structure to sleep statements.
Larry
> Routine rtl_addr_delay() uses delay statements in code that can
> sleep. To improve system responsiveness, the various delay statements
> are changed.
>
> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are
> rewritten to use the code in rtl_addr_delay() for most of their
> input values.
>
> Suggested-by: Byeoungwook Kim <[email protected]>
> Signed-off-by: Larry Finger <[email protected]>
Thanks, applied to wireless-drivers-next.git.
Kalle Valo
On 2016-06-04 18:52, Kalle Valo wrote:
> Jan Kiszka <[email protected]> writes:
>
>> On 2016-02-15 23:12, Larry Finger wrote:
>>> Routine rtl_addr_delay() uses delay statements in code that can
>>> sleep. To improve system responsiveness, the various delay statements
>>> are changed.
>>>
>>> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are
>>> rewritten to use the code in rtl_addr_delay() for most of their
>>> input values.
>>>
>>> Suggested-by: Byeoungwook Kim <[email protected]>
>>> Signed-off-by: Larry Finger <[email protected]>
>
> [...]
>
>> This breaks spectacularly when turning on a little bit of correctness
>> checking:
>>
>> BUG: scheduling while atomic: wpa_supplicant/1116/0x00000002
>
> This should fix it:
>
> https://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=de26859dcf363d520cc44e59f6dcaf20ebe0aadf
>
Probably, will test later. But you should really work on making all
these task-context-only. Threaded IRQs?
Jan
On 2016-02-15 23:12, Larry Finger wrote:
> Routine rtl_addr_delay() uses delay statements in code that can
> sleep. To improve system responsiveness, the various delay statements
> are changed.
>
> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are
> rewritten to use the code in rtl_addr_delay() for most of their
> input values.
>
> Suggested-by: Byeoungwook Kim <[email protected]>
> Signed-off-by: Larry Finger <[email protected]>
> ---
>
> Kalle,
>
> This patch will interfere with a set of 3 patches submitted by Byeoungwook Kim
> <[email protected]> on Feb. 3, 2016 that are currently in the deferred list
> at patchwork.
>
> Larry
>
> drivers/net/wireless/realtek/rtlwifi/core.c | 44 ++++++++---------------------
> 1 file changed, 12 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c
> index 02eba0e..16ad0d6 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/core.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/core.c
> @@ -54,59 +54,39 @@ EXPORT_SYMBOL(channel5g_80m);
> void rtl_addr_delay(u32 addr)
> {
> if (addr == 0xfe)
> - mdelay(50);
> + msleep(50);
> else if (addr == 0xfd)
> - mdelay(5);
> + msleep(5);
> else if (addr == 0xfc)
> - mdelay(1);
> + msleep(1);
> else if (addr == 0xfb)
> - udelay(50);
> + usleep_range(50, 100);
> else if (addr == 0xfa)
> - udelay(5);
> + usleep_range(5, 10);
> else if (addr == 0xf9)
> - udelay(1);
> + usleep_range(1, 2);
> }
> EXPORT_SYMBOL(rtl_addr_delay);
>
> void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32 addr,
> u32 mask, u32 data)
> {
> - if (addr == 0xfe) {
> - mdelay(50);
> - } else if (addr == 0xfd) {
> - mdelay(5);
> - } else if (addr == 0xfc) {
> - mdelay(1);
> - } else if (addr == 0xfb) {
> - udelay(50);
> - } else if (addr == 0xfa) {
> - udelay(5);
> - } else if (addr == 0xf9) {
> - udelay(1);
> + if (addr >= 0xf9 && addr <= 0xfe) {
> + rtl_addr_delay(addr);
> } else {
> rtl_set_rfreg(hw, rfpath, addr, mask, data);
> - udelay(1);
> + usleep_range(1, 2);
> }
> }
> EXPORT_SYMBOL(rtl_rfreg_delay);
>
> void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data)
> {
> - if (addr == 0xfe) {
> - mdelay(50);
> - } else if (addr == 0xfd) {
> - mdelay(5);
> - } else if (addr == 0xfc) {
> - mdelay(1);
> - } else if (addr == 0xfb) {
> - udelay(50);
> - } else if (addr == 0xfa) {
> - udelay(5);
> - } else if (addr == 0xf9) {
> - udelay(1);
> + if (addr >= 0xf9 && addr <= 0xfe) {
> + rtl_addr_delay(addr);
> } else {
> rtl_set_bbreg(hw, addr, MASKDWORD, data);
> - udelay(1);
> + usleep_range(1, 2);
> }
> }
> EXPORT_SYMBOL(rtl_bb_delay);
>
This breaks spectacularly when turning on a little bit of correctness
checking:
BUG: scheduling while atomic: wpa_supplicant/1116/0x00000002
[...]
Call Trace:
[<ffffffff81030009>] dump_trace+0x59/0x340
[<ffffffff810303ec>] show_stack_log_lvl+0xfc/0x190
[<ffffffff81031191>] show_stack+0x21/0x40
[<ffffffff813827ae>] dump_stack+0x5c/0x7e
[<ffffffff81181337>] __schedule_bug+0x4b/0x59
[<ffffffff81680f12>] thread_return+0x562/0x6b0
[<ffffffff8168109c>] schedule+0x3c/0x90
[<ffffffff81683dfb>] schedule_hrtimeout_range_clock+0xab/0x130
[<ffffffff8168386b>] usleep_range+0x3b/0x40
[<ffffffffa0951ee4>] rtl92c_phy_config_rf_with_headerfile+0x104/0x2f0 [rtl8192ce]
[<ffffffffa0953d30>] rtl92ce_phy_rf6052_config+0x1b0/0x310 [rtl8192ce]
[<ffffffffa095055b>] rtl92ce_hw_init+0xa8b/0x17e0 [rtl8192ce]
[<ffffffffa060f22b>] rtl_ps_enable_nic+0x3b/0xc0 [rtlwifi]
[<ffffffffa0952d39>] rtl92c_phy_set_rf_power_state+0x509/0x760 [rtl8192ce]
[<ffffffffa060f3d4>] rtl_ps_set_rf_state+0xd4/0x200 [rtlwifi]
[<ffffffffa060f538>] _rtl_ps_inactive_ps+0x38/0xb0 [rtlwifi]
[<ffffffffa060f628>] rtl_ips_nic_on+0x78/0xb0 [rtlwifi]
[<ffffffffa060c678>] rtl_op_config+0x278/0x440 [rtlwifi]
[<ffffffffa072c9b6>] ieee80211_hw_config+0x56/0x3a0 [mac80211]
[<ffffffffa076123a>] ieee80211_add_chanctx+0x17a/0x1c0 [mac80211]
[<ffffffffa0761e2d>] ieee80211_new_chanctx+0x2d/0x80 [mac80211]
[<ffffffffa0763f6b>] ieee80211_vif_use_channel+0x1fb/0x250 [mac80211]
[<ffffffffa0776718>] ieee80211_prep_connection+0x188/0x8e0 [mac80211]
[<ffffffffa077cb0a>] ieee80211_mgd_auth+0x22a/0x330 [mac80211]
[<ffffffffa0453e57>] cfg80211_mlme_auth+0x117/0x230 [cfg80211]
[<ffffffffa043a917>] nl80211_authenticate+0x2a7/0x300 [cfg80211]
[<ffffffff815d4c56>] genl_family_rcv_msg+0x1b6/0x380
[<ffffffff815d4e99>] genl_rcv_msg+0x79/0xb0
[<ffffffff815d3ea2>] netlink_rcv_skb+0x92/0xa0
[<ffffffff815d44b4>] genl_rcv+0x24/0x40
[<ffffffff815d3896>] netlink_unicast+0x146/0x1f0
[<ffffffff815d3c63>] netlink_sendmsg+0x323/0x3a0
[<ffffffff81588c80>] sock_sendmsg+0x30/0x40
[<ffffffff81589529>] ___sys_sendmsg+0x279/0x290
[<ffffffff81589d81>] __sys_sendmsg+0x41/0x70
[<ffffffff81003aa9>] do_syscall_64+0x59/0xb0
[<ffffffff81684da5>] entry_SYSCALL64_slow_path+0x25/0x25
So please revert this commit. I agree with the general goal, but this
approach does not work. I had to in order to make my wifi work again.
Then, when looking deeper, those local_irq_enable constructs in the
hw_init handlers are making me nervous. Are you sure that no task
context that did something like spin_lock_irq* enters this code? Please
redesign this. I'm not really into the various code paths to make safe
suggestions, sorry.
Jan
Jan Kiszka <[email protected]> writes:
> On 2016-02-15 23:12, Larry Finger wrote:
>> Routine rtl_addr_delay() uses delay statements in code that can
>> sleep. To improve system responsiveness, the various delay statements
>> are changed.
>>
>> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are
>> rewritten to use the code in rtl_addr_delay() for most of their
>> input values.
>>
>> Suggested-by: Byeoungwook Kim <[email protected]>
>> Signed-off-by: Larry Finger <[email protected]>
[...]
> This breaks spectacularly when turning on a little bit of correctness
> checking:
>
> BUG: scheduling while atomic: wpa_supplicant/1116/0x00000002
This should fix it:
https://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=de26859dcf363d520cc44e59f6dcaf20ebe0aadf
--
Kalle Valo