Re: [RFC v4 1/5] PM: Add a sysfs file to represent the percentage of sleep in hardware state

[John Stultz]

On Thu, Nov 17, 2022 at 2:58 PM Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index f72b9f1de178..49119a942cb2 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1806,16 +1806,16 @@ void timekeeping_resume(void)
>         cycle_now = tk_clock_read(&tk->tkr_mono);
>         nsec = clocksource_stop_suspend_timing(clock, cycle_now);
>         if (nsec > 0) {
> -               ts_delta = ns_to_timespec64(nsec);
> +               timekeeping_suspend_time = ns_to_timespec64(nsec);
>                 inject_sleeptime = true;
>         } else if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) {
> -               ts_delta = timespec64_sub(ts_new, timekeeping_suspend_time);
> +               timekeeping_suspend_time = timespec64_sub(ts_new, timekeeping_suspend_time);
>                 inject_sleeptime = true;
>         }
>
>         if (inject_sleeptime) {
>                 suspend_timing_needed = false;
> -               __timekeeping_inject_sleeptime(tk, &ts_delta);
> +               __timekeeping_inject_sleeptime(tk, &timekeeping_suspend_time);
>         }
>
>         /* Re-base the last cycle value */
> @@ -2232,6 +2232,18 @@ void update_wall_time(void)
>                 clock_was_set_delayed();
>  }
>
> +/**
> + * get_suspend_duration_ns - Return the duration of a system suspend.
> + *
> + * Returns the calculation of the duration of time that passed while a
> + * system was suspended.
> + *
> + */
> +u64 get_suspend_duration_ns(void)
> +{
> +       return timespec64_to_ns(&timekeeping_suspend_time);
> +}
> +

Hey Mario, thanks for sending out this patch!

Though, I feel like this overloads the meaning of
timekeeping_suspend_time in a confusing way.  It's supposed to be the
time from the persistent clock that we started suspend.
But this patch overloads it to also keep track of the time in suspend,
but only for the last suspend event. I'd avoid overloading variables
in this way.

Further, I think this will miss suspend time accounting on systems
that do not have a persistent clock (see drivers/rtc/class.c's use of
timekeeping_inject_sleeptime64).

get_suspend_duration_ns() also is confusing as it doesn't clearly
indicate if it should return cumulative time-in-suspend or the last
time-in-suspend.

I think the earlier version of this patch that hooked into
__timekeeping_inject_sleeptime was a better general approach.

thanks
-john