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. 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.
Thanks,
tglx
