* Re: [PATCH] rtlwifi: Change long delays to sleeps
[not found] <1455574327-2591-1-git-send-email-Larry.Finger@lwfinger.net>
@ 2016-06-04 16:40 ` Jan Kiszka
2016-06-04 16:52 ` Kalle Valo
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kiszka @ 2016-06-04 16:40 UTC (permalink / raw)
To: Larry Finger, kvalo; +Cc: devel, linux-wireless, Linux Kernel Mailing List
[-- Attachment #1.1: Type: text/plain, Size: 5592 bytes --]
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 <quddnr145@gmail.com>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
>
> Kalle,
>
> This patch will interfere with a set of 3 patches submitted by Byeoungwook Kim
> <quddnr145@gmail.com> 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread