[PATCH 0/8] soundwire: amd: SoundWire manager driver bug fixes

[PATCH 0/8] soundwire: amd: SoundWire manager driver bug fixes

[Vijendar Mukunda]

This series collects eight bug fixes for the AMD SoundWire manager
driver (ACP6.3/7.0/7.1/7.2). The patches address correctness issues
in command response handling, slave status caching, work-queue teardown
ordering, resource cleanup on probe failure, and power-management
suspend/resume sequences.

Patch 1 fixes a silent type widening in amd_sdw_send_cmd_get_resp()
where -ETIMEDOUT was returned through a u64, producing a large positive
value instead of an error.

Patch 2 caches the last successful ping slave status so that a command
timeout during sdw_show_ping_status() does not report a spurious
"no peripherals attached" warning.

Patch 3 fixes the work-drain sequence in amd_sdw_manager_remove(): the
interrupt delivery gate must be closed before cancel_work_sync() to
prevent an in-flight IRQ handler from re-queuing work after the cancel
returns. A pm_runtime_disable() imbalance on partially-started managers
is also corrected.

Patch 4 adds the missing sdw_amd_exit() call and ctx pointer clear on
the sdw_amd_startup() failure path in sdw_amd_probe().

Patch 5 propagates the amd_init_sdw_manager() return value in
amd_resume_runtime() instead of ignoring it.

Patch 6 removes the amd_deinit_sdw_manager() helper and expands its
two call sites in the POWER_OFF suspend path to direct calls, making
the sequence explicit with no functional change.

Patch 7 replaces open-ended acp_rev >= ACP70_PCI_REV_ID comparisons
in the PM callbacks with explicit per-revision handling covering
ACP63, ACP70, ACP71 and ACP72, with a dev_err + -EINVAL default. The
repeated host-wake-by-revision block is factored into a helper,
amd_sdw_host_wake_enable_by_rev().

Patch 8 fixes the interrupt gate and work drain ordering in all four
suspend mode/path combinations: the ACP_EXTERNAL_INTR_CNTL gate must
be closed before cancel_work_sync() calls to prevent a racing ISR from
re-queuing work after the drain completes.

Vijendar Mukunda (8):
  soundwire: amd: fix SDW command timeout return value handling
  soundwire: amd: cache ping slave status to avoid spurious disconnect
    on timeout
  soundwire: amd: fix work drain ordering and pm_runtime guard in remove
    path
  soundwire: amd: fix ctx leak when sdw_amd_startup() fails
  soundwire: amd: propagate amd_init_sdw_manager() error on resume
  soundwire: amd: drop amd_deinit_sdw_manager() in POWER_OFF suspend
  soundwire: amd: replace >= ACP70 with explicit switch/case in PM paths
  soundwire: amd: fix interrupt gate and work drain ordering in PM ops

 drivers/soundwire/amd_init.c      |   9 +-
 drivers/soundwire/amd_manager.c   | 204 ++++++++++++++++++++----------
 include/linux/soundwire/sdw_amd.h |   3 +
 3 files changed, 148 insertions(+), 68 deletions(-)

-- 
2.48.1

[PATCH 1/8] soundwire: amd: fix SDW command timeout return value handling

[Vijendar Mukunda]

amd_sdw_send_cmd_get_resp() returned u64 but was returning -ETIMEDOUT
(a negative int) on readl_poll_timeout() failures, which silently
widens to a large u64 value.

Change the return type to int and pass the raw u64 response through an
out-parameter. Timeout is detected directly from readl_poll_timeout()
return value.

Update all callers: amd_program_scp_addr(), _amd_sdw_xfer_msg(),
amd_sdw_read_and_process_ping_status(), and amd_sdw_read_ping_status()
to check the return value and handle timeout explicitly. In
amd_sdw_read_and_process_ping_status(), the mutex unlock is also moved
to after amd_sdw_process_ping_status() so the lock is held across the
full status processing step.

Fixes: d8f48fbdfd9a ("soundwire: amd: Add support for AMD Manager driver")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 56 +++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index a3316efdf8ac..fa3f4e797edd 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -250,10 +250,9 @@ static void amd_sdw_ctl_word_prep(u32 *lower_word, u32 *upper_word, struct sdw_m
 	*lower_word = lower_data;
 }
 
-static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lower_data,
-				     u32 upper_data)
+static int amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lower_data,
+				     u32 upper_data, u64 *response)
 {
-	u64 resp;
 	u32 lower_resp, upper_resp;
 	u32 sts;
 	int ret;
@@ -291,9 +290,8 @@ static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lo
 			amd_manager->instance);
 		return ret;
 	}
-	resp = upper_resp;
-	resp = (resp << 32) | lower_resp;
-	return resp;
+	*response = ((u64)upper_resp << 32) | lower_resp;
+	return 0;
 }
 
 static enum sdw_command_response
@@ -309,19 +307,22 @@ amd_program_scp_addr(struct amd_sdw_manager *amd_manager, struct sdw_msg *msg)
 	scp_msg.buf = &msg->addr_page1;
 	scp_msg.flags = SDW_MSG_FLAG_WRITE;
 	amd_sdw_ctl_word_prep(&lower_data, &upper_data, &scp_msg, 0);
-	response_buf[0] = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data);
+	if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data, &response_buf[0])) {
+		dev_err_ratelimited(amd_manager->dev,
+				    "SCP_addrpage command timeout for Slave %d\n", msg->dev_num);
+		return SDW_CMD_TIMEOUT;
+	}
 	scp_msg.addr = SDW_SCP_ADDRPAGE2;
 	scp_msg.buf = &msg->addr_page2;
 	amd_sdw_ctl_word_prep(&lower_data, &upper_data, &scp_msg, 0);
-	response_buf[1] = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data);
+	if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data, &response_buf[1])) {
+		dev_err_ratelimited(amd_manager->dev,
+				    "SCP_addrpage command timeout for Slave %d\n", msg->dev_num);
+		return SDW_CMD_TIMEOUT;
+	}
 
 	for (index = 0; index < 2; index++) {
-		if (response_buf[index] == -ETIMEDOUT) {
-			dev_err_ratelimited(amd_manager->dev,
-					    "SCP_addrpage command timeout for Slave %d\n",
-					    msg->dev_num);
-			return SDW_CMD_TIMEOUT;
-		} else if (!(response_buf[index] & AMD_SDW_MCP_RESP_ACK)) {
+		if (!(response_buf[index] & AMD_SDW_MCP_RESP_ACK)) {
 			if (response_buf[index] & AMD_SDW_MCP_RESP_NACK) {
 				dev_err_ratelimited(amd_manager->dev,
 						    "SCP_addrpage NACKed for Slave %d\n",
@@ -366,11 +367,7 @@ static enum sdw_command_response amd_sdw_fill_msg_resp(struct amd_sdw_manager *a
 		if (msg->flags == SDW_MSG_FLAG_READ)
 			msg->buf[offset] = FIELD_GET(AMD_SDW_MCP_RESP_RDATA, response);
 	} else {
-		if (response == -ETIMEDOUT) {
-			dev_err_ratelimited(amd_manager->dev, "command timeout for Slave %d\n",
-					    msg->dev_num);
-			return SDW_CMD_TIMEOUT;
-		} else if (response & AMD_SDW_MCP_RESP_NACK) {
+		if (response & AMD_SDW_MCP_RESP_NACK) {
 			dev_err_ratelimited(amd_manager->dev,
 					    "command response NACK received for Slave %d\n",
 					    msg->dev_num);
@@ -390,7 +387,11 @@ static unsigned int _amd_sdw_xfer_msg(struct amd_sdw_manager *amd_manager, struc
 	u32 upper_data = 0, lower_data = 0;
 
 	amd_sdw_ctl_word_prep(&lower_data, &upper_data, msg, cmd_offset);
-	response = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data);
+	if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data, &response)) {
+		dev_err_ratelimited(amd_manager->dev, "command timeout for Slave %d\n",
+				    msg->dev_num);
+		return SDW_CMD_TIMEOUT;
+	}
 	return amd_sdw_fill_msg_resp(amd_manager, msg, response, cmd_offset);
 }
 
@@ -446,9 +447,14 @@ static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_man
 	u64 response;
 
 	mutex_lock(&amd_manager->bus.msg_lock);
-	response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0);
-	mutex_unlock(&amd_manager->bus.msg_lock);
+	if (amd_sdw_send_cmd_get_resp(amd_manager, 0, 0, &response)) {
+		mutex_unlock(&amd_manager->bus.msg_lock);
+		dev_err_ratelimited(amd_manager->dev, "SDW%x ping status timeout\n",
+				    amd_manager->instance);
+		return;
+	}
 	amd_sdw_process_ping_status(response, amd_manager);
+	mutex_unlock(&amd_manager->bus.msg_lock);
 }
 
 static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
@@ -457,7 +463,11 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
 	u64 response;
 	u32 slave_stat;
 
-	response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0);
+	if (amd_sdw_send_cmd_get_resp(amd_manager, 0, 0, &response)) {
+		dev_err_ratelimited(amd_manager->dev, "SDW%x ping status timeout\n",
+				    amd_manager->instance);
+		return 0;
+	}
 	/* slave status from ping response */
 	slave_stat = FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_0_3, response);
 	slave_stat |= FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_4_11, response) << 8;
-- 
2.48.1

[PATCH 2/8] soundwire: amd: cache ping slave status to avoid spurious disconnect on timeout

[Vijendar Mukunda]

Add ping_slave_stat to struct amd_sdw_manager to cache the slave_stat
from the last successful ping response. On timeout,
amd_sdw_read_ping_status() returns the cached value (or 0 if no
successful response yet) to avoid the spurious "no peripherals
attached" warning from sdw_show_ping_status().

ping_slave_stat is initialised to U32_MAX in probe and reset to
U32_MAX under msg_lock on entry to the POWER_OFF_MODE resume path,
before any hardware step that can fail and return early, so that a
partially-failed resume cannot leave a stale pre-suspend value behind
for a subsequent ping timeout to report.

Add lockdep_assert_held() in amd_sdw_read_ping_status() to document
that the caller must hold msg_lock.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c   | 21 ++++++++++++++++++++-
 include/linux/soundwire/sdw_amd.h |  3 +++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index fa3f4e797edd..a57b59609bfe 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -453,6 +453,9 @@ static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_man
 				    amd_manager->instance);
 		return;
 	}
+	amd_manager->ping_slave_stat =
+		FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_0_3, response) |
+		(FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_4_11, response) << 8);
 	amd_sdw_process_ping_status(response, amd_manager);
 	mutex_unlock(&amd_manager->bus.msg_lock);
 }
@@ -463,15 +466,19 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
 	u64 response;
 	u32 slave_stat;
 
+	/* Called by sdw_show_ping_status() which holds msg_lock. */
+	lockdep_assert_held(&bus->msg_lock);
+
 	if (amd_sdw_send_cmd_get_resp(amd_manager, 0, 0, &response)) {
 		dev_err_ratelimited(amd_manager->dev, "SDW%x ping status timeout\n",
 				    amd_manager->instance);
-		return 0;
+		return amd_manager->ping_slave_stat == U32_MAX ? 0 : amd_manager->ping_slave_stat;
 	}
 	/* slave status from ping response */
 	slave_stat = FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_0_3, response);
 	slave_stat |= FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_4_11, response) << 8;
 	dev_dbg(amd_manager->dev, "slave_stat:0x%x\n", slave_stat);
+	amd_manager->ping_slave_stat = slave_stat;
 	return slave_stat;
 }
 
@@ -1087,6 +1094,7 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)
 		dev_err(dev, "mmio not found\n");
 		return -ENOMEM;
 	}
+	amd_manager->ping_slave_stat = U32_MAX;
 	amd_manager->instance = pdata->instance;
 	amd_manager->mmio = amd_manager->acp_mmio +
 			    (amd_manager->instance * SDW_MANAGER_REG_OFFSET);
@@ -1414,6 +1422,17 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 		}
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
 		writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance));
+		/*
+		 * POWER_OFF_MODE tears the bus down completely; invalidate the
+		 * cached ping status on entry, before any step that can fail and
+		 * return early, so that a partially-failed resume leaves the first
+		 * post-resume ping timeout returning 0 rather than stale
+		 * pre-suspend data. CLK_STOP_MODE leaves peripherals attached, so
+		 * the cache remains valid there.
+		 */
+		mutex_lock(&amd_manager->bus.msg_lock);
+		amd_manager->ping_slave_stat = U32_MAX;
+		mutex_unlock(&amd_manager->bus.msg_lock);
 		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
 			ret = amd_sdw_host_wake_enable(amd_manager, false);
 			if (ret)
diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h
index 470360a2723c..12537904e34a 100644
--- a/include/linux/soundwire/sdw_amd.h
+++ b/include/linux/soundwire/sdw_amd.h
@@ -76,6 +76,8 @@ struct sdw_amd_dai_runtime {
  * @acp_rev: acp pci device revision id
  * @clk_stopped: flag set to true when clock is stopped
  * @power_mode_mask: flag interprets amd SoundWire manager power mode
+ * @ping_slave_stat: cached slave status from the last successful ping response;
+ *                   initialised to U32_MAX ("no valid reading yet")
  * @dai_runtime_array: dai runtime array
  */
 struct amd_sdw_manager {
@@ -105,6 +107,7 @@ struct amd_sdw_manager {
 	u32 wake_en_mask;
 	u32 power_mode_mask;
 	u32 acp_rev;
+	u32 ping_slave_stat;
 	bool clk_stopped;
 
 	struct sdw_amd_dai_runtime **dai_runtime_array;
-- 
2.48.1

[PATCH 3/8] soundwire: amd: fix work drain ordering and pm_runtime guard in remove path

[Vijendar Mukunda]

amd_sdw_manager_remove() cancelled amd_sdw_work but not
amd_sdw_irq_thread. Since amd_sdw_irq_thread() calls
schedule_work(&amd_sdw_work), an in-flight irq_thread item can
re-queue amd_sdw_work after its cancel returns, defeating the
cancellation.

Fix by calling amd_disable_sdw_interrupts() first to quiesce the
hardware IRQ source, then cancel_work_sync() for amd_sdw_irq_thread,
then cancel_work_sync() for amd_sdw_work. The existing
cancel_work_sync(amd_sdw_work) is also moved to after
amd_disable_sdw_interrupts() so that any work item queued between the
old cancel position and the interrupt disable cannot escape draining.

synchronize_irq() is deliberately not used before the
cancel_work_sync() calls. Once SoundWire interrupts are masked, no new
IRQ deliveries can occur. An IRQ handler already in flight may still
queue amd_sdw_irq_thread, so cancel_work_sync() is used to drain both
amd_sdw_irq_thread and any amd_sdw_work items it may have scheduled.
This fully quiesces the driver workqueues, making synchronize_irq()
unnecessary.

Also guard pm_runtime_disable() so it is only called when runtime PM
was actually enabled. amd_sdw_manager_start() calls pm_runtime_enable()
only at the very end, after several fallible hardware init steps. If
sdw_amd_startup() fails mid-loop (one manager started, the next fails
before pm_runtime_enable()), sdw_amd_exit() triggers
platform_device_unregister() for all managers. Calling
pm_runtime_disable() on the partially-started manager finds
disable_depth already at its initial value of 1, silently increments it
to 2 and returns without a warning, so a later pm_runtime_enable() would
only bring it back to 1 and leave runtime PM disabled. Use
pm_runtime_enabled() to skip the call when it was never paired with an
enable.

Fixes: f93b697ed98e ("soundwire: amd: cancel pending slave status handling workqueue during remove sequence")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index a57b59609bfe..bbe1e73ed255 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1172,9 +1172,11 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
 	struct amd_sdw_manager *amd_manager = dev_get_drvdata(&pdev->dev);
 	int ret;
 
-	pm_runtime_disable(&pdev->dev);
-	cancel_work_sync(&amd_manager->amd_sdw_work);
+	if (pm_runtime_enabled(&pdev->dev))
+		pm_runtime_disable(&pdev->dev);
 	amd_disable_sdw_interrupts(amd_manager);
+	cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
+	cancel_work_sync(&amd_manager->amd_sdw_work);
 	sdw_bus_master_delete(&amd_manager->bus);
 	ret = amd_disable_sdw_manager(amd_manager);
 	if (ret)
-- 
2.48.1

[PATCH 4/8] soundwire: amd: fix ctx leak when sdw_amd_startup() fails

[Vijendar Mukunda]

sdw_amd_probe() allocates a context and registers platform devices via
sdw_amd_probe_controller(), then calls sdw_amd_startup() to start each
manager. If sdw_amd_startup() fails, the error was returned directly,
leaving *sdw_ctx non-NULL with the allocated context and its registered
platform devices leaked.

Call sdw_amd_exit() on the failure path to unregister the platform
devices and free ctx, then clear *sdw_ctx so callers observe a
consistent NULL on error.

Fixes: ed5e8741b8db ("soundwire: amd: refactor amd soundwire manager device node creation")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_init.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
index 8e419ddfa516..88004d59322e 100644
--- a/drivers/soundwire/amd_init.c
+++ b/drivers/soundwire/amd_init.c
@@ -166,11 +166,18 @@ static int sdw_amd_startup(struct sdw_amd_ctx *ctx)
 
 int sdw_amd_probe(struct sdw_amd_res *res, struct sdw_amd_ctx **sdw_ctx)
 {
+	int ret;
+
 	*sdw_ctx = sdw_amd_probe_controller(res);
 	if (!*sdw_ctx)
 		return -ENODEV;
 
-	return sdw_amd_startup(*sdw_ctx);
+	ret = sdw_amd_startup(*sdw_ctx);
+	if (ret) {
+		sdw_amd_exit(*sdw_ctx);
+		*sdw_ctx = NULL;
+	}
+	return ret;
 }
 EXPORT_SYMBOL_NS(sdw_amd_probe, "SOUNDWIRE_AMD_INIT");
 
-- 
2.48.1

[PATCH 5/8] soundwire: amd: propagate amd_init_sdw_manager() error on resume

[Vijendar Mukunda]

In amd_resume_runtime() POWER_OFF_MODE, amd_init_sdw_manager() was
called without capturing its return value. The function performs
hardware bus resets using readl_poll_timeout() and can return
-ETIMEDOUT if the hardware does not respond. When that happens,
execution fell through to amd_enable_sdw_interrupts() and
amd_enable_sdw_manager() on hardware that may still be in a broken
reset state.

Fix by capturing the return value and returning early on failure.

Fixes: 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index bbe1e73ed255..2d2f628230ee 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1456,7 +1456,9 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 		ret = amd_sdw_clk_init_ctrl(amd_manager);
 		if (ret)
 			return ret;
-		amd_init_sdw_manager(amd_manager);
+		ret = amd_init_sdw_manager(amd_manager);
+		if (ret)
+			return ret;
 		amd_enable_sdw_interrupts(amd_manager);
 		ret = amd_enable_sdw_manager(amd_manager);
 		if (ret)
-- 
2.48.1

[PATCH 6/8] soundwire: amd: drop amd_deinit_sdw_manager() in POWER_OFF suspend

[Vijendar Mukunda]

amd_deinit_sdw_manager() was a small helper that bundled
amd_disable_sdw_interrupts() and amd_disable_sdw_manager() together.
Expand the two POWER_OFF_MODE call sites in amd_suspend() and
amd_suspend_runtime() to call those two functions directly, then remove
the helper. No functional change.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 2d2f628230ee..6af224ca54a3 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -146,12 +146,6 @@ static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager)
 	writel(0x00, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
 }
 
-static int amd_deinit_sdw_manager(struct amd_sdw_manager *amd_manager)
-{
-	amd_disable_sdw_interrupts(amd_manager);
-	return amd_disable_sdw_manager(amd_manager);
-}
-
 static void amd_sdw_set_frameshape(struct amd_sdw_manager *amd_manager)
 {
 	u32 frame_size;
@@ -1336,7 +1330,8 @@ static int __maybe_unused amd_suspend(struct device *dev)
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
-		ret = amd_deinit_sdw_manager(amd_manager);
+		amd_disable_sdw_interrupts(amd_manager);
+		ret = amd_disable_sdw_manager(amd_manager);
 		if (ret)
 			return ret;
 	}
@@ -1380,7 +1375,8 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
-		ret = amd_deinit_sdw_manager(amd_manager);
+		amd_disable_sdw_interrupts(amd_manager);
+		ret = amd_disable_sdw_manager(amd_manager);
 		if (ret)
 			return ret;
 	}
-- 
2.48.1

[PATCH 7/8] soundwire: amd: replace >= ACP70 with explicit switch/case in PM paths

[Vijendar Mukunda]

Replace open-ended acp_rev >= ACP70_PCI_REV_ID comparisons in
amd_suspend(), amd_suspend_runtime(), and amd_resume_runtime() with
explicit per-revision handling enumerating ACP63, ACP70, ACP71, and
ACP72, so that every supported revision is handled explicitly.

The host wake enable step is guarded by acp_rev at six PM call sites.
Factor that repeated block into amd_sdw_host_wake_enable_by_rev() to
avoid duplicating the switch/case at every site. The remaining device
state transitions keep an inline switch/case as one of them carries
additional PME handling.

An unrecognised revision cannot reach these PM callbacks because
amd_sdw_manager_probe() already validates acp_rev with the same switch
and fails registration for anything unknown. The switch default therefore
emits dev_err() and returns -EINVAL to make the unexpected case visible
while keeping the error path consistent across all PM operations.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 97 ++++++++++++++++++++++-----------
 1 file changed, 64 insertions(+), 33 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 6af224ca54a3..0cb5f4694e9a 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1292,6 +1292,22 @@ static int __maybe_unused amd_pm_prepare(struct device *dev)
 	return 0;
 }
 
+static int amd_sdw_host_wake_enable_by_rev(struct amd_sdw_manager *amd_manager, bool enable)
+{
+	switch (amd_manager->acp_rev) {
+	case ACP63_PCI_REV_ID:
+		break;
+	case ACP70_PCI_REV_ID:
+	case ACP71_PCI_REV_ID:
+	case ACP72_PCI_REV_ID:
+		return amd_sdw_host_wake_enable(amd_manager, enable);
+	default:
+		dev_err(amd_manager->dev, "unexpected acp_rev 0x%x\n", amd_manager->acp_rev);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int __maybe_unused amd_suspend(struct device *dev)
 {
 	struct amd_sdw_manager *amd_manager = dev_get_drvdata(dev);
@@ -1307,22 +1323,18 @@ static int __maybe_unused amd_suspend(struct device *dev)
 	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
 		cancel_work_sync(&amd_manager->amd_sdw_work);
 		amd_sdw_wake_enable(amd_manager, false);
-		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
-			ret = amd_sdw_host_wake_enable(amd_manager, false);
-			if (ret)
-				return ret;
-		}
+		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
+		if (ret)
+			return ret;
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
 		cancel_work_sync(&amd_manager->amd_sdw_work);
 		amd_sdw_wake_enable(amd_manager, false);
-		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
-			ret = amd_sdw_host_wake_enable(amd_manager, false);
-			if (ret)
-				return ret;
-		}
+		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
+		if (ret)
+			return ret;
 		/*
 		 * As per hardware programming sequence on AMD platforms,
 		 * clock stop should be invoked first before powering-off
@@ -1335,10 +1347,19 @@ static int __maybe_unused amd_suspend(struct device *dev)
 		if (ret)
 			return ret;
 	}
-	if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
+	switch (amd_manager->acp_rev) {
+	case ACP63_PCI_REV_ID:
+		break;
+	case ACP70_PCI_REV_ID:
+	case ACP71_PCI_REV_ID:
+	case ACP72_PCI_REV_ID:
 		ret = amd_sdw_set_device_state(amd_manager, AMD_SDW_DEVICE_STATE_D3);
 		if (ret)
 			return ret;
+		break;
+	default:
+		dev_err(amd_manager->dev, "unexpected acp_rev 0x%x\n", amd_manager->acp_rev);
+		return -EINVAL;
 	}
 	return 0;
 }
@@ -1357,21 +1378,17 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 	}
 	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
 		amd_sdw_wake_enable(amd_manager, true);
-		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
-			ret = amd_sdw_host_wake_enable(amd_manager, true);
-			if (ret)
-				return ret;
-		}
+		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, true);
+		if (ret)
+			return ret;
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
 		amd_sdw_wake_enable(amd_manager, true);
-		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
-			ret = amd_sdw_host_wake_enable(amd_manager, true);
-			if (ret)
-				return ret;
-		}
+		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, true);
+		if (ret)
+			return ret;
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
@@ -1380,7 +1397,12 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 		if (ret)
 			return ret;
 	}
-	if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
+	switch (amd_manager->acp_rev) {
+	case ACP63_PCI_REV_ID:
+		break;
+	case ACP70_PCI_REV_ID:
+	case ACP71_PCI_REV_ID:
+	case ACP72_PCI_REV_ID:
 		ret = amd_sdw_set_device_state(amd_manager, AMD_SDW_DEVICE_STATE_D3);
 		if (ret)
 			return ret;
@@ -1392,6 +1414,10 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 				dev_dbg(amd_manager->dev, "ACP_PME_EN:0x%x\n", val);
 			}
 		}
+		break;
+	default:
+		dev_err(amd_manager->dev, "unexpected acp_rev 0x%x\n", amd_manager->acp_rev);
+		return -EINVAL;
 	}
 	return 0;
 }
@@ -1413,11 +1439,9 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 		ret = amd_sdw_clock_stop_exit(amd_manager);
 		if (ret)
 			return ret;
-		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
-			ret = amd_sdw_host_wake_enable(amd_manager, false);
-			if (ret)
-				return ret;
-		}
+		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
+		if (ret)
+			return ret;
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
 		writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance));
 		/*
@@ -1431,11 +1455,9 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 		mutex_lock(&amd_manager->bus.msg_lock);
 		amd_manager->ping_slave_stat = U32_MAX;
 		mutex_unlock(&amd_manager->bus.msg_lock);
-		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
-			ret = amd_sdw_host_wake_enable(amd_manager, false);
-			if (ret)
-				return ret;
-		}
+		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
+		if (ret)
+			return ret;
 		val = readl(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
 		if (val) {
 			val |= AMD_SDW_CLK_RESUME_REQ;
@@ -1461,10 +1483,19 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 			return ret;
 		amd_sdw_set_frameshape(amd_manager);
 	}
-	if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
+	switch (amd_manager->acp_rev) {
+	case ACP63_PCI_REV_ID:
+		break;
+	case ACP70_PCI_REV_ID:
+	case ACP71_PCI_REV_ID:
+	case ACP72_PCI_REV_ID:
 		ret = amd_sdw_set_device_state(amd_manager, AMD_SDW_DEVICE_STATE_D0);
 		if (ret)
 			return ret;
+		break;
+	default:
+		dev_err(amd_manager->dev, "unexpected acp_rev 0x%x\n", amd_manager->acp_rev);
+		return -EINVAL;
 	}
 	return 0;
 }
-- 
2.48.1

[PATCH 8/8] soundwire: amd: fix interrupt gate and work drain ordering in PM ops

[Vijendar Mukunda]

The CLK_STOP_MODE branches in amd_suspend_runtime() and amd_suspend()
did not close the ACP_EXTERNAL_INTR_CNTL delivery gate before draining
amd_sdw_irq_thread and amd_sdw_work. The ACP PCI parent driver interrupt
handler dispatches SoundWire interrupts to the AMD SoundWire stack by
scheduling amd_sdw_irq_thread. Because the gate was open, an ISR already
running when the drains completed could re-queue work after
cancel_work_sync() returned, racing amd_sdw_clock_stop().

Fix both suspend paths by calling amd_disable_sdw_interrupts() first to
close the ACP-level gate before work drains.

In amd_suspend() CLK_STOP_MODE:
 - Add amd_disable_sdw_interrupts() and cancel_work_sync(irq_thread)
   before the existing cancel_work_sync(work).
 - Call amd_enable_sdw_interrupts() after clock stop to reopen the gate.
   ACP remains powered in CLK_STOP_MODE (only the SoundWire bus clock is
   gated), so the interrupt path must be live to allow wake events to
   reach the CPU during system sleep.

In amd_suspend() POWER_OFF_MODE:
 - Add amd_disable_sdw_interrupts() and cancel_work_sync(irq_thread)
   before the existing cancel_work_sync(work).
 - Remove the redundant post-clock-stop amd_disable_sdw_interrupts() call;
   the gate is already closed at the top of the branch.

In amd_suspend_runtime() CLK_STOP_MODE:
 - Add amd_disable_sdw_interrupts() and cancel_work_sync(irq_thread)
   before the existing cancel_work_sync(work).
 - Call amd_enable_sdw_interrupts() to reopen the gate. ACP remains
   powered in CLK_STOP_MODE (only the SoundWire bus clock is gated), so
   the interrupt path must be live to allow wake events to reach the CPU.

In amd_suspend_runtime() POWER_OFF_MODE:
 - Add amd_disable_sdw_interrupts() and cancel_work_sync(irq_thread)
   before the existing cancel_work_sync(work).
 - Remove the redundant post-clock-stop amd_disable_sdw_interrupts() call;
   the gate is already closed at the top of the branch.

Fixes: 86a4371b7697 ("soundwire: amd: fix for handling slave alerts after link is down")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 0cb5f4694e9a..1a252a81269a 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1321,6 +1321,8 @@ static int __maybe_unused amd_suspend(struct device *dev)
 	}
 
 	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
+		amd_disable_sdw_interrupts(amd_manager);
+		cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
 		cancel_work_sync(&amd_manager->amd_sdw_work);
 		amd_sdw_wake_enable(amd_manager, false);
 		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
@@ -1329,7 +1331,10 @@ static int __maybe_unused amd_suspend(struct device *dev)
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
+		amd_enable_sdw_interrupts(amd_manager);
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
+		amd_disable_sdw_interrupts(amd_manager);
+		cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
 		cancel_work_sync(&amd_manager->amd_sdw_work);
 		amd_sdw_wake_enable(amd_manager, false);
 		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
@@ -1342,7 +1347,6 @@ static int __maybe_unused amd_suspend(struct device *dev)
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
-		amd_disable_sdw_interrupts(amd_manager);
 		ret = amd_disable_sdw_manager(amd_manager);
 		if (ret)
 			return ret;
@@ -1377,6 +1381,9 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 		return 0;
 	}
 	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
+		amd_disable_sdw_interrupts(amd_manager);
+		cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
+		cancel_work_sync(&amd_manager->amd_sdw_work);
 		amd_sdw_wake_enable(amd_manager, true);
 		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, true);
 		if (ret)
@@ -1384,7 +1391,11 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
+		amd_enable_sdw_interrupts(amd_manager);
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
+		amd_disable_sdw_interrupts(amd_manager);
+		cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
+		cancel_work_sync(&amd_manager->amd_sdw_work);
 		amd_sdw_wake_enable(amd_manager, true);
 		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, true);
 		if (ret)
@@ -1392,7 +1403,6 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
-		amd_disable_sdw_interrupts(amd_manager);
 		ret = amd_disable_sdw_manager(amd_manager);
 		if (ret)
 			return ret;
-- 
2.48.1

Re: [PATCH 1/8] soundwire: amd: fix SDW command timeout return value handling

[Mario Limonciello]

On 9/10/26 14:00, Vijendar Mukunda wrote:
> amd_sdw_send_cmd_get_resp() returned u64 but was returning -ETIMEDOUT
> (a negative int) on readl_poll_timeout() failures, which silently
> widens to a large u64 value.
> 
> Change the return type to int and pass the raw u64 response through an
> out-parameter. Timeout is detected directly from readl_poll_timeout()
> return value.
> 
> Update all callers: amd_program_scp_addr(), _amd_sdw_xfer_msg(),
> amd_sdw_read_and_process_ping_status(), and amd_sdw_read_ping_status()
> to check the return value and handle timeout explicitly. In
> amd_sdw_read_and_process_ping_status(), the mutex unlock is also moved
> to after amd_sdw_process_ping_status() so the lock is held across the
> full status processing step.
> 
> Fixes: d8f48fbdfd9a ("soundwire: amd: Add support for AMD Manager driver")
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> ---
>   drivers/soundwire/amd_manager.c | 56 +++++++++++++++++++--------------
>   1 file changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index a3316efdf8ac..fa3f4e797edd 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -250,10 +250,9 @@ static void amd_sdw_ctl_word_prep(u32 *lower_word, u32 *upper_word, struct sdw_m
>   	*lower_word = lower_data;
>   }
>   
> -static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lower_data,
> -				     u32 upper_data)
> +static int amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lower_data,
> +				     u32 upper_data, u64 *response)
>   {
> -	u64 resp;
>   	u32 lower_resp, upper_resp;
>   	u32 sts;
>   	int ret;
> @@ -291,9 +290,8 @@ static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lo
>   			amd_manager->instance);
>   		return ret;
>   	}
> -	resp = upper_resp;
> -	resp = (resp << 32) | lower_resp;
> -	return resp;
> +	*response = ((u64)upper_resp << 32) | lower_resp;
> +	return 0;
>   }
>   
>   static enum sdw_command_response
> @@ -309,19 +307,22 @@ amd_program_scp_addr(struct amd_sdw_manager *amd_manager, struct sdw_msg *msg)
>   	scp_msg.buf = &msg->addr_page1;
>   	scp_msg.flags = SDW_MSG_FLAG_WRITE;
>   	amd_sdw_ctl_word_prep(&lower_data, &upper_data, &scp_msg, 0);
> -	response_buf[0] = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data);
> +	if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data, &response_buf[0])) {
> +		dev_err_ratelimited(amd_manager->dev,
> +				    "SCP_addrpage command timeout for Slave %d\n", msg->dev_num);
> +		return SDW_CMD_TIMEOUT;
> +	}
>   	scp_msg.addr = SDW_SCP_ADDRPAGE2;
>   	scp_msg.buf = &msg->addr_page2;
>   	amd_sdw_ctl_word_prep(&lower_data, &upper_data, &scp_msg, 0);
> -	response_buf[1] = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data);
> +	if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data, &response_buf[1])) {
> +		dev_err_ratelimited(amd_manager->dev,
> +				    "SCP_addrpage command timeout for Slave %d\n", msg->dev_num);
> +		return SDW_CMD_TIMEOUT;
> +	}
>   
>   	for (index = 0; index < 2; index++) {
> -		if (response_buf[index] == -ETIMEDOUT) {
> -			dev_err_ratelimited(amd_manager->dev,
> -					    "SCP_addrpage command timeout for Slave %d\n",
> -					    msg->dev_num);
> -			return SDW_CMD_TIMEOUT;
> -		} else if (!(response_buf[index] & AMD_SDW_MCP_RESP_ACK)) {
> +		if (!(response_buf[index] & AMD_SDW_MCP_RESP_ACK)) {
>   			if (response_buf[index] & AMD_SDW_MCP_RESP_NACK) {
>   				dev_err_ratelimited(amd_manager->dev,
>   						    "SCP_addrpage NACKed for Slave %d\n",
> @@ -366,11 +367,7 @@ static enum sdw_command_response amd_sdw_fill_msg_resp(struct amd_sdw_manager *a
>   		if (msg->flags == SDW_MSG_FLAG_READ)
>   			msg->buf[offset] = FIELD_GET(AMD_SDW_MCP_RESP_RDATA, response);
>   	} else {
> -		if (response == -ETIMEDOUT) {
> -			dev_err_ratelimited(amd_manager->dev, "command timeout for Slave %d\n",
> -					    msg->dev_num);
> -			return SDW_CMD_TIMEOUT;
> -		} else if (response & AMD_SDW_MCP_RESP_NACK) {
> +		if (response & AMD_SDW_MCP_RESP_NACK) {
>   			dev_err_ratelimited(amd_manager->dev,
>   					    "command response NACK received for Slave %d\n",
>   					    msg->dev_num);
> @@ -390,7 +387,11 @@ static unsigned int _amd_sdw_xfer_msg(struct amd_sdw_manager *amd_manager, struc
>   	u32 upper_data = 0, lower_data = 0;
>   
>   	amd_sdw_ctl_word_prep(&lower_data, &upper_data, msg, cmd_offset);
> -	response = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data);
> +	if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data, &response)) {
> +		dev_err_ratelimited(amd_manager->dev, "command timeout for Slave %d\n",
> +				    msg->dev_num);
> +		return SDW_CMD_TIMEOUT;
> +	}
>   	return amd_sdw_fill_msg_resp(amd_manager, msg, response, cmd_offset);
>   }
>   
> @@ -446,9 +447,14 @@ static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_man
>   	u64 response;
>   
>   	mutex_lock(&amd_manager->bus.msg_lock);

guard(mutex) might work better here so you don't need to cover every 
exit path with a mutex_unlock() manually.

> -	response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0);
> -	mutex_unlock(&amd_manager->bus.msg_lock);
> +	if (amd_sdw_send_cmd_get_resp(amd_manager, 0, 0, &response)) {
> +		mutex_unlock(&amd_manager->bus.msg_lock);
> +		dev_err_ratelimited(amd_manager->dev, "SDW%x ping status timeout\n",
> +				    amd_manager->instance);
> +		return;
> +	}
>   	amd_sdw_process_ping_status(response, amd_manager);
> +	mutex_unlock(&amd_manager->bus.msg_lock);
>   }
>   
>   static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
> @@ -457,7 +463,11 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
>   	u64 response;
>   	u32 slave_stat;
>   
> -	response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0);
> +	if (amd_sdw_send_cmd_get_resp(amd_manager, 0, 0, &response)) {
> +		dev_err_ratelimited(amd_manager->dev, "SDW%x ping status timeout\n",
> +				    amd_manager->instance);
> +		return 0;
> +	}
>   	/* slave status from ping response */
>   	slave_stat = FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_0_3, response);
>   	slave_stat |= FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_4_11, response) << 8;

Re: [PATCH 0/8] soundwire: amd: SoundWire manager driver bug fixes

[Mario Limonciello]

On 9/10/26 14:00, Vijendar Mukunda wrote:
> This series collects eight bug fixes for the AMD SoundWire manager
> driver (ACP6.3/7.0/7.1/7.2). The patches address correctness issues
> in command response handling, slave status caching, work-queue teardown
> ordering, resource cleanup on probe failure, and power-management
> suspend/resume sequences.
> 
> Patch 1 fixes a silent type widening in amd_sdw_send_cmd_get_resp()
> where -ETIMEDOUT was returned through a u64, producing a large positive
> value instead of an error.
> 
> Patch 2 caches the last successful ping slave status so that a command
> timeout during sdw_show_ping_status() does not report a spurious
> "no peripherals attached" warning.
> 
> Patch 3 fixes the work-drain sequence in amd_sdw_manager_remove(): the
> interrupt delivery gate must be closed before cancel_work_sync() to
> prevent an in-flight IRQ handler from re-queuing work after the cancel
> returns. A pm_runtime_disable() imbalance on partially-started managers
> is also corrected.
> 
> Patch 4 adds the missing sdw_amd_exit() call and ctx pointer clear on
> the sdw_amd_startup() failure path in sdw_amd_probe().
> 
> Patch 5 propagates the amd_init_sdw_manager() return value in
> amd_resume_runtime() instead of ignoring it.
> 
> Patch 6 removes the amd_deinit_sdw_manager() helper and expands its
> two call sites in the POWER_OFF suspend path to direct calls, making
> the sequence explicit with no functional change.
> 
> Patch 7 replaces open-ended acp_rev >= ACP70_PCI_REV_ID comparisons
> in the PM callbacks with explicit per-revision handling covering
> ACP63, ACP70, ACP71 and ACP72, with a dev_err + -EINVAL default. The
> repeated host-wake-by-revision block is factored into a helper,
> amd_sdw_host_wake_enable_by_rev().
> 
> Patch 8 fixes the interrupt gate and work drain ordering in all four
> suspend mode/path combinations: the ACP_EXTERNAL_INTR_CNTL gate must
> be closed before cancel_work_sync() calls to prevent a racing ISR from
> re-queuing work after the drain completes.
> 
> Vijendar Mukunda (8):
>    soundwire: amd: fix SDW command timeout return value handling
>    soundwire: amd: cache ping slave status to avoid spurious disconnect
>      on timeout
>    soundwire: amd: fix work drain ordering and pm_runtime guard in remove
>      path
>    soundwire: amd: fix ctx leak when sdw_amd_startup() fails
>    soundwire: amd: propagate amd_init_sdw_manager() error on resume
>    soundwire: amd: drop amd_deinit_sdw_manager() in POWER_OFF suspend
>    soundwire: amd: replace >= ACP70 with explicit switch/case in PM paths
>    soundwire: amd: fix interrupt gate and work drain ordering in PM ops
> 
>   drivers/soundwire/amd_init.c      |   9 +-
>   drivers/soundwire/amd_manager.c   | 204 ++++++++++++++++++++----------
>   include/linux/soundwire/sdw_amd.h |   3 +
>   3 files changed, 148 insertions(+), 68 deletions(-)
> 
Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>

Re: [PATCH 1/8] soundwire: amd: fix SDW command timeout return value handling

[Mukunda,Vijendar]

On 9/11/26 23:18, Mario Limonciello wrote:
>
>
> On 9/10/26 14:00, Vijendar Mukunda wrote:
>> amd_sdw_send_cmd_get_resp() returned u64 but was returning -ETIMEDOUT
>> (a negative int) on readl_poll_timeout() failures, which silently
>> widens to a large u64 value.
>>
>> Change the return type to int and pass the raw u64 response through an
>> out-parameter. Timeout is detected directly from readl_poll_timeout()
>> return value.
>>
>> Update all callers: amd_program_scp_addr(), _amd_sdw_xfer_msg(),
>> amd_sdw_read_and_process_ping_status(), and amd_sdw_read_ping_status()
>> to check the return value and handle timeout explicitly. In
>> amd_sdw_read_and_process_ping_status(), the mutex unlock is also moved
>> to after amd_sdw_process_ping_status() so the lock is held across the
>> full status processing step.
>>
>> Fixes: d8f48fbdfd9a ("soundwire: amd: Add support for AMD Manager 
>> driver")
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>> ---
>>   drivers/soundwire/amd_manager.c | 56 +++++++++++++++++++--------------
>>   1 file changed, 33 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/soundwire/amd_manager.c 
>> b/drivers/soundwire/amd_manager.c
>> index a3316efdf8ac..fa3f4e797edd 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -250,10 +250,9 @@ static void amd_sdw_ctl_word_prep(u32 
>> *lower_word, u32 *upper_word, struct sdw_m
>>       *lower_word = lower_data;
>>   }
>>   -static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager 
>> *amd_manager, u32 lower_data,
>> -                     u32 upper_data)
>> +static int amd_sdw_send_cmd_get_resp(struct amd_sdw_manager 
>> *amd_manager, u32 lower_data,
>> +                     u32 upper_data, u64 *response)
>>   {
>> -    u64 resp;
>>       u32 lower_resp, upper_resp;
>>       u32 sts;
>>       int ret;
>> @@ -291,9 +290,8 @@ static u64 amd_sdw_send_cmd_get_resp(struct 
>> amd_sdw_manager *amd_manager, u32 lo
>>               amd_manager->instance);
>>           return ret;
>>       }
>> -    resp = upper_resp;
>> -    resp = (resp << 32) | lower_resp;
>> -    return resp;
>> +    *response = ((u64)upper_resp << 32) | lower_resp;
>> +    return 0;
>>   }
>>     static enum sdw_command_response
>> @@ -309,19 +307,22 @@ amd_program_scp_addr(struct amd_sdw_manager 
>> *amd_manager, struct sdw_msg *msg)
>>       scp_msg.buf = &msg->addr_page1;
>>       scp_msg.flags = SDW_MSG_FLAG_WRITE;
>>       amd_sdw_ctl_word_prep(&lower_data, &upper_data, &scp_msg, 0);
>> -    response_buf[0] = amd_sdw_send_cmd_get_resp(amd_manager, 
>> lower_data, upper_data);
>> +    if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, 
>> upper_data, &response_buf[0])) {
>> +        dev_err_ratelimited(amd_manager->dev,
>> +                    "SCP_addrpage command timeout for Slave %d\n", 
>> msg->dev_num);
>> +        return SDW_CMD_TIMEOUT;
>> +    }
>>       scp_msg.addr = SDW_SCP_ADDRPAGE2;
>>       scp_msg.buf = &msg->addr_page2;
>>       amd_sdw_ctl_word_prep(&lower_data, &upper_data, &scp_msg, 0);
>> -    response_buf[1] = amd_sdw_send_cmd_get_resp(amd_manager, 
>> lower_data, upper_data);
>> +    if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, 
>> upper_data, &response_buf[1])) {
>> +        dev_err_ratelimited(amd_manager->dev,
>> +                    "SCP_addrpage command timeout for Slave %d\n", 
>> msg->dev_num);
>> +        return SDW_CMD_TIMEOUT;
>> +    }
>>         for (index = 0; index < 2; index++) {
>> -        if (response_buf[index] == -ETIMEDOUT) {
>> -            dev_err_ratelimited(amd_manager->dev,
>> -                        "SCP_addrpage command timeout for Slave %d\n",
>> -                        msg->dev_num);
>> -            return SDW_CMD_TIMEOUT;
>> -        } else if (!(response_buf[index] & AMD_SDW_MCP_RESP_ACK)) {
>> +        if (!(response_buf[index] & AMD_SDW_MCP_RESP_ACK)) {
>>               if (response_buf[index] & AMD_SDW_MCP_RESP_NACK) {
>>                   dev_err_ratelimited(amd_manager->dev,
>>                               "SCP_addrpage NACKed for Slave %d\n",
>> @@ -366,11 +367,7 @@ static enum sdw_command_response 
>> amd_sdw_fill_msg_resp(struct amd_sdw_manager *a
>>           if (msg->flags == SDW_MSG_FLAG_READ)
>>               msg->buf[offset] = FIELD_GET(AMD_SDW_MCP_RESP_RDATA, 
>> response);
>>       } else {
>> -        if (response == -ETIMEDOUT) {
>> -            dev_err_ratelimited(amd_manager->dev, "command timeout 
>> for Slave %d\n",
>> -                        msg->dev_num);
>> -            return SDW_CMD_TIMEOUT;
>> -        } else if (response & AMD_SDW_MCP_RESP_NACK) {
>> +        if (response & AMD_SDW_MCP_RESP_NACK) {
>>               dev_err_ratelimited(amd_manager->dev,
>>                           "command response NACK received for Slave 
>> %d\n",
>>                           msg->dev_num);
>> @@ -390,7 +387,11 @@ static unsigned int _amd_sdw_xfer_msg(struct 
>> amd_sdw_manager *amd_manager, struc
>>       u32 upper_data = 0, lower_data = 0;
>>         amd_sdw_ctl_word_prep(&lower_data, &upper_data, msg, 
>> cmd_offset);
>> -    response = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, 
>> upper_data);
>> +    if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, 
>> upper_data, &response)) {
>> +        dev_err_ratelimited(amd_manager->dev, "command timeout for 
>> Slave %d\n",
>> +                    msg->dev_num);
>> +        return SDW_CMD_TIMEOUT;
>> +    }
>>       return amd_sdw_fill_msg_resp(amd_manager, msg, response, 
>> cmd_offset);
>>   }
>>   @@ -446,9 +447,14 @@ static void 
>> amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_man
>>       u64 response;
>>         mutex_lock(&amd_manager->bus.msg_lock);
>
> guard(mutex) might work better here so you don't need to cover every 
> exit path with a mutex_unlock() manually.

Thanks for the suggestion. I agree that guard(mutex) would simplify
the locking and make the error paths cleaner.

For this patch, I wanted to keep the change focused on fixing the
-ETIMEDOUT handling issue and avoid mixing it with unrelated cleanup.
The driver currently uses explicit mutex_lock() / mutex_unlock() pairs
throughout amd_manager.c, so introducing guard(mutex) in a single
function would make the locking style inconsistent.

I'll plan to follow up with a separate cleanup patch that converts the
eligible mutex-protected paths to guard(mutex) consistently across the
driver. Keeping that work separate should make both the bug fix and the
cleanup easier to review and bisect.
>
>> -    response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0);
>> -    mutex_unlock(&amd_manager->bus.msg_lock);
>> +    if (amd_sdw_send_cmd_get_resp(amd_manager, 0, 0, &response)) {
>> +        mutex_unlock(&amd_manager->bus.msg_lock);
>> +        dev_err_ratelimited(amd_manager->dev, "SDW%x ping status 
>> timeout\n",
>> +                    amd_manager->instance);
>> +        return;
>> +    }
>>       amd_sdw_process_ping_status(response, amd_manager);
>> +    mutex_unlock(&amd_manager->bus.msg_lock);
>>   }
>>     static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
>> @@ -457,7 +463,11 @@ static u32 amd_sdw_read_ping_status(struct 
>> sdw_bus *bus)
>>       u64 response;
>>       u32 slave_stat;
>>   -    response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0);
>> +    if (amd_sdw_send_cmd_get_resp(amd_manager, 0, 0, &response)) {
>> +        dev_err_ratelimited(amd_manager->dev, "SDW%x ping status 
>> timeout\n",
>> +                    amd_manager->instance);
>> +        return 0;
>> +    }
>>       /* slave status from ping response */
>>       slave_stat = FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_0_3, response);
>>       slave_stat |= FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_4_11, response) 
>> << 8;
>

Re: [PATCH 2/8] soundwire: amd: cache ping slave status to avoid spurious disconnect on timeout

[Pierre-Louis Bossart]

On 9/10/26 21:00, Vijendar Mukunda wrote:
> + * @ping_slave_stat: cached slave status from the last successful ping response;

It'd be good to clarify the definition of a 'successful ping response',
since this concept doesn't exist in the SoundWire spec.
The only valid combined responses are Command_Aborted or Command_Ignored...

Re: [PATCH 3/8] soundwire: amd: fix work drain ordering and pm_runtime guard in remove path

[Pierre-Louis Bossart]

On 9/10/26 21:00, Vijendar Mukunda wrote:
> amd_sdw_manager_remove() cancelled amd_sdw_work but not
> amd_sdw_irq_thread. Since amd_sdw_irq_thread() calls
> schedule_work(&amd_sdw_work), an in-flight irq_thread item can
> re-queue amd_sdw_work after its cancel returns, defeating the
> cancellation.
> 
> Fix by calling amd_disable_sdw_interrupts() first to quiesce the
> hardware IRQ source, then cancel_work_sync() for amd_sdw_irq_thread,
> then cancel_work_sync() for amd_sdw_work. The existing
> cancel_work_sync(amd_sdw_work) is also moved to after
> amd_disable_sdw_interrupts() so that any work item queued between the
> old cancel position and the interrupt disable cannot escape draining.
> 
> synchronize_irq() is deliberately not used before the
> cancel_work_sync() calls. Once SoundWire interrupts are masked, no new
> IRQ deliveries can occur. An IRQ handler already in flight may still
> queue amd_sdw_irq_thread, so cancel_work_sync() is used to drain both
> amd_sdw_irq_thread and any amd_sdw_work items it may have scheduled.
> This fully quiesces the driver workqueues, making synchronize_irq()
> unnecessary.
> 
> Also guard pm_runtime_disable() so it is only called when runtime PM
> was actually enabled. amd_sdw_manager_start() calls pm_runtime_enable()
> only at the very end, after several fallible hardware init steps. If
> sdw_amd_startup() fails mid-loop (one manager started, the next fails
> before pm_runtime_enable()), sdw_amd_exit() triggers
> platform_device_unregister() for all managers. Calling
> pm_runtime_disable() on the partially-started manager finds
> disable_depth already at its initial value of 1, silently increments it
> to 2 and returns without a warning, so a later pm_runtime_enable() would
> only bring it back to 1 and leave runtime PM disabled. Use
> pm_runtime_enabled() to skip the call when it was never paired with an
> enable.

The alternative is to do a pm_runtime_enable() in the probe(), and later
a pm_runtime_set_active().

That way if the probe is successful, then the remove() will always deal
a balanced enable.

Maybe only put a single 'fix' per patch?
> Fixes: f93b697ed98e ("soundwire: amd: cancel pending slave status handling workqueue during remove sequence")
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> ---
>  drivers/soundwire/amd_manager.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index a57b59609bfe..bbe1e73ed255 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -1172,9 +1172,11 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
>  	struct amd_sdw_manager *amd_manager = dev_get_drvdata(&pdev->dev);
>  	int ret;
>  
> -	pm_runtime_disable(&pdev->dev);
> -	cancel_work_sync(&amd_manager->amd_sdw_work);
> +	if (pm_runtime_enabled(&pdev->dev))
> +		pm_runtime_disable(&pdev->dev);
>  	amd_disable_sdw_interrupts(amd_manager);
> +	cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
> +	cancel_work_sync(&amd_manager->amd_sdw_work);
>  	sdw_bus_master_delete(&amd_manager->bus);
>  	ret = amd_disable_sdw_manager(amd_manager);
>  	if (ret)