[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

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 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;
>

[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

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 2/8] soundwire: amd: cache ping slave status to avoid spurious disconnect on timeout

[Mukunda,Vijendar]

On 9/14/26 01:29, Pierre-Louis Bossart wrote:
> 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...
Thanks, that's a good point.
The intent here is not to introduce a new SoundWire protocol concept,
but simply to cache the slave_stat field from the last ping transaction
that completed without a command timeout. The cached value is only used
as a fallback when a later ping transaction times out, to avoid
treating a transient transport timeout as "no peripherals attached".

I'll update the comment to avoid the term "successful ping response"
and instead describe it as the slave_stat value from the last ping
transaction that returned a response.

[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

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)

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

[Mukunda,Vijendar]

On 9/14/26 01:35, Pierre-Louis Bossart wrote:
> 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?
Thanks for the comments. I agree that it is preferable to keep each patch
focused, but in this case both changes are part of the same remove-path
cleanup bug and are tightly coupled.

I do not think moving pm_runtime_enable() to probe() is the right fix
here. In this driver, runtime PM is intentionally enabled only at the end
of amd_sdw_manager_start(), after the fallible hardware bring-up sequence.
If one instance succeeds and the next fails before pm_runtime_enable(), the
remove path can still run for the partially started instance. In that
scenario, an unconditional pm_runtime_disable() is incorrect because there
was no matching enable for that instance.

This driver is multi-instance: the controller loops over each link and
starts them independently. The PM state and workqueue state are per
instance, so the guard in remove() is needed to avoid an unbalanced PM
state from the partial-start failure path. Moving the enable into probe()
would also change the device lifecycle semantics by enabling PM before the
hardware bring-up sequence completes, which is broader than the actual bug
being fixed.

The work-drain ordering change is part of the same teardown correctness
issue: amd_sdw_irq_thread() can requeue amd_sdw_work, so the interrupt
source must be masked before draining both work items. That is a remove-
path issue and not something that belongs in probe().

I kept the patch scoped to the actual cleanup bug instead of restructuring
the startup lifecycle. If needed, I can split the PM guard and the work-
drain ordering into separate patches, but they are both required for the
same failure mode.


>> 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)

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

[Pierre-Louis Bossart]

On 9/14/26 08:07, Mukunda,Vijendar wrote:
> 
> 
> On 9/14/26 01:35, Pierre-Louis Bossart wrote:
>> 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?
> Thanks for the comments. I agree that it is preferable to keep each patch
> focused, but in this case both changes are part of the same remove-path
> cleanup bug and are tightly coupled.
> 
> I do not think moving pm_runtime_enable() to probe() is the right fix
> here. In this driver, runtime PM is intentionally enabled only at the end
> of amd_sdw_manager_start(), after the fallible hardware bring-up sequence.
> If one instance succeeds and the next fails before pm_runtime_enable(), the
> remove path can still run for the partially started instance. In that
> scenario, an unconditional pm_runtime_disable() is incorrect because there
> was no matching enable for that instance.

My take on error handling is to avoid partially functional setups. Keep
things simple, fail big and fail early. Well-intended concealment
schemes will introduce more problems, e.g. if the link for the right amp
fails the left one might work, but users will complain about left-only
sounds...

That said, I am not going to lay on the tracks if this is the design you
want for your IP.

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

[Mukunda,Vijendar]

On 9/14/26 23:00, Pierre-Louis Bossart wrote:
> On 9/14/26 08:07, Mukunda,Vijendar wrote:
>>
>> On 9/14/26 01:35, Pierre-Louis Bossart wrote:
>>> 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?
>> Thanks for the comments. I agree that it is preferable to keep each patch
>> focused, but in this case both changes are part of the same remove-path
>> cleanup bug and are tightly coupled.
>>
>> I do not think moving pm_runtime_enable() to probe() is the right fix
>> here. In this driver, runtime PM is intentionally enabled only at the end
>> of amd_sdw_manager_start(), after the fallible hardware bring-up sequence.
>> If one instance succeeds and the next fails before pm_runtime_enable(), the
>> remove path can still run for the partially started instance. In that
>> scenario, an unconditional pm_runtime_disable() is incorrect because there
>> was no matching enable for that instance.
> My take on error handling is to avoid partially functional setups. Keep
> things simple, fail big and fail early. Well-intended concealment
> schemes will introduce more problems, e.g. if the link for the right amp
> fails the left one might work, but users will complain about left-only
> sounds...
>
> That said, I am not going to lay on the tracks if this is the design you
> want for your IP.
Thanks for the suggestion Pierre.  I considered moving pm_runtime_enable()
into probe(), but I do not think that is safe given how the AMD SoundWire
driver is structured.

The current placement in amd_sdw_manager_start() follows the same model
used by the Intel SoundWire driver, where runtime PM is enabled only
after the hardware has been powered up and fully initialized.

More importantly, pm_runtime_set_active() requires the hardware to be in
a known operational state. For AMD, that is only true after
acp_sdw_clk_init_ctrl(), acp_init_sdw_manager(),
acp_enable_sdw_interrupts(), acp_enable_sdw_manager(), and
acp_sdw_set_frameshape() have all completed successfully. Calling
pm_runtime_set_active() from probe() would advertise the device as
active before any of this initialization has occurred.

Enabling runtime PM in probe() would also create a race window between
probe() and sdw_amd_startup(). During that window, the PM core could
invoke the runtime suspend callback, which accesses SoundWire manager
registers and performs clock-stop sequences. Since the hardware has not
yet been initialized, those register accesses would occur on an
uninitialized manager.

The failure path that motivated this change is also a real scenario.
sdw_amd_startup() iterates over all manager instances. If one instance
successfully completes startup and another fails later, the cleanup path
must handle a mix of initialized and non-initialized managers. The
pm_runtime_enabled() check added here ensures that
pm_runtime_disable() is only called for instances that actually reached
the point where runtime PM was enabled.

The probe/startup split is intentional and follows the existing
SoundWire subsystem design. Hardware bring-up is deferred until startup,
and runtime PM is enabled only after the manager is known to be fully
operational. Since the runtime PM callbacks directly access hardware
registers, allowing them to run before startup completes would be
unsafe.

Finally, moving pm_runtime_enable() into probe() would separate it from
pm_runtime_set_active(). The current ordering of
pm_runtime_set_active() followed by pm_runtime_enable() is the standard
runtime PM pattern and avoids additional synchronization requirements.

This design is not new. The placement of pm_runtime_enable() inside
amd_sdw_manager_start() was introduced by commit 81ff58ff71ad
("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")
and has been part of the upstream kernel since v6.4.

In summary, moving pm_runtime_enable() to probe() would expose runtime
PM callbacks before the SoundWire manager is initialized, creating a
real race between probe() and startup. Keeping it in
amd_sdw_manager_start() satisfies the requirements of
pm_runtime_set_active() and makes the pm_runtime_enabled() guard in the
remove path both correct and necessary.

We will split the patch and push the pm_runtime guard change separately.

[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 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>