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
`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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Hi Mario, comments below.
On Thu, Nov 17, 2022 at 5:58 PM Mario Limonciello
<[email protected]> 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 <[email protected]>
> ---
> 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
>
[Public]
> -----Original Message-----
> From: Sven van Ashbrook <[email protected]>
> Sent: Wednesday, November 23, 2022 11:47
> To: Limonciello, Mario <[email protected]>
> Cc: Rafael J . Wysocki <[email protected]>; Rajneesh Bhardwaj
> <[email protected]>; David E Box <[email protected]>; Raul
> Rangel <[email protected]>; [email protected]; platform-
> [email protected]; Pavel Machek <[email protected]>; Len Brown
> <[email protected]>; John Stultz <[email protected]>; Thomas
> Gleixner <[email protected]>; Stephen Boyd <[email protected]>; S-k,
> Shyam-sundar <[email protected]>; Rajat Jain
> <[email protected]>; Hans de Goede <[email protected]>; linux-
> [email protected]; Mark Gross <[email protected]>
> 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
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >