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

[PATCH 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 ->enter() 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 entering s2idle

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

-- 
2.34.1

[PATCH 1/4] ACPI: s2idle: Add a new ->enter() callback for platform_s2idle_ops

[Mario Limonciello]

On some platforms it is found that Linux more aggressively enters s2idle
than Windows enters Modern Standby and this uncovers some synchronization
issues for the platform.  To aid in debugging this class of problems in
the future, add support for an extra optional callback intended for
drivers to emit extra debugging.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/x86/s2idle.c | 14 ++++++++++++++
 include/linux/acpi.h      |  1 +
 include/linux/suspend.h   |  1 +
 kernel/power/suspend.c    |  3 +++
 4 files changed, 19 insertions(+)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index f9ac12b778e6..c984093a3657 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -486,6 +486,19 @@ int acpi_s2idle_prepare_late(void)
 	return 0;
 }
 
+void acpi_s2idle_enter(void)
+{
+	struct acpi_s2idle_dev_ops *handler;
+
+	if (!lps0_device_handle || sleep_no_lps0)
+		return;
+
+	list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node) {
+		if (handler->enter)
+			handler->enter();
+	}
+}
+
 void acpi_s2idle_restore_early(void)
 {
 	struct acpi_s2idle_dev_ops *handler;
@@ -527,6 +540,7 @@ static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = {
 	.begin = acpi_s2idle_begin,
 	.prepare = acpi_s2idle_prepare,
 	.prepare_late = acpi_s2idle_prepare_late,
+	.enter = acpi_s2idle_enter,
 	.wake = acpi_s2idle_wake,
 	.restore_early = acpi_s2idle_restore_early,
 	.restore = acpi_s2idle_restore,
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6f64b2f3dc54..9df14b5a875c 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1076,6 +1076,7 @@ struct acpi_s2idle_dev_ops {
 	struct list_head list_node;
 	void (*prepare)(void);
 	void (*restore)(void);
+	void (*enter)(void);
 };
 int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
 void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg);
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 70f2921e2e70..5a3fdca0a628 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -191,6 +191,7 @@ struct platform_s2idle_ops {
 	int (*begin)(void);
 	int (*prepare)(void);
 	int (*prepare_late)(void);
+	void (*enter)(void);
 	bool (*wake)(void);
 	void (*restore_early)(void);
 	void (*restore)(void);
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 827075944d28..0c08032d6b50 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -136,6 +136,9 @@ static void s2idle_loop(void)
 			break;
 		}
 
+		if (s2idle_ops && s2idle_ops->enter)
+			s2idle_ops->enter();
+
 		s2idle_enter();
 	}
 
-- 
2.34.1

[PATCH 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 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 4/4] platform/x86/amd: pmc: Add an extra STB message for entering s2idle

[Mario Limonciello]

The `enter` 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>
---
In this patch it's intentional that `int rc` isn't directly set to
`amd_pmc_write_stb()` so that if a workaround is added related to this
in the future it can come before the STB message about entering s2idle
without having to change either of those lines.

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

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index fba42036682d..0f1b5b1a0bb8 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_ENTER	0xC6000003
 
 /* STB S2D(Spill to DRAM) has different message port offset */
 #define STB_SPILL_TO_DRAM		0xBE
@@ -706,6 +707,17 @@ static void amd_pmc_s2idle_prepare(void)
 		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
 }
 
+static void amd_pmc_s2idle_enter(void)
+{
+	struct amd_pmc_dev *pdev = &pmc;
+	int rc;
+
+	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_ENTER);
+	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 +745,7 @@ static void amd_pmc_s2idle_restore(void)
 
 static struct acpi_s2idle_dev_ops amd_pmc_s2idle_dev_ops = {
 	.prepare = amd_pmc_s2idle_prepare,
+	.enter = amd_pmc_s2idle_enter,
 	.restore = amd_pmc_s2idle_restore,
 };
 #endif
-- 
2.34.1

Re: [PATCH 1/4] ACPI: s2idle: Add a new ->enter() callback for platform_s2idle_ops

[kernel test robot]

Hi Mario,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on linus/master v6.0-rc2 next-20220826]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/Add-some-extra-debugging-mechanisms-for-s0i3/20220829-062334
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20220829/202208290836.C3cKDij9-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/af6400b51370a2bc04906697aeec5a938e6ee446
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mario-Limonciello/Add-some-extra-debugging-mechanisms-for-s0i3/20220829-062334
        git checkout af6400b51370a2bc04906697aeec5a938e6ee446
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/acpi/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/acpi/x86/s2idle.c:489:6: warning: no previous prototype for 'acpi_s2idle_enter' [-Wmissing-prototypes]
     489 | void acpi_s2idle_enter(void)
         |      ^~~~~~~~~~~~~~~~~


vim +/acpi_s2idle_enter +489 drivers/acpi/x86/s2idle.c

   488	
 > 489	void acpi_s2idle_enter(void)
   490	{
   491		struct acpi_s2idle_dev_ops *handler;
   492	
   493		if (!lps0_device_handle || sleep_no_lps0)
   494			return;
   495	
   496		list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node) {
   497			if (handler->enter)
   498				handler->enter();
   499		}
   500	}
   501	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 1/4] ACPI: s2idle: Add a new ->enter() callback for platform_s2idle_ops

[kernel test robot]

Hi Mario,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on linus/master v6.0-rc3 next-20220826]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/Add-some-extra-debugging-mechanisms-for-s0i3/20220829-062334
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-randconfig-a011-20220829 (https://download.01.org/0day-ci/archive/20220829/202208291045.mtIt51vk-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/af6400b51370a2bc04906697aeec5a938e6ee446
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mario-Limonciello/Add-some-extra-debugging-mechanisms-for-s0i3/20220829-062334
        git checkout af6400b51370a2bc04906697aeec5a938e6ee446
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/acpi/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/acpi/x86/s2idle.c:489:6: warning: no previous prototype for function 'acpi_s2idle_enter' [-Wmissing-prototypes]
   void acpi_s2idle_enter(void)
        ^
   drivers/acpi/x86/s2idle.c:489:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void acpi_s2idle_enter(void)
   ^
   static 
   1 warning generated.


vim +/acpi_s2idle_enter +489 drivers/acpi/x86/s2idle.c

   488	
 > 489	void acpi_s2idle_enter(void)
   490	{
   491		struct acpi_s2idle_dev_ops *handler;
   492	
   493		if (!lps0_device_handle || sleep_no_lps0)
   494			return;
   495	
   496		list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node) {
   497			if (handler->enter)
   498				handler->enter();
   499		}
   500	}
   501	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 1/4] ACPI: s2idle: Add a new ->enter() callback for platform_s2idle_ops

[kernel test robot]

Hi Mario,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on linus/master v6.0-rc3 next-20220826]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/Add-some-extra-debugging-mechanisms-for-s0i3/20220829-062334
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-s003 (https://download.01.org/0day-ci/archive/20220829/202208291354.DVB7CN3N-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/af6400b51370a2bc04906697aeec5a938e6ee446
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mario-Limonciello/Add-some-extra-debugging-mechanisms-for-s0i3/20220829-062334
        git checkout af6400b51370a2bc04906697aeec5a938e6ee446
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/acpi/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

sparse warnings: (new ones prefixed by >>)
   drivers/acpi/x86/s2idle.c:426:13: sparse: sparse: restricted suspend_state_t degrades to integer
   drivers/acpi/x86/s2idle.c:426:33: sparse: sparse: restricted suspend_state_t degrades to integer
>> drivers/acpi/x86/s2idle.c:489:6: sparse: sparse: symbol 'acpi_s2idle_enter' was not declared. Should it be static?

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp