On Tue, Oct 16, 2018 at 10:42 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
> 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.
OK, thanks!
I prefer the Bart's one due to the lack of the state conditional. I'd
rather avoid those things if possible.
So Bart, please resend the last patch of yours with a changelog, but
please add a comment to acpi_os_get_timer() to explain why ktime_get()
doesn't work there.
Thanks,
Rafael
