[RFC v2 0/3] Introduce infrastructure to report time in hardware sleep state

[RFC v2 0/3] Introduce infrastructure to report time in hardware sleep state

[Mario Limonciello]

Sven van Ashbrook brought a patch to the kernel mailing list that
attempted to change the reporting level of a s0ix entry issue to a
different debugging level so that infastructure used by Google could
better scan logs to catch problems.

This approach was rejected, but during the conversation another
suggestion was made by David E. Box to introduce some infrastructure
into the kernel to report this information.

As it's information that is reported by both AMD and Intel platforms
over s2idle, this seems to make sense.

This series introduces new sysfs files representing duration in a hardware
sleep state and total sleep duration.

The expectation is that userspace could read these file after s2idle
occurred to infer whta percentage of time was spent in a hardware sleep
state.

RFC v1->v2:
 * Rename sysfs file for time in hardware state
 * Export a sysfs file for total time in suspend
 * Only export sysfs file for hardware state if system supports low
   power idle.

Mario Limonciello (3):
  PM: Add a sysfs files to represent sleep duration
  platform/x86/amd: pmc: Report duration of time in deepest hw state
  platform/x86/intel/pmc: core: Report duration of time in deepest HW
    state

 Documentation/ABI/testing/sysfs-power | 17 +++++++++++
 drivers/platform/x86/amd/pmc.c        |  4 +--
 drivers/platform/x86/intel/pmc/core.c |  2 ++
 include/linux/suspend.h               |  5 ++++
 kernel/power/main.c                   | 42 +++++++++++++++++++++++++++
 kernel/power/suspend.c                |  2 ++
 kernel/time/timekeeping.c             |  2 ++
 7 files changed, 71 insertions(+), 3 deletions(-)

-- 
2.34.1

[RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Mario Limonciello]

Both AMD and Intel SoCs have a concept of reporting whether the hardware
reached a hardware sleep state over s2idle as well as how much
time was spent in such a state.

This information is valuable to both chip designers and system designers
as it helps to identify when there are problems with power consumption
over an s2idle cycle.

To make the information discoverable, create a new sysfs file and a symbol
that drivers from supported manufacturers can use to advertise this
information. This file will only be exported when the system supports low
power idle in the ACPI table.

In order to effectively use this information you will ideally want to
compare against the total duration of sleep, so export a second sysfs file
that will show total time. This file will be exported on all systems and
used both for s2idle and s3.

Suggested-by: David E Box <david.e.box@intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 Documentation/ABI/testing/sysfs-power | 17 +++++++++++
 include/linux/suspend.h               |  4 +++
 kernel/power/main.c                   | 42 +++++++++++++++++++++++++++
 kernel/power/suspend.c                |  2 ++
 kernel/time/timekeeping.c             |  2 ++
 5 files changed, 67 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index f99d433ff311..5b47cbb4dc9e 100644
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -413,6 +413,23 @@ Description:
 		The /sys/power/suspend_stats/last_failed_step file contains
 		the last failed step in the suspend/resume path.
 
+What:		/sys/power/suspend_stats/last_hw_state_residency
+Date:		December 2022
+Contact:	Mario Limonciello <mario.limonciello@amd.com>
+Description:
+		The /sys/power/suspend_stats/last_hw_state_residency file contains
+		the amount of time spent in a hardware sleep state.
+		This attribute is only available if the system supports
+		low power idle.  This is measured in microseconds.
+
+What:		/sys/power/suspend_stats/last_suspend_total
+Date:		December 2022
+Contact:	Mario Limonciello <mario.limonciello@amd.com>
+Description:
+		The /sys/power/suspend_stats/last_suspend_total file contains
+		the total duration of the sleep cycle.
+		This is measured in microseconds.
+
 What:		/sys/power/sync_on_suspend
 Date:		October 2019
 Contact:	Jonas Meurer <jonas@freesources.org>
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index cfe19a028918..af343c3f8198 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -68,6 +68,8 @@ struct suspend_stats {
 	int	last_failed_errno;
 	int	errno[REC_FAILED_NUM];
 	int	last_failed_step;
+	u64	last_hw_state_residency;
+	u64	last_suspend_total;
 	enum suspend_stat_step	failed_steps[REC_FAILED_NUM];
 };
 
@@ -489,6 +491,8 @@ void restore_processor_state(void);
 extern int register_pm_notifier(struct notifier_block *nb);
 extern int unregister_pm_notifier(struct notifier_block *nb);
 extern void ksys_sync_helper(void);
+extern void pm_set_hw_state_residency(u64 duration);
+extern void pm_account_suspend_type(const struct timespec64 *t);
 
 #define pm_notifier(fn, pri) {				\
 	static struct notifier_block fn##_nb =			\
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 31ec4a9b9d70..11bd658583b0 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -6,6 +6,7 @@
  * Copyright (c) 2003 Open Source Development Lab
  */
 
+#include <linux/acpi.h>
 #include <linux/export.h>
 #include <linux/kobject.h>
 #include <linux/string.h>
@@ -54,6 +55,19 @@ void unlock_system_sleep(unsigned int flags)
 }
 EXPORT_SYMBOL_GPL(unlock_system_sleep);
 
+void pm_set_hw_state_residency(u64 duration)
+{
+	suspend_stats.last_hw_state_residency = duration;
+}
+EXPORT_SYMBOL_GPL(pm_set_hw_state_residency);
+
+void pm_account_suspend_type(const struct timespec64 *t)
+{
+	suspend_stats.last_suspend_total += (s64)t->tv_sec * USEC_PER_SEC +
+						 t->tv_nsec / NSEC_PER_USEC;
+}
+EXPORT_SYMBOL_GPL(pm_account_suspend_type);
+
 void ksys_sync_helper(void)
 {
 	ktime_t start;
@@ -377,6 +391,20 @@ static ssize_t last_failed_step_show(struct kobject *kobj,
 }
 static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
 
+static ssize_t last_hw_state_residency_show(struct kobject *kobj,
+		struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%llu\n", suspend_stats.last_hw_state_residency);
+}
+static struct kobj_attribute last_hw_state_residency = __ATTR_RO(last_hw_state_residency);
+
+static ssize_t last_suspend_total_show(struct kobject *kobj,
+		struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%llu\n", suspend_stats.last_suspend_total);
+}
+static struct kobj_attribute last_suspend_total = __ATTR_RO(last_suspend_total);
+
 static struct attribute *suspend_attrs[] = {
 	&success.attr,
 	&fail.attr,
@@ -391,12 +419,26 @@ static struct attribute *suspend_attrs[] = {
 	&last_failed_dev.attr,
 	&last_failed_errno.attr,
 	&last_failed_step.attr,
+	&last_hw_state_residency.attr,
+	&last_suspend_total.attr,
 	NULL,
 };
 
+static umode_t suspend_attr_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
+{
+	if (attr != &last_hw_state_residency.attr)
+		return 0444;
+#ifdef CONFIG_ACPI
+	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
+		return 0444;
+#endif
+	return 0;
+}
+
 static const struct attribute_group suspend_attr_group = {
 	.name = "suspend_stats",
 	.attrs = suspend_attrs,
+	.is_visible = suspend_attr_is_visible,
 };
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index fa3bf161d13f..b6c4a3733212 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -423,6 +423,8 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 	if (suspend_test(TEST_PLATFORM))
 		goto Platform_wake;
 
+	suspend_stats.last_suspend_total = 0;
+
 	if (state == PM_SUSPEND_TO_IDLE) {
 		s2idle_loop();
 		goto Platform_wake;
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index f72b9f1de178..e1b356787e53 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -24,6 +24,7 @@
 #include <linux/compiler.h>
 #include <linux/audit.h>
 #include <linux/random.h>
+#include <linux/suspend.h>
 
 #include "tick-internal.h"
 #include "ntp_internal.h"
@@ -1698,6 +1699,7 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
 	tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, *delta));
 	tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
 	tk_debug_account_sleep_time(delta);
+	pm_account_suspend_type(delta);
 }
 
 #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE)
-- 
2.34.1

[RFC v2 2/3] platform/x86/amd: pmc: Report duration of time in deepest hw state

[Mario Limonciello]

amd_pmc displays a warning when a suspend didn't get to the deepest
state and a dynamic debugging message with the duration if it did.

Rather than logging to dynamic debugging the duration spent in the
deepest state, report this to the standard kernel reporting infrastructure
that can be accessed from sysfs.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index 96e790e639a2..e583bf966f47 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -363,9 +363,7 @@ static void amd_pmc_validate_deepest(struct amd_pmc_dev *pdev)
 
 	if (!table.s0i3_last_entry_status)
 		dev_warn(pdev->dev, "Last suspend didn't reach deepest state\n");
-	else
-		dev_dbg(pdev->dev, "Last suspend in deepest state for %lluus\n",
-			 table.timein_s0i3_lastcapture);
+	pm_set_hw_state_residency(table.s0i3_last_entry_status ? table.timein_s0i3_lastcapture : 0);
 }
 #endif
 
-- 
2.34.1

[RFC v2 3/3] platform/x86/intel/pmc: core: Report duration of time in HW sleep state

[Mario Limonciello]

intel_pmc_core displays a warning when a suspend didn't get to a HW
sleep state.

This information is generally useful to userspace as well which may use
it to collect further debugging data. Report this to the standard kernel
reporting infrastructure that can be accessed from sysfs.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/intel/pmc/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
index 17ec5825d13d..ef2055209213 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -2116,6 +2116,8 @@ static __maybe_unused int pmc_core_resume(struct device *dev)
 	if (!pmcdev->check_counters)
 		return 0;
 
+	pm_set_hw_state_residency(pmcdev->s0ix_counter);
+
 	if (!pmc_core_is_s0ix_failed(pmcdev))
 		return 0;
 
-- 
2.34.1

Re: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Thomas Gleixner]

On Thu, Nov 10 2022 at 00:47, Mario Limonciello wrote:

'Add a sysfs files'?

Can you please decide whether that's 'a file' or 'multiple files'?

> Both AMD and Intel SoCs have a concept of reporting whether the hardware
> reached a hardware sleep state over s2idle as well as how much
> time was spent in such a state.

Nice, but ...

> This information is valuable to both chip designers and system designers
> as it helps to identify when there are problems with power consumption
> over an s2idle cycle.
>
> To make the information discoverable, create a new sysfs file and a symbol
> that drivers from supported manufacturers can use to advertise this
> information. This file will only be exported when the system supports low
> power idle in the ACPI table.
>
> In order to effectively use this information you will ideally want to
> compare against the total duration of sleep, so export a second sysfs file
> that will show total time. This file will be exported on all systems and
> used both for s2idle and s3.

The above is incomprehensible word salad. Can you come up with some
coherent explanation of what you are trying to achieve please?

> +void pm_set_hw_state_residency(u64 duration)
> +{
> +	suspend_stats.last_hw_state_residency = duration;
> +}
> +EXPORT_SYMBOL_GPL(pm_set_hw_state_residency);
> +
> +void pm_account_suspend_type(const struct timespec64 *t)
> +{
> +	suspend_stats.last_suspend_total += (s64)t->tv_sec * USEC_PER_SEC +
> +						 t->tv_nsec / NSEC_PER_USEC;

Conversion functions for timespecs to scalar nanoseconds exist for a
reason. Why does this need special treatment and open code it?

> +}
> +EXPORT_SYMBOL_GPL(pm_account_suspend_type);

So none of these functions has any kind of documentation. kernel-doc
exists for a reason especially for exported functions.

That said, what's the justification to export any of these functions at
all? AFAICT pm_account_suspend_type() is only used by builtin code...

> +static umode_t suspend_attr_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
> +{
> +	if (attr != &last_hw_state_residency.attr)
> +		return 0444;
> +#ifdef CONFIG_ACPI
> +	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
> +		return 0444;
> +#endif
> +	return 0;
> +}
> +
>  static const struct attribute_group suspend_attr_group = {
>  	.name = "suspend_stats",
>  	.attrs = suspend_attrs,
> +	.is_visible = suspend_attr_is_visible,

How is this change related to the changelog above? We are not hiding
subtle changes to the existing code in some conglomorate patch. See
Documentation/process/...

> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -24,6 +24,7 @@
>  #include <linux/compiler.h>
>  #include <linux/audit.h>
>  #include <linux/random.h>
> +#include <linux/suspend.h>
>  
>  #include "tick-internal.h"
>  #include "ntp_internal.h"
> @@ -1698,6 +1699,7 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
>  	tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, *delta));
>  	tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
>  	tk_debug_account_sleep_time(delta);
> +	pm_account_suspend_type(delta);

That function name is really self explaining - NOT !

     pm_account_suspend_type(delta);

So this will account a suspend type depending on the time spent in
suspend, right?

It's totally obvious that the suspend type (whatever it is) depends on
the time delta argument... especially when the function at hand has
absolutely nothing to do with a type:

> +void pm_account_suspend_type(const struct timespec64 *t)
> +{
> +	suspend_stats.last_suspend_total += (s64)t->tv_sec * USEC_PER_SEC +
> +						 t->tv_nsec / NSEC_PER_USEC;
> +}

Sigh....

Thanks,

        tglx

Re: [RFC v2 3/3] platform/x86/intel/pmc: core: Report duration of time in HW sleep state

[Thomas Gleixner]

On Thu, Nov 10 2022 at 00:47, Mario Limonciello wrote:
> intel_pmc_core displays a warning when a suspend didn't get to a HW
> sleep state.

Where? Copy and paste is wonderful...

Re: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Greg KH]

On Thu, Nov 10, 2022 at 12:47:21AM -0600, Mario Limonciello wrote:
> +static ssize_t last_hw_state_residency_show(struct kobject *kobj,
> +		struct kobj_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%llu\n", suspend_stats.last_hw_state_residency);

sysfs_emit() please for sysfs files, not a "raw" sprintf().
checkpatch.pl should have caught that for you, but sometimes it doesn't.

thanks,

greg k-h

RE: [RFC v2 3/3] platform/x86/intel/pmc: core: Report duration of time in HW sleep state

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Sunday, November 13, 2022 17:57
> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Sven van Ashbrook
> <svenva@chromium.org>; Rafael J Wysocki <rafael@kernel.org>; linux-
> pm@vger.kernel.org; platform-driver-x86@vger.kernel.org; Rajneesh
> Bhardwaj <irenic.rajneesh@gmail.com>; David E Box
> <david.e.box@intel.com>
> Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> rrangel@chromium.org; Rajat Jain <rajatja@google.com>; Hans de Goede
> <hdegoede@redhat.com>; linux-kernel@vger.kernel.org; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Mark Gross <markgross@kernel.org>
> Subject: Re: [RFC v2 3/3] platform/x86/intel/pmc: core: Report duration of
> time in HW sleep state
> 
> On Thu, Nov 10 2022 at 00:47, Mario Limonciello wrote:
> > intel_pmc_core displays a warning when a suspend didn't get to a HW
> > sleep state.
> 
> Where? Copy and paste is wonderful...

In current mainline, drivers/platform/x86/intel/pmc/core.c line 2130 will
show such a warning.

"CPU did not enter SLP_S0!!! (S0ix cnt=%llu)"

RE: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Limonciello, Mario]

[Public]

Thanks! Appreciate the comments.
At least conceptually is there agreement to this idea for the two sysfs files
and userspace can use them to do this comparison?

A few nested replies below, but I'll clean it up for
RFC v3 or submit as PATCH v1 if there is conceptual alignment before then.

> On Thu, Nov 10 2022 at 00:47, Mario Limonciello wrote:
> 
> 'Add a sysfs files'?
> 
> Can you please decide whether that's 'a file' or 'multiple files'?

Yup thanks; bad find and replace in the commit message when I added
the second file.

> 
> > Both AMD and Intel SoCs have a concept of reporting whether the
> hardware
> > reached a hardware sleep state over s2idle as well as how much
> > time was spent in such a state.
> 
> Nice, but ...
> 
> > This information is valuable to both chip designers and system designers
> > as it helps to identify when there are problems with power consumption
> > over an s2idle cycle.
> >
> > To make the information discoverable, create a new sysfs file and a symbol
> > that drivers from supported manufacturers can use to advertise this
> > information. This file will only be exported when the system supports low
> > power idle in the ACPI table.
> >
> > In order to effectively use this information you will ideally want to
> > compare against the total duration of sleep, so export a second sysfs file
> > that will show total time. This file will be exported on all systems and
> > used both for s2idle and s3.
> 
> The above is incomprehensible word salad. Can you come up with some
> coherent explanation of what you are trying to achieve please?
> 
> > +void pm_set_hw_state_residency(u64 duration)
> > +{
> > +	suspend_stats.last_hw_state_residency = duration;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_set_hw_state_residency);
> > +
> > +void pm_account_suspend_type(const struct timespec64 *t)
> > +{
> > +	suspend_stats.last_suspend_total += (s64)t->tv_sec *
> USEC_PER_SEC +
> > +						 t->tv_nsec /
> NSEC_PER_USEC;
> 
> Conversion functions for timespecs to scalar nanoseconds exist for a
> reason. Why does this need special treatment and open code it?

Will fixup to use conversion functions.

> 
> > +}
> > +EXPORT_SYMBOL_GPL(pm_account_suspend_type);
> 
> So none of these functions has any kind of documentation. kernel-doc
> exists for a reason especially for exported functions.
> 
> That said, what's the justification to export any of these functions at
> all? AFAICT pm_account_suspend_type() is only used by builtin code...

I think you're right; they shouldn't export; will fix.

> 
> > +static umode_t suspend_attr_is_visible(struct kobject *kobj, struct
> attribute *attr, int idx)
> > +{
> > +	if (attr != &last_hw_state_residency.attr)
> > +		return 0444;
> > +#ifdef CONFIG_ACPI
> > +	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
> > +		return 0444;
> > +#endif
> > +	return 0;
> > +}
> > +
> >  static const struct attribute_group suspend_attr_group = {
> >  	.name = "suspend_stats",
> >  	.attrs = suspend_attrs,
> > +	.is_visible = suspend_attr_is_visible,
> 
> How is this change related to the changelog above? We are not hiding
> subtle changes to the existing code in some conglomorate patch. See
> Documentation/process/...

It was from feedback from RFC v1 from David Box that this file should only
be visible when s2idle is supported on the hardware.  Will adjust commit
message to make it clearer.

> 
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/compiler.h>
> >  #include <linux/audit.h>
> >  #include <linux/random.h>
> > +#include <linux/suspend.h>
> >
> >  #include "tick-internal.h"
> >  #include "ntp_internal.h"
> > @@ -1698,6 +1699,7 @@ static void
> __timekeeping_inject_sleeptime(struct timekeeper *tk,
> >  	tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic,
> *delta));
> >  	tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
> >  	tk_debug_account_sleep_time(delta);
> > +	pm_account_suspend_type(delta);
> 
> That function name is really self explaining - NOT !
> 
>      pm_account_suspend_type(delta);
> 
> So this will account a suspend type depending on the time spent in
> suspend, right?
> 
> It's totally obvious that the suspend type (whatever it is) depends on
> the time delta argument... especially when the function at hand has
> absolutely nothing to do with a type:
> 

I fat fingered this.  In my mind I thought I wrote pm_account_suspend_time()
Will fix.

> > +void pm_account_suspend_type(const struct timespec64 *t)
> > +{
> > +	suspend_stats.last_suspend_total += (s64)t->tv_sec *
> USEC_PER_SEC +
> > +						 t->tv_nsec /
> NSEC_PER_USEC;
> > +}
> 
> Sigh....
> 
> Thanks,
> 
>         tglx

Re: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Hans de Goede]

Hi Mario,

On 11/14/22 20:12, Limonciello, Mario wrote:
> [Public]
> 
> Thanks! Appreciate the comments.
> At least conceptually is there agreement to this idea for the two sysfs files
> and userspace can use them to do this comparison?

First of all let me say that I think that having some generic mechanism
which allows userspace to check if deep enough sleep-state were reached
is a good idea.  And thank you for working on this!

I wonder though if it would not be better to have some mechanism
where a list of sleep states + time spend in each time is printed ?

E.g. I know that on Intel Bay Trail and Cherry Trail devices (just an
example I'm familiar with) there are S0i0 - S0i3 and we really want
to reach S0i3 during suspend.

Sometimes on S0i1 or S0i2 is reached due to some part of the hw
not getting suspended properly.

So then we have reached "a hardware sleep state over s2idle"
but no the one we want.

OTOH I can image that if we start adding support for functionality
like standby-connect under Linux that then we may not always
reach the deepest hw sleep-state.

So I'm a bit worried that having just a single number for
last_hw_state_residency is not enough.

I think that it might be better to have a mechanism to set
a set of names for hw-states (once) and then set the residency
per state (*) after resume and have the sysfs file print
the entire list.

This list could then also always include the total suspend time,
also avoiding the need for a second sysfs file and we could also
use the same format for non s2idle suspend having it print
only the total suspend time when no hw-state names are set.

Regards,

Hans


*) Using an array, so up to MAX_HW_RESIDENCY_STATES


> 
> A few nested replies below, but I'll clean it up for
> RFC v3 or submit as PATCH v1 if there is conceptual alignment before then.
> 
>> On Thu, Nov 10 2022 at 00:47, Mario Limonciello wrote:
>>
>> 'Add a sysfs files'?
>>
>> Can you please decide whether that's 'a file' or 'multiple files'?
> 
> Yup thanks; bad find and replace in the commit message when I added
> the second file.
> 
>>
>>> Both AMD and Intel SoCs have a concept of reporting whether the
>> hardware
>>> reached a hardware sleep state over s2idle as well as how much
>>> time was spent in such a state.
>>
>> Nice, but ...
>>
>>> This information is valuable to both chip designers and system designers
>>> as it helps to identify when there are problems with power consumption
>>> over an s2idle cycle.
>>>
>>> To make the information discoverable, create a new sysfs file and a symbol
>>> that drivers from supported manufacturers can use to advertise this
>>> information. This file will only be exported when the system supports low
>>> power idle in the ACPI table.
>>>
>>> In order to effectively use this information you will ideally want to
>>> compare against the total duration of sleep, so export a second sysfs file
>>> that will show total time. This file will be exported on all systems and
>>> used both for s2idle and s3.
>>
>> The above is incomprehensible word salad. Can you come up with some
>> coherent explanation of what you are trying to achieve please?
>>
>>> +void pm_set_hw_state_residency(u64 duration)
>>> +{
>>> +	suspend_stats.last_hw_state_residency = duration;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pm_set_hw_state_residency);
>>> +
>>> +void pm_account_suspend_type(const struct timespec64 *t)
>>> +{
>>> +	suspend_stats.last_suspend_total += (s64)t->tv_sec *
>> USEC_PER_SEC +
>>> +						 t->tv_nsec /
>> NSEC_PER_USEC;
>>
>> Conversion functions for timespecs to scalar nanoseconds exist for a
>> reason. Why does this need special treatment and open code it?
> 
> Will fixup to use conversion functions.
> 
>>
>>> +}
>>> +EXPORT_SYMBOL_GPL(pm_account_suspend_type);
>>
>> So none of these functions has any kind of documentation. kernel-doc
>> exists for a reason especially for exported functions.
>>
>> That said, what's the justification to export any of these functions at
>> all? AFAICT pm_account_suspend_type() is only used by builtin code...
> 
> I think you're right; they shouldn't export; will fix.
> 
>>
>>> +static umode_t suspend_attr_is_visible(struct kobject *kobj, struct
>> attribute *attr, int idx)
>>> +{
>>> +	if (attr != &last_hw_state_residency.attr)
>>> +		return 0444;
>>> +#ifdef CONFIG_ACPI
>>> +	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
>>> +		return 0444;
>>> +#endif
>>> +	return 0;
>>> +}
>>> +
>>>  static const struct attribute_group suspend_attr_group = {
>>>  	.name = "suspend_stats",
>>>  	.attrs = suspend_attrs,
>>> +	.is_visible = suspend_attr_is_visible,
>>
>> How is this change related to the changelog above? We are not hiding
>> subtle changes to the existing code in some conglomorate patch. See
>> Documentation/process/...
> 
> It was from feedback from RFC v1 from David Box that this file should only
> be visible when s2idle is supported on the hardware.  Will adjust commit
> message to make it clearer.
> 
>>
>>> --- a/kernel/time/timekeeping.c
>>> +++ b/kernel/time/timekeeping.c
>>> @@ -24,6 +24,7 @@
>>>  #include <linux/compiler.h>
>>>  #include <linux/audit.h>
>>>  #include <linux/random.h>
>>> +#include <linux/suspend.h>
>>>
>>>  #include "tick-internal.h"
>>>  #include "ntp_internal.h"
>>> @@ -1698,6 +1699,7 @@ static void
>> __timekeeping_inject_sleeptime(struct timekeeper *tk,
>>>  	tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic,
>> *delta));
>>>  	tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
>>>  	tk_debug_account_sleep_time(delta);
>>> +	pm_account_suspend_type(delta);
>>
>> That function name is really self explaining - NOT !
>>
>>      pm_account_suspend_type(delta);
>>
>> So this will account a suspend type depending on the time spent in
>> suspend, right?
>>
>> It's totally obvious that the suspend type (whatever it is) depends on
>> the time delta argument... especially when the function at hand has
>> absolutely nothing to do with a type:
>>
> 
> I fat fingered this.  In my mind I thought I wrote pm_account_suspend_time()
> Will fix.
> 
>>> +void pm_account_suspend_type(const struct timespec64 *t)
>>> +{
>>> +	suspend_stats.last_suspend_total += (s64)t->tv_sec *
>> USEC_PER_SEC +
>>> +						 t->tv_nsec /
>> NSEC_PER_USEC;
>>> +}
>>
>> Sigh....
>>
>> Thanks,
>>
>>         tglx
> 

Re: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Limonciello, Mario]

On 11/15/2022 04:32, Hans de Goede wrote:
> Hi Mario,
> 
> On 11/14/22 20:12, Limonciello, Mario wrote:
>> [Public]
>>
>> Thanks! Appreciate the comments.
>> At least conceptually is there agreement to this idea for the two sysfs files
>> and userspace can use them to do this comparison?
> 
> First of all let me say that I think that having some generic mechanism
> which allows userspace to check if deep enough sleep-state were reached
> is a good idea.  And thank you for working on this!
> 

Sure!

> I wonder though if it would not be better to have some mechanism
> where a list of sleep states + time spend in each time is printed ?
> 
> E.g. I know that on Intel Bay Trail and Cherry Trail devices (just an
> example I'm familiar with) there are S0i0 - S0i3 and we really want
> to reach S0i3 during suspend.
> 
> Sometimes on S0i1 or S0i2 is reached due to some part of the hw
> not getting suspended properly.
> 
> So then we have reached "a hardware sleep state over s2idle"
> but no the one we want.

At least the way it's built right now it's tracking the s0ix counter for 
Intel and the s0i3 counter for AMD.

BTW - when I did all the cleanups suggested in RFC v2 I notice I was 
taking the raw number for Intel, and I have that fixed for the next version.

I don't know if other counters exist for Intel for various hardware states.
On the current AMD silicon this is the interesting metric.

> 
> OTOH I can image that if we start adding support for functionality
> like standby-connect under Linux that then we may not always
> reach the deepest hw sleep-state.

Can you elaborate what you mean by standby connect?  WoWLAN?
At least on the current AMD platforms WoWLAN can happen while the 
silicon is in the deepest hardware sleep state.

> 
> So I'm a bit worried that having just a single number for
> last_hw_state_residency is not enough.
> 
> I think that it might be better to have a mechanism to set
> a set of names for hw-states (once) and then set the residency
> per state (*) after resume and have the sysfs file print
> the entire list. >
> This list could then also always include the total suspend time,
> also avoiding the need for a second sysfs file and we could also
> use the same format for non s2idle suspend having it print
> only the total suspend time when no hw-state names are set.

So is your thought is to have a single sysfs file something like 
/sys/power/suspend_stats/s2idle_stats that would show this?

state \t % \t duration (us)
s0i3  \t 99.5% \t 1000

For AMD that would be a single line and I don't think it's worth the 
extra code.  I would like to know if it actually makes sense for Intel 
though.

We also need to think about what will be actionable with this 
information by consumers of it because I'm certain it will be leading to 
bug reports.

Let's think about a hypothetical bug report:
"Intel System only spent 20% of time in deepest hardware state".
They attach to the bug report s2idle_stats that looks like this:

state \t % \t duration (us)
s0i2  \t 80.0% \t 1000000
s0i3  \t 20.0% \t 100000

Is that any more actionable than
/sys/power/last_hw_state_residency showing 100000
and
/sys/power/suspend_total showing 500000

I think in either case the next action is more debugging will be needed, 
such as turning on dynamic debug or some module parameters.

"Practically" I expect software like systemd or powerd to be reading 
these sysfs files.

> 
> Regards,
> 
> Hans
> 
> 
> *) Using an array, so up to MAX_HW_RESIDENCY_STATES
> 
> 
>>
>> A few nested replies below, but I'll clean it up for
>> RFC v3 or submit as PATCH v1 if there is conceptual alignment before then.
>>
>>> On Thu, Nov 10 2022 at 00:47, Mario Limonciello wrote:
>>>
>>> 'Add a sysfs files'?
>>>
>>> Can you please decide whether that's 'a file' or 'multiple files'?
>>
>> Yup thanks; bad find and replace in the commit message when I added
>> the second file.
>>
>>>
>>>> Both AMD and Intel SoCs have a concept of reporting whether the
>>> hardware
>>>> reached a hardware sleep state over s2idle as well as how much
>>>> time was spent in such a state.
>>>
>>> Nice, but ...
>>>
>>>> This information is valuable to both chip designers and system designers
>>>> as it helps to identify when there are problems with power consumption
>>>> over an s2idle cycle.
>>>>
>>>> To make the information discoverable, create a new sysfs file and a symbol
>>>> that drivers from supported manufacturers can use to advertise this
>>>> information. This file will only be exported when the system supports low
>>>> power idle in the ACPI table.
>>>>
>>>> In order to effectively use this information you will ideally want to
>>>> compare against the total duration of sleep, so export a second sysfs file
>>>> that will show total time. This file will be exported on all systems and
>>>> used both for s2idle and s3.
>>>
>>> The above is incomprehensible word salad. Can you come up with some
>>> coherent explanation of what you are trying to achieve please?
>>>
>>>> +void pm_set_hw_state_residency(u64 duration)
>>>> +{
>>>> +	suspend_stats.last_hw_state_residency = duration;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pm_set_hw_state_residency);
>>>> +
>>>> +void pm_account_suspend_type(const struct timespec64 *t)
>>>> +{
>>>> +	suspend_stats.last_suspend_total += (s64)t->tv_sec *
>>> USEC_PER_SEC +
>>>> +						 t->tv_nsec /
>>> NSEC_PER_USEC;
>>>
>>> Conversion functions for timespecs to scalar nanoseconds exist for a
>>> reason. Why does this need special treatment and open code it?
>>
>> Will fixup to use conversion functions.
>>
>>>
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pm_account_suspend_type);
>>>
>>> So none of these functions has any kind of documentation. kernel-doc
>>> exists for a reason especially for exported functions.
>>>
>>> That said, what's the justification to export any of these functions at
>>> all? AFAICT pm_account_suspend_type() is only used by builtin code...
>>
>> I think you're right; they shouldn't export; will fix.
>>
>>>
>>>> +static umode_t suspend_attr_is_visible(struct kobject *kobj, struct
>>> attribute *attr, int idx)
>>>> +{
>>>> +	if (attr != &last_hw_state_residency.attr)
>>>> +		return 0444;
>>>> +#ifdef CONFIG_ACPI
>>>> +	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
>>>> +		return 0444;
>>>> +#endif
>>>> +	return 0;
>>>> +}
>>>> +
>>>>   static const struct attribute_group suspend_attr_group = {
>>>>   	.name = "suspend_stats",
>>>>   	.attrs = suspend_attrs,
>>>> +	.is_visible = suspend_attr_is_visible,
>>>
>>> How is this change related to the changelog above? We are not hiding
>>> subtle changes to the existing code in some conglomorate patch. See
>>> Documentation/process/...
>>
>> It was from feedback from RFC v1 from David Box that this file should only
>> be visible when s2idle is supported on the hardware.  Will adjust commit
>> message to make it clearer.
>>
>>>
>>>> --- a/kernel/time/timekeeping.c
>>>> +++ b/kernel/time/timekeeping.c
>>>> @@ -24,6 +24,7 @@
>>>>   #include <linux/compiler.h>
>>>>   #include <linux/audit.h>
>>>>   #include <linux/random.h>
>>>> +#include <linux/suspend.h>
>>>>
>>>>   #include "tick-internal.h"
>>>>   #include "ntp_internal.h"
>>>> @@ -1698,6 +1699,7 @@ static void
>>> __timekeeping_inject_sleeptime(struct timekeeper *tk,
>>>>   	tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic,
>>> *delta));
>>>>   	tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
>>>>   	tk_debug_account_sleep_time(delta);
>>>> +	pm_account_suspend_type(delta);
>>>
>>> That function name is really self explaining - NOT !
>>>
>>>       pm_account_suspend_type(delta);
>>>
>>> So this will account a suspend type depending on the time spent in
>>> suspend, right?
>>>
>>> It's totally obvious that the suspend type (whatever it is) depends on
>>> the time delta argument... especially when the function at hand has
>>> absolutely nothing to do with a type:
>>>
>>
>> I fat fingered this.  In my mind I thought I wrote pm_account_suspend_time()
>> Will fix.
>>
>>>> +void pm_account_suspend_type(const struct timespec64 *t)
>>>> +{
>>>> +	suspend_stats.last_suspend_total += (s64)t->tv_sec *
>>> USEC_PER_SEC +
>>>> +						 t->tv_nsec /
>>> NSEC_PER_USEC;
>>>> +}
>>>
>>> Sigh....
>>>
>>> Thanks,
>>>
>>>          tglx
>>
> 

Re: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Rafael J. Wysocki]

On Thu, Nov 10, 2022 at 7:49 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> Both AMD and Intel SoCs have a concept of reporting whether the hardware
> reached a hardware sleep state over s2idle as well as how much
> time was spent in such a state.
>
> This information is valuable to both chip designers and system designers
> as it helps to identify when there are problems with power consumption
> over an s2idle cycle.
>
> To make the information discoverable, create a new sysfs file and a symbol
> that drivers from supported manufacturers can use to advertise this
> information. This file will only be exported when the system supports low
> power idle in the ACPI table.
>
> In order to effectively use this information you will ideally want to
> compare against the total duration of sleep, so export a second sysfs file
> that will show total time. This file will be exported on all systems and
> used both for s2idle and s3.

Well, my first question would be how this is related to

/sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us

and

/sys/devices/system/cpu/cpuidle/low_power_idle_cpu_residency_us

> Suggested-by: David E Box <david.e.box@intel.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  Documentation/ABI/testing/sysfs-power | 17 +++++++++++
>  include/linux/suspend.h               |  4 +++
>  kernel/power/main.c                   | 42 +++++++++++++++++++++++++++
>  kernel/power/suspend.c                |  2 ++
>  kernel/time/timekeeping.c             |  2 ++
>  5 files changed, 67 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
> index f99d433ff311..5b47cbb4dc9e 100644
> --- a/Documentation/ABI/testing/sysfs-power
> +++ b/Documentation/ABI/testing/sysfs-power
> @@ -413,6 +413,23 @@ Description:
>                 The /sys/power/suspend_stats/last_failed_step file contains
>                 the last failed step in the suspend/resume path.
>
> +What:          /sys/power/suspend_stats/last_hw_state_residency
> +Date:          December 2022
> +Contact:       Mario Limonciello <mario.limonciello@amd.com>
> +Description:
> +               The /sys/power/suspend_stats/last_hw_state_residency file contains
> +               the amount of time spent in a hardware sleep state.
> +               This attribute is only available if the system supports
> +               low power idle.  This is measured in microseconds.
> +
> +What:          /sys/power/suspend_stats/last_suspend_total
> +Date:          December 2022
> +Contact:       Mario Limonciello <mario.limonciello@amd.com>
> +Description:
> +               The /sys/power/suspend_stats/last_suspend_total file contains
> +               the total duration of the sleep cycle.
> +               This is measured in microseconds.
> +
>  What:          /sys/power/sync_on_suspend
>  Date:          October 2019
>  Contact:       Jonas Meurer <jonas@freesources.org>
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index cfe19a028918..af343c3f8198 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -68,6 +68,8 @@ struct suspend_stats {
>         int     last_failed_errno;
>         int     errno[REC_FAILED_NUM];
>         int     last_failed_step;
> +       u64     last_hw_state_residency;
> +       u64     last_suspend_total;
>         enum suspend_stat_step  failed_steps[REC_FAILED_NUM];
>  };
>
> @@ -489,6 +491,8 @@ void restore_processor_state(void);
>  extern int register_pm_notifier(struct notifier_block *nb);
>  extern int unregister_pm_notifier(struct notifier_block *nb);
>  extern void ksys_sync_helper(void);
> +extern void pm_set_hw_state_residency(u64 duration);
> +extern void pm_account_suspend_type(const struct timespec64 *t);
>
>  #define pm_notifier(fn, pri) {                         \
>         static struct notifier_block fn##_nb =                  \
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 31ec4a9b9d70..11bd658583b0 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -6,6 +6,7 @@
>   * Copyright (c) 2003 Open Source Development Lab
>   */
>
> +#include <linux/acpi.h>
>  #include <linux/export.h>
>  #include <linux/kobject.h>
>  #include <linux/string.h>
> @@ -54,6 +55,19 @@ void unlock_system_sleep(unsigned int flags)
>  }
>  EXPORT_SYMBOL_GPL(unlock_system_sleep);
>
> +void pm_set_hw_state_residency(u64 duration)
> +{
> +       suspend_stats.last_hw_state_residency = duration;
> +}
> +EXPORT_SYMBOL_GPL(pm_set_hw_state_residency);
> +
> +void pm_account_suspend_type(const struct timespec64 *t)
> +{
> +       suspend_stats.last_suspend_total += (s64)t->tv_sec * USEC_PER_SEC +
> +                                                t->tv_nsec / NSEC_PER_USEC;
> +}
> +EXPORT_SYMBOL_GPL(pm_account_suspend_type);
> +
>  void ksys_sync_helper(void)
>  {
>         ktime_t start;
> @@ -377,6 +391,20 @@ static ssize_t last_failed_step_show(struct kobject *kobj,
>  }
>  static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
>
> +static ssize_t last_hw_state_residency_show(struct kobject *kobj,
> +               struct kobj_attribute *attr, char *buf)
> +{
> +       return sprintf(buf, "%llu\n", suspend_stats.last_hw_state_residency);
> +}
> +static struct kobj_attribute last_hw_state_residency = __ATTR_RO(last_hw_state_residency);
> +
> +static ssize_t last_suspend_total_show(struct kobject *kobj,
> +               struct kobj_attribute *attr, char *buf)
> +{
> +       return sprintf(buf, "%llu\n", suspend_stats.last_suspend_total);
> +}
> +static struct kobj_attribute last_suspend_total = __ATTR_RO(last_suspend_total);
> +
>  static struct attribute *suspend_attrs[] = {
>         &success.attr,
>         &fail.attr,
> @@ -391,12 +419,26 @@ static struct attribute *suspend_attrs[] = {
>         &last_failed_dev.attr,
>         &last_failed_errno.attr,
>         &last_failed_step.attr,
> +       &last_hw_state_residency.attr,
> +       &last_suspend_total.attr,
>         NULL,
>  };
>
> +static umode_t suspend_attr_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
> +{
> +       if (attr != &last_hw_state_residency.attr)
> +               return 0444;
> +#ifdef CONFIG_ACPI
> +       if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
> +               return 0444;
> +#endif
> +       return 0;
> +}
> +
>  static const struct attribute_group suspend_attr_group = {
>         .name = "suspend_stats",
>         .attrs = suspend_attrs,
> +       .is_visible = suspend_attr_is_visible,
>  };
>
>  #ifdef CONFIG_DEBUG_FS
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index fa3bf161d13f..b6c4a3733212 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -423,6 +423,8 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
>         if (suspend_test(TEST_PLATFORM))
>                 goto Platform_wake;
>
> +       suspend_stats.last_suspend_total = 0;
> +
>         if (state == PM_SUSPEND_TO_IDLE) {
>                 s2idle_loop();
>                 goto Platform_wake;
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index f72b9f1de178..e1b356787e53 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -24,6 +24,7 @@
>  #include <linux/compiler.h>
>  #include <linux/audit.h>
>  #include <linux/random.h>
> +#include <linux/suspend.h>
>
>  #include "tick-internal.h"
>  #include "ntp_internal.h"
> @@ -1698,6 +1699,7 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
>         tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, *delta));
>         tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
>         tk_debug_account_sleep_time(delta);
> +       pm_account_suspend_type(delta);
>  }
>
>  #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE)
> --
> 2.34.1
>

Re: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Limonciello, Mario]

On 11/15/2022 08:45, Rafael J. Wysocki wrote:
> On Thu, Nov 10, 2022 at 7:49 AM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> Both AMD and Intel SoCs have a concept of reporting whether the hardware
>> reached a hardware sleep state over s2idle as well as how much
>> time was spent in such a state.
>>
>> This information is valuable to both chip designers and system designers
>> as it helps to identify when there are problems with power consumption
>> over an s2idle cycle.
>>
>> To make the information discoverable, create a new sysfs file and a symbol
>> that drivers from supported manufacturers can use to advertise this
>> information. This file will only be exported when the system supports low
>> power idle in the ACPI table.
>>
>> In order to effectively use this information you will ideally want to
>> compare against the total duration of sleep, so export a second sysfs file
>> that will show total time. This file will be exported on all systems and
>> used both for s2idle and s3.
> 
> Well, my first question would be how this is related to
> 
> /sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us
> 

This has a dependency on the platform firmware offering an ACPI LPIT 
table.  I don't know how common that is.  As this series started from 
the needs on ChromeOS I would ask is that typically populated by coreboot?

I would hope it's the same number that is populated in that file on 
supported systems though.

> and
> 
> /sys/devices/system/cpu/cpuidle/low_power_idle_cpu_residency_us
> 

No relation to this one for what's in the series.

>> Suggested-by: David E Box <david.e.box@intel.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   Documentation/ABI/testing/sysfs-power | 17 +++++++++++
>>   include/linux/suspend.h               |  4 +++
>>   kernel/power/main.c                   | 42 +++++++++++++++++++++++++++
>>   kernel/power/suspend.c                |  2 ++
>>   kernel/time/timekeeping.c             |  2 ++
>>   5 files changed, 67 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
>> index f99d433ff311..5b47cbb4dc9e 100644
>> --- a/Documentation/ABI/testing/sysfs-power
>> +++ b/Documentation/ABI/testing/sysfs-power
>> @@ -413,6 +413,23 @@ Description:
>>                  The /sys/power/suspend_stats/last_failed_step file contains
>>                  the last failed step in the suspend/resume path.
>>
>> +What:          /sys/power/suspend_stats/last_hw_state_residency
>> +Date:          December 2022
>> +Contact:       Mario Limonciello <mario.limonciello@amd.com>
>> +Description:
>> +               The /sys/power/suspend_stats/last_hw_state_residency file contains
>> +               the amount of time spent in a hardware sleep state.
>> +               This attribute is only available if the system supports
>> +               low power idle.  This is measured in microseconds.
>> +
>> +What:          /sys/power/suspend_stats/last_suspend_total
>> +Date:          December 2022
>> +Contact:       Mario Limonciello <mario.limonciello@amd.com>
>> +Description:
>> +               The /sys/power/suspend_stats/last_suspend_total file contains
>> +               the total duration of the sleep cycle.
>> +               This is measured in microseconds.
>> +
>>   What:          /sys/power/sync_on_suspend
>>   Date:          October 2019
>>   Contact:       Jonas Meurer <jonas@freesources.org>
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index cfe19a028918..af343c3f8198 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -68,6 +68,8 @@ struct suspend_stats {
>>          int     last_failed_errno;
>>          int     errno[REC_FAILED_NUM];
>>          int     last_failed_step;
>> +       u64     last_hw_state_residency;
>> +       u64     last_suspend_total;
>>          enum suspend_stat_step  failed_steps[REC_FAILED_NUM];
>>   };
>>
>> @@ -489,6 +491,8 @@ void restore_processor_state(void);
>>   extern int register_pm_notifier(struct notifier_block *nb);
>>   extern int unregister_pm_notifier(struct notifier_block *nb);
>>   extern void ksys_sync_helper(void);
>> +extern void pm_set_hw_state_residency(u64 duration);
>> +extern void pm_account_suspend_type(const struct timespec64 *t);
>>
>>   #define pm_notifier(fn, pri) {                         \
>>          static struct notifier_block fn##_nb =                  \
>> diff --git a/kernel/power/main.c b/kernel/power/main.c
>> index 31ec4a9b9d70..11bd658583b0 100644
>> --- a/kernel/power/main.c
>> +++ b/kernel/power/main.c
>> @@ -6,6 +6,7 @@
>>    * Copyright (c) 2003 Open Source Development Lab
>>    */
>>
>> +#include <linux/acpi.h>
>>   #include <linux/export.h>
>>   #include <linux/kobject.h>
>>   #include <linux/string.h>
>> @@ -54,6 +55,19 @@ void unlock_system_sleep(unsigned int flags)
>>   }
>>   EXPORT_SYMBOL_GPL(unlock_system_sleep);
>>
>> +void pm_set_hw_state_residency(u64 duration)
>> +{
>> +       suspend_stats.last_hw_state_residency = duration;
>> +}
>> +EXPORT_SYMBOL_GPL(pm_set_hw_state_residency);
>> +
>> +void pm_account_suspend_type(const struct timespec64 *t)
>> +{
>> +       suspend_stats.last_suspend_total += (s64)t->tv_sec * USEC_PER_SEC +
>> +                                                t->tv_nsec / NSEC_PER_USEC;
>> +}
>> +EXPORT_SYMBOL_GPL(pm_account_suspend_type);
>> +
>>   void ksys_sync_helper(void)
>>   {
>>          ktime_t start;
>> @@ -377,6 +391,20 @@ static ssize_t last_failed_step_show(struct kobject *kobj,
>>   }
>>   static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
>>
>> +static ssize_t last_hw_state_residency_show(struct kobject *kobj,
>> +               struct kobj_attribute *attr, char *buf)
>> +{
>> +       return sprintf(buf, "%llu\n", suspend_stats.last_hw_state_residency);
>> +}
>> +static struct kobj_attribute last_hw_state_residency = __ATTR_RO(last_hw_state_residency);
>> +
>> +static ssize_t last_suspend_total_show(struct kobject *kobj,
>> +               struct kobj_attribute *attr, char *buf)
>> +{
>> +       return sprintf(buf, "%llu\n", suspend_stats.last_suspend_total);
>> +}
>> +static struct kobj_attribute last_suspend_total = __ATTR_RO(last_suspend_total);
>> +
>>   static struct attribute *suspend_attrs[] = {
>>          &success.attr,
>>          &fail.attr,
>> @@ -391,12 +419,26 @@ static struct attribute *suspend_attrs[] = {
>>          &last_failed_dev.attr,
>>          &last_failed_errno.attr,
>>          &last_failed_step.attr,
>> +       &last_hw_state_residency.attr,
>> +       &last_suspend_total.attr,
>>          NULL,
>>   };
>>
>> +static umode_t suspend_attr_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
>> +{
>> +       if (attr != &last_hw_state_residency.attr)
>> +               return 0444;
>> +#ifdef CONFIG_ACPI
>> +       if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
>> +               return 0444;
>> +#endif
>> +       return 0;
>> +}
>> +
>>   static const struct attribute_group suspend_attr_group = {
>>          .name = "suspend_stats",
>>          .attrs = suspend_attrs,
>> +       .is_visible = suspend_attr_is_visible,
>>   };
>>
>>   #ifdef CONFIG_DEBUG_FS
>> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
>> index fa3bf161d13f..b6c4a3733212 100644
>> --- a/kernel/power/suspend.c
>> +++ b/kernel/power/suspend.c
>> @@ -423,6 +423,8 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
>>          if (suspend_test(TEST_PLATFORM))
>>                  goto Platform_wake;
>>
>> +       suspend_stats.last_suspend_total = 0;
>> +
>>          if (state == PM_SUSPEND_TO_IDLE) {
>>                  s2idle_loop();
>>                  goto Platform_wake;
>> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
>> index f72b9f1de178..e1b356787e53 100644
>> --- a/kernel/time/timekeeping.c
>> +++ b/kernel/time/timekeeping.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/compiler.h>
>>   #include <linux/audit.h>
>>   #include <linux/random.h>
>> +#include <linux/suspend.h>
>>
>>   #include "tick-internal.h"
>>   #include "ntp_internal.h"
>> @@ -1698,6 +1699,7 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
>>          tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, *delta));
>>          tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
>>          tk_debug_account_sleep_time(delta);
>> +       pm_account_suspend_type(delta);
>>   }
>>
>>   #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE)
>> --
>> 2.34.1
>>

Re: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Rafael J. Wysocki]

On Tue, Nov 15, 2022 at 4:17 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
> On 11/15/2022 08:45, Rafael J. Wysocki wrote:
> > On Thu, Nov 10, 2022 at 7:49 AM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> Both AMD and Intel SoCs have a concept of reporting whether the hardware
> >> reached a hardware sleep state over s2idle as well as how much
> >> time was spent in such a state.
> >>
> >> This information is valuable to both chip designers and system designers
> >> as it helps to identify when there are problems with power consumption
> >> over an s2idle cycle.
> >>
> >> To make the information discoverable, create a new sysfs file and a symbol
> >> that drivers from supported manufacturers can use to advertise this
> >> information. This file will only be exported when the system supports low
> >> power idle in the ACPI table.
> >>
> >> In order to effectively use this information you will ideally want to
> >> compare against the total duration of sleep, so export a second sysfs file
> >> that will show total time. This file will be exported on all systems and
> >> used both for s2idle and s3.
> >
> > Well, my first question would be how this is related to
> >
> > /sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us
> >
>
> This has a dependency on the platform firmware offering an ACPI LPIT
> table.  I don't know how common that is.

Required for running Windows with Modern Standby AFAICS.

> As this series started from the needs on ChromeOS I would ask is that typically populated by coreboot?

It should be, but I'd need to ask for confirmation.

> I would hope it's the same number that is populated in that file on
> supported systems though.

Well, which is exactly where I'm going.

Since there is one sysfs file for exposing this value already and it
is used (for example, by sleepgraph), perhaps the way to go would be
to extend this interface to systems that don't have LPIT instead of
introducing a new one possibly exposing the same value?

> > and
> >
> > /sys/devices/system/cpu/cpuidle/low_power_idle_cpu_residency_us
> >
>
> No relation to this one for what's in the series.
>
> >> Suggested-by: David E Box <david.e.box@intel.com>
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> ---
> >>   Documentation/ABI/testing/sysfs-power | 17 +++++++++++
> >>   include/linux/suspend.h               |  4 +++
> >>   kernel/power/main.c                   | 42 +++++++++++++++++++++++++++
> >>   kernel/power/suspend.c                |  2 ++
> >>   kernel/time/timekeeping.c             |  2 ++
> >>   5 files changed, 67 insertions(+)
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
> >> index f99d433ff311..5b47cbb4dc9e 100644
> >> --- a/Documentation/ABI/testing/sysfs-power
> >> +++ b/Documentation/ABI/testing/sysfs-power
> >> @@ -413,6 +413,23 @@ Description:
> >>                  The /sys/power/suspend_stats/last_failed_step file contains
> >>                  the last failed step in the suspend/resume path.
> >>
> >> +What:          /sys/power/suspend_stats/last_hw_state_residency
> >> +Date:          December 2022
> >> +Contact:       Mario Limonciello <mario.limonciello@amd.com>
> >> +Description:
> >> +               The /sys/power/suspend_stats/last_hw_state_residency file contains
> >> +               the amount of time spent in a hardware sleep state.
> >> +               This attribute is only available if the system supports
> >> +               low power idle.  This is measured in microseconds.
> >> +
> >> +What:          /sys/power/suspend_stats/last_suspend_total
> >> +Date:          December 2022
> >> +Contact:       Mario Limonciello <mario.limonciello@amd.com>
> >> +Description:
> >> +               The /sys/power/suspend_stats/last_suspend_total file contains
> >> +               the total duration of the sleep cycle.
> >> +               This is measured in microseconds.
> >> +
> >>   What:          /sys/power/sync_on_suspend
> >>   Date:          October 2019
> >>   Contact:       Jonas Meurer <jonas@freesources.org>
> >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> >> index cfe19a028918..af343c3f8198 100644
> >> --- a/include/linux/suspend.h
> >> +++ b/include/linux/suspend.h
> >> @@ -68,6 +68,8 @@ struct suspend_stats {
> >>          int     last_failed_errno;
> >>          int     errno[REC_FAILED_NUM];
> >>          int     last_failed_step;
> >> +       u64     last_hw_state_residency;
> >> +       u64     last_suspend_total;
> >>          enum suspend_stat_step  failed_steps[REC_FAILED_NUM];
> >>   };
> >>
> >> @@ -489,6 +491,8 @@ void restore_processor_state(void);
> >>   extern int register_pm_notifier(struct notifier_block *nb);
> >>   extern int unregister_pm_notifier(struct notifier_block *nb);
> >>   extern void ksys_sync_helper(void);
> >> +extern void pm_set_hw_state_residency(u64 duration);
> >> +extern void pm_account_suspend_type(const struct timespec64 *t);
> >>
> >>   #define pm_notifier(fn, pri) {                         \
> >>          static struct notifier_block fn##_nb =                  \
> >> diff --git a/kernel/power/main.c b/kernel/power/main.c
> >> index 31ec4a9b9d70..11bd658583b0 100644
> >> --- a/kernel/power/main.c
> >> +++ b/kernel/power/main.c
> >> @@ -6,6 +6,7 @@
> >>    * Copyright (c) 2003 Open Source Development Lab
> >>    */
> >>
> >> +#include <linux/acpi.h>
> >>   #include <linux/export.h>
> >>   #include <linux/kobject.h>
> >>   #include <linux/string.h>
> >> @@ -54,6 +55,19 @@ void unlock_system_sleep(unsigned int flags)
> >>   }
> >>   EXPORT_SYMBOL_GPL(unlock_system_sleep);
> >>
> >> +void pm_set_hw_state_residency(u64 duration)
> >> +{
> >> +       suspend_stats.last_hw_state_residency = duration;
> >> +}
> >> +EXPORT_SYMBOL_GPL(pm_set_hw_state_residency);
> >> +
> >> +void pm_account_suspend_type(const struct timespec64 *t)
> >> +{
> >> +       suspend_stats.last_suspend_total += (s64)t->tv_sec * USEC_PER_SEC +
> >> +                                                t->tv_nsec / NSEC_PER_USEC;
> >> +}
> >> +EXPORT_SYMBOL_GPL(pm_account_suspend_type);
> >> +
> >>   void ksys_sync_helper(void)
> >>   {
> >>          ktime_t start;
> >> @@ -377,6 +391,20 @@ static ssize_t last_failed_step_show(struct kobject *kobj,
> >>   }
> >>   static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
> >>
> >> +static ssize_t last_hw_state_residency_show(struct kobject *kobj,
> >> +               struct kobj_attribute *attr, char *buf)
> >> +{
> >> +       return sprintf(buf, "%llu\n", suspend_stats.last_hw_state_residency);
> >> +}
> >> +static struct kobj_attribute last_hw_state_residency = __ATTR_RO(last_hw_state_residency);
> >> +
> >> +static ssize_t last_suspend_total_show(struct kobject *kobj,
> >> +               struct kobj_attribute *attr, char *buf)
> >> +{
> >> +       return sprintf(buf, "%llu\n", suspend_stats.last_suspend_total);
> >> +}
> >> +static struct kobj_attribute last_suspend_total = __ATTR_RO(last_suspend_total);
> >> +
> >>   static struct attribute *suspend_attrs[] = {
> >>          &success.attr,
> >>          &fail.attr,
> >> @@ -391,12 +419,26 @@ static struct attribute *suspend_attrs[] = {
> >>          &last_failed_dev.attr,
> >>          &last_failed_errno.attr,
> >>          &last_failed_step.attr,
> >> +       &last_hw_state_residency.attr,
> >> +       &last_suspend_total.attr,
> >>          NULL,
> >>   };
> >>
> >> +static umode_t suspend_attr_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
> >> +{
> >> +       if (attr != &last_hw_state_residency.attr)
> >> +               return 0444;
> >> +#ifdef CONFIG_ACPI
> >> +       if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
> >> +               return 0444;
> >> +#endif
> >> +       return 0;
> >> +}
> >> +
> >>   static const struct attribute_group suspend_attr_group = {
> >>          .name = "suspend_stats",
> >>          .attrs = suspend_attrs,
> >> +       .is_visible = suspend_attr_is_visible,
> >>   };
> >>
> >>   #ifdef CONFIG_DEBUG_FS
> >> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> >> index fa3bf161d13f..b6c4a3733212 100644
> >> --- a/kernel/power/suspend.c
> >> +++ b/kernel/power/suspend.c
> >> @@ -423,6 +423,8 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
> >>          if (suspend_test(TEST_PLATFORM))
> >>                  goto Platform_wake;
> >>
> >> +       suspend_stats.last_suspend_total = 0;
> >> +
> >>          if (state == PM_SUSPEND_TO_IDLE) {
> >>                  s2idle_loop();
> >>                  goto Platform_wake;
> >> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> >> index f72b9f1de178..e1b356787e53 100644
> >> --- a/kernel/time/timekeeping.c
> >> +++ b/kernel/time/timekeeping.c
> >> @@ -24,6 +24,7 @@
> >>   #include <linux/compiler.h>
> >>   #include <linux/audit.h>
> >>   #include <linux/random.h>
> >> +#include <linux/suspend.h>
> >>
> >>   #include "tick-internal.h"
> >>   #include "ntp_internal.h"
> >> @@ -1698,6 +1699,7 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
> >>          tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, *delta));
> >>          tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
> >>          tk_debug_account_sleep_time(delta);
> >> +       pm_account_suspend_type(delta);
> >>   }
> >>
> >>   #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE)
> >> --
> >> 2.34.1
> >>
>

Re: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Limonciello, Mario]

On 11/15/2022 11:20, Raul Rangel wrote:
> 
> 
> On Tue, Nov 15, 2022 at 9:35 AM Rafael J. Wysocki <rafael@kernel.org 
> <mailto:rafael@kernel.org>> wrote:
> 
>     On Tue, Nov 15, 2022 at 4:17 PM Limonciello, Mario
>     <mario.limonciello@amd.com <mailto:mario.limonciello@amd.com>> wrote:
>      >
>      > On 11/15/2022 08:45, Rafael J. Wysocki wrote:
>      > > On Thu, Nov 10, 2022 at 7:49 AM Mario Limonciello
>      > > <mario.limonciello@amd.com <mailto:mario.limonciello@amd.com>>
>     wrote:
>      > >>
>      > >> Both AMD and Intel SoCs have a concept of reporting whether
>     the hardware
>      > >> reached a hardware sleep state over s2idle as well as how much
>      > >> time was spent in such a state.
>      > >>
>      > >> This information is valuable to both chip designers and system
>     designers
>      > >> as it helps to identify when there are problems with power
>     consumption
>      > >> over an s2idle cycle.
>      > >>
>      > >> To make the information discoverable, create a new sysfs file
>     and a symbol
>      > >> that drivers from supported manufacturers can use to advertise
>     this
>      > >> information. This file will only be exported when the system
>     supports low
>      > >> power idle in the ACPI table.
>      > >>
>      > >> In order to effectively use this information you will ideally
>     want to
>      > >> compare against the total duration of sleep, so export a
>     second sysfs file
>      > >> that will show total time. This file will be exported on all
>     systems and
>      > >> used both for s2idle and s3.
>      > >
>      > > Well, my first question would be how this is related to
>      > >
>      > > /sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us
>      > >
>      >
>      > This has a dependency on the platform firmware offering an ACPI LPIT
>      > table.  I don't know how common that is.
> 
>     Required for running Windows with Modern Standby AFAICS.
> 
>      > As this series started from the needs on ChromeOS I would ask is
>     that typically populated by coreboot?
> 
>     It should be, but I'd need to ask for confirmation.
> 
> 
> It looks like Intel platforms have support for the LPIT table: 
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/coreboot/src/soc/intel/common/block/acpi/lpit.c?q=f:LPIT%20f:coreboot&ss=chromiumos <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.chromium.org%2Fchromiumos%2Fchromiumos%2Fcodesearch%2F%2B%2Fmain%3Asrc%2Fthird_party%2Fcoreboot%2Fsrc%2Fsoc%2Fintel%2Fcommon%2Fblock%2Facpi%2Flpit.c%3Fq%3Df%3ALPIT%2520f%3Acoreboot%26ss%3Dchromiumos&data=05%7C01%7Cmario.limonciello%40amd.com%7C701602845ad14f37abbb08dac72db514%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041296400209575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=9ig2jlDevXMjzmTUf42WS5Ey3rLd2lDUXjncz3mbyMI%3D&reserved=0>
> 
> For AMD, we had some patches to add _LPIL 
> https://review.coreboot.org/c/coreboot/+/52381/1 
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F52381%2F1&data=05%7C01%7Cmario.limonciello%40amd.com%7C701602845ad14f37abbb08dac72db514%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041296400209575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=KV6ASbdfNOex%2FZtJYcdItZU1gdjCIXEcP1ExiY0pkf8%3D&reserved=0>
> They never got merged though. We could add an LPIT table to coreboot for 
> AMD platforms if necessary.

_LPI I don't think makes a lot of sense on X86 today, which is why this 
was sent up:
eb087f305919e ("ACPI: processor idle: Check for architectural support 
for LPI")

As for LPIT - I've never seen LPIT on AMD UEFI systems either.  I guess 
it's an Intel specific table?

> 
>      > I would hope it's the same number that is populated in that file on
>      > supported systems though.
> 
>     Well, which is exactly where I'm going.
> 
>     Since there is one sysfs file for exposing this value already and it
>     is used (for example, by sleepgraph), perhaps the way to go would be
>     to extend this interface to systems that don't have LPIT instead of
>     introducing a new one possibly exposing the same value?
> 

Ah; so since Raul confirmed coreboot on Chrome exports that maybe we 
just need to add another way to populate that sysfs file for systems 
without LPIT (IE AMD).  I think that's a very good idea; thanks.

I think we still probably want to have a way to get the total suspend 
time out programmatically though to compare to.  So perhaps the other 
sysfs file I had in the RFC v2 makes sense still.

>      > > and
>      > >
>      > > /sys/devices/system/cpu/cpuidle/low_power_idle_cpu_residency_us
>      > >
>      >
>      > No relation to this one for what's in the series.
>      >
>      > >> Suggested-by: David E Box <david.e.box@intel.com
>     <mailto:david.e.box@intel.com>>
>      > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com
>     <mailto:mario.limonciello@amd.com>>
>      > >> ---
>      > >>   Documentation/ABI/testing/sysfs-power | 17 +++++++++++
>      > >>   include/linux/suspend.h               |  4 +++
>      > >>   kernel/power/main.c                   | 42
>     +++++++++++++++++++++++++++
>      > >>   kernel/power/suspend.c                |  2 ++
>      > >>   kernel/time/timekeeping.c             |  2 ++
>      > >>   5 files changed, 67 insertions(+)
>      > >>
>      > >> diff --git a/Documentation/ABI/testing/sysfs-power
>     b/Documentation/ABI/testing/sysfs-power
>      > >> index f99d433ff311..5b47cbb4dc9e 100644
>      > >> --- a/Documentation/ABI/testing/sysfs-power
>      > >> +++ b/Documentation/ABI/testing/sysfs-power
>      > >> @@ -413,6 +413,23 @@ Description:
>      > >>                  The /sys/power/suspend_stats/last_failed_step
>     file contains
>      > >>                  the last failed step in the suspend/resume path.
>      > >>
>      > >> +What:          /sys/power/suspend_stats/last_hw_state_residency
>      > >> +Date:          December 2022
>      > >> +Contact:       Mario Limonciello <mario.limonciello@amd.com
>     <mailto:mario.limonciello@amd.com>>
>      > >> +Description:
>      > >> +               The
>     /sys/power/suspend_stats/last_hw_state_residency file contains
>      > >> +               the amount of time spent in a hardware sleep
>     state.
>      > >> +               This attribute is only available if the system
>     supports
>      > >> +               low power idle.  This is measured in microseconds.
>      > >> +
>      > >> +What:          /sys/power/suspend_stats/last_suspend_total
>      > >> +Date:          December 2022
>      > >> +Contact:       Mario Limonciello <mario.limonciello@amd.com
>     <mailto:mario.limonciello@amd.com>>
>      > >> +Description:
>      > >> +               The
>     /sys/power/suspend_stats/last_suspend_total file contains
>      > >> +               the total duration of the sleep cycle.
>      > >> +               This is measured in microseconds.
>      > >> +
>      > >>   What:          /sys/power/sync_on_suspend
>      > >>   Date:          October 2019
>      > >>   Contact:       Jonas Meurer <jonas@freesources.org
>     <mailto:jonas@freesources.org>>
>      > >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>      > >> index cfe19a028918..af343c3f8198 100644
>      > >> --- a/include/linux/suspend.h
>      > >> +++ b/include/linux/suspend.h
>      > >> @@ -68,6 +68,8 @@ struct suspend_stats {
>      > >>          int     last_failed_errno;
>      > >>          int     errno[REC_FAILED_NUM];
>      > >>          int     last_failed_step;
>      > >> +       u64     last_hw_state_residency;
>      > >> +       u64     last_suspend_total;
>      > >>          enum suspend_stat_step  failed_steps[REC_FAILED_NUM];
>      > >>   };
>      > >>
>      > >> @@ -489,6 +491,8 @@ void restore_processor_state(void);
>      > >>   extern int register_pm_notifier(struct notifier_block *nb);
>      > >>   extern int unregister_pm_notifier(struct notifier_block *nb);
>      > >>   extern void ksys_sync_helper(void);
>      > >> +extern void pm_set_hw_state_residency(u64 duration);
>      > >> +extern void pm_account_suspend_type(const struct timespec64 *t);
>      > >>
>      > >>   #define pm_notifier(fn, pri) {                         \
>      > >>          static struct notifier_block fn##_nb =                  \
>      > >> diff --git a/kernel/power/main.c b/kernel/power/main.c
>      > >> index 31ec4a9b9d70..11bd658583b0 100644
>      > >> --- a/kernel/power/main.c
>      > >> +++ b/kernel/power/main.c
>      > >> @@ -6,6 +6,7 @@
>      > >>    * Copyright (c) 2003 Open Source Development Lab
>      > >>    */
>      > >>
>      > >> +#include <linux/acpi.h>
>      > >>   #include <linux/export.h>
>      > >>   #include <linux/kobject.h>
>      > >>   #include <linux/string.h>
>      > >> @@ -54,6 +55,19 @@ void unlock_system_sleep(unsigned int flags)
>      > >>   }
>      > >>   EXPORT_SYMBOL_GPL(unlock_system_sleep);
>      > >>
>      > >> +void pm_set_hw_state_residency(u64 duration)
>      > >> +{
>      > >> +       suspend_stats.last_hw_state_residency = duration;
>      > >> +}
>      > >> +EXPORT_SYMBOL_GPL(pm_set_hw_state_residency);
>      > >> +
>      > >> +void pm_account_suspend_type(const struct timespec64 *t)
>      > >> +{
>      > >> +       suspend_stats.last_suspend_total += (s64)t->tv_sec *
>     USEC_PER_SEC +
>      > >> +                                                t->tv_nsec /
>     NSEC_PER_USEC;
>      > >> +}
>      > >> +EXPORT_SYMBOL_GPL(pm_account_suspend_type);
>      > >> +
>      > >>   void ksys_sync_helper(void)
>      > >>   {
>      > >>          ktime_t start;
>      > >> @@ -377,6 +391,20 @@ static ssize_t
>     last_failed_step_show(struct kobject *kobj,
>      > >>   }
>      > >>   static struct kobj_attribute last_failed_step =
>     __ATTR_RO(last_failed_step);
>      > >>
>      > >> +static ssize_t last_hw_state_residency_show(struct kobject *kobj,
>      > >> +               struct kobj_attribute *attr, char *buf)
>      > >> +{
>      > >> +       return sprintf(buf, "%llu\n",
>     suspend_stats.last_hw_state_residency);
>      > >> +}
>      > >> +static struct kobj_attribute last_hw_state_residency =
>     __ATTR_RO(last_hw_state_residency);
>      > >> +
>      > >> +static ssize_t last_suspend_total_show(struct kobject *kobj,
>      > >> +               struct kobj_attribute *attr, char *buf)
>      > >> +{
>      > >> +       return sprintf(buf, "%llu\n",
>     suspend_stats.last_suspend_total);
>      > >> +}
>      > >> +static struct kobj_attribute last_suspend_total =
>     __ATTR_RO(last_suspend_total);
>      > >> +
>      > >>   static struct attribute *suspend_attrs[] = {
>      > >>          &success.attr,
>      > >>          &fail.attr,
>      > >> @@ -391,12 +419,26 @@ static struct attribute *suspend_attrs[] = {
>      > >>          &last_failed_dev.attr,
>      > >>          &last_failed_errno.attr,
>      > >>          &last_failed_step.attr,
>      > >> +       &last_hw_state_residency.attr,
>      > >> +       &last_suspend_total.attr,
>      > >>          NULL,
>      > >>   };
>      > >>
>      > >> +static umode_t suspend_attr_is_visible(struct kobject *kobj,
>     struct attribute *attr, int idx)
>      > >> +{
>      > >> +       if (attr != &last_hw_state_residency.attr)
>      > >> +               return 0444;
>      > >> +#ifdef CONFIG_ACPI
>      > >> +       if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
>      > >> +               return 0444;
>      > >> +#endif
>      > >> +       return 0;
>      > >> +}
>      > >> +
>      > >>   static const struct attribute_group suspend_attr_group = {
>      > >>          .name = "suspend_stats",
>      > >>          .attrs = suspend_attrs,
>      > >> +       .is_visible = suspend_attr_is_visible,
>      > >>   };
>      > >>
>      > >>   #ifdef CONFIG_DEBUG_FS
>      > >> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
>      > >> index fa3bf161d13f..b6c4a3733212 100644
>      > >> --- a/kernel/power/suspend.c
>      > >> +++ b/kernel/power/suspend.c
>      > >> @@ -423,6 +423,8 @@ static int suspend_enter(suspend_state_t
>     state, bool *wakeup)
>      > >>          if (suspend_test(TEST_PLATFORM))
>      > >>                  goto Platform_wake;
>      > >>
>      > >> +       suspend_stats.last_suspend_total = 0;
>      > >> +
>      > >>          if (state == PM_SUSPEND_TO_IDLE) {
>      > >>                  s2idle_loop();
>      > >>                  goto Platform_wake;
>      > >> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
>      > >> index f72b9f1de178..e1b356787e53 100644
>      > >> --- a/kernel/time/timekeeping.c
>      > >> +++ b/kernel/time/timekeeping.c
>      > >> @@ -24,6 +24,7 @@
>      > >>   #include <linux/compiler.h>
>      > >>   #include <linux/audit.h>
>      > >>   #include <linux/random.h>
>      > >> +#include <linux/suspend.h>
>      > >>
>      > >>   #include "tick-internal.h"
>      > >>   #include "ntp_internal.h"
>      > >> @@ -1698,6 +1699,7 @@ static void
>     __timekeeping_inject_sleeptime(struct timekeeper *tk,
>      > >>          tk_set_wall_to_mono(tk,
>     timespec64_sub(tk->wall_to_monotonic, *delta));
>      > >>          tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
>      > >>          tk_debug_account_sleep_time(delta);
>      > >> +       pm_account_suspend_type(delta);
>      > >>   }
>      > >>
>      > >>   #if defined(CONFIG_PM_SLEEP) &&
>     defined(CONFIG_RTC_HCTOSYS_DEVICE)
>      > >> --
>      > >> 2.34.1
>      > >>
>      >
> 

Re: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Rafael J. Wysocki]

On Tue, Nov 15, 2022 at 6:27 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
> On 11/15/2022 11:20, Raul Rangel wrote:
> >
> >
> > On Tue, Nov 15, 2022 at 9:35 AM Rafael J. Wysocki <rafael@kernel.org
> > <mailto:rafael@kernel.org>> wrote:
> >
> >     On Tue, Nov 15, 2022 at 4:17 PM Limonciello, Mario
> >     <mario.limonciello@amd.com <mailto:mario.limonciello@amd.com>> wrote:
> >      >
> >      > On 11/15/2022 08:45, Rafael J. Wysocki wrote:
> >      > > On Thu, Nov 10, 2022 at 7:49 AM Mario Limonciello
> >      > > <mario.limonciello@amd.com <mailto:mario.limonciello@amd.com>>
> >     wrote:
> >      > >>
> >      > >> Both AMD and Intel SoCs have a concept of reporting whether
> >     the hardware
> >      > >> reached a hardware sleep state over s2idle as well as how much
> >      > >> time was spent in such a state.
> >      > >>
> >      > >> This information is valuable to both chip designers and system
> >     designers
> >      > >> as it helps to identify when there are problems with power
> >     consumption
> >      > >> over an s2idle cycle.
> >      > >>
> >      > >> To make the information discoverable, create a new sysfs file
> >     and a symbol
> >      > >> that drivers from supported manufacturers can use to advertise
> >     this
> >      > >> information. This file will only be exported when the system
> >     supports low
> >      > >> power idle in the ACPI table.
> >      > >>
> >      > >> In order to effectively use this information you will ideally
> >     want to
> >      > >> compare against the total duration of sleep, so export a
> >     second sysfs file
> >      > >> that will show total time. This file will be exported on all
> >     systems and
> >      > >> used both for s2idle and s3.
> >      > >
> >      > > Well, my first question would be how this is related to
> >      > >
> >      > > /sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us
> >      > >
> >      >
> >      > This has a dependency on the platform firmware offering an ACPI LPIT
> >      > table.  I don't know how common that is.
> >
> >     Required for running Windows with Modern Standby AFAICS.
> >
> >      > As this series started from the needs on ChromeOS I would ask is
> >     that typically populated by coreboot?
> >
> >     It should be, but I'd need to ask for confirmation.
> >
> >
> > It looks like Intel platforms have support for the LPIT table:
> > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/coreboot/src/soc/intel/common/block/acpi/lpit.c?q=f:LPIT%20f:coreboot&ss=chromiumos <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.chromium.org%2Fchromiumos%2Fchromiumos%2Fcodesearch%2F%2B%2Fmain%3Asrc%2Fthird_party%2Fcoreboot%2Fsrc%2Fsoc%2Fintel%2Fcommon%2Fblock%2Facpi%2Flpit.c%3Fq%3Df%3ALPIT%2520f%3Acoreboot%26ss%3Dchromiumos&data=05%7C01%7Cmario.limonciello%40amd.com%7C701602845ad14f37abbb08dac72db514%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041296400209575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=9ig2jlDevXMjzmTUf42WS5Ey3rLd2lDUXjncz3mbyMI%3D&reserved=0>
> >
> > For AMD, we had some patches to add _LPIL
> > https://review.coreboot.org/c/coreboot/+/52381/1
> > <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F52381%2F1&data=05%7C01%7Cmario.limonciello%40amd.com%7C701602845ad14f37abbb08dac72db514%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041296400209575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=KV6ASbdfNOex%2FZtJYcdItZU1gdjCIXEcP1ExiY0pkf8%3D&reserved=0>
> > They never got merged though. We could add an LPIT table to coreboot for
> > AMD platforms if necessary.
>
> _LPI I don't think makes a lot of sense on X86 today, which is why this
> was sent up:
> eb087f305919e ("ACPI: processor idle: Check for architectural support
> for LPI")

Well, LPI has nothing to do with LPIT.  [I guess this could not be
even more confusing, but that's what you get in the world of 4-letter
TLAs.]

> As for LPIT - I've never seen LPIT on AMD UEFI systems either.  I guess
> it's an Intel specific table?

It used to be.  The spec is UEFI-hosted now.

> >
> >      > I would hope it's the same number that is populated in that file on
> >      > supported systems though.
> >
> >     Well, which is exactly where I'm going.
> >
> >     Since there is one sysfs file for exposing this value already and it
> >     is used (for example, by sleepgraph), perhaps the way to go would be
> >     to extend this interface to systems that don't have LPIT instead of
> >     introducing a new one possibly exposing the same value?
> >
>
> Ah; so since Raul confirmed coreboot on Chrome exports that maybe we
> just need to add another way to populate that sysfs file for systems
> without LPIT (IE AMD).  I think that's a very good idea; thanks.
>
> I think we still probably want to have a way to get the total suspend
> time out programmatically though to compare to.  So perhaps the other
> sysfs file I had in the RFC v2 makes sense still.

Well there are trace points to get that (sleepgraph uses these too),
see Documentation/trace/events-power.rst (and you can git grep for
"machine_suspend" to find where this comes from).

I guess there could be a sysfs file in addition to them, but I'm not
sure if the extra overhead would be worth the benefit.

> >      > > and
> >      > >
> >      > > /sys/devices/system/cpu/cpuidle/low_power_idle_cpu_residency_us
> >      > >
> >      >
> >      > No relation to this one for what's in the series.
> >      >
> >      > >> Suggested-by: David E Box <david.e.box@intel.com
> >     <mailto:david.e.box@intel.com>>
> >      > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com
> >     <mailto:mario.limonciello@amd.com>>
> >      > >> ---
> >      > >>   Documentation/ABI/testing/sysfs-power | 17 +++++++++++
> >      > >>   include/linux/suspend.h               |  4 +++
> >      > >>   kernel/power/main.c                   | 42
> >     +++++++++++++++++++++++++++
> >      > >>   kernel/power/suspend.c                |  2 ++
> >      > >>   kernel/time/timekeeping.c             |  2 ++
> >      > >>   5 files changed, 67 insertions(+)
> >      > >>
> >      > >> diff --git a/Documentation/ABI/testing/sysfs-power
> >     b/Documentation/ABI/testing/sysfs-power
> >      > >> index f99d433ff311..5b47cbb4dc9e 100644
> >      > >> --- a/Documentation/ABI/testing/sysfs-power
> >      > >> +++ b/Documentation/ABI/testing/sysfs-power
> >      > >> @@ -413,6 +413,23 @@ Description:
> >      > >>                  The /sys/power/suspend_stats/last_failed_step
> >     file contains
> >      > >>                  the last failed step in the suspend/resume path.
> >      > >>
> >      > >> +What:          /sys/power/suspend_stats/last_hw_state_residency
> >      > >> +Date:          December 2022
> >      > >> +Contact:       Mario Limonciello <mario.limonciello@amd.com
> >     <mailto:mario.limonciello@amd.com>>
> >      > >> +Description:
> >      > >> +               The
> >     /sys/power/suspend_stats/last_hw_state_residency file contains
> >      > >> +               the amount of time spent in a hardware sleep
> >     state.
> >      > >> +               This attribute is only available if the system
> >     supports
> >      > >> +               low power idle.  This is measured in microseconds.
> >      > >> +
> >      > >> +What:          /sys/power/suspend_stats/last_suspend_total
> >      > >> +Date:          December 2022
> >      > >> +Contact:       Mario Limonciello <mario.limonciello@amd.com
> >     <mailto:mario.limonciello@amd.com>>
> >      > >> +Description:
> >      > >> +               The
> >     /sys/power/suspend_stats/last_suspend_total file contains
> >      > >> +               the total duration of the sleep cycle.
> >      > >> +               This is measured in microseconds.
> >      > >> +
> >      > >>   What:          /sys/power/sync_on_suspend
> >      > >>   Date:          October 2019
> >      > >>   Contact:       Jonas Meurer <jonas@freesources.org
> >     <mailto:jonas@freesources.org>>
> >      > >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> >      > >> index cfe19a028918..af343c3f8198 100644
> >      > >> --- a/include/linux/suspend.h
> >      > >> +++ b/include/linux/suspend.h
> >      > >> @@ -68,6 +68,8 @@ struct suspend_stats {
> >      > >>          int     last_failed_errno;
> >      > >>          int     errno[REC_FAILED_NUM];
> >      > >>          int     last_failed_step;
> >      > >> +       u64     last_hw_state_residency;
> >      > >> +       u64     last_suspend_total;
> >      > >>          enum suspend_stat_step  failed_steps[REC_FAILED_NUM];
> >      > >>   };
> >      > >>
> >      > >> @@ -489,6 +491,8 @@ void restore_processor_state(void);
> >      > >>   extern int register_pm_notifier(struct notifier_block *nb);
> >      > >>   extern int unregister_pm_notifier(struct notifier_block *nb);
> >      > >>   extern void ksys_sync_helper(void);
> >      > >> +extern void pm_set_hw_state_residency(u64 duration);
> >      > >> +extern void pm_account_suspend_type(const struct timespec64 *t);
> >      > >>
> >      > >>   #define pm_notifier(fn, pri) {                         \
> >      > >>          static struct notifier_block fn##_nb =                  \
> >      > >> diff --git a/kernel/power/main.c b/kernel/power/main.c
> >      > >> index 31ec4a9b9d70..11bd658583b0 100644
> >      > >> --- a/kernel/power/main.c
> >      > >> +++ b/kernel/power/main.c
> >      > >> @@ -6,6 +6,7 @@
> >      > >>    * Copyright (c) 2003 Open Source Development Lab
> >      > >>    */
> >      > >>
> >      > >> +#include <linux/acpi.h>
> >      > >>   #include <linux/export.h>
> >      > >>   #include <linux/kobject.h>
> >      > >>   #include <linux/string.h>
> >      > >> @@ -54,6 +55,19 @@ void unlock_system_sleep(unsigned int flags)
> >      > >>   }
> >      > >>   EXPORT_SYMBOL_GPL(unlock_system_sleep);
> >      > >>
> >      > >> +void pm_set_hw_state_residency(u64 duration)
> >      > >> +{
> >      > >> +       suspend_stats.last_hw_state_residency = duration;
> >      > >> +}
> >      > >> +EXPORT_SYMBOL_GPL(pm_set_hw_state_residency);
> >      > >> +
> >      > >> +void pm_account_suspend_type(const struct timespec64 *t)
> >      > >> +{
> >      > >> +       suspend_stats.last_suspend_total += (s64)t->tv_sec *
> >     USEC_PER_SEC +
> >      > >> +                                                t->tv_nsec /
> >     NSEC_PER_USEC;
> >      > >> +}
> >      > >> +EXPORT_SYMBOL_GPL(pm_account_suspend_type);
> >      > >> +
> >      > >>   void ksys_sync_helper(void)
> >      > >>   {
> >      > >>          ktime_t start;
> >      > >> @@ -377,6 +391,20 @@ static ssize_t
> >     last_failed_step_show(struct kobject *kobj,
> >      > >>   }
> >      > >>   static struct kobj_attribute last_failed_step =
> >     __ATTR_RO(last_failed_step);
> >      > >>
> >      > >> +static ssize_t last_hw_state_residency_show(struct kobject *kobj,
> >      > >> +               struct kobj_attribute *attr, char *buf)
> >      > >> +{
> >      > >> +       return sprintf(buf, "%llu\n",
> >     suspend_stats.last_hw_state_residency);
> >      > >> +}
> >      > >> +static struct kobj_attribute last_hw_state_residency =
> >     __ATTR_RO(last_hw_state_residency);
> >      > >> +
> >      > >> +static ssize_t last_suspend_total_show(struct kobject *kobj,
> >      > >> +               struct kobj_attribute *attr, char *buf)
> >      > >> +{
> >      > >> +       return sprintf(buf, "%llu\n",
> >     suspend_stats.last_suspend_total);
> >      > >> +}
> >      > >> +static struct kobj_attribute last_suspend_total =
> >     __ATTR_RO(last_suspend_total);
> >      > >> +
> >      > >>   static struct attribute *suspend_attrs[] = {
> >      > >>          &success.attr,
> >      > >>          &fail.attr,
> >      > >> @@ -391,12 +419,26 @@ static struct attribute *suspend_attrs[] = {
> >      > >>          &last_failed_dev.attr,
> >      > >>          &last_failed_errno.attr,
> >      > >>          &last_failed_step.attr,
> >      > >> +       &last_hw_state_residency.attr,
> >      > >> +       &last_suspend_total.attr,
> >      > >>          NULL,
> >      > >>   };
> >      > >>
> >      > >> +static umode_t suspend_attr_is_visible(struct kobject *kobj,
> >     struct attribute *attr, int idx)
> >      > >> +{
> >      > >> +       if (attr != &last_hw_state_residency.attr)
> >      > >> +               return 0444;
> >      > >> +#ifdef CONFIG_ACPI
> >      > >> +       if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
> >      > >> +               return 0444;
> >      > >> +#endif
> >      > >> +       return 0;
> >      > >> +}
> >      > >> +
> >      > >>   static const struct attribute_group suspend_attr_group = {
> >      > >>          .name = "suspend_stats",
> >      > >>          .attrs = suspend_attrs,
> >      > >> +       .is_visible = suspend_attr_is_visible,
> >      > >>   };
> >      > >>
> >      > >>   #ifdef CONFIG_DEBUG_FS
> >      > >> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> >      > >> index fa3bf161d13f..b6c4a3733212 100644
> >      > >> --- a/kernel/power/suspend.c
> >      > >> +++ b/kernel/power/suspend.c
> >      > >> @@ -423,6 +423,8 @@ static int suspend_enter(suspend_state_t
> >     state, bool *wakeup)
> >      > >>          if (suspend_test(TEST_PLATFORM))
> >      > >>                  goto Platform_wake;
> >      > >>
> >      > >> +       suspend_stats.last_suspend_total = 0;
> >      > >> +
> >      > >>          if (state == PM_SUSPEND_TO_IDLE) {
> >      > >>                  s2idle_loop();
> >      > >>                  goto Platform_wake;
> >      > >> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> >      > >> index f72b9f1de178..e1b356787e53 100644
> >      > >> --- a/kernel/time/timekeeping.c
> >      > >> +++ b/kernel/time/timekeeping.c
> >      > >> @@ -24,6 +24,7 @@
> >      > >>   #include <linux/compiler.h>
> >      > >>   #include <linux/audit.h>
> >      > >>   #include <linux/random.h>
> >      > >> +#include <linux/suspend.h>
> >      > >>
> >      > >>   #include "tick-internal.h"
> >      > >>   #include "ntp_internal.h"
> >      > >> @@ -1698,6 +1699,7 @@ static void
> >     __timekeeping_inject_sleeptime(struct timekeeper *tk,
> >      > >>          tk_set_wall_to_mono(tk,
> >     timespec64_sub(tk->wall_to_monotonic, *delta));
> >      > >>          tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
> >      > >>          tk_debug_account_sleep_time(delta);
> >      > >> +       pm_account_suspend_type(delta);
> >      > >>   }
> >      > >>
> >      > >>   #if defined(CONFIG_PM_SLEEP) &&
> >     defined(CONFIG_RTC_HCTOSYS_DEVICE)
> >      > >> --
> >      > >> 2.34.1
> >      > >>
> >      >
> >
>

Re: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Limonciello, Mario]

On 11/15/2022 11:52, Rafael J. Wysocki wrote:
> On Tue, Nov 15, 2022 at 6:27 PM Limonciello, Mario
> <mario.limonciello@amd.com> wrote:
>>
>> On 11/15/2022 11:20, Raul Rangel wrote:
>>>
>>>
>>> On Tue, Nov 15, 2022 at 9:35 AM Rafael J. Wysocki <rafael@kernel.org
>>> <mailto:rafael@kernel.org>> wrote:
>>>
>>>      On Tue, Nov 15, 2022 at 4:17 PM Limonciello, Mario
>>>      <mario.limonciello@amd.com <mailto:mario.limonciello@amd.com>> wrote:
>>>       >
>>>       > On 11/15/2022 08:45, Rafael J. Wysocki wrote:
>>>       > > On Thu, Nov 10, 2022 at 7:49 AM Mario Limonciello
>>>       > > <mario.limonciello@amd.com <mailto:mario.limonciello@amd.com>>
>>>      wrote:
>>>       > >>
>>>       > >> Both AMD and Intel SoCs have a concept of reporting whether
>>>      the hardware
>>>       > >> reached a hardware sleep state over s2idle as well as how much
>>>       > >> time was spent in such a state.
>>>       > >>
>>>       > >> This information is valuable to both chip designers and system
>>>      designers
>>>       > >> as it helps to identify when there are problems with power
>>>      consumption
>>>       > >> over an s2idle cycle.
>>>       > >>
>>>       > >> To make the information discoverable, create a new sysfs file
>>>      and a symbol
>>>       > >> that drivers from supported manufacturers can use to advertise
>>>      this
>>>       > >> information. This file will only be exported when the system
>>>      supports low
>>>       > >> power idle in the ACPI table.
>>>       > >>
>>>       > >> In order to effectively use this information you will ideally
>>>      want to
>>>       > >> compare against the total duration of sleep, so export a
>>>      second sysfs file
>>>       > >> that will show total time. This file will be exported on all
>>>      systems and
>>>       > >> used both for s2idle and s3.
>>>       > >
>>>       > > Well, my first question would be how this is related to
>>>       > >
>>>       > > /sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us
>>>       > >
>>>       >
>>>       > This has a dependency on the platform firmware offering an ACPI LPIT
>>>       > table.  I don't know how common that is.
>>>
>>>      Required for running Windows with Modern Standby AFAICS.
>>>
>>>       > As this series started from the needs on ChromeOS I would ask is
>>>      that typically populated by coreboot?
>>>
>>>      It should be, but I'd need to ask for confirmation.
>>>
>>>
>>> It looks like Intel platforms have support for the LPIT table:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.chromium.org%2Fchromiumos%2Fchromiumos%2Fcodesearch%2F%2B%2Fmain%3Asrc%2Fthird_party%2Fcoreboot%2Fsrc%2Fsoc%2Fintel%2Fcommon%2Fblock%2Facpi%2Flpit.c%3Fq%3Df%3ALPIT%2520f%3Acoreboot%26ss%3Dchromiumos&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7C37e6dda56f924fe641f008dac7323c01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041315852648377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=PusftlebIMFtbaMy1XkBjHFMXLjdOzt7hA%2Fm3AM7v7A%3D&amp;reserved=0 <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.chromium.org%2Fchromiumos%2Fchromiumos%2Fcodesearch%2F%2B%2Fmain%3Asrc%2Fthird_party%2Fcoreboot%2Fsrc%2Fsoc%2Fintel%2Fcommon%2Fblock%2Facpi%2Flpit.c%3Fq%3Df%3ALPIT%2520f%3Acoreboot%26ss%3Dchromiumos&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7C37e6dda56f924fe641f008dac7323c01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041315852648377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=PusftlebIMFtbaMy1XkBjHFMXLjdOzt7hA%2Fm3AM7v7A%3D&amp;reserved=0>
>>>
>>> For AMD, we had some patches to add _LPIL
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F52381%2F1&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7C37e6dda56f924fe641f008dac7323c01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041315852648377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=gUYdMWZBNVALF8Xzhgswlyw9hCUv7LQ6eomz6gfIYrk%3D&amp;reserved=0
>>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F52381%2F1&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7C37e6dda56f924fe641f008dac7323c01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041315852648377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=gUYdMWZBNVALF8Xzhgswlyw9hCUv7LQ6eomz6gfIYrk%3D&amp;reserved=0>
>>> They never got merged though. We could add an LPIT table to coreboot for
>>> AMD platforms if necessary.
>>
>> _LPI I don't think makes a lot of sense on X86 today, which is why this
>> was sent up:
>> eb087f305919e ("ACPI: processor idle: Check for architectural support
>> for LPI")
> 
> Well, LPI has nothing to do with LPIT.  [I guess this could not be
> even more confusing, but that's what you get in the world of 4-letter
> TLAs.]
> 
>> As for LPIT - I've never seen LPIT on AMD UEFI systems either.  I guess
>> it's an Intel specific table?
> 
> It used to be.  The spec is UEFI-hosted now.
> 

Got it.

>>>
>>>       > I would hope it's the same number that is populated in that file on
>>>       > supported systems though.
>>>
>>>      Well, which is exactly where I'm going.
>>>
>>>      Since there is one sysfs file for exposing this value already and it
>>>      is used (for example, by sleepgraph), perhaps the way to go would be
>>>      to extend this interface to systems that don't have LPIT instead of
>>>      introducing a new one possibly exposing the same value?
>>>
>>
>> Ah; so since Raul confirmed coreboot on Chrome exports that maybe we
>> just need to add another way to populate that sysfs file for systems
>> without LPIT (IE AMD).  I think that's a very good idea; thanks.
>>
>> I think we still probably want to have a way to get the total suspend
>> time out programmatically though to compare to.  So perhaps the other
>> sysfs file I had in the RFC v2 makes sense still.
> 
> Well there are trace points to get that (sleepgraph uses these too),
> see Documentation/trace/events-power.rst (and you can git grep for
> "machine_suspend" to find where this comes from).
> 
> I guess there could be a sysfs file in addition to them, but I'm not
> sure if the extra overhead would be worth the benefit.

At least the way that I envisioned this all working was that userspace 
software that wanted to could query some sysfs files and figure out a 
percentage of time spent.  If it was below a threshold users could be 
notified, or logs can be sent up to a server for analysis etc.

Trace points would mean that userspace software like systemd and powerd 
would need to turn on the tracing every time to get the raw total 
numbers to do such a comparison.

> 
>>>       > > and
>>>       > >
>>>       > > /sys/devices/system/cpu/cpuidle/low_power_idle_cpu_residency_us
>>>       > >
>>>       >
>>>       > No relation to this one for what's in the series.
>>>       >
>>>       > >> Suggested-by: David E Box <david.e.box@intel.com
>>>      <mailto:david.e.box@intel.com>>
>>>       > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com
>>>      <mailto:mario.limonciello@amd.com>>
>>>       > >> ---
>>>       > >>   Documentation/ABI/testing/sysfs-power | 17 +++++++++++
>>>       > >>   include/linux/suspend.h               |  4 +++
>>>       > >>   kernel/power/main.c                   | 42
>>>      +++++++++++++++++++++++++++
>>>       > >>   kernel/power/suspend.c                |  2 ++
>>>       > >>   kernel/time/timekeeping.c             |  2 ++
>>>       > >>   5 files changed, 67 insertions(+)
>>>       > >>
>>>       > >> diff --git a/Documentation/ABI/testing/sysfs-power
>>>      b/Documentation/ABI/testing/sysfs-power
>>>       > >> index f99d433ff311..5b47cbb4dc9e 100644
>>>       > >> --- a/Documentation/ABI/testing/sysfs-power
>>>       > >> +++ b/Documentation/ABI/testing/sysfs-power
>>>       > >> @@ -413,6 +413,23 @@ Description:
>>>       > >>                  The /sys/power/suspend_stats/last_failed_step
>>>      file contains
>>>       > >>                  the last failed step in the suspend/resume path.
>>>       > >>
>>>       > >> +What:          /sys/power/suspend_stats/last_hw_state_residency
>>>       > >> +Date:          December 2022
>>>       > >> +Contact:       Mario Limonciello <mario.limonciello@amd.com
>>>      <mailto:mario.limonciello@amd.com>>
>>>       > >> +Description:
>>>       > >> +               The
>>>      /sys/power/suspend_stats/last_hw_state_residency file contains
>>>       > >> +               the amount of time spent in a hardware sleep
>>>      state.
>>>       > >> +               This attribute is only available if the system
>>>      supports
>>>       > >> +               low power idle.  This is measured in microseconds.
>>>       > >> +
>>>       > >> +What:          /sys/power/suspend_stats/last_suspend_total
>>>       > >> +Date:          December 2022
>>>       > >> +Contact:       Mario Limonciello <mario.limonciello@amd.com
>>>      <mailto:mario.limonciello@amd.com>>
>>>       > >> +Description:
>>>       > >> +               The
>>>      /sys/power/suspend_stats/last_suspend_total file contains
>>>       > >> +               the total duration of the sleep cycle.
>>>       > >> +               This is measured in microseconds.
>>>       > >> +
>>>       > >>   What:          /sys/power/sync_on_suspend
>>>       > >>   Date:          October 2019
>>>       > >>   Contact:       Jonas Meurer <jonas@freesources.org
>>>      <mailto:jonas@freesources.org>>
>>>       > >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>>>       > >> index cfe19a028918..af343c3f8198 100644
>>>       > >> --- a/include/linux/suspend.h
>>>       > >> +++ b/include/linux/suspend.h
>>>       > >> @@ -68,6 +68,8 @@ struct suspend_stats {
>>>       > >>          int     last_failed_errno;
>>>       > >>          int     errno[REC_FAILED_NUM];
>>>       > >>          int     last_failed_step;
>>>       > >> +       u64     last_hw_state_residency;
>>>       > >> +       u64     last_suspend_total;
>>>       > >>          enum suspend_stat_step  failed_steps[REC_FAILED_NUM];
>>>       > >>   };
>>>       > >>
>>>       > >> @@ -489,6 +491,8 @@ void restore_processor_state(void);
>>>       > >>   extern int register_pm_notifier(struct notifier_block *nb);
>>>       > >>   extern int unregister_pm_notifier(struct notifier_block *nb);
>>>       > >>   extern void ksys_sync_helper(void);
>>>       > >> +extern void pm_set_hw_state_residency(u64 duration);
>>>       > >> +extern void pm_account_suspend_type(const struct timespec64 *t);
>>>       > >>
>>>       > >>   #define pm_notifier(fn, pri) {                         \
>>>       > >>          static struct notifier_block fn##_nb =                  \
>>>       > >> diff --git a/kernel/power/main.c b/kernel/power/main.c
>>>       > >> index 31ec4a9b9d70..11bd658583b0 100644
>>>       > >> --- a/kernel/power/main.c
>>>       > >> +++ b/kernel/power/main.c
>>>       > >> @@ -6,6 +6,7 @@
>>>       > >>    * Copyright (c) 2003 Open Source Development Lab
>>>       > >>    */
>>>       > >>
>>>       > >> +#include <linux/acpi.h>
>>>       > >>   #include <linux/export.h>
>>>       > >>   #include <linux/kobject.h>
>>>       > >>   #include <linux/string.h>
>>>       > >> @@ -54,6 +55,19 @@ void unlock_system_sleep(unsigned int flags)
>>>       > >>   }
>>>       > >>   EXPORT_SYMBOL_GPL(unlock_system_sleep);
>>>       > >>
>>>       > >> +void pm_set_hw_state_residency(u64 duration)
>>>       > >> +{
>>>       > >> +       suspend_stats.last_hw_state_residency = duration;
>>>       > >> +}
>>>       > >> +EXPORT_SYMBOL_GPL(pm_set_hw_state_residency);
>>>       > >> +
>>>       > >> +void pm_account_suspend_type(const struct timespec64 *t)
>>>       > >> +{
>>>       > >> +       suspend_stats.last_suspend_total += (s64)t->tv_sec *
>>>      USEC_PER_SEC +
>>>       > >> +                                                t->tv_nsec /
>>>      NSEC_PER_USEC;
>>>       > >> +}
>>>       > >> +EXPORT_SYMBOL_GPL(pm_account_suspend_type);
>>>       > >> +
>>>       > >>   void ksys_sync_helper(void)
>>>       > >>   {
>>>       > >>          ktime_t start;
>>>       > >> @@ -377,6 +391,20 @@ static ssize_t
>>>      last_failed_step_show(struct kobject *kobj,
>>>       > >>   }
>>>       > >>   static struct kobj_attribute last_failed_step =
>>>      __ATTR_RO(last_failed_step);
>>>       > >>
>>>       > >> +static ssize_t last_hw_state_residency_show(struct kobject *kobj,
>>>       > >> +               struct kobj_attribute *attr, char *buf)
>>>       > >> +{
>>>       > >> +       return sprintf(buf, "%llu\n",
>>>      suspend_stats.last_hw_state_residency);
>>>       > >> +}
>>>       > >> +static struct kobj_attribute last_hw_state_residency =
>>>      __ATTR_RO(last_hw_state_residency);
>>>       > >> +
>>>       > >> +static ssize_t last_suspend_total_show(struct kobject *kobj,
>>>       > >> +               struct kobj_attribute *attr, char *buf)
>>>       > >> +{
>>>       > >> +       return sprintf(buf, "%llu\n",
>>>      suspend_stats.last_suspend_total);
>>>       > >> +}
>>>       > >> +static struct kobj_attribute last_suspend_total =
>>>      __ATTR_RO(last_suspend_total);
>>>       > >> +
>>>       > >>   static struct attribute *suspend_attrs[] = {
>>>       > >>          &success.attr,
>>>       > >>          &fail.attr,
>>>       > >> @@ -391,12 +419,26 @@ static struct attribute *suspend_attrs[] = {
>>>       > >>          &last_failed_dev.attr,
>>>       > >>          &last_failed_errno.attr,
>>>       > >>          &last_failed_step.attr,
>>>       > >> +       &last_hw_state_residency.attr,
>>>       > >> +       &last_suspend_total.attr,
>>>       > >>          NULL,
>>>       > >>   };
>>>       > >>
>>>       > >> +static umode_t suspend_attr_is_visible(struct kobject *kobj,
>>>      struct attribute *attr, int idx)
>>>       > >> +{
>>>       > >> +       if (attr != &last_hw_state_residency.attr)
>>>       > >> +               return 0444;
>>>       > >> +#ifdef CONFIG_ACPI
>>>       > >> +       if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
>>>       > >> +               return 0444;
>>>       > >> +#endif
>>>       > >> +       return 0;
>>>       > >> +}
>>>       > >> +
>>>       > >>   static const struct attribute_group suspend_attr_group = {
>>>       > >>          .name = "suspend_stats",
>>>       > >>          .attrs = suspend_attrs,
>>>       > >> +       .is_visible = suspend_attr_is_visible,
>>>       > >>   };
>>>       > >>
>>>       > >>   #ifdef CONFIG_DEBUG_FS
>>>       > >> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
>>>       > >> index fa3bf161d13f..b6c4a3733212 100644
>>>       > >> --- a/kernel/power/suspend.c
>>>       > >> +++ b/kernel/power/suspend.c
>>>       > >> @@ -423,6 +423,8 @@ static int suspend_enter(suspend_state_t
>>>      state, bool *wakeup)
>>>       > >>          if (suspend_test(TEST_PLATFORM))
>>>       > >>                  goto Platform_wake;
>>>       > >>
>>>       > >> +       suspend_stats.last_suspend_total = 0;
>>>       > >> +
>>>       > >>          if (state == PM_SUSPEND_TO_IDLE) {
>>>       > >>                  s2idle_loop();
>>>       > >>                  goto Platform_wake;
>>>       > >> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
>>>       > >> index f72b9f1de178..e1b356787e53 100644
>>>       > >> --- a/kernel/time/timekeeping.c
>>>       > >> +++ b/kernel/time/timekeeping.c
>>>       > >> @@ -24,6 +24,7 @@
>>>       > >>   #include <linux/compiler.h>
>>>       > >>   #include <linux/audit.h>
>>>       > >>   #include <linux/random.h>
>>>       > >> +#include <linux/suspend.h>
>>>       > >>
>>>       > >>   #include "tick-internal.h"
>>>       > >>   #include "ntp_internal.h"
>>>       > >> @@ -1698,6 +1699,7 @@ static void
>>>      __timekeeping_inject_sleeptime(struct timekeeper *tk,
>>>       > >>          tk_set_wall_to_mono(tk,
>>>      timespec64_sub(tk->wall_to_monotonic, *delta));
>>>       > >>          tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
>>>       > >>          tk_debug_account_sleep_time(delta);
>>>       > >> +       pm_account_suspend_type(delta);
>>>       > >>   }
>>>       > >>
>>>       > >>   #if defined(CONFIG_PM_SLEEP) &&
>>>      defined(CONFIG_RTC_HCTOSYS_DEVICE)
>>>       > >> --
>>>       > >> 2.34.1
>>>       > >>
>>>       >
>>>
>>

Re: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Rafael J. Wysocki]

On Tue, Nov 15, 2022 at 6:58 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
> On 11/15/2022 11:52, Rafael J. Wysocki wrote:
> > On Tue, Nov 15, 2022 at 6:27 PM Limonciello, Mario
> > <mario.limonciello@amd.com> wrote:
> >>
> >> On 11/15/2022 11:20, Raul Rangel wrote:
> >>>
> >>>
> >>> On Tue, Nov 15, 2022 at 9:35 AM Rafael J. Wysocki <rafael@kernel.org
> >>> <mailto:rafael@kernel.org>> wrote:
> >>>
> >>>      On Tue, Nov 15, 2022 at 4:17 PM Limonciello, Mario
> >>>      <mario.limonciello@amd.com <mailto:mario.limonciello@amd.com>> wrote:
> >>>       >
> >>>       > On 11/15/2022 08:45, Rafael J. Wysocki wrote:
> >>>       > > On Thu, Nov 10, 2022 at 7:49 AM Mario Limonciello
> >>>       > > <mario.limonciello@amd.com <mailto:mario.limonciello@amd.com>>
> >>>      wrote:
> >>>       > >>
> >>>       > >> Both AMD and Intel SoCs have a concept of reporting whether
> >>>      the hardware
> >>>       > >> reached a hardware sleep state over s2idle as well as how much
> >>>       > >> time was spent in such a state.
> >>>       > >>
> >>>       > >> This information is valuable to both chip designers and system
> >>>      designers
> >>>       > >> as it helps to identify when there are problems with power
> >>>      consumption
> >>>       > >> over an s2idle cycle.
> >>>       > >>
> >>>       > >> To make the information discoverable, create a new sysfs file
> >>>      and a symbol
> >>>       > >> that drivers from supported manufacturers can use to advertise
> >>>      this
> >>>       > >> information. This file will only be exported when the system
> >>>      supports low
> >>>       > >> power idle in the ACPI table.
> >>>       > >>
> >>>       > >> In order to effectively use this information you will ideally
> >>>      want to
> >>>       > >> compare against the total duration of sleep, so export a
> >>>      second sysfs file
> >>>       > >> that will show total time. This file will be exported on all
> >>>      systems and
> >>>       > >> used both for s2idle and s3.
> >>>       > >
> >>>       > > Well, my first question would be how this is related to
> >>>       > >
> >>>       > > /sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us
> >>>       > >
> >>>       >
> >>>       > This has a dependency on the platform firmware offering an ACPI LPIT
> >>>       > table.  I don't know how common that is.
> >>>
> >>>      Required for running Windows with Modern Standby AFAICS.
> >>>
> >>>       > As this series started from the needs on ChromeOS I would ask is
> >>>      that typically populated by coreboot?
> >>>
> >>>      It should be, but I'd need to ask for confirmation.
> >>>
> >>>
> >>> It looks like Intel platforms have support for the LPIT table:
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.chromium.org%2Fchromiumos%2Fchromiumos%2Fcodesearch%2F%2B%2Fmain%3Asrc%2Fthird_party%2Fcoreboot%2Fsrc%2Fsoc%2Fintel%2Fcommon%2Fblock%2Facpi%2Flpit.c%3Fq%3Df%3ALPIT%2520f%3Acoreboot%26ss%3Dchromiumos&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7C37e6dda56f924fe641f008dac7323c01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041315852648377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=PusftlebIMFtbaMy1XkBjHFMXLjdOzt7hA%2Fm3AM7v7A%3D&amp;reserved=0 <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.chromium.org%2Fchromiumos%2Fchromiumos%2Fcodesearch%2F%2B%2Fmain%3Asrc%2Fthird_party%2Fcoreboot%2Fsrc%2Fsoc%2Fintel%2Fcommon%2Fblock%2Facpi%2Flpit.c%3Fq%3Df%3ALPIT%2520f%3Acoreboot%26ss%3Dchromiumos&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7C37e6dda56f924fe641f008dac7323c01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041315852648377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=PusftlebIMFtbaMy1XkBjHFMXLjdOzt7hA%2Fm3AM7v7A%3D&amp;reserved=0>
> >>>
> >>> For AMD, we had some patches to add _LPIL
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F52381%2F1&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7C37e6dda56f924fe641f008dac7323c01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041315852648377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=gUYdMWZBNVALF8Xzhgswlyw9hCUv7LQ6eomz6gfIYrk%3D&amp;reserved=0
> >>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F52381%2F1&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7C37e6dda56f924fe641f008dac7323c01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638041315852648377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=gUYdMWZBNVALF8Xzhgswlyw9hCUv7LQ6eomz6gfIYrk%3D&amp;reserved=0>
> >>> They never got merged though. We could add an LPIT table to coreboot for
> >>> AMD platforms if necessary.
> >>
> >> _LPI I don't think makes a lot of sense on X86 today, which is why this
> >> was sent up:
> >> eb087f305919e ("ACPI: processor idle: Check for architectural support
> >> for LPI")
> >
> > Well, LPI has nothing to do with LPIT.  [I guess this could not be
> > even more confusing, but that's what you get in the world of 4-letter
> > TLAs.]
> >
> >> As for LPIT - I've never seen LPIT on AMD UEFI systems either.  I guess
> >> it's an Intel specific table?
> >
> > It used to be.  The spec is UEFI-hosted now.
> >
>
> Got it.
>
> >>>
> >>>       > I would hope it's the same number that is populated in that file on
> >>>       > supported systems though.
> >>>
> >>>      Well, which is exactly where I'm going.
> >>>
> >>>      Since there is one sysfs file for exposing this value already and it
> >>>      is used (for example, by sleepgraph), perhaps the way to go would be
> >>>      to extend this interface to systems that don't have LPIT instead of
> >>>      introducing a new one possibly exposing the same value?
> >>>
> >>
> >> Ah; so since Raul confirmed coreboot on Chrome exports that maybe we
> >> just need to add another way to populate that sysfs file for systems
> >> without LPIT (IE AMD).  I think that's a very good idea; thanks.
> >>
> >> I think we still probably want to have a way to get the total suspend
> >> time out programmatically though to compare to.  So perhaps the other
> >> sysfs file I had in the RFC v2 makes sense still.
> >
> > Well there are trace points to get that (sleepgraph uses these too),
> > see Documentation/trace/events-power.rst (and you can git grep for
> > "machine_suspend" to find where this comes from).
> >
> > I guess there could be a sysfs file in addition to them, but I'm not
> > sure if the extra overhead would be worth the benefit.
>
> At least the way that I envisioned this all working was that userspace
> software that wanted to could query some sysfs files and figure out a
> percentage of time spent.  If it was below a threshold users could be
> notified, or logs can be sent up to a server for analysis etc.
>
> Trace points would mean that userspace software like systemd and powerd
> would need to turn on the tracing every time to get the raw total
> numbers to do such a comparison.

Fair enough, but there are quite some considerations to be made here
regarding what exactly is included in the "total sleep time" and how
to compare that with the residency value (note: this needs to work
cross-platform).

Re: [RFC v2 1/3] PM: Add a sysfs files to represent sleep duration

[Box, David E]

On Tue, 2022-11-15 at 08:13 -0600, Limonciello, Mario wrote:
> On 11/15/2022 04:32, Hans de Goede wrote:
> > Hi Mario,
> > 
> > On 11/14/22 20:12, Limonciello, Mario wrote:
> > > [Public]
> > > 
> > > Thanks! Appreciate the comments.
> > > At least conceptually is there agreement to this idea for the two sysfs
> > > files
> > > and userspace can use them to do this comparison?
> > 
> > First of all let me say that I think that having some generic mechanism
> > which allows userspace to check if deep enough sleep-state were reached
> > is a good idea.  And thank you for working on this!
> > 
> 
> Sure!
> 
> > I wonder though if it would not be better to have some mechanism
> > where a list of sleep states + time spend in each time is printed ?
> > 
> > E.g. I know that on Intel Bay Trail and Cherry Trail devices (just an
> > example I'm familiar with) there are S0i0 - S0i3 and we really want
> > to reach S0i3 during suspend.
> > 
> > Sometimes on S0i1 or S0i2 is reached due to some part of the hw
> > not getting suspended properly.
> > 
> > So then we have reached "a hardware sleep state over s2idle"
> > but no the one we want.
> 
> At least the way it's built right now it's tracking the s0ix counter for 
> Intel and the s0i3 counter for AMD.
> 
> BTW - when I did all the cleanups suggested in RFC v2 I notice I was 
> taking the raw number for Intel, and I have that fixed for the next version.
> 
> I don't know if other counters exist for Intel for various hardware states.

They do, but the implementation is highly platform specific.

> On the current AMD silicon this is the interesting metric.
> 
> > 
> > OTOH I can image that if we start adding support for functionality
> > like standby-connect under Linux that then we may not always
> > reach the deepest hw sleep-state.
> 
> Can you elaborate what you mean by standby connect?  WoWLAN?
> At least on the current AMD platforms WoWLAN can happen while the 
> silicon is in the deepest hardware sleep state.
> 
> > 
> > So I'm a bit worried that having just a single number for
> > last_hw_state_residency is not enough.
> > 
> > I think that it might be better to have a mechanism to set
> > a set of names for hw-states (once) and then set the residency
> > per state (*) after resume and have the sysfs file print
> > the entire list. >
> > This list could then also always include the total suspend time,
> > also avoiding the need for a second sysfs file and we could also
> > use the same format for non s2idle suspend having it print
> > only the total suspend time when no hw-state names are set.
> 
> So is your thought is to have a single sysfs file something like 
> /sys/power/suspend_stats/s2idle_stats that would show this?
> 
> state \t % \t duration (us)
> s0i3  \t 99.5% \t 1000
> 
> For AMD that would be a single line and I don't think it's worth the 
> extra code.  I would like to know if it actually makes sense for Intel 
> though.

Not here. Engineers care, but the pmc driver already provides this. Most users
are only concerned about whether their systems reach low power idle, whichever
S0ix state it is.

> 
> We also need to think about what will be actionable with this 
> information by consumers of it because I'm certain it will be leading to 
> bug reports.

I agree. While Intel SoCs may support multiple states, it is not always the case
(particularly for Tiger Lake and newer) that you need to reach the deepest state
in order to achieve very good power savings.

David

> 
> Let's think about a hypothetical bug report:
> "Intel System only spent 20% of time in deepest hardware state".
> They attach to the bug report s2idle_stats that looks like this:
> 
> state \t % \t duration (us)
> s0i2  \t 80.0% \t 1000000
> s0i3  \t 20.0% \t 100000
> 
> Is that any more actionable than
> /sys/power/last_hw_state_residency showing 100000
> and
> /sys/power/suspend_total showing 500000
> 
> I think in either case the next action is more debugging will be needed, 
> such as turning on dynamic debug or some module parameters.
> 
> "Practically" I expect software like systemd or powerd to be reading 
> these sysfs files.
> 
> > 
> > Regards,
> > 
> > Hans
> > 
> > 
> > *) Using an array, so up to MAX_HW_RESIDENCY_STATES
> > 
> > 
> > > 
> > > A few nested replies below, but I'll clean it up for
> > > RFC v3 or submit as PATCH v1 if there is conceptual alignment before then.
> > > 
> > > > On Thu, Nov 10 2022 at 00:47, Mario Limonciello wrote:
> > > > 
> > > > 'Add a sysfs files'?
> > > > 
> > > > Can you please decide whether that's 'a file' or 'multiple files'?
> > > 
> > > Yup thanks; bad find and replace in the commit message when I added
> > > the second file.
> > > 
> > > > 
> > > > > Both AMD and Intel SoCs have a concept of reporting whether the
> > > > hardware
> > > > > reached a hardware sleep state over s2idle as well as how much
> > > > > time was spent in such a state.
> > > > 
> > > > Nice, but ...
> > > > 
> > > > > This information is valuable to both chip designers and system
> > > > > designers
> > > > > as it helps to identify when there are problems with power consumption
> > > > > over an s2idle cycle.
> > > > > 
> > > > > To make the information discoverable, create a new sysfs file and a
> > > > > symbol
> > > > > that drivers from supported manufacturers can use to advertise this
> > > > > information. This file will only be exported when the system supports
> > > > > low
> > > > > power idle in the ACPI table.
> > > > > 
> > > > > In order to effectively use this information you will ideally want to
> > > > > compare against the total duration of sleep, so export a second sysfs
> > > > > file
> > > > > that will show total time. This file will be exported on all systems
> > > > > and
> > > > > used both for s2idle and s3.
> > > > 
> > > > The above is incomprehensible word salad. Can you come up with some
> > > > coherent explanation of what you are trying to achieve please?
> > > > 
> > > > > +void pm_set_hw_state_residency(u64 duration)
> > > > > +{
> > > > > +       suspend_stats.last_hw_state_residency = duration;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(pm_set_hw_state_residency);
> > > > > +
> > > > > +void pm_account_suspend_type(const struct timespec64 *t)
> > > > > +{
> > > > > +       suspend_stats.last_suspend_total += (s64)t->tv_sec *
> > > > USEC_PER_SEC +
> > > > > +                                                t->tv_nsec /
> > > > NSEC_PER_USEC;
> > > > 
> > > > Conversion functions for timespecs to scalar nanoseconds exist for a
> > > > reason. Why does this need special treatment and open code it?
> > > 
> > > Will fixup to use conversion functions.
> > > 
> > > > 
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(pm_account_suspend_type);
> > > > 
> > > > So none of these functions has any kind of documentation. kernel-doc
> > > > exists for a reason especially for exported functions.
> > > > 
> > > > That said, what's the justification to export any of these functions at
> > > > all? AFAICT pm_account_suspend_type() is only used by builtin code...
> > > 
> > > I think you're right; they shouldn't export; will fix.
> > > 
> > > > 
> > > > > +static umode_t suspend_attr_is_visible(struct kobject *kobj, struct
> > > > attribute *attr, int idx)
> > > > > +{
> > > > > +       if (attr != &last_hw_state_residency.attr)
> > > > > +               return 0444;
> > > > > +#ifdef CONFIG_ACPI
> > > > > +       if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
> > > > > +               return 0444;
> > > > > +#endif
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > >   static const struct attribute_group suspend_attr_group = {
> > > > >         .name = "suspend_stats",
> > > > >         .attrs = suspend_attrs,
> > > > > +       .is_visible = suspend_attr_is_visible,
> > > > 
> > > > How is this change related to the changelog above? We are not hiding
> > > > subtle changes to the existing code in some conglomorate patch. See
> > > > Documentation/process/...
> > > 
> > > It was from feedback from RFC v1 from David Box that this file should only
> > > be visible when s2idle is supported on the hardware.  Will adjust commit
> > > message to make it clearer.
> > > 
> > > > 
> > > > > --- a/kernel/time/timekeeping.c
> > > > > +++ b/kernel/time/timekeeping.c
> > > > > @@ -24,6 +24,7 @@
> > > > >   #include <linux/compiler.h>
> > > > >   #include <linux/audit.h>
> > > > >   #include <linux/random.h>
> > > > > +#include <linux/suspend.h>
> > > > > 
> > > > >   #include "tick-internal.h"
> > > > >   #include "ntp_internal.h"
> > > > > @@ -1698,6 +1699,7 @@ static void
> > > > __timekeeping_inject_sleeptime(struct timekeeper *tk,
> > > > >         tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic,
> > > > *delta));
> > > > >         tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
> > > > >         tk_debug_account_sleep_time(delta);
> > > > > +       pm_account_suspend_type(delta);
> > > > 
> > > > That function name is really self explaining - NOT !
> > > > 
> > > >       pm_account_suspend_type(delta);
> > > > 
> > > > So this will account a suspend type depending on the time spent in
> > > > suspend, right?
> > > > 
> > > > It's totally obvious that the suspend type (whatever it is) depends on
> > > > the time delta argument... especially when the function at hand has
> > > > absolutely nothing to do with a type:
> > > > 
> > > 
> > > I fat fingered this.  In my mind I thought I wrote
> > > pm_account_suspend_time()
> > > Will fix.
> > > 
> > > > > +void pm_account_suspend_type(const struct timespec64 *t)
> > > > > +{
> > > > > +       suspend_stats.last_suspend_total += (s64)t->tv_sec *
> > > > USEC_PER_SEC +
> > > > > +                                                t->tv_nsec /
> > > > NSEC_PER_USEC;
> > > > > +}
> > > > 
> > > > Sigh....
> > > > 
> > > > Thanks,
> > > > 
> > > >          tglx
> > > 
> > 
>