[PATCH 0/4] Some fixes for amd-pmc handling of the STB

[PATCH 0/4] Some fixes for amd-pmc handling of the STB

[Mario Limonciello]

Not all platforms support STB, and by activating it on a platform without
support a few bugs were exposed.

This series cleans them up.

Cc: Francis De Brabandere <francisdb@gmail.com>

Mario Limonciello (4):
  platform/x86/amd/pmc: Fix LPS0 and debugfs leaks when STB init fails
  platform/x86/amd/pmc: Fix error handling in amd_stb_s2d_init()
  platform/x86/amd/pmc: Do not fail probe when STB init fails
  platform/x86/amd/pmc: Validate S2D physical address before ioremap

 drivers/platform/x86/amd/pmc/mp1_stb.c | 43 +++++++++++++++++---------
 drivers/platform/x86/amd/pmc/pmc.c     | 11 ++++++-
 2 files changed, 39 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH 1/4] platform/x86/amd/pmc: Fix LPS0 and debugfs leaks when STB init fails

[Mario Limonciello]

amd_pmc_probe() registers the LPS0 s2idle handler with
acpi_register_lps0_dev() and creates the driver's debugfs directory
before calling amd_stb_s2d_init(), which is the last step in probe that
can fail.

When amd_stb_s2d_init() fails (for example the S2D telemetry region
cannot be ioremapped on a long-running system, or the SMU rejects the
S2D setup) the error path only calls pci_dev_put() and returns.  This
leaves amd_pmc_s2idle_dev_ops on the global lps0_s2idle_devops_head list
and leaks the debugfs directory, while the devm-managed resources
backing the handler are torn down.

Reloading the module then walks the corrupted list in
acpi_register_lps0_dev() and hits:

  list_add corruption. next->prev should be prev, but was NULL.
  kernel BUG at lib/list_debug.c:29!
   acpi_register_lps0_dev+0x44/0x80
   amd_pmc_probe+0x224/0x380 [amd_pmc]
   platform_probe+0x67/0x90

Even without a reload, the stale registration means the next s2idle
transition calls into torn-down driver state.

Unwind the debugfs directory and the LPS0 registration on the
amd_stb_s2d_init() error path.  acpi_unregister_lps0_dev() is safe to
call unconditionally here: it is guarded on the same conditions as
acpi_register_lps0_dev(), which is exactly what amd_pmc_remove() already
relies on.

Assisted-by: Claude:opus
Reported-by: Francis De Brabandere <francisdb@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221759
Tested-by: Francis De Brabandere <francisdb@gmail.com>
Fixes: 83ad6974dd3b ("platform/x86/amd/pmc: Move STB block into amd_pmc_s2d_init()")
Cc: stable@vger.kernel.org
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmc/pmc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index d50ea62fa2f3a..630a664bdd2f4 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -919,13 +919,17 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	amd_pmc_dbgfs_register(dev);
 	err = amd_stb_s2d_init(dev);
 	if (err)
-		goto err_pci_dev_put;
+		goto err_dbgfs_unregister;
 
 	if (IS_ENABLED(CONFIG_AMD_MP2_STB))
 		amd_mp2_stb_init(dev);
 	pm_report_max_hw_sleep(U64_MAX);
 	return 0;
 
+err_dbgfs_unregister:
+	amd_pmc_dbgfs_unregister(dev);
+	if (IS_ENABLED(CONFIG_SUSPEND))
+		acpi_unregister_lps0_dev(&amd_pmc_s2idle_dev_ops);
 err_pci_dev_put:
 	pci_dev_put(rdev);
 	return err;
-- 
2.43.0

[PATCH 2/4] platform/x86/amd/pmc: Fix error handling in amd_stb_s2d_init()

[Mario Limonciello]

amd_stb_s2d_init() has two problems on its error paths:

 - The return value of the S2D_TELEMETRY_SIZE SMU command is discarded.
   When the SMU refuses the command (e.g. "SMU cmd failed. err: 0xff")
   the failure is only noticed indirectly through the telemetry size
   check and reported as -EIO, masking the real error.

 - dev->msg_port is switched to MSG_PORT_S2D before issuing the S2D SMU
   commands but is only restored to MSG_PORT_PMC on the success path.
   The early "return -EIO" leaves the port stuck on MSG_PORT_S2D, so all
   subsequent SMU communication - including the s2idle prepare/restore
   handlers - is directed at the wrong mailbox.

Consolidate the exit path through a single label so the message port is
always restored, and propagate the SMU command error directly instead of
inferring it from the size.

Assisted-by: Claude:opus
Reported-by: Francis De Brabandere <francisdb@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221759
Tested-by: Francis De Brabandere <francisdb@gmail.com>
Fixes: 3d7d407dfb05 ("platform/x86: amd-pmc: Add support for AMD Spill to DRAM STB feature")
Cc: stable@vger.kernel.org
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmc/mp1_stb.c | 28 +++++++++++++++-----------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
index 753d630f3283d..6a048cb2605ec 100644
--- a/drivers/platform/x86/amd/pmc/mp1_stb.c
+++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
@@ -289,7 +289,7 @@ int amd_stb_s2d_init(struct amd_pmc_dev *dev)
 	u32 phys_addr_low, phys_addr_hi;
 	u64 stb_phys_addr;
 	u32 size = 0;
-	int ret;
+	int ret = 0;
 
 	if (!enable_stb)
 		return 0;
@@ -306,13 +306,17 @@ int amd_stb_s2d_init(struct amd_pmc_dev *dev)
 	/* Spill to DRAM feature uses separate SMU message port */
 	dev->msg_port = MSG_PORT_S2D;
 
-	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->stb_arg.s2d_msg_id, true);
-	if (size != S2D_TELEMETRY_BYTES_MAX)
-		return -EIO;
+	ret = amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->stb_arg.s2d_msg_id, true);
+	if (ret)
+		goto out;
+	if (size != S2D_TELEMETRY_BYTES_MAX) {
+		ret = -EIO;
+		goto out;
+	}
 
-	/* Get DRAM size */
-	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->stb_arg.s2d_msg_id, true);
-	if (ret || !dev->dram_size)
+	/* Get DRAM size; fall back to the default if the query fails */
+	if (amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->stb_arg.s2d_msg_id, true) ||
+	    !dev->dram_size)
 		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
 
 	/* Get STB DRAM address */
@@ -321,12 +325,12 @@ int amd_stb_s2d_init(struct amd_pmc_dev *dev)
 
 	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
 
-	/* Clear msg_port for other SMU operation */
-	dev->msg_port = MSG_PORT_PMC;
-
 	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
 	if (!dev->stb_virt_addr)
-		return -ENOMEM;
+		ret = -ENOMEM;
 
-	return 0;
+out:
+	/* Restore the default message port for subsequent SMU operations */
+	dev->msg_port = MSG_PORT_PMC;
+	return ret;
 }
-- 
2.43.0

[PATCH 3/4] platform/x86/amd/pmc: Do not fail probe when STB init fails

[Mario Limonciello]

STB (Spill to DRAM) is an optional debugging facility that is only
enabled through the enable_stb module parameter.  On some platforms the
SMU refuses the S2D setup outright, and on long-running systems the large
telemetry region can fail to ioremap.  In either case amd_stb_s2d_init()
returns an error and, because probe treated that as fatal, the entire
PMC driver failed to load - silently disabling s0i3 support even though
STB is only a debug aid.

Downgrade the failure to a warning and continue probing so that s0i3
support via the LPS0 handler no longer depends on an optional debug
feature.

Since probe no longer aborts on this path, the LPS0 and debugfs
unwinding added by the earlier fix in this series becomes unreachable
and is removed.

Assisted-by: Claude:opus
Reported-by: Francis De Brabandere <francisdb@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221759
Tested-by: Francis De Brabandere <francisdb@gmail.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmc/pmc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index 630a664bdd2f4..5b6c90abd4e5f 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -917,19 +917,24 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	}
 
 	amd_pmc_dbgfs_register(dev);
+
+	/*
+	 * STB is an optional debugging facility, only enabled via the
+	 * enable_stb module parameter.  A failure to initialize it (e.g. the
+	 * SMU refusing the request, or the telemetry region failing to map)
+	 * must not prevent the rest of the driver - most importantly the s0i3
+	 * LPS0 handler - from working, so treat it as non-fatal.
+	 */
 	err = amd_stb_s2d_init(dev);
 	if (err)
-		goto err_dbgfs_unregister;
+		dev_warn(dev->dev, "STB initialization failed (%d), continuing without STB support\n",
+			 err);
 
 	if (IS_ENABLED(CONFIG_AMD_MP2_STB))
 		amd_mp2_stb_init(dev);
 	pm_report_max_hw_sleep(U64_MAX);
 	return 0;
 
-err_dbgfs_unregister:
-	amd_pmc_dbgfs_unregister(dev);
-	if (IS_ENABLED(CONFIG_SUSPEND))
-		acpi_unregister_lps0_dev(&amd_pmc_s2idle_dev_ops);
 err_pci_dev_put:
 	pci_dev_put(rdev);
 	return err;
-- 
2.43.0

[PATCH 4/4] platform/x86/amd/pmc: Validate S2D physical address before ioremap

[Mario Limonciello]

amd_stb_s2d_init() retrieves the S2D telemetry buffer's physical address
via two SMU commands (low and high 32 bits) but does not check their
return values.  On some platforms the SMU refuses these commands
(observed: "SMU cmd failed. err: 0xff") and the phys_addr_low/high
output parameters are left uninitialized at zero.

The driver then attempts devm_ioremap(dev, 0x0, 16 MB), which trips the
ioremap-on-RAM warning and fails:

  amd_pmc AMDI000B:00: SMU cmd failed. err: 0xff
  ioremap on RAM at 0x0000000000000000 - 0x0000000000ffffff
  WARNING: CPU: 13 PID: 4592 at arch/x86/mm/ioremap.c:216
   __ioremap_caller+0xfc/0x3b0
   devm_ioremap+0x5a/0xb0
   amd_stb_s2d_init+0x239/0x280 [amd_pmc]
  amd_pmc AMDI000B:00: STB initialization failed (-12), continuing...

Probe completes successfully but the kernel is tainted by the WARN.

Check both physical-address command return codes and reject a zero
address before attempting the ioremap.

Assisted-by: Claude:opus
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmc/mp1_stb.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
index 6a048cb2605ec..a507444a945ae 100644
--- a/drivers/platform/x86/amd/pmc/mp1_stb.c
+++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
@@ -320,10 +320,21 @@ int amd_stb_s2d_init(struct amd_pmc_dev *dev)
 		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
 
 	/* Get STB DRAM address */
-	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->stb_arg.s2d_msg_id, true);
-	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->stb_arg.s2d_msg_id, true);
+	ret = amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low,
+			       dev->stb_arg.s2d_msg_id, true);
+	if (ret)
+		goto out;
+	ret = amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi,
+			       dev->stb_arg.s2d_msg_id, true);
+	if (ret)
+		goto out;
 
 	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
+	if (!stb_phys_addr) {
+		dev_err(dev->dev, "S2D phys addr query returned invalid address\n");
+		ret = -ENXIO;
+		goto out;
+	}
 
 	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
 	if (!dev->stb_virt_addr)
-- 
2.43.0