* rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
@ 2020-04-21 21:15 Tobias S. Predel
2020-04-21 22:56 ` Brian Norris
0 siblings, 1 reply; 9+ messages in thread
From: Tobias S. Predel @ 2020-04-21 21:15 UTC (permalink / raw)
To: linux-wireless
Hi,
I'm using linux-next 20200421 on Arch Linux
with rtw88 for
$ lspci -k
Subsystem: Hewlett-Packard Company Realtek RTL8822BE 802.11ac 2 × 2 Wi-Fi + Bluetooth 4.2 Combo Adapter (MU-MIMO supported)
Kernel driver in use: rtw_pci
Kernel modules: rtwpci
and
$ ethtool -i wlp2s0
driver: rtw_pci
version: 5.7.0-rc2-next-20200421-1-next-
firmware-version: N/A
expansion-rom-version:
bus-info: 0000:02:00.0
supports-statistics: yes
supports-test: no
supports-eeprom-access: no
supports-register-dump: no
supports-priv-flags: no
and I'm experiencing this
$ dmesg
[26628.497854] rtw_pci 0000:02:00.0: firmware failed to restore hardware setting
[26663.488394] rtw_pci 0000:02:00.0: firmware failed to restore hardware setting
[26973.646044] rtw_pci 0000:02:00.0: firmware failed to restore hardware setting
[26981.542221] rtw_pci 0000:02:00.0: firmware failed to restore hardware setting
[27047.668256] rtw_pci 0000:02:00.0: firmware failed to restore hardware setting
[27095.665971] rtw_pci 0000:02:00.0: firmware failed to restore hardware setting
[27131.504263] rtw_pci 0000:02:00.0: firmware failed to restore hardware setting
[and a lot more of this messages]
and following non-fatal bug
[28125.482259] BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
[28125.482266] Modules linked in: nfnetlink xt_conntrack xt_tcpudp wireguard curve25519_x86_64 libchacha20poly1305 chacha_x86_64 poly1305_x86_64 libblake2s blake2s_x86_64 ip6_udp_tunnel udp_tunnel libcurve25519_generic libchacha libblake2s_generic iptable_security iptable_raw iptable_mangle iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 libcrc32c dm_crypt dm_mod rfcomm fuse 8021q garp mrp stp llc ccm ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter cmac algif_hash algif_skcipher af_alg snd_hda_codec_hdmi snd_hda_codec_conexant snd_hda_codec_generic ledtrig_audio bnep x86_pkg_temp_thermal btusb intel_powerclamp joydev btrtl btbcm btintel coretemp mousedev kvm_intel bluetooth uas kvm hid_multitouch usb_storage ecdh_generic iTCO_wdt uvcvideo ecc mei_hdcp hid_generic iTCO_vendor_support hp_wmi videobuf2_vmalloc irqbypass intel_rapl_msr usbhid sparse_keymap wmi_bmof snd_soc_skl videobuf2_memops videobuf2_v4l2 videobuf2_common crct10dif_pclmul videodev
[28125.482336] snd_soc_sst_ipc crc32_pclmul snd_soc_sst_dsp ghash_clmulni_intel snd_hda_ext_core snd_soc_acpi_intel_match snd_soc_acpi mc aesni_intel snd_soc_core crypto_simd snd_compress ac97_bus rtwpci cryptd snd_pcm_dmaengine glue_helper nls_iso8859_1 rtw88 snd_hda_intel intel_cstate nls_cp437 snd_intel_dspcfg intel_uncore vfat mac80211 fat i915 snd_hda_codec intel_rapl_perf snd_hda_core snd_hwdep snd_pcm snd_timer pcspkr psmouse snd input_leds i2c_i801 cfg80211 soundcore r8169 realtek i2c_algo_bit rfkill libarc4 libphy drm_kms_helper mei_me cec mei rc_core intel_lpss_pci intel_lpss intel_gtt idma64 syscopyarea sysfillrect processor_thermal_device intel_xhci_usb_role_switch intel_pch_thermal sysimgblt roles intel_rapl_common fb_sys_fops intel_soc_dts_iosf i2c_hid tpm_crb wmi battery hid tpm_tis tpm_tis_core tpm int3400_thermal hp_accel int3403_thermal int340x_thermal_zone acpi_thermal_rel lis3lv02d hp_wireless evdev rng_core ac mac_hid drm crypto_user agpgart ip_tables x_tables ext4
[28125.482418] crc32c_generic crc16 mbcache jbd2 rtsx_pci_sdmmc mmc_core serio_raw atkbd libps2 xhci_pci crc32c_intel xhci_hcd rtsx_pci i8042 serio
[28125.482436] Preemption disabled at:
[28125.482443] [<0000000000000000>] 0x0
[28125.482452] CPU: 5 PID: 33416 Comm: kworker/u16:0 Tainted: G W 5.7.0-rc2-next-20200421-1-next-git #1
[28125.482456] Hardware name: HP HP ProBook 430 G5/8377, BIOS Q85 Ver. 01.09.01 10/15/2019
[28125.482477] Workqueue: phy0 rtw_watch_dog_work [rtw88]
[28125.482481] Call Trace:
[28125.482495] dump_stack+0x66/0x90
[28125.482505] __schedule_bug.cold+0x8e/0x9b
[28125.482512] __schedule+0x686/0x7b0
[28125.482520] ? _raw_spin_unlock_irqrestore+0x20/0x40
[28125.482525] schedule+0x46/0xf0
[28125.482531] schedule_hrtimeout_range_clock+0xa5/0x120
[28125.482540] ? hrtimer_init_sleeper+0xa0/0xa0
[28125.482546] usleep_range+0x67/0x90
[28125.482568] rtw_fw_send_h2c_command+0xe0/0x1a0 [rtw88]
[28125.482590] rtw_fw_set_pwr_mode+0x95/0xb0 [rtw88]
[28125.482610] rtw_enter_lps+0xa1/0x100 [rtw88]
[28125.482625] rtw_watch_dog_work+0x21c/0x230 [rtw88]
[28125.482635] process_one_work+0x1da/0x3d0
[28125.482643] worker_thread+0x4a/0x3d0
[28125.482651] kthread+0x122/0x160
[28125.482658] ? process_one_work+0x3d0/0x3d0
[28125.482663] ? kthread_park+0x90/0x90
[28125.482670] ret_from_fork+0x1f/0x40
[28169.561821] rtw_pci 0000:02:00.0: firmware failed to restore hardware setting
[28231.427898] rtw_pci 0000:02:00.0: firmware failed to restore hardware setting
[28299.480530] rtw_pci 0000:02:00.0: firmware failed to restore hardware setting
Maybe that's interesting for you.
Kind regards,
Tobias Predel
--
Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail ausdrucken.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
2020-04-21 21:15 rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002 Tobias S. Predel
@ 2020-04-21 22:56 ` Brian Norris
2020-04-22 2:21 ` Tony Chuang
0 siblings, 1 reply; 9+ messages in thread
From: Brian Norris @ 2020-04-21 22:56 UTC (permalink / raw)
To: Tobias S. Predel; +Cc: linux-wireless, Kai-Heng Feng, Tony Chuang
I'm not sure about the first half your problem, but for the
scheduling-while-atomic:
On Tue, Apr 21, 2020 at 2:16 PM Tobias S. Predel
<tobias.predel@gmail.com> wrote:
> [28125.482259] BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
...
> [28125.482436] Preemption disabled at:
> [28125.482443] [<0000000000000000>] 0x0
^^ This line is a bit weird -- shouldn't this have a real PC?
> [28125.482452] CPU: 5 PID: 33416 Comm: kworker/u16:0 Tainted: G W 5.7.0-rc2-next-20200421-1-next-git #1
> [28125.482456] Hardware name: HP HP ProBook 430 G5/8377, BIOS Q85 Ver. 01.09.01 10/15/2019
> [28125.482477] Workqueue: phy0 rtw_watch_dog_work [rtw88]
> [28125.482481] Call Trace:
> [28125.482495] dump_stack+0x66/0x90
> [28125.482505] __schedule_bug.cold+0x8e/0x9b
> [28125.482512] __schedule+0x686/0x7b0
> [28125.482520] ? _raw_spin_unlock_irqrestore+0x20/0x40
> [28125.482525] schedule+0x46/0xf0
> [28125.482531] schedule_hrtimeout_range_clock+0xa5/0x120
> [28125.482540] ? hrtimer_init_sleeper+0xa0/0xa0
> [28125.482546] usleep_range+0x67/0x90
> [28125.482568] rtw_fw_send_h2c_command+0xe0/0x1a0 [rtw88]
> [28125.482590] rtw_fw_set_pwr_mode+0x95/0xb0 [rtw88]
> [28125.482610] rtw_enter_lps+0xa1/0x100 [rtw88]
> [28125.482625] rtw_watch_dog_work+0x21c/0x230 [rtw88]
> [28125.482635] process_one_work+0x1da/0x3d0
> [28125.482643] worker_thread+0x4a/0x3d0
> [28125.482651] kthread+0x122/0x160
> [28125.482658] ? process_one_work+0x3d0/0x3d0
> [28125.482663] ? kthread_park+0x90/0x90
> [28125.482670] ret_from_fork+0x1f/0x40
This looks like it might be a regression here:
commit 6343a6d4b2130be9323f347d60af8a7ba8f7242c
Author: Kai-Heng Feng <kai.heng.feng@canonical.com>
Date: Tue Apr 7 15:33:31 2020 +0800
rtw88: Add delay on polling h2c command status bit
That poll macros is using usleep, which obviously can sleep. We need
to be using a udelay-variant instead.
Brian
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
2020-04-21 22:56 ` Brian Norris
@ 2020-04-22 2:21 ` Tony Chuang
2020-04-22 16:48 ` Kai-Heng Feng
0 siblings, 1 reply; 9+ messages in thread
From: Tony Chuang @ 2020-04-22 2:21 UTC (permalink / raw)
To: Brian Norris, Tobias S. Predel; +Cc: linux-wireless, Kai-Heng Feng
Brian Norris <briannorris@chromium.org> :
>
> I'm not sure about the first half your problem, but for the
> scheduling-while-atomic:
>
> On Tue, Apr 21, 2020 at 2:16 PM Tobias S. Predel
> <tobias.predel@gmail.com> wrote:
> > [28125.482259] BUG: scheduling while atomic:
> kworker/u16:0/33416/0x00000002
> ...
> > [28125.482436] Preemption disabled at:
> > [28125.482443] [<0000000000000000>] 0x0
>
> ^^ This line is a bit weird -- shouldn't this have a real PC?
>
> > [28125.482452] CPU: 5 PID: 33416 Comm: kworker/u16:0 Tainted: G
> W 5.7.0-rc2-next-20200421-1-next-git #1
> > [28125.482456] Hardware name: HP HP ProBook 430 G5/8377, BIOS Q85
> Ver. 01.09.01 10/15/2019
> > [28125.482477] Workqueue: phy0 rtw_watch_dog_work [rtw88]
> > [28125.482481] Call Trace:
> > [28125.482495] dump_stack+0x66/0x90
> > [28125.482505] __schedule_bug.cold+0x8e/0x9b
> > [28125.482512] __schedule+0x686/0x7b0
> > [28125.482520] ? _raw_spin_unlock_irqrestore+0x20/0x40
> > [28125.482525] schedule+0x46/0xf0
> > [28125.482531] schedule_hrtimeout_range_clock+0xa5/0x120
> > [28125.482540] ? hrtimer_init_sleeper+0xa0/0xa0
> > [28125.482546] usleep_range+0x67/0x90
> > [28125.482568] rtw_fw_send_h2c_command+0xe0/0x1a0 [rtw88]
> > [28125.482590] rtw_fw_set_pwr_mode+0x95/0xb0 [rtw88]
> > [28125.482610] rtw_enter_lps+0xa1/0x100 [rtw88]
> > [28125.482625] rtw_watch_dog_work+0x21c/0x230 [rtw88]
> > [28125.482635] process_one_work+0x1da/0x3d0
> > [28125.482643] worker_thread+0x4a/0x3d0
> > [28125.482651] kthread+0x122/0x160
> > [28125.482658] ? process_one_work+0x3d0/0x3d0
> > [28125.482663] ? kthread_park+0x90/0x90
> > [28125.482670] ret_from_fork+0x1f/0x40
>
> This looks like it might be a regression here:
>
> commit 6343a6d4b2130be9323f347d60af8a7ba8f7242c
> Author: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Date: Tue Apr 7 15:33:31 2020 +0800
>
> rtw88: Add delay on polling h2c command status bit
>
> That poll macros is using usleep, which obviously can sleep. We need
> to be using a udelay-variant instead.
>
Maybe we need an atomic version of read_poll_timeout() ?
I am not sure if this is required, but seems like it is useful for me.
Noticed much of them have its atomic version, but not for this new added one.
Yen-Hsuan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
2020-04-22 2:21 ` Tony Chuang
@ 2020-04-22 16:48 ` Kai-Heng Feng
2020-04-22 19:25 ` Tobias S. Predel
0 siblings, 1 reply; 9+ messages in thread
From: Kai-Heng Feng @ 2020-04-22 16:48 UTC (permalink / raw)
To: Tobias S. Predel; +Cc: Tony Chuang, Brian Norris, linux-wireless
Hi Tobias,
> On Apr 22, 2020, at 10:21, Tony Chuang <yhchuang@realtek.com> wrote:
>
> Brian Norris <briannorris@chromium.org> :
>>
>> I'm not sure about the first half your problem, but for the
>> scheduling-while-atomic:
>>
>> On Tue, Apr 21, 2020 at 2:16 PM Tobias S. Predel
>> <tobias.predel@gmail.com> wrote:
>>> [28125.482259] BUG: scheduling while atomic:
>> kworker/u16:0/33416/0x00000002
>> ...
>>> [28125.482436] Preemption disabled at:
>>> [28125.482443] [<0000000000000000>] 0x0
>>
>> ^^ This line is a bit weird -- shouldn't this have a real PC?
>>
>>> [28125.482452] CPU: 5 PID: 33416 Comm: kworker/u16:0 Tainted: G
>> W 5.7.0-rc2-next-20200421-1-next-git #1
>>> [28125.482456] Hardware name: HP HP ProBook 430 G5/8377, BIOS Q85
>> Ver. 01.09.01 10/15/2019
>>> [28125.482477] Workqueue: phy0 rtw_watch_dog_work [rtw88]
>>> [28125.482481] Call Trace:
>>> [28125.482495] dump_stack+0x66/0x90
>>> [28125.482505] __schedule_bug.cold+0x8e/0x9b
>>> [28125.482512] __schedule+0x686/0x7b0
>>> [28125.482520] ? _raw_spin_unlock_irqrestore+0x20/0x40
>>> [28125.482525] schedule+0x46/0xf0
>>> [28125.482531] schedule_hrtimeout_range_clock+0xa5/0x120
>>> [28125.482540] ? hrtimer_init_sleeper+0xa0/0xa0
>>> [28125.482546] usleep_range+0x67/0x90
>>> [28125.482568] rtw_fw_send_h2c_command+0xe0/0x1a0 [rtw88]
>>> [28125.482590] rtw_fw_set_pwr_mode+0x95/0xb0 [rtw88]
>>> [28125.482610] rtw_enter_lps+0xa1/0x100 [rtw88]
>>> [28125.482625] rtw_watch_dog_work+0x21c/0x230 [rtw88]
>>> [28125.482635] process_one_work+0x1da/0x3d0
>>> [28125.482643] worker_thread+0x4a/0x3d0
>>> [28125.482651] kthread+0x122/0x160
>>> [28125.482658] ? process_one_work+0x3d0/0x3d0
>>> [28125.482663] ? kthread_park+0x90/0x90
>>> [28125.482670] ret_from_fork+0x1f/0x40
>>
>> This looks like it might be a regression here:
>>
>> commit 6343a6d4b2130be9323f347d60af8a7ba8f7242c
>> Author: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> Date: Tue Apr 7 15:33:31 2020 +0800
>>
>> rtw88: Add delay on polling h2c command status bit
>>
>> That poll macros is using usleep, which obviously can sleep. We need
>> to be using a udelay-variant instead.
>>
>
> Maybe we need an atomic version of read_poll_timeout() ?
> I am not sure if this is required, but seems like it is useful for me.
> Noticed much of them have its atomic version, but not for this new added one.
Tony and Brian are right.
Tobias, can you please test the following patch:
diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
index 245da96dfddc..e44767ec0532 100644
--- a/drivers/net/wireless/realtek/rtw88/fw.c
+++ b/drivers/net/wireless/realtek/rtw88/fw.c
@@ -228,7 +228,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
goto out;
}
- ret = read_poll_timeout(rtw_read8, box_state,
+ ret = read_poll_timeout_atomic(rtw_read8, box_state,
!((box_state >> box) & 0x1), 100, 3000, false,
rtwdev, REG_HMETFR);
diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index cb20c733b15a..bc89ac625f26 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -57,6 +57,48 @@
(cond) ? 0 : -ETIMEDOUT; \
})
+/**
+ * read_poll_timeout_atomic - Periodically poll an address until a condition is
+ * met or a timeout occurs
+ * @op: accessor function (takes @addr as its only argument)
+ * @addr: Address to poll
+ * @val: Variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @delay_us: Time to udelay between reads in us (0 tight-loops). Should
+ * be less than ~10us since udelay is used (see
+ * Documentation/timers/timers-howto.rst).
+ * @timeout_us: Timeout in us, 0 means never timeout
+ * @delay_before_read: if it is true, delay @delay_us before read.
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
+ * case, the last read value at @args is stored in @val.
+ *
+ * When available, you'll probably want to use one of the specialized
+ * macros defined below rather than this macro directly.
+ */
+#define read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \
+ delay_before_read, args...) \
+({ \
+ u64 __timeout_us = (timeout_us); \
+ unsigned long __delay_us = (delay_us); \
+ ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
+ if (delay_before_read && __delay_us) \
+ udelay(__delay_us); \
+ for (;;) { \
+ (val) = op(args); \
+ if (cond) \
+ break; \
+ if (__timeout_us && \
+ ktime_compare(ktime_get(), __timeout) > 0) { \
+ (val) = op(args); \
+ break; \
+ } \
+ if (__delay_us) \
+ } \
+ (cond) ? 0 : -ETIMEDOUT; \
+})
+
/**
* readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
* @op: accessor function (takes @addr as its only argument)
@@ -96,25 +138,7 @@
* macros defined below rather than this macro directly.
*/
#define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
-({ \
- u64 __timeout_us = (timeout_us); \
- unsigned long __delay_us = (delay_us); \
- ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
- for (;;) { \
- (val) = op(addr); \
- if (cond) \
- break; \
- if (__timeout_us && \
- ktime_compare(ktime_get(), __timeout) > 0) { \
- (val) = op(addr); \
- break; \
- } \
- if (__delay_us) \
- udelay(__delay_us); \
- } \
- (cond) ? 0 : -ETIMEDOUT; \
-})
-
+ read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
#define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
>
> Yen-Hsuan
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
2020-04-22 16:48 ` Kai-Heng Feng
@ 2020-04-22 19:25 ` Tobias S. Predel
2020-04-22 20:57 ` Tobias S. Predel
0 siblings, 1 reply; 9+ messages in thread
From: Tobias S. Predel @ 2020-04-22 19:25 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: Tony Chuang, Brian Norris, linux-wireless
Hi Kai-Heng,
On Thu, Apr 23, 2020 at 12:48:55AM +0800, Kai-Heng Feng wrote:
> Hi Tobias,
>
> > On Apr 22, 2020, at 10:21, Tony Chuang <yhchuang@realtek.com> wrote:
> >
> > Brian Norris <briannorris@chromium.org> :
> >>
> >> I'm not sure about the first half your problem, but for the
> >> scheduling-while-atomic:
> >>
> >> On Tue, Apr 21, 2020 at 2:16 PM Tobias S. Predel
> >> <tobias.predel@gmail.com> wrote:
> >>> [28125.482259] BUG: scheduling while atomic:
> >> kworker/u16:0/33416/0x00000002
> >> ...
> >>> [28125.482436] Preemption disabled at:
> >>> [28125.482443] [<0000000000000000>] 0x0
> >>
> >> ^^ This line is a bit weird -- shouldn't this have a real PC?
> >>
> >>> [28125.482452] CPU: 5 PID: 33416 Comm: kworker/u16:0 Tainted: G
> >> W 5.7.0-rc2-next-20200421-1-next-git #1
> >>> [28125.482456] Hardware name: HP HP ProBook 430 G5/8377, BIOS Q85
> >> Ver. 01.09.01 10/15/2019
> >>> [28125.482477] Workqueue: phy0 rtw_watch_dog_work [rtw88]
> >>> [28125.482481] Call Trace:
> >>> [28125.482495] dump_stack+0x66/0x90
> >>> [28125.482505] __schedule_bug.cold+0x8e/0x9b
> >>> [28125.482512] __schedule+0x686/0x7b0
> >>> [28125.482520] ? _raw_spin_unlock_irqrestore+0x20/0x40
> >>> [28125.482525] schedule+0x46/0xf0
> >>> [28125.482531] schedule_hrtimeout_range_clock+0xa5/0x120
> >>> [28125.482540] ? hrtimer_init_sleeper+0xa0/0xa0
> >>> [28125.482546] usleep_range+0x67/0x90
> >>> [28125.482568] rtw_fw_send_h2c_command+0xe0/0x1a0 [rtw88]
> >>> [28125.482590] rtw_fw_set_pwr_mode+0x95/0xb0 [rtw88]
> >>> [28125.482610] rtw_enter_lps+0xa1/0x100 [rtw88]
> >>> [28125.482625] rtw_watch_dog_work+0x21c/0x230 [rtw88]
> >>> [28125.482635] process_one_work+0x1da/0x3d0
> >>> [28125.482643] worker_thread+0x4a/0x3d0
> >>> [28125.482651] kthread+0x122/0x160
> >>> [28125.482658] ? process_one_work+0x3d0/0x3d0
> >>> [28125.482663] ? kthread_park+0x90/0x90
> >>> [28125.482670] ret_from_fork+0x1f/0x40
> >>
> >> This looks like it might be a regression here:
> >>
> >> commit 6343a6d4b2130be9323f347d60af8a7ba8f7242c
> >> Author: Kai-Heng Feng <kai.heng.feng@canonical.com>
> >> Date: Tue Apr 7 15:33:31 2020 +0800
> >>
> >> rtw88: Add delay on polling h2c command status bit
> >>
> >> That poll macros is using usleep, which obviously can sleep. We need
> >> to be using a udelay-variant instead.
> >>
> >
> > Maybe we need an atomic version of read_poll_timeout() ?
> > I am not sure if this is required, but seems like it is useful for me.
> > Noticed much of them have its atomic version, but not for this new added one.
>
> Tony and Brian are right.
>
> Tobias, can you please test the following patch:
>
> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
> index 245da96dfddc..e44767ec0532 100644
> --- a/drivers/net/wireless/realtek/rtw88/fw.c
> +++ b/drivers/net/wireless/realtek/rtw88/fw.c
> @@ -228,7 +228,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
> goto out;
> }
>
> - ret = read_poll_timeout(rtw_read8, box_state,
> + ret = read_poll_timeout_atomic(rtw_read8, box_state,
> !((box_state >> box) & 0x1), 100, 3000, false,
> rtwdev, REG_HMETFR);
>
> diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> index cb20c733b15a..bc89ac625f26 100644
> --- a/include/linux/iopoll.h
> +++ b/include/linux/iopoll.h
> @@ -57,6 +57,48 @@
> (cond) ? 0 : -ETIMEDOUT; \
> })
>
> +/**
> + * read_poll_timeout_atomic - Periodically poll an address until a condition is
> + * met or a timeout occurs
> + * @op: accessor function (takes @addr as its only argument)
> + * @addr: Address to poll
> + * @val: Variable to read the value into
> + * @cond: Break condition (usually involving @val)
> + * @delay_us: Time to udelay between reads in us (0 tight-loops). Should
> + * be less than ~10us since udelay is used (see
> + * Documentation/timers/timers-howto.rst).
> + * @timeout_us: Timeout in us, 0 means never timeout
> + * @delay_before_read: if it is true, delay @delay_us before read.
> + *
> + * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
> + * case, the last read value at @args is stored in @val.
> + *
> + * When available, you'll probably want to use one of the specialized
> + * macros defined below rather than this macro directly.
> + */
> +#define read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \
> + delay_before_read, args...) \
> +({ \
> + u64 __timeout_us = (timeout_us); \
> + unsigned long __delay_us = (delay_us); \
> + ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> + if (delay_before_read && __delay_us) \
> + udelay(__delay_us); \
> + for (;;) { \
> + (val) = op(args); \
> + if (cond) \
> + break; \
> + if (__timeout_us && \
> + ktime_compare(ktime_get(), __timeout) > 0) { \
> + (val) = op(args); \
> + break; \
> + } \
> + if (__delay_us) \
> + } \
> + (cond) ? 0 : -ETIMEDOUT; \
> +})
> +
> /**
> * readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
> * @op: accessor function (takes @addr as its only argument)
> @@ -96,25 +138,7 @@
> * macros defined below rather than this macro directly.
> */
> #define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
> -({ \
> - u64 __timeout_us = (timeout_us); \
> - unsigned long __delay_us = (delay_us); \
> - ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> - for (;;) { \
> - (val) = op(addr); \
> - if (cond) \
> - break; \
> - if (__timeout_us && \
> - ktime_compare(ktime_get(), __timeout) > 0) { \
> - (val) = op(addr); \
> - break; \
> - } \
> - if (__delay_us) \
> - udelay(__delay_us); \
> - } \
> - (cond) ? 0 : -ETIMEDOUT; \
> -})
> -
> + read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
>
> #define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
>
thanks for your reply!
I tried to patch it against
$ git describe --long --tags
next-20200422-0-ga5840f9618a9
with
$ git am < ~/atomic-patch.mbox
but I get
Applying: rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
error: corrupt patch at line 66
Patch failed at 0001 rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
and
in am mode, then
$ git am --show-current-patch=diff
diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
index 245da96dfddc..e44767ec0532 100644
--- a/drivers/net/wireless/realtek/rtw88/fw.c
+++ b/drivers/net/wireless/realtek/rtw88/fw.c
@@ -228,7 +228,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
goto out;
}
- ret = read_poll_timeout(rtw_read8, box_state,
+ ret = read_poll_timeout_atomic(rtw_read8, box_state,
!((box_state >> box) & 0x1), 100, 3000, false,
rtwdev, REG_HMETFR);
diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index cb20c733b15a..bc89ac625f26 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -57,6 +57,48 @@
(cond) ? 0 : -ETIMEDOUT; \
})
+/**
+ * read_poll_timeout_atomic - Periodically poll an address until a condition is
+ * met or a timeout occurs
+ * @op: accessor function (takes @addr as its only argument)
+ * @addr: Address to poll
+ * @val: Variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @delay_us: Time to udelay between reads in us (0 tight-loops). Should
+ * be less than ~10us since udelay is used (see
+ * Documentation/timers/timers-howto.rst).
+ * @timeout_us: Timeout in us, 0 means never timeout
+ * @delay_before_read: if it is true, delay @delay_us before read.
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
+ * case, the last read value at @args is stored in @val.
+ *
+ * When available, you'll probably want to use one of the specialized
+ * macros defined below rather than this macro directly.
+ */
+#define read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \
+ delay_before_read, args...) \
+({ \
+ u64 __timeout_us = (timeout_us); \
+ unsigned long __delay_us = (delay_us); \
+ ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
+ if (delay_before_read && __delay_us) \
+ udelay(__delay_us); \
+ for (;;) { \
+ (val) = op(args); \
+ if (cond) \
+ break; \
+ if (__timeout_us && \
+ ktime_compare(ktime_get(), __timeout) > 0) { \
+ (val) = op(args); \
+ break; \
+ } \
+ if (__delay_us) \
+ } \
+ (cond) ? 0 : -ETIMEDOUT; \
+})
+
/**
* readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
* @op: accessor function (takes @addr as its only argument)
@@ -96,25 +138,7 @@
* macros defined below rather than this macro directly.
*/
#define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
-({ \
- u64 __timeout_us = (timeout_us); \
- unsigned long __delay_us = (delay_us); \
- ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
- for (;;) { \
- (val) = op(addr); \
- if (cond) \
- break; \
- if (__timeout_us && \
- ktime_compare(ktime_get(), __timeout) > 0) { \
- (val) = op(addr); \
- break; \
- } \
- if (__delay_us) \
- udelay(__delay_us); \
- } \
- (cond) ? 0 : -ETIMEDOUT; \
-})
-
+ read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
#define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
>
> Yen-Hsuan
I'll try to patch manually and send back when there are news.
Kind regards,
Tobias
--
Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail ausdrucken.
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
2020-04-22 19:25 ` Tobias S. Predel
@ 2020-04-22 20:57 ` Tobias S. Predel
2020-04-22 22:55 ` Tobias S. Predel
0 siblings, 1 reply; 9+ messages in thread
From: Tobias S. Predel @ 2020-04-22 20:57 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: Tony Chuang, Brian Norris, linux-wireless
Hi Kai-Heng,
On Wed, Apr 22, 2020 at 09:25:24PM +0200, Tobias S. Predel wrote:
> Hi Kai-Heng,
>
> On Thu, Apr 23, 2020 at 12:48:55AM +0800, Kai-Heng Feng wrote:
> > Hi Tobias,
> >
> > > On Apr 22, 2020, at 10:21, Tony Chuang <yhchuang@realtek.com> wrote:
> > >
> > > Brian Norris <briannorris@chromium.org> :
> > >>
> > >> I'm not sure about the first half your problem, but for the
> > >> scheduling-while-atomic:
> > >>
> > >> On Tue, Apr 21, 2020 at 2:16 PM Tobias S. Predel
> > >> <tobias.predel@gmail.com> wrote:
> > >>> [28125.482259] BUG: scheduling while atomic:
> > >> kworker/u16:0/33416/0x00000002
> > >> ...
> > >>> [28125.482436] Preemption disabled at:
> > >>> [28125.482443] [<0000000000000000>] 0x0
> > >>
> > >> ^^ This line is a bit weird -- shouldn't this have a real PC?
> > >>
> > >>> [28125.482452] CPU: 5 PID: 33416 Comm: kworker/u16:0 Tainted: G
> > >> W 5.7.0-rc2-next-20200421-1-next-git #1
> > >>> [28125.482456] Hardware name: HP HP ProBook 430 G5/8377, BIOS Q85
> > >> Ver. 01.09.01 10/15/2019
> > >>> [28125.482477] Workqueue: phy0 rtw_watch_dog_work [rtw88]
> > >>> [28125.482481] Call Trace:
> > >>> [28125.482495] dump_stack+0x66/0x90
> > >>> [28125.482505] __schedule_bug.cold+0x8e/0x9b
> > >>> [28125.482512] __schedule+0x686/0x7b0
> > >>> [28125.482520] ? _raw_spin_unlock_irqrestore+0x20/0x40
> > >>> [28125.482525] schedule+0x46/0xf0
> > >>> [28125.482531] schedule_hrtimeout_range_clock+0xa5/0x120
> > >>> [28125.482540] ? hrtimer_init_sleeper+0xa0/0xa0
> > >>> [28125.482546] usleep_range+0x67/0x90
> > >>> [28125.482568] rtw_fw_send_h2c_command+0xe0/0x1a0 [rtw88]
> > >>> [28125.482590] rtw_fw_set_pwr_mode+0x95/0xb0 [rtw88]
> > >>> [28125.482610] rtw_enter_lps+0xa1/0x100 [rtw88]
> > >>> [28125.482625] rtw_watch_dog_work+0x21c/0x230 [rtw88]
> > >>> [28125.482635] process_one_work+0x1da/0x3d0
> > >>> [28125.482643] worker_thread+0x4a/0x3d0
> > >>> [28125.482651] kthread+0x122/0x160
> > >>> [28125.482658] ? process_one_work+0x3d0/0x3d0
> > >>> [28125.482663] ? kthread_park+0x90/0x90
> > >>> [28125.482670] ret_from_fork+0x1f/0x40
> > >>
> > >> This looks like it might be a regression here:
> > >>
> > >> commit 6343a6d4b2130be9323f347d60af8a7ba8f7242c
> > >> Author: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > >> Date: Tue Apr 7 15:33:31 2020 +0800
> > >>
> > >> rtw88: Add delay on polling h2c command status bit
> > >>
> > >> That poll macros is using usleep, which obviously can sleep. We need
> > >> to be using a udelay-variant instead.
> > >>
> > >
> > > Maybe we need an atomic version of read_poll_timeout() ?
> > > I am not sure if this is required, but seems like it is useful for me.
> > > Noticed much of them have its atomic version, but not for this new added one.
> >
> > Tony and Brian are right.
> >
> > Tobias, can you please test the following patch:
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
> > index 245da96dfddc..e44767ec0532 100644
> > --- a/drivers/net/wireless/realtek/rtw88/fw.c
> > +++ b/drivers/net/wireless/realtek/rtw88/fw.c
> > @@ -228,7 +228,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
> > goto out;
> > }
> >
> > - ret = read_poll_timeout(rtw_read8, box_state,
> > + ret = read_poll_timeout_atomic(rtw_read8, box_state,
> > !((box_state >> box) & 0x1), 100, 3000, false,
> > rtwdev, REG_HMETFR);
> >
> > diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> > index cb20c733b15a..bc89ac625f26 100644
> > --- a/include/linux/iopoll.h
> > +++ b/include/linux/iopoll.h
> > @@ -57,6 +57,48 @@
> > (cond) ? 0 : -ETIMEDOUT; \
> > })
> >
> > +/**
> > + * read_poll_timeout_atomic - Periodically poll an address until a condition is
> > + * met or a timeout occurs
> > + * @op: accessor function (takes @addr as its only argument)
> > + * @addr: Address to poll
> > + * @val: Variable to read the value into
> > + * @cond: Break condition (usually involving @val)
> > + * @delay_us: Time to udelay between reads in us (0 tight-loops). Should
> > + * be less than ~10us since udelay is used (see
> > + * Documentation/timers/timers-howto.rst).
> > + * @timeout_us: Timeout in us, 0 means never timeout
> > + * @delay_before_read: if it is true, delay @delay_us before read.
> > + *
> > + * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
> > + * case, the last read value at @args is stored in @val.
> > + *
> > + * When available, you'll probably want to use one of the specialized
> > + * macros defined below rather than this macro directly.
> > + */
> > +#define read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \
> > + delay_before_read, args...) \
> > +({ \
> > + u64 __timeout_us = (timeout_us); \
> > + unsigned long __delay_us = (delay_us); \
> > + ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> > + if (delay_before_read && __delay_us) \
> > + udelay(__delay_us); \
> > + for (;;) { \
> > + (val) = op(args); \
> > + if (cond) \
> > + break; \
> > + if (__timeout_us && \
> > + ktime_compare(ktime_get(), __timeout) > 0) { \
> > + (val) = op(args); \
> > + break; \
> > + } \
> > + if (__delay_us) \
Isn't there something missing here after __delay_us?
I got compiler error, misses ;.
> > + } \
> > + (cond) ? 0 : -ETIMEDOUT; \
> > +})
> > +
> > /**
> > * readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
> > * @op: accessor function (takes @addr as its only argument)
> > @@ -96,25 +138,7 @@
> > * macros defined below rather than this macro directly.
> > */
> > #define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
> > -({ \
> > - u64 __timeout_us = (timeout_us); \
> > - unsigned long __delay_us = (delay_us); \
> > - ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> > - for (;;) { \
> > - (val) = op(addr); \
> > - if (cond) \
> > - break; \
> > - if (__timeout_us && \
> > - ktime_compare(ktime_get(), __timeout) > 0) { \
> > - (val) = op(addr); \
> > - break; \
> > - } \
> > - if (__delay_us) \
> > - udelay(__delay_us); \
> > - } \
> > - (cond) ? 0 : -ETIMEDOUT; \
> > -})
> > -
> > + read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
> >
> > #define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> > readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
> >
>
--
Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail ausdrucken.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
2020-04-22 20:57 ` Tobias S. Predel
@ 2020-04-22 22:55 ` Tobias S. Predel
2020-04-23 5:43 ` Kai-Heng Feng
0 siblings, 1 reply; 9+ messages in thread
From: Tobias S. Predel @ 2020-04-22 22:55 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: Tony Chuang, Brian Norris, linux-wireless
[-- Attachment #1: Type: text/plain, Size: 11146 bytes --]
Hello,
I guessed from the deleted lines that
udelay(__delay_us) was missing.
So I compiled successfully and will observe how it will work out.
But for the first start on the patched kernel it's working and I will report.
Thanks for your patch! Patch against commit a5840f9618a90ecbe1617f7632482563c0ee307e
is attached.
Kind regards,
Tobias
diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
index 245da96dfddc..e44767ec0532 100644
--- a/drivers/net/wireless/realtek/rtw88/fw.c
+++ b/drivers/net/wireless/realtek/rtw88/fw.c
@@ -228,7 +228,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
goto out;
}
- ret = read_poll_timeout(rtw_read8, box_state,
+ ret = read_poll_timeout_atomic(rtw_read8, box_state,
!((box_state >> box) & 0x1), 100, 3000, false,
rtwdev, REG_HMETFR);
diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index cb20c733b15a..e4462712b541 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -57,6 +57,49 @@
(cond) ? 0 : -ETIMEDOUT; \
})
+/**
+ * read_poll_timeout_atomic - Periodically poll an address until a condition is
+ * met or a timeout occurs
+ * @op: accessor function (takes @addr as its only argument)
+ * @addr: Address to poll
+ * @val: Variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @delay_us: Time to udelay between reads in us (0 tight-loops). Should
+ * be less than ~10us since udelay is used (see
+ * Documentation/timers/timers-howto.rst).
+ * @timeout_us: Timeout in us, 0 means never timeout
+ * @delay_before_read: if it is true, delay @delay_us before read.
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
+ * case, the last read value at @args is stored in @val.
+ *
+ * When available, you'll probably want to use one of the specialized
+ * macros defined below rather than this macro directly.
+ */
+#define read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \
+ delay_before_read, args...) \
+({ \
+ u64 __timeout_us = (timeout_us); \
+ unsigned long __delay_us = (delay_us); \
+ ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
+ if (delay_before_read && __delay_us) \
+ udelay(__delay_us); \
+ for (;;) { \
+ (val) = op(args); \
+ if (cond) \
+ break; \
+ if (__timeout_us && \
+ ktime_compare(ktime_get(), __timeout) > 0) { \
+ (val) = op(args); \
+ break; \
+ } \
+ if (__delay_us) \
+ udelay(__delay_us); \
+ } \
+ (cond) ? 0 : -ETIMEDOUT; \
+})
+
+
/**
* readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
* @op: accessor function (takes @addr as its only argument)
@@ -96,25 +139,7 @@
* macros defined below rather than this macro directly.
*/
#define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
-({ \
- u64 __timeout_us = (timeout_us); \
- unsigned long __delay_us = (delay_us); \
- ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
- for (;;) { \
- (val) = op(addr); \
- if (cond) \
- break; \
- if (__timeout_us && \
- ktime_compare(ktime_get(), __timeout) > 0) { \
- (val) = op(addr); \
- break; \
- } \
- if (__delay_us) \
- udelay(__delay_us); \
- } \
- (cond) ? 0 : -ETIMEDOUT; \
-})
-
+ read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
#define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
On Wed, Apr 22, 2020 at 10:57:31PM +0200, Tobias S. Predel wrote:
> Hi Kai-Heng,
>
> On Wed, Apr 22, 2020 at 09:25:24PM +0200, Tobias S. Predel wrote:
> > Hi Kai-Heng,
> >
> > On Thu, Apr 23, 2020 at 12:48:55AM +0800, Kai-Heng Feng wrote:
> > > Hi Tobias,
> > >
> > > > On Apr 22, 2020, at 10:21, Tony Chuang <yhchuang@realtek.com> wrote:
> > > >
> > > > Brian Norris <briannorris@chromium.org> :
> > > >>
> > > >> I'm not sure about the first half your problem, but for the
> > > >> scheduling-while-atomic:
> > > >>
> > > >> On Tue, Apr 21, 2020 at 2:16 PM Tobias S. Predel
> > > >> <tobias.predel@gmail.com> wrote:
> > > >>> [28125.482259] BUG: scheduling while atomic:
> > > >> kworker/u16:0/33416/0x00000002
> > > >> ...
> > > >>> [28125.482436] Preemption disabled at:
> > > >>> [28125.482443] [<0000000000000000>] 0x0
> > > >>
> > > >> ^^ This line is a bit weird -- shouldn't this have a real PC?
> > > >>
> > > >>> [28125.482452] CPU: 5 PID: 33416 Comm: kworker/u16:0 Tainted: G
> > > >> W 5.7.0-rc2-next-20200421-1-next-git #1
> > > >>> [28125.482456] Hardware name: HP HP ProBook 430 G5/8377, BIOS Q85
> > > >> Ver. 01.09.01 10/15/2019
> > > >>> [28125.482477] Workqueue: phy0 rtw_watch_dog_work [rtw88]
> > > >>> [28125.482481] Call Trace:
> > > >>> [28125.482495] dump_stack+0x66/0x90
> > > >>> [28125.482505] __schedule_bug.cold+0x8e/0x9b
> > > >>> [28125.482512] __schedule+0x686/0x7b0
> > > >>> [28125.482520] ? _raw_spin_unlock_irqrestore+0x20/0x40
> > > >>> [28125.482525] schedule+0x46/0xf0
> > > >>> [28125.482531] schedule_hrtimeout_range_clock+0xa5/0x120
> > > >>> [28125.482540] ? hrtimer_init_sleeper+0xa0/0xa0
> > > >>> [28125.482546] usleep_range+0x67/0x90
> > > >>> [28125.482568] rtw_fw_send_h2c_command+0xe0/0x1a0 [rtw88]
> > > >>> [28125.482590] rtw_fw_set_pwr_mode+0x95/0xb0 [rtw88]
> > > >>> [28125.482610] rtw_enter_lps+0xa1/0x100 [rtw88]
> > > >>> [28125.482625] rtw_watch_dog_work+0x21c/0x230 [rtw88]
> > > >>> [28125.482635] process_one_work+0x1da/0x3d0
> > > >>> [28125.482643] worker_thread+0x4a/0x3d0
> > > >>> [28125.482651] kthread+0x122/0x160
> > > >>> [28125.482658] ? process_one_work+0x3d0/0x3d0
> > > >>> [28125.482663] ? kthread_park+0x90/0x90
> > > >>> [28125.482670] ret_from_fork+0x1f/0x40
> > > >>
> > > >> This looks like it might be a regression here:
> > > >>
> > > >> commit 6343a6d4b2130be9323f347d60af8a7ba8f7242c
> > > >> Author: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > >> Date: Tue Apr 7 15:33:31 2020 +0800
> > > >>
> > > >> rtw88: Add delay on polling h2c command status bit
> > > >>
> > > >> That poll macros is using usleep, which obviously can sleep. We need
> > > >> to be using a udelay-variant instead.
> > > >>
> > > >
> > > > Maybe we need an atomic version of read_poll_timeout() ?
> > > > I am not sure if this is required, but seems like it is useful for me.
> > > > Noticed much of them have its atomic version, but not for this new added one.
> > >
> > > Tony and Brian are right.
> > >
> > > Tobias, can you please test the following patch:
> > >
> > > diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
> > > index 245da96dfddc..e44767ec0532 100644
> > > --- a/drivers/net/wireless/realtek/rtw88/fw.c
> > > +++ b/drivers/net/wireless/realtek/rtw88/fw.c
> > > @@ -228,7 +228,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
> > > goto out;
> > > }
> > >
> > > - ret = read_poll_timeout(rtw_read8, box_state,
> > > + ret = read_poll_timeout_atomic(rtw_read8, box_state,
> > > !((box_state >> box) & 0x1), 100, 3000, false,
> > > rtwdev, REG_HMETFR);
> > >
> > > diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> > > index cb20c733b15a..bc89ac625f26 100644
> > > --- a/include/linux/iopoll.h
> > > +++ b/include/linux/iopoll.h
> > > @@ -57,6 +57,48 @@
> > > (cond) ? 0 : -ETIMEDOUT; \
> > > })
> > >
> > > +/**
> > > + * read_poll_timeout_atomic - Periodically poll an address until a condition is
> > > + * met or a timeout occurs
> > > + * @op: accessor function (takes @addr as its only argument)
> > > + * @addr: Address to poll
> > > + * @val: Variable to read the value into
> > > + * @cond: Break condition (usually involving @val)
> > > + * @delay_us: Time to udelay between reads in us (0 tight-loops). Should
> > > + * be less than ~10us since udelay is used (see
> > > + * Documentation/timers/timers-howto.rst).
> > > + * @timeout_us: Timeout in us, 0 means never timeout
> > > + * @delay_before_read: if it is true, delay @delay_us before read.
> > > + *
> > > + * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
> > > + * case, the last read value at @args is stored in @val.
> > > + *
> > > + * When available, you'll probably want to use one of the specialized
> > > + * macros defined below rather than this macro directly.
> > > + */
> > > +#define read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \
> > > + delay_before_read, args...) \
> > > +({ \
> > > + u64 __timeout_us = (timeout_us); \
> > > + unsigned long __delay_us = (delay_us); \
> > > + ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> > > + if (delay_before_read && __delay_us) \
> > > + udelay(__delay_us); \
> > > + for (;;) { \
> > > + (val) = op(args); \
> > > + if (cond) \
> > > + break; \
> > > + if (__timeout_us && \
> > > + ktime_compare(ktime_get(), __timeout) > 0) { \
> > > + (val) = op(args); \
> > > + break; \
> > > + } \
> > > + if (__delay_us) \
>
> Isn't there something missing here after __delay_us?
> I got compiler error, misses ;.
>
> > > + } \
> > > + (cond) ? 0 : -ETIMEDOUT; \
> > > +})
> > > +
> > > /**
> > > * readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
> > > * @op: accessor function (takes @addr as its only argument)
> > > @@ -96,25 +138,7 @@
> > > * macros defined below rather than this macro directly.
> > > */
> > > #define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
> > > -({ \
> > > - u64 __timeout_us = (timeout_us); \
> > > - unsigned long __delay_us = (delay_us); \
> > > - ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> > > - for (;;) { \
> > > - (val) = op(addr); \
> > > - if (cond) \
> > > - break; \
> > > - if (__timeout_us && \
> > > - ktime_compare(ktime_get(), __timeout) > 0) { \
> > > - (val) = op(addr); \
> > > - break; \
> > > - } \
> > > - if (__delay_us) \
> > > - udelay(__delay_us); \
> > > - } \
> > > - (cond) ? 0 : -ETIMEDOUT; \
> > > -})
> > > -
> > > + read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
> > >
> > > #define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> > > readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
> > >
> >
>
> --
> Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail ausdrucken.
--
Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail ausdrucken.
[-- Attachment #2: 0001-rtw88-Make-read_poll_timeout-atomic.patch --]
[-- Type: text/plain, Size: 3604 bytes --]
From 2652ea8d9120b67c79dc85a9073cc5e85d6fc2dc Mon Sep 17 00:00:00 2001
From: Tobias Predel <tobias.predel@gmail.com>
Date: Thu, 23 Apr 2020 00:44:15 +0200
Subject: [PATCH] rtw88: Make read_poll_timeout atomic
---
drivers/net/wireless/realtek/rtw88/fw.c | 2 +-
include/linux/iopoll.h | 63 +++++++++++++++++--------
2 files changed, 45 insertions(+), 20 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
index 245da96dfddc..e44767ec0532 100644
--- a/drivers/net/wireless/realtek/rtw88/fw.c
+++ b/drivers/net/wireless/realtek/rtw88/fw.c
@@ -228,7 +228,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
goto out;
}
- ret = read_poll_timeout(rtw_read8, box_state,
+ ret = read_poll_timeout_atomic(rtw_read8, box_state,
!((box_state >> box) & 0x1), 100, 3000, false,
rtwdev, REG_HMETFR);
diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index cb20c733b15a..e4462712b541 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -57,6 +57,49 @@
(cond) ? 0 : -ETIMEDOUT; \
})
+/**
+ * read_poll_timeout_atomic - Periodically poll an address until a condition is
+ * met or a timeout occurs
+ * @op: accessor function (takes @addr as its only argument)
+ * @addr: Address to poll
+ * @val: Variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @delay_us: Time to udelay between reads in us (0 tight-loops). Should
+ * be less than ~10us since udelay is used (see
+ * Documentation/timers/timers-howto.rst).
+ * @timeout_us: Timeout in us, 0 means never timeout
+ * @delay_before_read: if it is true, delay @delay_us before read.
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
+ * case, the last read value at @args is stored in @val.
+ *
+ * When available, you'll probably want to use one of the specialized
+ * macros defined below rather than this macro directly.
+ */
+#define read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \
+ delay_before_read, args...) \
+({ \
+ u64 __timeout_us = (timeout_us); \
+ unsigned long __delay_us = (delay_us); \
+ ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
+ if (delay_before_read && __delay_us) \
+ udelay(__delay_us); \
+ for (;;) { \
+ (val) = op(args); \
+ if (cond) \
+ break; \
+ if (__timeout_us && \
+ ktime_compare(ktime_get(), __timeout) > 0) { \
+ (val) = op(args); \
+ break; \
+ } \
+ if (__delay_us) \
+ udelay(__delay_us); \
+ } \
+ (cond) ? 0 : -ETIMEDOUT; \
+})
+
+
/**
* readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
* @op: accessor function (takes @addr as its only argument)
@@ -96,25 +139,7 @@
* macros defined below rather than this macro directly.
*/
#define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
-({ \
- u64 __timeout_us = (timeout_us); \
- unsigned long __delay_us = (delay_us); \
- ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
- for (;;) { \
- (val) = op(addr); \
- if (cond) \
- break; \
- if (__timeout_us && \
- ktime_compare(ktime_get(), __timeout) > 0) { \
- (val) = op(addr); \
- break; \
- } \
- if (__delay_us) \
- udelay(__delay_us); \
- } \
- (cond) ? 0 : -ETIMEDOUT; \
-})
-
+ read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
#define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
--
2.26.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
2020-04-22 22:55 ` Tobias S. Predel
@ 2020-04-23 5:43 ` Kai-Heng Feng
2020-04-23 9:03 ` Tobias S. Predel
0 siblings, 1 reply; 9+ messages in thread
From: Kai-Heng Feng @ 2020-04-23 5:43 UTC (permalink / raw)
To: Tobias S. Predel; +Cc: Tony Chuang, Brian Norris, linux-wireless
Hi,
> On Apr 23, 2020, at 06:55, Tobias S. Predel <tobias.predel@gmail.com> wrote:
>
> Hello,
>
> I guessed from the deleted lines that
> udelay(__delay_us) was missing.
Yea I fail to copy that line :(
>
> So I compiled successfully and will observe how it will work out.
> But for the first start on the patched kernel it's working and I will report.
Thanks. I'll send the patch.
Kai-Heng
>
> Thanks for your patch! Patch against commit a5840f9618a90ecbe1617f7632482563c0ee307e
> is attached.
>
> Kind regards,
> Tobias
>
>
> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
> index 245da96dfddc..e44767ec0532 100644
> --- a/drivers/net/wireless/realtek/rtw88/fw.c
> +++ b/drivers/net/wireless/realtek/rtw88/fw.c
> @@ -228,7 +228,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
> goto out;
> }
>
> - ret = read_poll_timeout(rtw_read8, box_state,
> + ret = read_poll_timeout_atomic(rtw_read8, box_state,
> !((box_state >> box) & 0x1), 100, 3000, false,
> rtwdev, REG_HMETFR);
>
> diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> index cb20c733b15a..e4462712b541 100644
> --- a/include/linux/iopoll.h
> +++ b/include/linux/iopoll.h
> @@ -57,6 +57,49 @@
> (cond) ? 0 : -ETIMEDOUT; \
> })
>
> +/**
> + * read_poll_timeout_atomic - Periodically poll an address until a condition is
> + * met or a timeout occurs
> + * @op: accessor function (takes @addr as its only argument)
> + * @addr: Address to poll
> + * @val: Variable to read the value into
> + * @cond: Break condition (usually involving @val)
> + * @delay_us: Time to udelay between reads in us (0 tight-loops). Should
> + * be less than ~10us since udelay is used (see
> + * Documentation/timers/timers-howto.rst).
> + * @timeout_us: Timeout in us, 0 means never timeout
> + * @delay_before_read: if it is true, delay @delay_us before read.
> + *
> + * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
> + * case, the last read value at @args is stored in @val.
> + *
> + * When available, you'll probably want to use one of the specialized
> + * macros defined below rather than this macro directly.
> + */
> +#define read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \
> + delay_before_read, args...) \
> +({ \
> + u64 __timeout_us = (timeout_us); \
> + unsigned long __delay_us = (delay_us); \
> + ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> + if (delay_before_read && __delay_us) \
> + udelay(__delay_us); \
> + for (;;) { \
> + (val) = op(args); \
> + if (cond) \
> + break; \
> + if (__timeout_us && \
> + ktime_compare(ktime_get(), __timeout) > 0) { \
> + (val) = op(args); \
> + break; \
> + } \
> + if (__delay_us) \
> + udelay(__delay_us); \
> + } \
> + (cond) ? 0 : -ETIMEDOUT; \
> +})
> +
> +
> /**
> * readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
> * @op: accessor function (takes @addr as its only argument)
> @@ -96,25 +139,7 @@
> * macros defined below rather than this macro directly.
> */
> #define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
> -({ \
> - u64 __timeout_us = (timeout_us); \
> - unsigned long __delay_us = (delay_us); \
> - ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> - for (;;) { \
> - (val) = op(addr); \
> - if (cond) \
> - break; \
> - if (__timeout_us && \
> - ktime_compare(ktime_get(), __timeout) > 0) { \
> - (val) = op(addr); \
> - break; \
> - } \
> - if (__delay_us) \
> - udelay(__delay_us); \
> - } \
> - (cond) ? 0 : -ETIMEDOUT; \
> -})
> -
> + read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
>
> #define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
>
>
> On Wed, Apr 22, 2020 at 10:57:31PM +0200, Tobias S. Predel wrote:
>> Hi Kai-Heng,
>>
>> On Wed, Apr 22, 2020 at 09:25:24PM +0200, Tobias S. Predel wrote:
>>> Hi Kai-Heng,
>>>
>>> On Thu, Apr 23, 2020 at 12:48:55AM +0800, Kai-Heng Feng wrote:
>>>> Hi Tobias,
>>>>
>>>>> On Apr 22, 2020, at 10:21, Tony Chuang <yhchuang@realtek.com> wrote:
>>>>>
>>>>> Brian Norris <briannorris@chromium.org> :
>>>>>>
>>>>>> I'm not sure about the first half your problem, but for the
>>>>>> scheduling-while-atomic:
>>>>>>
>>>>>> On Tue, Apr 21, 2020 at 2:16 PM Tobias S. Predel
>>>>>> <tobias.predel@gmail.com> wrote:
>>>>>>> [28125.482259] BUG: scheduling while atomic:
>>>>>> kworker/u16:0/33416/0x00000002
>>>>>> ...
>>>>>>> [28125.482436] Preemption disabled at:
>>>>>>> [28125.482443] [<0000000000000000>] 0x0
>>>>>>
>>>>>> ^^ This line is a bit weird -- shouldn't this have a real PC?
>>>>>>
>>>>>>> [28125.482452] CPU: 5 PID: 33416 Comm: kworker/u16:0 Tainted: G
>>>>>> W 5.7.0-rc2-next-20200421-1-next-git #1
>>>>>>> [28125.482456] Hardware name: HP HP ProBook 430 G5/8377, BIOS Q85
>>>>>> Ver. 01.09.01 10/15/2019
>>>>>>> [28125.482477] Workqueue: phy0 rtw_watch_dog_work [rtw88]
>>>>>>> [28125.482481] Call Trace:
>>>>>>> [28125.482495] dump_stack+0x66/0x90
>>>>>>> [28125.482505] __schedule_bug.cold+0x8e/0x9b
>>>>>>> [28125.482512] __schedule+0x686/0x7b0
>>>>>>> [28125.482520] ? _raw_spin_unlock_irqrestore+0x20/0x40
>>>>>>> [28125.482525] schedule+0x46/0xf0
>>>>>>> [28125.482531] schedule_hrtimeout_range_clock+0xa5/0x120
>>>>>>> [28125.482540] ? hrtimer_init_sleeper+0xa0/0xa0
>>>>>>> [28125.482546] usleep_range+0x67/0x90
>>>>>>> [28125.482568] rtw_fw_send_h2c_command+0xe0/0x1a0 [rtw88]
>>>>>>> [28125.482590] rtw_fw_set_pwr_mode+0x95/0xb0 [rtw88]
>>>>>>> [28125.482610] rtw_enter_lps+0xa1/0x100 [rtw88]
>>>>>>> [28125.482625] rtw_watch_dog_work+0x21c/0x230 [rtw88]
>>>>>>> [28125.482635] process_one_work+0x1da/0x3d0
>>>>>>> [28125.482643] worker_thread+0x4a/0x3d0
>>>>>>> [28125.482651] kthread+0x122/0x160
>>>>>>> [28125.482658] ? process_one_work+0x3d0/0x3d0
>>>>>>> [28125.482663] ? kthread_park+0x90/0x90
>>>>>>> [28125.482670] ret_from_fork+0x1f/0x40
>>>>>>
>>>>>> This looks like it might be a regression here:
>>>>>>
>>>>>> commit 6343a6d4b2130be9323f347d60af8a7ba8f7242c
>>>>>> Author: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>>>> Date: Tue Apr 7 15:33:31 2020 +0800
>>>>>>
>>>>>> rtw88: Add delay on polling h2c command status bit
>>>>>>
>>>>>> That poll macros is using usleep, which obviously can sleep. We need
>>>>>> to be using a udelay-variant instead.
>>>>>>
>>>>>
>>>>> Maybe we need an atomic version of read_poll_timeout() ?
>>>>> I am not sure if this is required, but seems like it is useful for me.
>>>>> Noticed much of them have its atomic version, but not for this new added one.
>>>>
>>>> Tony and Brian are right.
>>>>
>>>> Tobias, can you please test the following patch:
>>>>
>>>> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
>>>> index 245da96dfddc..e44767ec0532 100644
>>>> --- a/drivers/net/wireless/realtek/rtw88/fw.c
>>>> +++ b/drivers/net/wireless/realtek/rtw88/fw.c
>>>> @@ -228,7 +228,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
>>>> goto out;
>>>> }
>>>>
>>>> - ret = read_poll_timeout(rtw_read8, box_state,
>>>> + ret = read_poll_timeout_atomic(rtw_read8, box_state,
>>>> !((box_state >> box) & 0x1), 100, 3000, false,
>>>> rtwdev, REG_HMETFR);
>>>>
>>>> diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
>>>> index cb20c733b15a..bc89ac625f26 100644
>>>> --- a/include/linux/iopoll.h
>>>> +++ b/include/linux/iopoll.h
>>>> @@ -57,6 +57,48 @@
>>>> (cond) ? 0 : -ETIMEDOUT; \
>>>> })
>>>>
>>>> +/**
>>>> + * read_poll_timeout_atomic - Periodically poll an address until a condition is
>>>> + * met or a timeout occurs
>>>> + * @op: accessor function (takes @addr as its only argument)
>>>> + * @addr: Address to poll
>>>> + * @val: Variable to read the value into
>>>> + * @cond: Break condition (usually involving @val)
>>>> + * @delay_us: Time to udelay between reads in us (0 tight-loops). Should
>>>> + * be less than ~10us since udelay is used (see
>>>> + * Documentation/timers/timers-howto.rst).
>>>> + * @timeout_us: Timeout in us, 0 means never timeout
>>>> + * @delay_before_read: if it is true, delay @delay_us before read.
>>>> + *
>>>> + * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
>>>> + * case, the last read value at @args is stored in @val.
>>>> + *
>>>> + * When available, you'll probably want to use one of the specialized
>>>> + * macros defined below rather than this macro directly.
>>>> + */
>>>> +#define read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \
>>>> + delay_before_read, args...) \
>>>> +({ \
>>>> + u64 __timeout_us = (timeout_us); \
>>>> + unsigned long __delay_us = (delay_us); \
>>>> + ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
>>>> + if (delay_before_read && __delay_us) \
>>>> + udelay(__delay_us); \
>>>> + for (;;) { \
>>>> + (val) = op(args); \
>>>> + if (cond) \
>>>> + break; \
>>>> + if (__timeout_us && \
>>>> + ktime_compare(ktime_get(), __timeout) > 0) { \
>>>> + (val) = op(args); \
>>>> + break; \
>>>> + } \
>>>> + if (__delay_us) \
>>
>> Isn't there something missing here after __delay_us?
>> I got compiler error, misses ;.
>>
>>>> + } \
>>>> + (cond) ? 0 : -ETIMEDOUT; \
>>>> +})
>>>> +
>>>> /**
>>>> * readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
>>>> * @op: accessor function (takes @addr as its only argument)
>>>> @@ -96,25 +138,7 @@
>>>> * macros defined below rather than this macro directly.
>>>> */
>>>> #define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
>>>> -({ \
>>>> - u64 __timeout_us = (timeout_us); \
>>>> - unsigned long __delay_us = (delay_us); \
>>>> - ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
>>>> - for (;;) { \
>>>> - (val) = op(addr); \
>>>> - if (cond) \
>>>> - break; \
>>>> - if (__timeout_us && \
>>>> - ktime_compare(ktime_get(), __timeout) > 0) { \
>>>> - (val) = op(addr); \
>>>> - break; \
>>>> - } \
>>>> - if (__delay_us) \
>>>> - udelay(__delay_us); \
>>>> - } \
>>>> - (cond) ? 0 : -ETIMEDOUT; \
>>>> -})
>>>> -
>>>> + read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
>>>>
>>>> #define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
>>>> readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
>>>>
>>>
>>
>> --
>> Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail ausdrucken.
>
> --
> Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail ausdrucken.
> <0001-rtw88-Make-read_poll_timeout-atomic.patch>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002
2020-04-23 5:43 ` Kai-Heng Feng
@ 2020-04-23 9:03 ` Tobias S. Predel
0 siblings, 0 replies; 9+ messages in thread
From: Tobias S. Predel @ 2020-04-23 9:03 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: Tony Chuang, Brian Norris, linux-wireless
Hi,
On Thu, Apr 23, 2020 at 01:43:05PM +0800, Kai-Heng Feng wrote:
> Hi,
>
> > On Apr 23, 2020, at 06:55, Tobias S. Predel <tobias.predel@gmail.com> wrote:
> >
> > Hello,
> >
> > I guessed from the deleted lines that
> > udelay(__delay_us) was missing.
>
> Yea I fail to copy that line :(
>
> >
> > So I compiled successfully and will observe how it will work out.
> > But for the first start on the patched kernel it's working and I will report.
>
> Thanks. I'll send the patch.
You're welcome. Thanks for your work!
Have a nice day and stay healthy!
>
> Kai-Heng
>
Kind regards,
Tobias
> >
> > Thanks for your patch! Patch against commit a5840f9618a90ecbe1617f7632482563c0ee307e
> > is attached.
> >
> > Kind regards,
> > Tobias
> >
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
> > index 245da96dfddc..e44767ec0532 100644
> > --- a/drivers/net/wireless/realtek/rtw88/fw.c
> > +++ b/drivers/net/wireless/realtek/rtw88/fw.c
> > @@ -228,7 +228,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
> > goto out;
> > }
> >
> > - ret = read_poll_timeout(rtw_read8, box_state,
> > + ret = read_poll_timeout_atomic(rtw_read8, box_state,
> > !((box_state >> box) & 0x1), 100, 3000, false,
> > rtwdev, REG_HMETFR);
> >
> > diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> > index cb20c733b15a..e4462712b541 100644
> > --- a/include/linux/iopoll.h
> > +++ b/include/linux/iopoll.h
> > @@ -57,6 +57,49 @@
> > (cond) ? 0 : -ETIMEDOUT; \
> > })
> >
> > +/**
> > + * read_poll_timeout_atomic - Periodically poll an address until a condition is
> > + * met or a timeout occurs
> > + * @op: accessor function (takes @addr as its only argument)
> > + * @addr: Address to poll
> > + * @val: Variable to read the value into
> > + * @cond: Break condition (usually involving @val)
> > + * @delay_us: Time to udelay between reads in us (0 tight-loops). Should
> > + * be less than ~10us since udelay is used (see
> > + * Documentation/timers/timers-howto.rst).
> > + * @timeout_us: Timeout in us, 0 means never timeout
> > + * @delay_before_read: if it is true, delay @delay_us before read.
> > + *
> > + * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
> > + * case, the last read value at @args is stored in @val.
> > + *
> > + * When available, you'll probably want to use one of the specialized
> > + * macros defined below rather than this macro directly.
> > + */
> > +#define read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \
> > + delay_before_read, args...) \
> > +({ \
> > + u64 __timeout_us = (timeout_us); \
> > + unsigned long __delay_us = (delay_us); \
> > + ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> > + if (delay_before_read && __delay_us) \
> > + udelay(__delay_us); \
> > + for (;;) { \
> > + (val) = op(args); \
> > + if (cond) \
> > + break; \
> > + if (__timeout_us && \
> > + ktime_compare(ktime_get(), __timeout) > 0) { \
> > + (val) = op(args); \
> > + break; \
> > + } \
> > + if (__delay_us) \
> > + udelay(__delay_us); \
> > + } \
> > + (cond) ? 0 : -ETIMEDOUT; \
> > +})
> > +
> > +
> > /**
> > * readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
> > * @op: accessor function (takes @addr as its only argument)
> > @@ -96,25 +139,7 @@
> > * macros defined below rather than this macro directly.
> > */
> > #define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
> > -({ \
> > - u64 __timeout_us = (timeout_us); \
> > - unsigned long __delay_us = (delay_us); \
> > - ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> > - for (;;) { \
> > - (val) = op(addr); \
> > - if (cond) \
> > - break; \
> > - if (__timeout_us && \
> > - ktime_compare(ktime_get(), __timeout) > 0) { \
> > - (val) = op(addr); \
> > - break; \
> > - } \
> > - if (__delay_us) \
> > - udelay(__delay_us); \
> > - } \
> > - (cond) ? 0 : -ETIMEDOUT; \
> > -})
> > -
> > + read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
> >
> > #define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> > readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
> >
> >
> > On Wed, Apr 22, 2020 at 10:57:31PM +0200, Tobias S. Predel wrote:
> >> Hi Kai-Heng,
> >>
> >> On Wed, Apr 22, 2020 at 09:25:24PM +0200, Tobias S. Predel wrote:
> >>> Hi Kai-Heng,
> >>>
> >>> On Thu, Apr 23, 2020 at 12:48:55AM +0800, Kai-Heng Feng wrote:
> >>>> Hi Tobias,
> >>>>
> >>>>> On Apr 22, 2020, at 10:21, Tony Chuang <yhchuang@realtek.com> wrote:
> >>>>>
> >>>>> Brian Norris <briannorris@chromium.org> :
> >>>>>>
> >>>>>> I'm not sure about the first half your problem, but for the
> >>>>>> scheduling-while-atomic:
> >>>>>>
> >>>>>> On Tue, Apr 21, 2020 at 2:16 PM Tobias S. Predel
> >>>>>> <tobias.predel@gmail.com> wrote:
> >>>>>>> [28125.482259] BUG: scheduling while atomic:
> >>>>>> kworker/u16:0/33416/0x00000002
> >>>>>> ...
> >>>>>>> [28125.482436] Preemption disabled at:
> >>>>>>> [28125.482443] [<0000000000000000>] 0x0
> >>>>>>
> >>>>>> ^^ This line is a bit weird -- shouldn't this have a real PC?
> >>>>>>
> >>>>>>> [28125.482452] CPU: 5 PID: 33416 Comm: kworker/u16:0 Tainted: G
> >>>>>> W 5.7.0-rc2-next-20200421-1-next-git #1
> >>>>>>> [28125.482456] Hardware name: HP HP ProBook 430 G5/8377, BIOS Q85
> >>>>>> Ver. 01.09.01 10/15/2019
> >>>>>>> [28125.482477] Workqueue: phy0 rtw_watch_dog_work [rtw88]
> >>>>>>> [28125.482481] Call Trace:
> >>>>>>> [28125.482495] dump_stack+0x66/0x90
> >>>>>>> [28125.482505] __schedule_bug.cold+0x8e/0x9b
> >>>>>>> [28125.482512] __schedule+0x686/0x7b0
> >>>>>>> [28125.482520] ? _raw_spin_unlock_irqrestore+0x20/0x40
> >>>>>>> [28125.482525] schedule+0x46/0xf0
> >>>>>>> [28125.482531] schedule_hrtimeout_range_clock+0xa5/0x120
> >>>>>>> [28125.482540] ? hrtimer_init_sleeper+0xa0/0xa0
> >>>>>>> [28125.482546] usleep_range+0x67/0x90
> >>>>>>> [28125.482568] rtw_fw_send_h2c_command+0xe0/0x1a0 [rtw88]
> >>>>>>> [28125.482590] rtw_fw_set_pwr_mode+0x95/0xb0 [rtw88]
> >>>>>>> [28125.482610] rtw_enter_lps+0xa1/0x100 [rtw88]
> >>>>>>> [28125.482625] rtw_watch_dog_work+0x21c/0x230 [rtw88]
> >>>>>>> [28125.482635] process_one_work+0x1da/0x3d0
> >>>>>>> [28125.482643] worker_thread+0x4a/0x3d0
> >>>>>>> [28125.482651] kthread+0x122/0x160
> >>>>>>> [28125.482658] ? process_one_work+0x3d0/0x3d0
> >>>>>>> [28125.482663] ? kthread_park+0x90/0x90
> >>>>>>> [28125.482670] ret_from_fork+0x1f/0x40
> >>>>>>
> >>>>>> This looks like it might be a regression here:
> >>>>>>
> >>>>>> commit 6343a6d4b2130be9323f347d60af8a7ba8f7242c
> >>>>>> Author: Kai-Heng Feng <kai.heng.feng@canonical.com>
> >>>>>> Date: Tue Apr 7 15:33:31 2020 +0800
> >>>>>>
> >>>>>> rtw88: Add delay on polling h2c command status bit
> >>>>>>
> >>>>>> That poll macros is using usleep, which obviously can sleep. We need
> >>>>>> to be using a udelay-variant instead.
> >>>>>>
> >>>>>
> >>>>> Maybe we need an atomic version of read_poll_timeout() ?
> >>>>> I am not sure if this is required, but seems like it is useful for me.
> >>>>> Noticed much of them have its atomic version, but not for this new added one.
> >>>>
> >>>> Tony and Brian are right.
> >>>>
> >>>> Tobias, can you please test the following patch:
> >>>>
> >>>> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
> >>>> index 245da96dfddc..e44767ec0532 100644
> >>>> --- a/drivers/net/wireless/realtek/rtw88/fw.c
> >>>> +++ b/drivers/net/wireless/realtek/rtw88/fw.c
> >>>> @@ -228,7 +228,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
> >>>> goto out;
> >>>> }
> >>>>
> >>>> - ret = read_poll_timeout(rtw_read8, box_state,
> >>>> + ret = read_poll_timeout_atomic(rtw_read8, box_state,
> >>>> !((box_state >> box) & 0x1), 100, 3000, false,
> >>>> rtwdev, REG_HMETFR);
> >>>>
> >>>> diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> >>>> index cb20c733b15a..bc89ac625f26 100644
> >>>> --- a/include/linux/iopoll.h
> >>>> +++ b/include/linux/iopoll.h
> >>>> @@ -57,6 +57,48 @@
> >>>> (cond) ? 0 : -ETIMEDOUT; \
> >>>> })
> >>>>
> >>>> +/**
> >>>> + * read_poll_timeout_atomic - Periodically poll an address until a condition is
> >>>> + * met or a timeout occurs
> >>>> + * @op: accessor function (takes @addr as its only argument)
> >>>> + * @addr: Address to poll
> >>>> + * @val: Variable to read the value into
> >>>> + * @cond: Break condition (usually involving @val)
> >>>> + * @delay_us: Time to udelay between reads in us (0 tight-loops). Should
> >>>> + * be less than ~10us since udelay is used (see
> >>>> + * Documentation/timers/timers-howto.rst).
> >>>> + * @timeout_us: Timeout in us, 0 means never timeout
> >>>> + * @delay_before_read: if it is true, delay @delay_us before read.
> >>>> + *
> >>>> + * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
> >>>> + * case, the last read value at @args is stored in @val.
> >>>> + *
> >>>> + * When available, you'll probably want to use one of the specialized
> >>>> + * macros defined below rather than this macro directly.
> >>>> + */
> >>>> +#define read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \
> >>>> + delay_before_read, args...) \
> >>>> +({ \
> >>>> + u64 __timeout_us = (timeout_us); \
> >>>> + unsigned long __delay_us = (delay_us); \
> >>>> + ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> >>>> + if (delay_before_read && __delay_us) \
> >>>> + udelay(__delay_us); \
> >>>> + for (;;) { \
> >>>> + (val) = op(args); \
> >>>> + if (cond) \
> >>>> + break; \
> >>>> + if (__timeout_us && \
> >>>> + ktime_compare(ktime_get(), __timeout) > 0) { \
> >>>> + (val) = op(args); \
> >>>> + break; \
> >>>> + } \
> >>>> + if (__delay_us) \
> >>
> >> Isn't there something missing here after __delay_us?
> >> I got compiler error, misses ;.
> >>
> >>>> + } \
> >>>> + (cond) ? 0 : -ETIMEDOUT; \
> >>>> +})
> >>>> +
> >>>> /**
> >>>> * readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
> >>>> * @op: accessor function (takes @addr as its only argument)
> >>>> @@ -96,25 +138,7 @@
> >>>> * macros defined below rather than this macro directly.
> >>>> */
> >>>> #define readx_poll_timeout_atomic(op, addr, val, cond, delay_us, timeout_us) \
> >>>> -({ \
> >>>> - u64 __timeout_us = (timeout_us); \
> >>>> - unsigned long __delay_us = (delay_us); \
> >>>> - ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> >>>> - for (;;) { \
> >>>> - (val) = op(addr); \
> >>>> - if (cond) \
> >>>> - break; \
> >>>> - if (__timeout_us && \
> >>>> - ktime_compare(ktime_get(), __timeout) > 0) { \
> >>>> - (val) = op(addr); \
> >>>> - break; \
> >>>> - } \
> >>>> - if (__delay_us) \
> >>>> - udelay(__delay_us); \
> >>>> - } \
> >>>> - (cond) ? 0 : -ETIMEDOUT; \
> >>>> -})
> >>>> -
> >>>> + read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, false, addr)
> >>>>
> >>>> #define readb_poll_timeout(addr, val, cond, delay_us, timeout_us) \
> >>>> readx_poll_timeout(readb, addr, val, cond, delay_us, timeout_us)
> >>>>
> >>>
> >>
> >> --
> >> Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail ausdrucken.
> >
> > --
> > Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail ausdrucken.
> > <0001-rtw88-Make-read_poll_timeout-atomic.patch>
>
--
Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail ausdrucken.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-04-23 9:03 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-21 21:15 rtw88: BUG: scheduling while atomic: kworker/u16:0/33416/0x00000002 Tobias S. Predel
2020-04-21 22:56 ` Brian Norris
2020-04-22 2:21 ` Tony Chuang
2020-04-22 16:48 ` Kai-Heng Feng
2020-04-22 19:25 ` Tobias S. Predel
2020-04-22 20:57 ` Tobias S. Predel
2020-04-22 22:55 ` Tobias S. Predel
2020-04-23 5:43 ` Kai-Heng Feng
2020-04-23 9:03 ` Tobias S. Predel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.