[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2676888c-e93f-53aa-a4f7-e85b66f351c8@amd.com>
Date: Fri, 31 Mar 2023 13:05:09 -0500
From: "Limonciello, Mario" <mario.limonciello@....com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Sven van Ashbrook <svenva@...omium.org>,
John Stultz <jstultz@...gle.com>,
Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
Raul Rangel <rrangel@...omium.org>,
David E Box <david.e.box@...el.com>,
Rajat Jain <rajatja@...gle.com>,
S-k Shyam-sundar <Shyam-sundar.S-k@....com>,
Hans de Goede <hdegoede@...hat.com>,
linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
linux-pm@...r.kernel.org
Subject: Re: [PATCH v5 1/4] PM: Add a sysfs file to represent time spent in
hardware sleep state
On 3/31/2023 13:01, Rafael J. Wysocki wrote:
> On Thu, Mar 30, 2023 at 9:45 PM Mario Limonciello
> <mario.limonciello@....com> wrote:
>>
>> Userspace can't easily discover how much of a sleep cycle was spent in a
>> hardware sleep state without using kernel tracing and vendor specific sysfs
>> or debugfs files.
>>
>> To make this information more discoverable, introduce a new sysfs file
>> to represent the time spent in a sleep state.
>
> This is only in the most recent suspend-resume cycle, isn't it?
Yes; that's correct.
>
> Wouldn't it be useful to have another attribute printing the
> accumulated total HW sleep time?
>
I had considered this; but I didn't think it was actually very useful
because userspace will get control at the end of every cycle and can
accumulate those numbers if desirable.
>> This file will be present only if the system supports s2idle.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>> ---
>> v4->v5:
>> * Provide time in microseconds instead of percent. Userspace can convert
>> this if desirable.
>> ---
>> Documentation/ABI/testing/sysfs-power | 9 +++++++++
>> include/linux/suspend.h | 2 ++
>> kernel/power/main.c | 29 +++++++++++++++++++++++++++
>> 3 files changed, 40 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
>> index f99d433ff311..9e0c31b9ce85 100644
>> --- a/Documentation/ABI/testing/sysfs-power
>> +++ b/Documentation/ABI/testing/sysfs-power
>> @@ -413,6 +413,15 @@ 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_sleep
>> +Date: June 2023
>> +Contact: Mario Limonciello <mario.limonciello@....com>
>> +Description:
>> + The /sys/power/suspend_stats/last_hw_sleep file
>> + contains the duration of time spent in a hardware sleep
>> + state during from the previous suspend cycle. This number
>
> "during from"?
>
> I would say "in the most recent system suspend-resume cycle".
Ack, thanks.
>
>> + is measured in microseconds.
>> +
>> What: /sys/power/sync_on_suspend
>> Date: October 2019
>> Contact: Jonas Meurer <jonas@...esources.org>
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index cfe19a028918..e0f2ac5f4406 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -68,6 +68,7 @@ struct suspend_stats {
>> int last_failed_errno;
>> int errno[REC_FAILED_NUM];
>> int last_failed_step;
>> + u64 last_hw_sleep;
>> enum suspend_stat_step failed_steps[REC_FAILED_NUM];
>> };
>>
>> @@ -489,6 +490,7 @@ 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_sleep_time(u64 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..6a2bf8784ce8 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>
>> @@ -83,6 +84,12 @@ int unregister_pm_notifier(struct notifier_block *nb)
>> }
>> EXPORT_SYMBOL_GPL(unregister_pm_notifier);
>>
>> +void pm_set_hw_sleep_time(u64 t)
>> +{
>> + suspend_stats.last_hw_sleep = t;
>> +}
>> +EXPORT_SYMBOL_GPL(pm_set_hw_sleep_time);
>> +
>> int pm_notifier_call_chain_robust(unsigned long val_up, unsigned long val_down)
>> {
>> int ret;
>> @@ -377,6 +384,13 @@ 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_sleep_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + return sysfs_emit(buf, "%llu\n", suspend_stats.last_hw_sleep);
>> +}
>> +static struct kobj_attribute last_hw_sleep = __ATTR_RO(last_hw_sleep);
>> +
>> static struct attribute *suspend_attrs[] = {
>> &success.attr,
>> &fail.attr,
>> @@ -391,12 +405,27 @@ static struct attribute *suspend_attrs[] = {
>> &last_failed_dev.attr,
>> &last_failed_errno.attr,
>> &last_failed_step.attr,
>> + &last_hw_sleep.attr,
>> NULL,
>> };
>>
>> +static umode_t suspend_attr_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
>> +{
>> + if (attr == &last_hw_sleep.attr) {
>> +#ifdef CONFIG_ACPI
>> + if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
>> + return 0444;
>> +#endif
>> + return 0;
>> + }
>> +
>> + return 0444;
>
> if (attr != &last_hw_sleep.attr)
> return 0444;
>
> #ifdef CONFIG_ACPI
> if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
> return 0444;
> #endif
>
> return 0;
>
Ack, thanks.
>> +}
>> +
>> static const struct attribute_group suspend_attr_group = {
>> .name = "suspend_stats",
>> .attrs = suspend_attrs,
>> + .is_visible = suspend_attr_is_visible,
>> };
>>
>> #ifdef CONFIG_DEBUG_FS
>> --
Powered by blists - more mailing lists