[PATCH v3 0/4] Add some extra debugging mechanisms for s0i3

[PATCH v3 0/4] Add some extra debugging mechanisms for s0i3

[Mario Limonciello]

Recently there have been reports of problems where the system consumes
too much power after certain interrupts occur that would notify the
kernel of some event but those events aren't marked for wakeup.

These problems have been root caused to the timing of the kernel moving
the cores into ACPI C3 relative to other events from the previous wakeup
not being settled.  Linux will more aggressively move the cores into C3
for s2idle than Windows does for Modern Standby.

To aide with debugging this class of problems in the future add a new
set of optional debugging infrastructure.

Mario Limonciello (4):
  ACPI: s2idle: Add a new ->check() callback for platform_s2idle_ops
  platform/x86/amd: pmc: Add defines for STB events
  platform/x86/amd: pmc: Always write to the STB
  platform/x86/amd: pmc: Add an extra STB message for checking s2idle
    entry

 drivers/acpi/sleep.h           |  1 +
 drivers/acpi/x86/s2idle.c      | 14 ++++++++++++++
 drivers/platform/x86/amd/pmc.c | 32 ++++++++++++++++++++------------
 include/linux/acpi.h           |  1 +
 include/linux/suspend.h        |  1 +
 kernel/power/suspend.c         |  3 +++
 6 files changed, 40 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH v3 2/4] platform/x86/amd: pmc: Add defines for STB events

[Mario Limonciello]

Currently `amd-pmc` has two events, but just adds one to the first to
distinguish the second.  Add a clear definition what these events mean.

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

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index 700eb19e8450..39e6ab88861d 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -39,7 +39,8 @@
 #define AMD_PMC_STB_INDEX_ADDRESS	0xF8
 #define AMD_PMC_STB_INDEX_DATA		0xFC
 #define AMD_PMC_STB_PMI_0		0x03E30600
-#define AMD_PMC_STB_PREDEF		0xC6000001
+#define AMD_PMC_STB_S2IDLE_PREPARE	0xC6000001
+#define AMD_PMC_STB_S2IDLE_RESTORE	0xC6000002
 
 /* STB S2D(Spill to DRAM) has different message port offset */
 #define STB_SPILL_TO_DRAM		0xBE
@@ -701,7 +702,7 @@ static void amd_pmc_s2idle_prepare(void)
 	}
 
 	if (enable_stb) {
-		rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF);
+		rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_PREPARE);
 		if (rc)
 			dev_err(pdev->dev, "error writing to STB: %d\n", rc);
 	}
@@ -724,9 +725,8 @@ static void amd_pmc_s2idle_restore(void)
 	/* Dump the IdleMask to see the blockers */
 	amd_pmc_idlemask_read(pdev, pdev->dev, NULL);
 
-	/* Write data incremented by 1 to distinguish in stb_read */
 	if (enable_stb) {
-		rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF + 1);
+		rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_RESTORE);
 		if (rc)
 			dev_err(pdev->dev, "error writing to STB: %d\n", rc);
 	}
-- 
2.34.1

[PATCH v3 3/4] platform/x86/amd: pmc: Always write to the STB

[Mario Limonciello]

The kernel parameter `enable_stb` currently gates the access to the STB
from debugfs and also controls whether the kernel writes events to the
STB.

Even if not accessing STB data from the kernel it's useful to have this
data stored to review the STB. So in suspend/resume always write it.

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

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index 39e6ab88861d..fba42036682d 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -701,11 +701,9 @@ static void amd_pmc_s2idle_prepare(void)
 		return;
 	}
 
-	if (enable_stb) {
-		rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_PREPARE);
-		if (rc)
-			dev_err(pdev->dev, "error writing to STB: %d\n", rc);
-	}
+	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_PREPARE);
+	if (rc)
+		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
 }
 
 static void amd_pmc_s2idle_restore(void)
@@ -725,11 +723,9 @@ static void amd_pmc_s2idle_restore(void)
 	/* Dump the IdleMask to see the blockers */
 	amd_pmc_idlemask_read(pdev, pdev->dev, NULL);
 
-	if (enable_stb) {
-		rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_RESTORE);
-		if (rc)
-			dev_err(pdev->dev, "error writing to STB: %d\n", rc);
-	}
+	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_RESTORE);
+	if (rc)
+		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
 
 	/* Notify on failed entry */
 	amd_pmc_validate_deepest(pdev);
-- 
2.34.1

[PATCH v3 4/4] platform/x86/amd: pmc: Add an extra STB message for checking s2idle entry

[Mario Limonciello]

The `check` callback is run right before the cores are put into HLT.
This will allow checking synchronization problems with other software
that writes into the STB.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v2->v3:
 * Update for changes in patch 1/4.

 drivers/platform/x86/amd/pmc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index fba42036682d..32887687f888 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -41,6 +41,7 @@
 #define AMD_PMC_STB_PMI_0		0x03E30600
 #define AMD_PMC_STB_S2IDLE_PREPARE	0xC6000001
 #define AMD_PMC_STB_S2IDLE_RESTORE	0xC6000002
+#define AMD_PMC_STB_S2IDLE_CHECK	0xC6000003
 
 /* STB S2D(Spill to DRAM) has different message port offset */
 #define STB_SPILL_TO_DRAM		0xBE
@@ -706,6 +707,16 @@ static void amd_pmc_s2idle_prepare(void)
 		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
 }
 
+static void amd_pmc_s2idle_check(void)
+{
+	struct amd_pmc_dev *pdev = &pmc;
+	int rc;
+
+	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_CHECK);
+	if (rc)
+		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
+}
+
 static void amd_pmc_s2idle_restore(void)
 {
 	struct amd_pmc_dev *pdev = &pmc;
@@ -733,6 +744,7 @@ static void amd_pmc_s2idle_restore(void)
 
 static struct acpi_s2idle_dev_ops amd_pmc_s2idle_dev_ops = {
 	.prepare = amd_pmc_s2idle_prepare,
+	.check = amd_pmc_s2idle_check,
 	.restore = amd_pmc_s2idle_restore,
 };
 #endif
-- 
2.34.1

Re: [PATCH v3 0/4] Add some extra debugging mechanisms for s0i3

[Hans de Goede]

Hi All,

On 8/29/22 18:29, Mario Limonciello wrote:
> Recently there have been reports of problems where the system consumes
> too much power after certain interrupts occur that would notify the
> kernel of some event but those events aren't marked for wakeup.
> 
> These problems have been root caused to the timing of the kernel moving
> the cores into ACPI C3 relative to other events from the previous wakeup
> not being settled.  Linux will more aggressively move the cores into C3
> for s2idle than Windows does for Modern Standby.
> 
> To aide with debugging this class of problems in the future add a new
> set of optional debugging infrastructure.
> 
> Mario Limonciello (4):
>   ACPI: s2idle: Add a new ->check() callback for platform_s2idle_ops
>   platform/x86/amd: pmc: Add defines for STB events
>   platform/x86/amd: pmc: Always write to the STB
>   platform/x86/amd: pmc: Add an extra STB message for checking s2idle
>     entry


Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> 
>  drivers/acpi/sleep.h           |  1 +
>  drivers/acpi/x86/s2idle.c      | 14 ++++++++++++++
>  drivers/platform/x86/amd/pmc.c | 32 ++++++++++++++++++++------------
>  include/linux/acpi.h           |  1 +
>  include/linux/suspend.h        |  1 +
>  kernel/power/suspend.c         |  3 +++
>  6 files changed, 40 insertions(+), 12 deletions(-)
>