On Tue, 16 Oct 2018, Rafael J. Wysocki wrote:
> 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.
Me neither. Ignore that, brain slip on my side.
> > > > + 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.
I can see that the system is royaly screwed.
> 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).
Me neither, but I wouldn't be surprised if there is.
Ok, so both variants of patches work. One has that state conditional the
other not. Both read stale time at that point. So no preference.
Thanks,
tglx
