* 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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).