[RFC v4 0/5] Report percentage of time in hardware sleep state

[RFC v4 0/5] Report percentage of 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.

One idea suggested in RFC v3 was to report the percentage of time instead
of the raw numbers.  This allows the details to how much time to be reported
to be abstracted by individual drivers instead.

RFC v3->v4:
 * Switch to percentage reporting
 * More changes to Intel drivers to hopefully report this properly.

Mario Limonciello (5):
  PM: Add a sysfs file to represent the percentage of sleep in hardware
    state
  platform/x86/amd: pmc: Report duration of time in deepest hw state
  platform/x86/intel/pmc: core: Drop check_counters
  platform/x86/intel/pmc: core: Always capture counters on suspend
  platform/x86/intel/pmc: core: Report duration of time in HW sleep
    state

 Documentation/ABI/testing/sysfs-power |  9 +++++++
 drivers/platform/x86/amd/pmc.c        |  5 ++--
 drivers/platform/x86/intel/pmc/core.c | 13 +++-------
 drivers/platform/x86/intel/pmc/core.h |  1 -
 include/linux/suspend.h               |  2 ++
 include/linux/timekeeping.h           |  1 +
 kernel/power/main.c                   | 36 +++++++++++++++++++++++++++
 kernel/time/timekeeping.c             | 20 ++++++++++++---
 8 files changed, 70 insertions(+), 17 deletions(-)

-- 
2.34.1

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

[Mario Limonciello]

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 percentage of time spent in a sleep state.
This file will be present only if the system supports s2idle.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
RFC v3->v4
 * Switch to a percentage for reporting
 * Hook into timekeeping differently
---
 Documentation/ABI/testing/sysfs-power |  9 +++++++
 include/linux/suspend.h               |  2 ++
 include/linux/timekeeping.h           |  1 +
 kernel/power/main.c                   | 36 +++++++++++++++++++++++++++
 kernel/time/timekeeping.c             | 20 ++++++++++++---
 5 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index f99d433ff311..60b6948f5982 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_percent
+Date:		December 2022
+Contact:	Mario Limonciello <mario.limonciello@amd.com>
+Description:
+		The /sys/power/suspend_stats/last_hw_sleep_percent file
+		contains the percentage of time that the last suspend cycle
+		was spent in a hardware sleep state.  It is expressed as an
+		integer between between 0 and 100.
+
 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..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/include/linux/timekeeping.h b/include/linux/timekeeping.h
index fe1e467ba046..2a81366f3e31 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -70,6 +70,7 @@ extern ktime_t ktime_get_coarse_with_offset(enum tk_offsets offs);
 extern ktime_t ktime_mono_to_any(ktime_t tmono, enum tk_offsets offs);
 extern ktime_t ktime_get_raw(void);
 extern u32 ktime_get_resolution_ns(void);
+extern u64 get_suspend_duration_ns(void);
 
 /**
  * ktime_get_real - get the real (wall-) time in ktime_t format
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 31ec4a9b9d70..be82f4a740c0 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,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_sleep_percent_show(struct kobject *kobj,
+		struct kobj_attribute *attr, char *buf)
+{
+	u64 t = get_suspend_duration_ns();
+	int p;
+
+	if (!t)
+		return -EINVAL;
+
+	p = min((100 * NSEC_PER_USEC * suspend_stats.last_hw_sleep) / t, 100);
+	return sysfs_emit(buf, "%llu\n", p);
+}
+static struct kobj_attribute last_hw_sleep_percent = __ATTR_RO(last_hw_sleep_percent);
+
 static struct attribute *suspend_attrs[] = {
 	&success.attr,
 	&fail.attr,
@@ -391,12 +412,27 @@ static struct attribute *suspend_attrs[] = {
 	&last_failed_dev.attr,
 	&last_failed_errno.attr,
 	&last_failed_step.attr,
+	&last_hw_sleep_percent.attr,
 	NULL,
 };
 
+static umode_t suspend_attr_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
+{
+	if (attr == &last_hw_sleep_percent.attr) {
+#ifdef CONFIG_ACPI
+		if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
+			return 0444;
+#endif
+		return 0;
+	}
+
+	return 0444;
+}
+
 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/time/timekeeping.c b/kernel/time/timekeeping.c
index f72b9f1de178..49119a942cb2 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1779,7 +1779,7 @@ void timekeeping_resume(void)
 	struct timekeeper *tk = &tk_core.timekeeper;
 	struct clocksource *clock = tk->tkr_mono.clock;
 	unsigned long flags;
-	struct timespec64 ts_new, ts_delta;
+	struct timespec64 ts_new;
 	u64 cycle_now, nsec;
 	bool inject_sleeptime = false;
 
@@ -1806,16 +1806,16 @@ void timekeeping_resume(void)
 	cycle_now = tk_clock_read(&tk->tkr_mono);
 	nsec = clocksource_stop_suspend_timing(clock, cycle_now);
 	if (nsec > 0) {
-		ts_delta = ns_to_timespec64(nsec);
+		timekeeping_suspend_time = ns_to_timespec64(nsec);
 		inject_sleeptime = true;
 	} else if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) {
-		ts_delta = timespec64_sub(ts_new, timekeeping_suspend_time);
+		timekeeping_suspend_time = timespec64_sub(ts_new, timekeeping_suspend_time);
 		inject_sleeptime = true;
 	}
 
 	if (inject_sleeptime) {
 		suspend_timing_needed = false;
-		__timekeeping_inject_sleeptime(tk, &ts_delta);
+		__timekeeping_inject_sleeptime(tk, &timekeeping_suspend_time);
 	}
 
 	/* Re-base the last cycle value */
@@ -2232,6 +2232,18 @@ void update_wall_time(void)
 		clock_was_set_delayed();
 }
 
+/**
+ * get_suspend_duration_ns - Return the duration of a system suspend.
+ *
+ * Returns the calculation of the duration of time that passed while a
+ * system was suspended.
+ *
+ */
+u64 get_suspend_duration_ns(void)
+{
+	return timespec64_to_ns(&timekeeping_suspend_time);
+}
+
 /**
  * getboottime64 - Return the real time of system boot.
  * @ts:		pointer to the timespec64 to be set
-- 
2.34.1

[RFC v4 2/5] 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 kernel for usage in displaying
to an end user.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
RFC v4:
 * Report time to kernel again, letting kernel calculate percentage
---
 drivers/platform/x86/amd/pmc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index 96e790e639a2..2e3d8ff5659e 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -363,9 +363,8 @@ 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_sleep_time(table.s0i3_last_entry_status ?
+			     table.timein_s0i3_lastcapture : 0);
 }
 #endif
 
-- 
2.34.1

[RFC v4 3/5] platform/x86/intel/pmc: core: Drop check_counters

[Mario Limonciello]

`check_counters` is a stateful variable for indicating whether or
not to be checking if counters incremented on resume from s2idle.

As the module already has code to gate whether to check the counters
that will fail the suspend when this is enabled, use that instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
RFC v3->v4:
 * No changes
---
 drivers/platform/x86/intel/pmc/core.c | 7 ++-----
 drivers/platform/x86/intel/pmc/core.h | 1 -
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
index 17ec5825d13d..adc2cae4db28 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -2059,8 +2059,6 @@ static __maybe_unused int pmc_core_suspend(struct device *dev)
 {
 	struct pmc_dev *pmcdev = dev_get_drvdata(dev);
 
-	pmcdev->check_counters = false;
-
 	/* No warnings on S0ix failures */
 	if (!warn_on_s0ix_failures)
 		return 0;
@@ -2077,7 +2075,6 @@ static __maybe_unused int pmc_core_suspend(struct device *dev)
 	if (pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
 		return -EIO;
 
-	pmcdev->check_counters = true;
 	return 0;
 }
 
@@ -2113,10 +2110,10 @@ static __maybe_unused int pmc_core_resume(struct device *dev)
 	const struct pmc_bit_map **maps = pmcdev->map->lpm_sts;
 	int offset = pmcdev->map->lpm_status_offset;
 
-	if (!pmcdev->check_counters)
+	if (!pmc_core_is_s0ix_failed(pmcdev))
 		return 0;
 
-	if (!pmc_core_is_s0ix_failed(pmcdev))
+	if (!warn_on_s0ix_failures)
 		return 0;
 
 	if (pmc_core_is_pc10_failed(pmcdev)) {
diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
index 7a059e02c265..5687e91e884c 100644
--- a/drivers/platform/x86/intel/pmc/core.h
+++ b/drivers/platform/x86/intel/pmc/core.h
@@ -316,7 +316,6 @@ struct pmc_reg_map {
  * @pmc_xram_read_bit:	flag to indicate whether PMC XRAM shadow registers
  *			used to read MPHY PG and PLL status are available
  * @mutex_lock:		mutex to complete one transcation
- * @check_counters:	On resume, check if counters are getting incremented
  * @pc10_counter:	PC10 residency counter
  * @s0ix_counter:	S0ix residency (step adjusted)
  * @num_lpm_modes:	Count of enabled modes
-- 
2.34.1

[RFC v4 4/5] platform/x86/intel/pmc: core: Always capture counters on suspend

[Mario Limonciello]

Currently counters are only captured during suspend when the
warn_on_s0ix_failures module parameter is set.

In order to relay this counter information to the kernel reporting
infrastructure adjust it so that the counters are always captured.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
RFC v3->v4:
 * New patch
---
 drivers/platform/x86/intel/pmc/core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
index adc2cae4db28..9baf2bb443c8 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -2059,10 +2059,6 @@ static __maybe_unused int pmc_core_suspend(struct device *dev)
 {
 	struct pmc_dev *pmcdev = dev_get_drvdata(dev);
 
-	/* No warnings on S0ix failures */
-	if (!warn_on_s0ix_failures)
-		return 0;
-
 	/* Check if the syspend will actually use S0ix */
 	if (pm_suspend_via_firmware())
 		return 0;
-- 
2.34.1

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

[Mario Limonciello]

intel_pmc_core displays a warning when the module parameter
`warn_on_s0ix_failures` is set and a suspend didn't get to a HW sleep
state.

Report this to the standard kernel reporting infrastructure that will
be used to display a percentage of time spent in a hw sleep state.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
RFC v3->v4:
 * New patch
---
 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 9baf2bb443c8..0e25348f1c2a 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -2094,6 +2094,8 @@ static inline bool pmc_core_is_s0ix_failed(struct pmc_dev *pmcdev)
 	if (pmc_core_dev_state_get(pmcdev, &s0ix_counter))
 		return false;
 
+	pm_set_hw_sleep_time(s0ix_counter - pmcdev->s0ix_counter);
+
 	if (s0ix_counter == pmcdev->s0ix_counter)
 		return true;
 
-- 
2.34.1

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

[John Stultz]

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

Hey Mario, thanks for sending out this patch!

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

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

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

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

thanks
-john

Re: [RFC v4 3/5] platform/x86/intel/pmc: core: Drop check_counters

[Sven van Ashbrook]

Hi Mario, comments below.

On Thu, Nov 17, 2022 at 5:58 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> `check_counters` is a stateful variable for indicating whether or
> not to be checking if counters incremented on resume from s2idle.
>
> As the module already has code to gate whether to check the counters
> that will fail the suspend when this is enabled, use that instead.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> RFC v3->v4:
>  * No changes
> ---
>  drivers/platform/x86/intel/pmc/core.c | 7 ++-----
>  drivers/platform/x86/intel/pmc/core.h | 1 -
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> index 17ec5825d13d..adc2cae4db28 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -2059,8 +2059,6 @@ static __maybe_unused int pmc_core_suspend(struct device *dev)
>  {
>         struct pmc_dev *pmcdev = dev_get_drvdata(dev);
>
> -       pmcdev->check_counters = false;
> -
>         /* No warnings on S0ix failures */
>         if (!warn_on_s0ix_failures)
>                 return 0;
> @@ -2077,7 +2075,6 @@ static __maybe_unused int pmc_core_suspend(struct device *dev)
>         if (pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
>                 return -EIO;
>
> -       pmcdev->check_counters = true;
>         return 0;
>  }
>
> @@ -2113,10 +2110,10 @@ static __maybe_unused int pmc_core_resume(struct device *dev)
>         const struct pmc_bit_map **maps = pmcdev->map->lpm_sts;
>         int offset = pmcdev->map->lpm_status_offset;
>
> -       if (!pmcdev->check_counters)
> +       if (!pmc_core_is_s0ix_failed(pmcdev))

Will this break the "CPU did not enter SLP_S0!!!" warning?

As far as I can tell,
If an Intel system uses S3 instead of S0ix, pmcdev->s0ix_counter will
not get updated in the
suspend callback. In the resume callback, the counter check in
pmc_core_is_s0ix_failed()
no longer makes any sense. It either fails all the time (if
pmcdev->s0ix_counter was inited with a non-
zero value) or succeeds all the time (if pmcdev->s0ix_counter was zero-inited).

>                 return 0;
>
> -       if (!pmc_core_is_s0ix_failed(pmcdev))
> +       if (!warn_on_s0ix_failures)
>                 return 0;
>
>         if (pmc_core_is_pc10_failed(pmcdev)) {
> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> index 7a059e02c265..5687e91e884c 100644
> --- a/drivers/platform/x86/intel/pmc/core.h
> +++ b/drivers/platform/x86/intel/pmc/core.h
> @@ -316,7 +316,6 @@ struct pmc_reg_map {
>   * @pmc_xram_read_bit: flag to indicate whether PMC XRAM shadow registers
>   *                     used to read MPHY PG and PLL status are available
>   * @mutex_lock:                mutex to complete one transcation
> - * @check_counters:    On resume, check if counters are getting incremented
>   * @pc10_counter:      PC10 residency counter
>   * @s0ix_counter:      S0ix residency (step adjusted)
>   * @num_lpm_modes:     Count of enabled modes
> --
> 2.34.1
>

RE: [RFC v4 3/5] platform/x86/intel/pmc: core: Drop check_counters

[Limonciello, Mario]

[-- Attachment #1: Type: text/plain, Size: 4495 bytes --]

[Public]



> -----Original Message-----
> From: Sven van Ashbrook <svenva@chromium.org>
> Sent: Wednesday, November 23, 2022 11:47
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Rafael J . Wysocki <rafael@kernel.org>; Rajneesh Bhardwaj
> <irenic.rajneesh@gmail.com>; David E Box <david.e.box@intel.com>; Raul
> Rangel <rrangel@chromium.org>; linux-pm@vger.kernel.org; platform-
> driver-x86@vger.kernel.org; Pavel Machek <pavel@ucw.cz>; Len Brown
> <len.brown@intel.com>; John Stultz <jstultz@google.com>; Thomas
> Gleixner <tglx@linutronix.de>; Stephen Boyd <sboyd@kernel.org>; S-k,
> Shyam-sundar <Shyam-sundar.S-k@amd.com>; Rajat Jain
> <rajatja@google.com>; Hans de Goede <hdegoede@redhat.com>; linux-
> kernel@vger.kernel.org; Mark Gross <markgross@kernel.org>
> Subject: Re: [RFC v4 3/5] platform/x86/intel/pmc: core: Drop check_counters
> 
> Hi Mario, comments below.
> 
> On Thu, Nov 17, 2022 at 5:58 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
> >
> > `check_counters` is a stateful variable for indicating whether or
> > not to be checking if counters incremented on resume from s2idle.
> >
> > As the module already has code to gate whether to check the counters
> > that will fail the suspend when this is enabled, use that instead.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> > RFC v3->v4:
> >  * No changes
> > ---
> >  drivers/platform/x86/intel/pmc/core.c | 7 ++-----
> >  drivers/platform/x86/intel/pmc/core.h | 1 -
> >  2 files changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel/pmc/core.c
> b/drivers/platform/x86/intel/pmc/core.c
> > index 17ec5825d13d..adc2cae4db28 100644
> > --- a/drivers/platform/x86/intel/pmc/core.c
> > +++ b/drivers/platform/x86/intel/pmc/core.c
> > @@ -2059,8 +2059,6 @@ static __maybe_unused int
> pmc_core_suspend(struct device *dev)
> >  {
> >         struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> >
> > -       pmcdev->check_counters = false;
> > -
> >         /* No warnings on S0ix failures */
> >         if (!warn_on_s0ix_failures)
> >                 return 0;
> > @@ -2077,7 +2075,6 @@ static __maybe_unused int
> pmc_core_suspend(struct device *dev)
> >         if (pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
> >                 return -EIO;
> >
> > -       pmcdev->check_counters = true;
> >         return 0;
> >  }
> >
> > @@ -2113,10 +2110,10 @@ static __maybe_unused int
> pmc_core_resume(struct device *dev)
> >         const struct pmc_bit_map **maps = pmcdev->map->lpm_sts;
> >         int offset = pmcdev->map->lpm_status_offset;
> >
> > -       if (!pmcdev->check_counters)
> > +       if (!pmc_core_is_s0ix_failed(pmcdev))
> 
> Will this break the "CPU did not enter SLP_S0!!!" warning?
> 
> As far as I can tell,
> If an Intel system uses S3 instead of S0ix, pmcdev->s0ix_counter will
> not get updated in the
> suspend callback. In the resume callback, the counter check in
> pmc_core_is_s0ix_failed()
> no longer makes any sense. It either fails all the time (if
> pmcdev->s0ix_counter was inited with a non-
> zero value) or succeeds all the time (if pmcdev->s0ix_counter was zero-
> inited).

Ah yeah;  So this patch probably doesn't make sense as is.  It would mean
either needing to check pm_suspend_via_firmware() in the resume callback
or just skipping it.  I'll drop it in the next revision of this.

> 
> >                 return 0;
> >
> > -       if (!pmc_core_is_s0ix_failed(pmcdev))
> > +       if (!warn_on_s0ix_failures)
> >                 return 0;
> >
> >         if (pmc_core_is_pc10_failed(pmcdev)) {
> > diff --git a/drivers/platform/x86/intel/pmc/core.h
> b/drivers/platform/x86/intel/pmc/core.h
> > index 7a059e02c265..5687e91e884c 100644
> > --- a/drivers/platform/x86/intel/pmc/core.h
> > +++ b/drivers/platform/x86/intel/pmc/core.h
> > @@ -316,7 +316,6 @@ struct pmc_reg_map {
> >   * @pmc_xram_read_bit: flag to indicate whether PMC XRAM shadow
> registers
> >   *                     used to read MPHY PG and PLL status are available
> >   * @mutex_lock:                mutex to complete one transcation
> > - * @check_counters:    On resume, check if counters are getting
> incremented
> >   * @pc10_counter:      PC10 residency counter
> >   * @s0ix_counter:      S0ix residency (step adjusted)
> >   * @num_lpm_modes:     Count of enabled modes
> > --
> > 2.34.1
> >

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 19283 bytes --]