[PATCH v9 0/4] Add vendor agnostic mechanism to report hardware sleep

[PATCH v9 0/4] Add vendor agnostic mechanism to report hardware sleep

[Mario Limonciello]

An important part of validating that s0ix worked properly is to check how
much of a cycle was spent in a hardware sleep state.

The reporting of hardware sleep is a mix of kernel messages and sysfs
files that vary from vendor to vendor. Collecting this information
requires extra information on the kernel command line or fetching from
debugfs.

To make this information more readily accessible introduce a new file in
suspend_stats that drivers can report into during their resume routine.

Userspace can fetch this information and compare it against the duration
of the cycle to allow determining residency percentages and flagging
problems.

Mario Limonciello (4):
  PM: Add sysfs files to represent time spent in hardware sleep state
  platform/x86/amd: pmc: Report duration of time in hw sleep state
  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 | 29 +++++++++++++
 drivers/platform/x86/amd/pmc.c        |  6 +--
 drivers/platform/x86/intel/pmc/core.c | 17 ++++----
 drivers/platform/x86/intel/pmc/core.h |  4 +-
 include/linux/suspend.h               |  8 ++++
 kernel/power/main.c                   | 59 +++++++++++++++++++++------
 6 files changed, 98 insertions(+), 25 deletions(-)


base-commit: 09a9639e56c01c7a00d6c0ca63f4c7c41abe075d
-- 
2.34.1

[PATCH v9 2/4] platform/x86/amd: pmc: Report duration of time in hw sleep 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 so that userspace software can query after the
suspend cycle is done.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v7->v8:
 * Report max hw sleep time as well.
---
 drivers/platform/x86/amd/pmc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index 2edaae04a691..e610457136e6 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -393,9 +393,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_report_hw_sleep_time(table.s0i3_last_entry_status ?
+				table.timein_s0i3_lastcapture : 0);
 }
 
 static int amd_pmc_get_smu_version(struct amd_pmc_dev *dev)
@@ -1015,6 +1014,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	}
 
 	amd_pmc_dbgfs_register(dev);
+	pm_report_max_hw_sleep(U64_MAX);
 	return 0;
 
 err_pci_dev_put:
-- 
2.34.1

[PATCH v9 3/4] 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.

warn_on_s0ix_failures will be utilized solely for messaging by
the driver instead.

Reviewed-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v5->v6:
 * Pick up tag
v4->v5:
 * Squash patches together
 * Add extra pm_suspend_via_firmware() check for resume routine too
---
 drivers/platform/x86/intel/pmc/core.c | 13 +++++--------
 drivers/platform/x86/intel/pmc/core.h |  2 --
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
index b9591969e0fa..925c5d676a43 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -1179,12 +1179,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;
-
 	/* Check if the syspend will actually use S0ix */
 	if (pm_suspend_via_firmware())
 		return 0;
@@ -1197,7 +1191,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;
 }
 
@@ -1233,12 +1226,16 @@ 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)
+	/* Check if the syspend used S0ix */
+	if (pm_suspend_via_firmware())
 		return 0;
 
 	if (!pmc_core_is_s0ix_failed(pmcdev))
 		return 0;
 
+	if (!warn_on_s0ix_failures)
+		return 0;
+
 	if (pmc_core_is_pc10_failed(pmcdev)) {
 		/* S0ix failed because of PC10 entry failure */
 		dev_info(dev, "CPU did not enter PC10!!! (PC10 cnt=0x%llx)\n",
diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
index 810204d758ab..51d73efceaf3 100644
--- a/drivers/platform/x86/intel/pmc/core.h
+++ b/drivers/platform/x86/intel/pmc/core.h
@@ -319,7 +319,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
@@ -338,7 +337,6 @@ struct pmc_dev {
 	int pmc_xram_read_bit;
 	struct mutex lock; /* generic mutex lock for PMC Core */
 
-	bool check_counters; /* Check for counter increments on resume */
 	u64 pc10_counter;
 	u64 s0ix_counter;
 	int num_lpm_modes;
-- 
2.34.1

[PATCH v9 4/4] 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 so that
userspace software can query after the suspend cycle is done.

Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v8->v9:
 * Use FIELD_MAX and GENMASK
 * Add tags
v7->v8:
 * Report max sleep as well
---
 drivers/platform/x86/intel/pmc/core.c | 4 ++++
 drivers/platform/x86/intel/pmc/core.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
index 925c5d676a43..298f27ba1e10 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -1153,6 +1153,8 @@ static int pmc_core_probe(struct platform_device *pdev)
 	pmc_core_do_dmi_quirks(pmcdev);
 
 	pmc_core_dbgfs_register(pmcdev);
+	pm_report_max_hw_sleep(FIELD_MAX(SLP_S0_RES_COUNTER_MASK) *
+			       pmc_core_adjust_slp_s0_step(pmcdev, 1));
 
 	device_initialized = true;
 	dev_info(&pdev->dev, " initialized\n");
@@ -1214,6 +1216,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_report_hw_sleep_time((u32)(s0ix_counter - pmcdev->s0ix_counter));
+
 	if (s0ix_counter == pmcdev->s0ix_counter)
 		return true;
 
diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
index 51d73efceaf3..9ca9b9746719 100644
--- a/drivers/platform/x86/intel/pmc/core.h
+++ b/drivers/platform/x86/intel/pmc/core.h
@@ -16,6 +16,8 @@
 #include <linux/bits.h>
 #include <linux/platform_device.h>
 
+#define SLP_S0_RES_COUNTER_MASK			GENMASK(31, 0)
+
 #define PMC_BASE_ADDR_DEFAULT			0xFE000000
 
 /* Sunrise Point Power Management Controller PCI Device ID */
-- 
2.34.1

Re: [PATCH v9 0/4] Add vendor agnostic mechanism to report hardware sleep

[Hans de Goede]

Hi Mario, et al.,

On 4/14/23 03:23, Mario Limonciello wrote:
> An important part of validating that s0ix worked properly is to check how
> much of a cycle was spent in a hardware sleep state.
> 
> The reporting of hardware sleep is a mix of kernel messages and sysfs
> files that vary from vendor to vendor. Collecting this information
> requires extra information on the kernel command line or fetching from
> debugfs.
> 
> To make this information more readily accessible introduce a new file in
> suspend_stats that drivers can report into during their resume routine.
> 
> Userspace can fetch this information and compare it against the duration
> of the cycle to allow determining residency percentages and flagging
> problems.
> 
> Mario Limonciello (4):
>   PM: Add sysfs files to represent time spent in hardware sleep state
>   platform/x86/amd: pmc: Report duration of time in hw sleep state
>   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 | 29 +++++++++++++
>  drivers/platform/x86/amd/pmc.c        |  6 +--
>  drivers/platform/x86/intel/pmc/core.c | 17 ++++----
>  drivers/platform/x86/intel/pmc/core.h |  4 +-
>  include/linux/suspend.h               |  8 ++++
>  kernel/power/main.c                   | 59 +++++++++++++++++++++------
>  6 files changed, 98 insertions(+), 25 deletions(-)
> 
> 
> base-commit: 09a9639e56c01c7a00d6c0ca63f4c7c41abe075d

Thank you for working on this, this looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

For the series. Since this also touches kernel/power/main.c
I think it would be best if the entire series is merged
through the linux-pm tree and I'm fine with the pdx86 bits
also getting merged through linux-pm.

Rafael ?

Regards,

Hans

Re: [PATCH v9 0/4] Add vendor agnostic mechanism to report hardware sleep

[Rafael J. Wysocki]

On Mon, Apr 17, 2023 at 10:25 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Mario, et al.,
>
> On 4/14/23 03:23, Mario Limonciello wrote:
> > An important part of validating that s0ix worked properly is to check how
> > much of a cycle was spent in a hardware sleep state.
> >
> > The reporting of hardware sleep is a mix of kernel messages and sysfs
> > files that vary from vendor to vendor. Collecting this information
> > requires extra information on the kernel command line or fetching from
> > debugfs.
> >
> > To make this information more readily accessible introduce a new file in
> > suspend_stats that drivers can report into during their resume routine.
> >
> > Userspace can fetch this information and compare it against the duration
> > of the cycle to allow determining residency percentages and flagging
> > problems.
> >
> > Mario Limonciello (4):
> >   PM: Add sysfs files to represent time spent in hardware sleep state
> >   platform/x86/amd: pmc: Report duration of time in hw sleep state
> >   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 | 29 +++++++++++++
> >  drivers/platform/x86/amd/pmc.c        |  6 +--
> >  drivers/platform/x86/intel/pmc/core.c | 17 ++++----
> >  drivers/platform/x86/intel/pmc/core.h |  4 +-
> >  include/linux/suspend.h               |  8 ++++
> >  kernel/power/main.c                   | 59 +++++++++++++++++++++------
> >  6 files changed, 98 insertions(+), 25 deletions(-)
> >
> >
> > base-commit: 09a9639e56c01c7a00d6c0ca63f4c7c41abe075d
>
> Thank you for working on this, this looks good to me:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>
> For the series. Since this also touches kernel/power/main.c
> I think it would be best if the entire series is merged
> through the linux-pm tree and I'm fine with the pdx86 bits
> also getting merged through linux-pm.
>
> Rafael ?

That would be fine with me, but I've only got the [1/4].

Mario, can you please resend the series with CCs to linux-pm and with
R-by from Hans?