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@xxxxxxxxx> 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@xxxxxxxxxxx> wrote:
> >>>>>
> >>>>> Brian Norris <briannorris@xxxxxxxxxxxx> :
> >>>>>>
> >>>>>> 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@xxxxxxxxx> 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@xxxxxxxxxxxxx>
> >>>>>> 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.
