On Tue, Oct 16, 2018 at 7:31 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
> On Tue, 16 Oct 2018, Rafael J. Wysocki wrote:
>
> > On Tue, Oct 16, 2018 at 5:23 PM Bart Van Assche <bvanassche@xxxxxxx> wrote:
> > >
> > > On Tue, 2018-10-16 at 09:54 +0200, Rafael J. Wysocki wrote:
> > > > drivers/acpi/osl.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > Index: linux-pm/drivers/acpi/osl.c
> > > > ===================================================================
> > > > --- linux-pm.orig/drivers/acpi/osl.c
> > > > +++ linux-pm/drivers/acpi/osl.c
> > > > @@ -623,7 +623,8 @@ void acpi_os_stall(u32 us)
> > > > */
> > > > u64 acpi_os_get_timer(void)
> > > > {
> > > > - u64 time_ns = ktime_to_ns(ktime_get());
> > > > + u64 time_ns = ktime_to_ns(unlikely(system_state == SYSTEM_SUSPEND) ?
> > > > + ktime_get_mono_fast_ns() : ktime_get());
>
> Well, this merily prevents the WARN_ON() and in fact you can use
> ktime_get_mono_fast_ns() unconditionally here because you cannot run into
> the time jump issue at all.
I'm not sure why that is so TBH.
> But see below.
>
> > > > do_div(time_ns, 100);
> > > > return time_ns;
> > > > }
> > >
> > > From the Documentation/core-api/timekeeping.rst section about
> > > ktime_get_mono_fast_ns(): "most drivers should never call them, since the
> > > time is allowed to jump under certain conditions".
> >
> > Which doesn't apply, because the code in question runs on on CPU with
> > interrupts disabled.
> >
> > > How about the following, also untested patch?
> >
> > Well, if this is guaranteed to behave like a monotonic clock, then why not.
> >
> > >
> > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > > index 8df9abfa947b..2fb7911b3a28 100644
> > > --- a/drivers/acpi/osl.c
> > > +++ b/drivers/acpi/osl.c
> > > @@ -623,9 +623,8 @@ void acpi_os_stall(u32 us)
> > > */
> > > u64 acpi_os_get_timer(void)
> > > {
> > > - u64 time_ns = ktime_to_ns(ktime_get());
> > > - do_div(time_ns, 100);
> > > - return time_ns;
> > > + return (get_jiffies_64() - INITIAL_JIFFIES) *
> > > + (ACPI_100NSEC_PER_SEC / HZ);
> >
> > Yeah, but I'm not sure what Thomas (CCed) thinks about that.
>
> In system_state SYSTEM_SUSPEND neither ktime_get_mono_fast_ns() nor jiffes
> are giving you anything meaningful. They are both stuck at the point in
> time where timekeeping was suspended.
>
> I have no clue in which context this is called and why SYSTEM_SUSPEND
> matters, so it might be a non issue.
The issue here is just theoretical IMO.
acpi_os_get_timer() is called by ACPICA to detect AML that takes too
much time to execute and we happen to invoke it during early resume
(before resuming the timekeeping). It will fail to detect "spinning"
AML at that point, but even if it detected it and aborted its
execution, the state of things would be extremely fragile after that
anyway, so it may even be safer to let the AML run for whatever time
it takes here. IOW if this happens, it is not a recoverable error
condition in general regardless.
Arguably, resuming the timekeeping before running that AML would be
more elegant, but for that we'd probably need to run
timekeeping_resume() directly from say the beginning of
syscore_resume() (and analogously for the suspend part) and if there
is anything in syscore that needs to resume before the timekeeping
(and suspend after it), that won't work (I'm not sure if there is
anything like that, though).
Cheers,
Rafael
