Re: [PATCH v2 2/2] iopoll: Do not use timekeeping in read_poll_timeout_atomic()

[Geert Uytterhoeven]

Hi Arnd,

On Wed, May 10, 2023 at 3:36 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Wed, May 10, 2023, at 15:23, Geert Uytterhoeven wrote:
> > read_poll_timeout_atomic() uses ktime_get() to implement the timeout
> > feature, just like its non-atomic counterpart.  However, there are
> > several issues with this, due to its use in atomic contexts:
> >
> >   1. When called in the s2ram path (as typically done by clock or PM
> >      domain drivers), timekeeping may be suspended, triggering the
> >      WARN_ON(timekeeping_suspended) in ktime_get():
> >
> >       WARNING: CPU: 0 PID: 654 at kernel/time/timekeeping.c:843 ktime_get+0x28/0x78
> >
> >      Calling ktime_get_mono_fast_ns() instead of ktime_get() would get
> >      rid of that warning.  However, that would break timeout handling,
> >      as (at least on systems with an ARM architectured timer), the time
> >      returned by ktime_get_mono_fast_ns() does not advance while
> >      timekeeping is suspended.
> >      Interestingly, (on the same ARM systems) the time returned by
> >      ktime_get() does advance while timekeeping is suspended, despite
> >      the warning.
> >
> >   2. Depending on the actual clock source, and especially before a
> >      high-resolution clocksource (e.g. the ARM architectured timer)
> >      becomes available, time may not advance in atomic contexts, thus
> >      breaking timeout handling.
> >
> > Fix this by abandoning the idea that one can rely on timekeeping to
> > implement timeout handling in all atomic contexts, and switch from a
> > global time-based to a locally-estimated timeout handling.  In most
> > (all?) cases the timeout condition is exceptional and an error
> > condition, hence any additional delays due to underestimating wall clock
> > time are irrelevant.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> This looks reasonable to me,
>
> Acked-by: Arnd Bergmann <arnd@xxxxxxxx>

Thanks!

> I assume you sent this because you ran into the bug on a
> particular driver. It might help to be more specific about
> how this can be reproduced.

I first ran into it when converting open-coded loops to
read*_poll_timeout_atomic().
Later, I also saw the issue with the existing
read*_poll_timeout_atomic() calls in the R-Car SYSC driver, but only
after applying additional patches from the BSP that impact the moment
PM Domains are powered during s2ram.
The various pointers to existing mitigations in the cover letter should
give you other suggestions for how to reproduce...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds