[PATCH v4 00/13] soundwire: amd: SoundWire manager driver bug fixes

[PATCH v4 00/13] soundwire: amd: SoundWire manager driver bug fixes

[Vijendar Mukunda]

This series collects bug fixes for the AMD SoundWire manager driver
(ACP6.3/7.0/7.1/7.2). The patches address correctness issues across
three areas:

Command response handling: fix silent widening of -ETIMEDOUT to u64,
return valid hardware response when only the IMM status cleanup poll
times out, and cache the last successful PING result to avoid spurious
disconnect warnings on transient timeouts.

Work-queue and interrupt teardown: fix drain ordering in the remove
path and all PM suspend paths so the interrupt gate is closed before
work queues are drained, preventing re-queuing races against
clock-stop and power-off sequences.

Power management and locking: propagate amd_init_sdw_manager() errors
on POWER_OFF resume, guard pm_runtime_disable() against unpaired
calls, fix the IRQ thread dropping STATUS_0TO7 on simultaneous
wake+state-change events, and fix a double-clear race on
STATUS_8TO11 in the wake handler.

Changes since v3:
- Add patch 12/13 to fix IRQ thread dropping STATUS_0TO7 when a
  wake event and a device-state change arrive simultaneously.
- Add patch 13/13 to fix STATUS_8TO11 double-clear race in the
  wake handler introduced by the simultaneous-wake fix.
- Add lockdep_assert_held() to amd_sdw_read_ping_status() (patch 4/13)
  to enforce the msg_lock contract machine-checkably.
- Add return response on IMM status cleanup timeout (patch 2/13).
- Update patch 11/13 (fix interrupt gate and work drain ordering in PM
  ops) to also fix amd_sdw_wake_enable() ordering in both CLK_STOP_MODE
  paths: move the call to after amd_enable_sdw_interrupts() so the
  unconditional interrupt mask write does not clobber the wake bit.

Changes since v2:
- Update comment for ping_slave_stat variable description in patch 3/13.
- Split the remove path work drain and pm ops guard patch into separate
  patches (patches 5/13 and 6/13) to improve clarity and maintainability.

Changes since v1:
- Rework patch 1 to fix the u64 return type properly via out-parameter.

Vijendar Mukunda (13):
  soundwire: amd: fix SDW command timeout return value handling
  soundwire: amd: return response on IMM status cleanup timeout
  soundwire: amd: cache ping slave status to avoid spurious disconnect
    on timeout
  soundwire: amd: assert msg_lock is held in amd_sdw_read_ping_status()
  soundwire: amd: fix work drain ordering in remove path
  soundwire: amd: 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
  soundwire: amd: fix IRQ thread dropping STATUS_0TO7 on simultaneous
    wake
  soundwire: amd: fix STATUS_8TO11 double-clear race in wake handler

 drivers/soundwire/amd_init.c      |   9 +-
 drivers/soundwire/amd_manager.c   | 217 ++++++++++++++++++++----------
 include/linux/soundwire/sdw_amd.h |   4 +
 3 files changed, 155 insertions(+), 75 deletions(-)

-- 
2.53.0

[PATCH v4 01/13] 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.

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

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index a3316efdf8ac..307cbfdb9442 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,7 +447,12 @@ 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);
+	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;
+	}
 	mutex_unlock(&amd_manager->bus.msg_lock);
 	amd_sdw_process_ping_status(response, amd_manager);
 }
@@ -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.53.0

[PATCH v4 02/13] soundwire: amd: return response on IMM status cleanup timeout

[Vijendar Mukunda]

amd_sdw_send_cmd_get_resp() reads the hardware response after
IMM_RES_VALID is set, then writes IMM_RES_VALID to acknowledge and
polls until the bit clears (status cleanup handshake).

When the cleanup poll timed out, the code returned the error without
writing *response, causing callers to report SDW_CMD_TIMEOUT and
discard a valid response - upper_resp and lower_resp had already been
read before the cleanup write.

The cleanup timeout indicates the hardware was slow to acknowledge
the clear of IMM_RES_VALID, not that the command exchange failed.
Log the anomaly, then write *response and return 0 so callers receive
the valid data. The stuck IMM_RES_VALID bit is cleared at the next
invocation by the existing bad-state guard.

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

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 307cbfdb9442..0b83126b5ce6 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -285,11 +285,9 @@ static int amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lo
 	writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS);
 	ret = readl_poll_timeout(amd_manager->mmio + ACP_SW_IMM_CMD_STS, sts,
 				 !(sts & AMD_SDW_IMM_RES_VALID), ACP_DELAY_US, AMD_SDW_TIMEOUT);
-	if (ret) {
+	if (ret)
 		dev_err(amd_manager->dev, "SDW%x cmd status retry failed\n",
 			amd_manager->instance);
-		return ret;
-	}
 	*response = ((u64)upper_resp << 32) | lower_resp;
 	return 0;
 }
-- 
2.53.0

Re: [PATCH v4 02/13] soundwire: amd: return response on IMM status cleanup timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soundwire: amd: unpolled IMM_RES_VALID clear in amd_sdw_send_cmd_get_resp()
--

commit 3a55a761f23cc08cf1a64f142c04e1aa78cc244d
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: return response on IMM status cleanup timeout

This commit changes amd_sdw_send_cmd_get_resp() to log an error, store the
read response, and return 0 when polling for AMD_SDW_IMM_RES_VALID to clear
times out. It relies on the bad-state check at the start of the next
invocation to clear the remaining AMD_SDW_IMM_RES_VALID bit.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index 307cbfdb94422..0b83126b5ce6c 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -285,11 +285,9 @@ static int amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lo
>  	writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>  	ret = readl_poll_timeout(amd_manager->mmio + ACP_SW_IMM_CMD_STS, sts,
>  				 !(sts & AMD_SDW_IMM_RES_VALID), ACP_DELAY_US, AMD_SDW_TIMEOUT);
> -	if (ret) {
> +	if (ret)
>  		dev_err(amd_manager->dev, "SDW%x cmd status retry failed\n",
>  			amd_manager->instance);
> -		return ret;
> -	}
>  	*response = ((u64)upper_resp << 32) | lower_resp;
>  	return 0;
>  }

[Severity: High]
When this cleanup poll for !(sts & AMD_SDW_IMM_RES_VALID) times out and
amd_sdw_send_cmd_get_resp() in drivers/soundwire/amd_manager.c returns 0,
can back-to-back commands read stale response data from
ACP_SW_IMM_RESP_UPPER_WORD and ACP_SW_IMM_RESP_LOWER_QWORD?

Callers such as amd_program_scp_addr() and amd_sdw_xfer_msg() issue multiple
commands in sequence:

sdw_transfer()
  do_transfer()
    amd_sdw_xfer_msg()
      amd_prep_msg()
        amd_program_scp_addr()
          amd_sdw_send_cmd_get_resp()
      _amd_sdw_xfer_msg()
        amd_sdw_send_cmd_get_resp()

On the immediate next call to amd_sdw_send_cmd_get_resp(), the bad-state
guard writes AMD_SDW_IMM_RES_VALID to ACP_SW_IMM_CMD_STS without polling for
!(sts & AMD_SDW_IMM_RES_VALID) before issuing the next command and polling
for (sts & AMD_SDW_IMM_RES_VALID):

	if (sts & AMD_SDW_IMM_RES_VALID) {
		dev_err(amd_manager->dev, "SDW%x manager is in bad state\n", amd_manager->instance);
		writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS);
	}
	writel(upper_data, amd_manager->mmio + ACP_SW_IMM_CMD_UPPER_WORD);
	writel(lower_data, amd_manager->mmio + ACP_SW_IMM_CMD_LOWER_QWORD);

	ret = readl_poll_timeout(amd_manager->mmio + ACP_SW_IMM_CMD_STS, sts,
				 (sts & AMD_SDW_IMM_RES_VALID), ACP_DELAY_US, AMD_SDW_TIMEOUT);

Because clearing AMD_SDW_IMM_RES_VALID requires an asynchronous hardware
handshake that already failed to complete within AMD_SDW_TIMEOUT on the
previous command, won't AMD_SDW_IMM_RES_VALID still be set when
readl_poll_timeout() immediately checks (sts & AMD_SDW_IMM_RES_VALID),
causing it to succeed on the stale bit and read the previous command's
response registers? Should the bad-state guard also poll until
!(sts & AMD_SDW_IMM_RES_VALID) before writing the new command words?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026044507.2310694-1-Vijendar.Mukunda@amd.com?part=2

Re: [PATCH v4 02/13] soundwire: amd: return response on IMM status cleanup timeout

[Mukunda,Vijendar]

On 10/26/26 14:38, sashiko-bot@kernel.org wrote:
> [You don't often get email from sashiko-bot@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] soundwire: amd: unpolled IMM_RES_VALID clear in amd_sdw_send_cmd_get_resp()
> --
>
> commit 3a55a761f23cc08cf1a64f142c04e1aa78cc244d
> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>
> soundwire: amd: return response on IMM status cleanup timeout
>
> This commit changes amd_sdw_send_cmd_get_resp() to log an error, store the
> read response, and return 0 when polling for AMD_SDW_IMM_RES_VALID to clear
> times out. It relies on the bad-state check at the start of the next
> invocation to clear the remaining AMD_SDW_IMM_RES_VALID bit.
>
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index 307cbfdb94422..0b83126b5ce6c 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -285,11 +285,9 @@ static int amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lo
>>        writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>>        ret = readl_poll_timeout(amd_manager->mmio + ACP_SW_IMM_CMD_STS, sts,
>>                                 !(sts & AMD_SDW_IMM_RES_VALID), ACP_DELAY_US, AMD_SDW_TIMEOUT);
>> -     if (ret) {
>> +     if (ret)
>>                dev_err(amd_manager->dev, "SDW%x cmd status retry failed\n",
>>                        amd_manager->instance);
>> -             return ret;
>> -     }
>>        *response = ((u64)upper_resp << 32) | lower_resp;
>>        return 0;
>>   }
> [Severity: High]
> When this cleanup poll for !(sts & AMD_SDW_IMM_RES_VALID) times out and
> amd_sdw_send_cmd_get_resp() in drivers/soundwire/amd_manager.c returns 0,
> can back-to-back commands read stale response data from
> ACP_SW_IMM_RESP_UPPER_WORD and ACP_SW_IMM_RESP_LOWER_QWORD?
No. The response for the current command has already been latched and read
before the cleanup poll is performed.

The first poll waits for AMD_SDW_IMM_RES_VALID to be set, indicating that
the response is available. A timeout there is still returned as an error
and this behavior is unchanged.

The trailing poll only waits for AMD_SDW_IMM_RES_VALID to be cleared after
the response has been read. Therefore, a timeout during this cleanup does
not invalidate the response that was already obtained.
>
> Callers such as amd_program_scp_addr() and amd_sdw_xfer_msg() issue multiple
> commands in sequence:
>
> sdw_transfer()
>    do_transfer()
>      amd_sdw_xfer_msg()
>        amd_prep_msg()
>          amd_program_scp_addr()
>            amd_sdw_send_cmd_get_resp()
>        _amd_sdw_xfer_msg()
>          amd_sdw_send_cmd_get_resp()
>
> On the immediate next call to amd_sdw_send_cmd_get_resp(), the bad-state
> guard writes AMD_SDW_IMM_RES_VALID to ACP_SW_IMM_CMD_STS without polling for
> !(sts & AMD_SDW_IMM_RES_VALID) before issuing the next command and polling
> for (sts & AMD_SDW_IMM_RES_VALID):
>
>          if (sts & AMD_SDW_IMM_RES_VALID) {
>                  dev_err(amd_manager->dev, "SDW%x manager is in bad state\n", amd_manager->instance);
>                  writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>          }
>          writel(upper_data, amd_manager->mmio + ACP_SW_IMM_CMD_UPPER_WORD);
>          writel(lower_data, amd_manager->mmio + ACP_SW_IMM_CMD_LOWER_QWORD);
>
>          ret = readl_poll_timeout(amd_manager->mmio + ACP_SW_IMM_CMD_STS, sts,
>                                   (sts & AMD_SDW_IMM_RES_VALID), ACP_DELAY_US, AMD_SDW_TIMEOUT);
>
> Because clearing AMD_SDW_IMM_RES_VALID requires an asynchronous hardware
> handshake that already failed to complete within AMD_SDW_TIMEOUT on the
> previous command, won't AMD_SDW_IMM_RES_VALID still be set when
> readl_poll_timeout() immediately checks (sts & AMD_SDW_IMM_RES_VALID),
> causing it to succeed on the stale bit and read the previous command's
> response registers? Should the bad-state guard also poll until
> !(sts & AMD_SDW_IMM_RES_VALID) before writing the new command words?
No. If AMD_SDW_IMM_RES_VALID is still set on the next invocation, the
bad-state path clears it by writing AMD_SDW_IMM_RES_VALID to
ACP_SW_IMM_CMD_STS before programming the next command.

This behavior is also consistent with the hardware reference
implementation, where the bad-state path uses the same write-1-to-clear
sequence without polling again for the status bit to clear.
Therefore, no additional change is required for this scenario.

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261026044507.2310694-1-Vijendar.Mukunda@amd.com?part=2

Re: [PATCH v4 02/13] soundwire: amd: return response on IMM status cleanup timeout

[Mukunda,Vijendar]

On 10/26/26 17:43, Mukunda,Vijendar wrote:
>
>
> On 10/26/26 14:38, sashiko-bot@kernel.org wrote:
>> [You don't often get email from sashiko-bot@kernel.org. Learn why 
>> this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Thank you for your contribution! Sashiko AI review found 1 potential 
>> issue(s) to consider:
>> - [High] soundwire: amd: unpolled IMM_RES_VALID clear in 
>> amd_sdw_send_cmd_get_resp()
>> -- 
>>
>> commit 3a55a761f23cc08cf1a64f142c04e1aa78cc244d
>> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>>
>> soundwire: amd: return response on IMM status cleanup timeout
>>
>> This commit changes amd_sdw_send_cmd_get_resp() to log an error, 
>> store the
>> read response, and return 0 when polling for AMD_SDW_IMM_RES_VALID to 
>> clear
>> times out. It relies on the bad-state check at the start of the next
>> invocation to clear the remaining AMD_SDW_IMM_RES_VALID bit.
>>
>>> diff --git a/drivers/soundwire/amd_manager.c 
>>> b/drivers/soundwire/amd_manager.c
>>> index 307cbfdb94422..0b83126b5ce6c 100644
>>> --- a/drivers/soundwire/amd_manager.c
>>> +++ b/drivers/soundwire/amd_manager.c
>>> @@ -285,11 +285,9 @@ static int amd_sdw_send_cmd_get_resp(struct 
>>> amd_sdw_manager *amd_manager, u32 lo
>>>        writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + 
>>> ACP_SW_IMM_CMD_STS);
>>>        ret = readl_poll_timeout(amd_manager->mmio + 
>>> ACP_SW_IMM_CMD_STS, sts,
>>>                                 !(sts & AMD_SDW_IMM_RES_VALID), 
>>> ACP_DELAY_US, AMD_SDW_TIMEOUT);
>>> -     if (ret) {
>>> +     if (ret)
>>>                dev_err(amd_manager->dev, "SDW%x cmd status retry 
>>> failed\n",
>>>                        amd_manager->instance);
>>> -             return ret;
>>> -     }
>>>        *response = ((u64)upper_resp << 32) | lower_resp;
>>>        return 0;
>>>   }
>> [Severity: High]
>> When this cleanup poll for !(sts & AMD_SDW_IMM_RES_VALID) times out and
>> amd_sdw_send_cmd_get_resp() in drivers/soundwire/amd_manager.c 
>> returns 0,
>> can back-to-back commands read stale response data from
>> ACP_SW_IMM_RESP_UPPER_WORD and ACP_SW_IMM_RESP_LOWER_QWORD?
++
> No. The response for the current command has already been latched and 
> read
> before the cleanup poll is performed.
>
> The first poll waits for AMD_SDW_IMM_RES_VALID to be set, indicating that
> the response is available. A timeout there is still returned as an error
> and this behavior is unchanged.
>
> The trailing poll only waits for AMD_SDW_IMM_RES_VALID to be cleared 
> after
> the response has been read. Therefore, a timeout during this cleanup does
> not invalidate the response that was already obtained.
>>
>> Callers such as amd_program_scp_addr() and amd_sdw_xfer_msg() issue 
>> multiple
>> commands in sequence:
>>
>> sdw_transfer()
>>    do_transfer()
>>      amd_sdw_xfer_msg()
>>        amd_prep_msg()
>>          amd_program_scp_addr()
>>            amd_sdw_send_cmd_get_resp()
>>        _amd_sdw_xfer_msg()
>>          amd_sdw_send_cmd_get_resp()
>>
>> On the immediate next call to amd_sdw_send_cmd_get_resp(), the bad-state
>> guard writes AMD_SDW_IMM_RES_VALID to ACP_SW_IMM_CMD_STS without 
>> polling for
>> !(sts & AMD_SDW_IMM_RES_VALID) before issuing the next command and 
>> polling
>> for (sts & AMD_SDW_IMM_RES_VALID):
>>
>>          if (sts & AMD_SDW_IMM_RES_VALID) {
>>                  dev_err(amd_manager->dev, "SDW%x manager is in bad 
>> state\n", amd_manager->instance);
>>                  writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + 
>> ACP_SW_IMM_CMD_STS);
>>          }
>>          writel(upper_data, amd_manager->mmio + 
>> ACP_SW_IMM_CMD_UPPER_WORD);
>>          writel(lower_data, amd_manager->mmio + 
>> ACP_SW_IMM_CMD_LOWER_QWORD);
>>
>>          ret = readl_poll_timeout(amd_manager->mmio + 
>> ACP_SW_IMM_CMD_STS, sts,
>>                                   (sts & AMD_SDW_IMM_RES_VALID), 
>> ACP_DELAY_US, AMD_SDW_TIMEOUT);
>>
>> Because clearing AMD_SDW_IMM_RES_VALID requires an asynchronous hardware
>> handshake that already failed to complete within AMD_SDW_TIMEOUT on the
>> previous command, won't AMD_SDW_IMM_RES_VALID still be set when
>> readl_poll_timeout() immediately checks (sts & AMD_SDW_IMM_RES_VALID),
>> causing it to succeed on the stale bit and read the previous command's
>> response registers? Should the bad-state guard also poll until
>> !(sts & AMD_SDW_IMM_RES_VALID) before writing the new command words?
> No. If AMD_SDW_IMM_RES_VALID is still set on the next invocation, the
> bad-state path clears it by writing AMD_SDW_IMM_RES_VALID to
> ACP_SW_IMM_CMD_STS before programming the next command.
>
> This behavior is also consistent with the hardware reference
> implementation, where the bad-state path uses the same write-1-to-clear
> sequence without polling again for the status bit to clear.
> Therefore, no additional change is required for this scenario.
>
>>
>> -- 
>> Sashiko AI review · 
>> https://sashiko.dev/#/patchset/20261026044507.2310694-1-Vijendar.Mukunda@amd.com?part=2
>

[PATCH v4 03/13] soundwire: amd: cache ping slave status to avoid spurious disconnect on timeout

[Vijendar Mukunda]

amd_sdw_read_ping_status() returned 0 on a PING command timeout.
This was indistinguishable from a valid "no peripherals attached"
response, causing sdw_show_ping_status() to log a spurious disconnect
warning even when devices were actually present.

Add ping_slave_stat to struct amd_sdw_manager to cache the slave_stat
from the last successful ping response. On timeout, the cached value is
returned instead of 0, so sdw_show_ping_status() sees the last known
good state. The timeout itself is still reported via
dev_err_ratelimited() - the cache does not suppress the error.

ping_slave_stat is initialised to U32_MAX in probe and reset to
U32_MAX under msg_lock before any failable step in the POWER_OFF_MODE
resume path, so a partially-failed resume cannot leave stale
pre-suspend data behind for a subsequent ping timeout to report.

Fixes: d22c1c2fcfb0 ("soundwire: amd: fix SDW command timeout return value handling")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c   | 18 +++++++++++++++++-
 include/linux/soundwire/sdw_amd.h |  4 ++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 0b83126b5ce6..b75d0104e28c 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -451,6 +451,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);
 	mutex_unlock(&amd_manager->bus.msg_lock);
 	amd_sdw_process_ping_status(response, amd_manager);
 }
@@ -464,12 +467,13 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
 	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;
 }
 
@@ -1085,6 +1089,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);
@@ -1412,6 +1417,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 before any failable step 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..e4375542aad2 100644
--- a/include/linux/soundwire/sdw_amd.h
+++ b/include/linux/soundwire/sdw_amd.h
@@ -76,6 +76,9 @@ 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 ping
+ *		     transaction that returned a response;
+ *		     initialised to U32_MAX ("no valid reading yet")
  * @dai_runtime_array: dai runtime array
  */
 struct amd_sdw_manager {
@@ -105,6 +108,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.53.0

[PATCH v4 04/13] soundwire: amd: assert msg_lock is held in amd_sdw_read_ping_status()

[Vijendar Mukunda]

amd_sdw_read_ping_status() is always called from sdw_show_ping_status()
which holds bus->msg_lock. Add lockdep_assert_held() to document this
locking requirement and catch any future callers that omit the lock.

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

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index b75d0104e28c..5e9d228d09d0 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -464,6 +464,7 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
 	u64 response;
 	u32 slave_stat;
 
+	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);
-- 
2.53.0

[PATCH v4 05/13] soundwire: amd: fix work drain ordering 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.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 5e9d228d09d0..a82714cc2f6c 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1169,8 +1169,9 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
 	int ret;
 
 	pm_runtime_disable(&pdev->dev);
-	cancel_work_sync(&amd_manager->amd_sdw_work);
 	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.53.0

Re: [PATCH v4 05/13] soundwire: amd: fix work drain ordering in remove path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soundwire: amd: race with in-flight IRQ handler in amd_sdw_manager_remove()
--

commit 25a684110adde4056bf6250565965402e67408ca
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: fix work drain ordering in remove path

This commit reorders work cancellation in amd_sdw_manager_remove() so that
interrupts are disabled before calling cancel_work_sync() on
amd_sdw_irq_thread and amd_sdw_work.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index 5e9d228d09d06..a82714cc2f6c4 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -1169,8 +1169,9 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
>  	int ret;
>  
>  	pm_runtime_disable(&pdev->dev);
> -	cancel_work_sync(&amd_manager->amd_sdw_work);
>  	amd_disable_sdw_interrupts(amd_manager);
> +	cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
> +	cancel_work_sync(&amd_manager->amd_sdw_work);

[Severity: High]
Can an in-flight parent ACP IRQ handler still schedule amd_sdw_irq_thread
after cancel_work_sync() returns, causing a use-after-free of amd_manager
and its MMIO regions?

The commit message states:

  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.

However, cancel_work_sync() only cancels a work item that is already
pending or waits for one that is currently executing at the moment
cancel_work_sync() checks it, and re-enables the work item before
returning. Unlike disable_work_sync(), it does not block future
schedule_work() calls, nor does it wait for an in-flight hardirq handler on
another CPU that has not yet called schedule_work().

In addition, amd_disable_sdw_interrupts() only clears the enable mask in
ACP_EXTERNAL_INTR_CNTL via a posted write without clearing already-latched
status bits in ACP_EXTERNAL_INTR_STAT:

drivers/soundwire/amd_manager.c:amd_disable_sdw_interrupts() {
	mutex_lock(amd_manager->acp_sdw_lock);
	irq_mask = sdw_manager_reg_mask_array[amd_manager->instance];
	amd_updatel(amd_manager->acp_mmio, ACP_EXTERNAL_INTR_CNTL(amd_manager->instance),
		    irq_mask, 0);
	mutex_unlock(amd_manager->acp_sdw_lock);
...
}

Meanwhile, the parent IRQ handlers such as acp63_irq_handler() in
sound/soc/amd/ps/pci-ps.c and acp_irq_handler() in sound/soc/sof/amd/acp.c
check ACP_EXTERNAL_INTR_STAT and call schedule_work():

sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
...
	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
	if (ext_intr_stat & ACP_SDW0_STAT) {
		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
		if (amd_manager)
			schedule_work(&amd_manager->amd_sdw_irq_thread);
		irq_flag = 1;
	}
...
}

If acp63_irq_handler() or acp_irq_handler() is executing on CPU 1 during
device unbind, module unload, or probe error unwind, and has read
ACP_EXTERNAL_INTR_STAT and dev_get_drvdata() just before CPU 0 runs
amd_disable_sdw_interrupts() and both cancel_work_sync() calls, both
cancel_work_sync() calls will observe idle work items and return
immediately.

CPU 1 can then call schedule_work(&amd_manager->amd_sdw_irq_thread),
queuing amd_sdw_irq_thread (and subsequently amd_sdw_work) to run
concurrently with sdw_bus_master_delete(), amd_disable_sdw_manager(), and
the devres release of amd_manager and its ioremapped mmio/acp_mmio regions.

Should this use disable_work_sync() instead of cancel_work_sync() (or
synchronize with the parent IRQ after masking and clearing pending status)
so that an in-flight IRQ handler cannot re-queue the work after the drain
completes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026044507.2310694-1-Vijendar.Mukunda@amd.com?part=5

Re: [PATCH v4 05/13] soundwire: amd: fix work drain ordering in remove path

[Mukunda,Vijendar]

On 10/26/26 14:57, sashiko-bot@kernel.org wrote:
> [You don't often get email from sashiko-bot@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] soundwire: amd: race with in-flight IRQ handler in amd_sdw_manager_remove()
> --
>
> commit 25a684110adde4056bf6250565965402e67408ca
> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>
> soundwire: amd: fix work drain ordering in remove path
>
> This commit reorders work cancellation in amd_sdw_manager_remove() so that
> interrupts are disabled before calling cancel_work_sync() on
> amd_sdw_irq_thread and amd_sdw_work.
>
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index 5e9d228d09d06..a82714cc2f6c4 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -1169,8 +1169,9 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
>>        int ret;
>>
>>        pm_runtime_disable(&pdev->dev);
>> -     cancel_work_sync(&amd_manager->amd_sdw_work);
>>        amd_disable_sdw_interrupts(amd_manager);
>> +     cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
>> +     cancel_work_sync(&amd_manager->amd_sdw_work);
> [Severity: High]
> Can an in-flight parent ACP IRQ handler still schedule amd_sdw_irq_thread
> after cancel_work_sync() returns, causing a use-after-free of amd_manager
> and its MMIO regions?
++

The ACP IP has a dedicated interrupt line and does not share it
with any other IP. Once amd_disable_sdw_interrupts() clears the
enable bit in ACP_EXTERNAL_INTR_CNTL, the hardware cannot assert
the interrupt line again for SoundWire events.

However, a narrow race exists where an IRQ handler already
executing on another CPU has obtained amd_manager via
dev_get_drvdata() but has not yet called schedule_work() when
both cancel_work_sync() calls complete on the remove path.

A follow-up fix will introduce a software irq_enabled flag in
amd_sdw_manager, cleared in amd_disable_sdw_interrupts() before
writing to ACP_EXTERNAL_INTR_CNTL and restored in
amd_enable_sdw_interrupts(). The IRQ handler will check this
flag before calling schedule_work():

     if (amd_manager && amd_manager->irq_enabled)
         schedule_work(&amd_manager->amd_sdw_irq_thread);

In the remove path, amd_enable_sdw_interrupts() is never called
after amd_disable_sdw_interrupts(), so irq_enabled stays false
and the race is closed.
>
> The commit message states:
>
>    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.
>
> However, cancel_work_sync() only cancels a work item that is already
> pending or waits for one that is currently executing at the moment
> cancel_work_sync() checks it, and re-enables the work item before
> returning. Unlike disable_work_sync(), it does not block future
> schedule_work() calls, nor does it wait for an in-flight hardirq handler on
> another CPU that has not yet called schedule_work().
This is acknowledged. The irq_enabled software flag introduced
by the follow-up fix addresses this. Clearing the flag in
amd_disable_sdw_interrupts() before cancel_work_sync() runs
ensures that an IRQ handler already executing on another CPU
reads irq_enabled == false and skips schedule_work(), preventing
any re-queue after the drain completes.

The wake path, which is handled inside amd_sdw_irq_thread()
via amd_sdw_process_wake_event() -> pm_request_resume(), is
not affected. In the CLK_STOP_MODE suspend path,
amd_enable_sdw_interrupts() is called after cancel_work_sync()
to restore irq_enabled = true, allowing wake interrupts to
re-schedule amd_sdw_irq_thread for the resume sequence. In
the remove path, amd_enable_sdw_interrupts() is never called,
so irq_enabled remains false permanently.

> In addition, amd_disable_sdw_interrupts() only clears the enable mask in
> ACP_EXTERNAL_INTR_CNTL via a posted write without clearing already-latched
> status bits in ACP_EXTERNAL_INTR_STAT:
The IRQ handler clears ACP_EXTERNAL_INTR_STAT before calling
schedule_work():

     writel(ACP_SDW0_STAT, adata->acp63_base +
            ACP_EXTERNAL_INTR_STAT);
     amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
     if (amd_manager)
         schedule_work(&amd_manager->amd_sdw_irq_thread);

Any latched status bit is therefore cleared by the IRQ handler
itself before work is queued. Since the ACP IP has a dedicated
interrupt line, once the source is masked no new interrupt can
fire to re-set those bits. The irq_enabled flag in the follow-up
fix closes the remaining narrow window for any handler already
in flight at the moment of masking.
> drivers/soundwire/amd_manager.c:amd_disable_sdw_interrupts() {
>          mutex_lock(amd_manager->acp_sdw_lock);
>          irq_mask = sdw_manager_reg_mask_array[amd_manager->instance];
>          amd_updatel(amd_manager->acp_mmio, ACP_EXTERNAL_INTR_CNTL(amd_manager->instance),
>                      irq_mask, 0);
>          mutex_unlock(amd_manager->acp_sdw_lock);
> ...
> }
>
> Meanwhile, the parent IRQ handlers such as acp63_irq_handler() in
> sound/soc/amd/ps/pci-ps.c and acp_irq_handler() in sound/soc/sof/amd/acp.c
> check ACP_EXTERNAL_INTR_STAT and call schedule_work():
>
> sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
> ...
>          ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>          if (ext_intr_stat & ACP_SDW0_STAT) {
>                  writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>                  amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
>                  if (amd_manager)
>                          schedule_work(&amd_manager->amd_sdw_irq_thread);
>                  irq_flag = 1;
>          }
> ...
> }
>
> If acp63_irq_handler() or acp_irq_handler() is executing on CPU 1 during
> device unbind, module unload, or probe error unwind, and has read
> ACP_EXTERNAL_INTR_STAT and dev_get_drvdata() just before CPU 0 runs
> amd_disable_sdw_interrupts() and both cancel_work_sync() calls, both
> cancel_work_sync() calls will observe idle work items and return
> immediately.
>
> CPU 1 can then call schedule_work(&amd_manager->amd_sdw_irq_thread),
> queuing amd_sdw_irq_thread (and subsequently amd_sdw_work) to run
> concurrently with sdw_bus_master_delete(), amd_disable_sdw_manager(), and
> the devres release of amd_manager and its ioremapped mmio/acp_mmio regions.
>
> Should this use disable_work_sync() instead of cancel_work_sync() (or
> synchronize with the parent IRQ after masking and clearing pending status)
> so that an in-flight IRQ handler cannot re-queue the work after the drain
> completes?

Neither disable_work_sync() nor synchronize_irq() is needed.
A follow-up fix will introduce the irq_enabled software flag in
amd_sdw_manager, tied to amd_enable_sdw_interrupts() and
amd_disable_sdw_interrupts(). Clearing the flag before
cancel_work_sync() in the remove path ensures that any IRQ
handler already executing on another CPU skips schedule_work()
when it checks irq_enabled. This closes the race without
requiring disable_work_sync() or synchronize_irq().

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261026044507.2310694-1-Vijendar.Mukunda@amd.com?part=5

[PATCH v4 06/13] soundwire: amd: pm_runtime guard in remove path

[Vijendar Mukunda]

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index a82714cc2f6c..ccd97bc6b757 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1168,7 +1168,8 @@ 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);
+	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);
-- 
2.53.0

[PATCH v4 07/13] 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.53.0

Re: [PATCH v4 07/13] soundwire: amd: fix ctx leak when sdw_amd_startup() fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soundwire: amd: race and UAF in sdw_amd_probe() error cleanup with active IRQ
--

commit 60bd17e72e211d96b5525ff2d9fb05330bb627b0
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: fix ctx leak when sdw_amd_startup() fails

This commit cleans up the SoundWire context and registered platform devices via
sdw_amd_exit() and clears *sdw_ctx if sdw_amd_startup() fails during
sdw_amd_probe().

> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
> index 8e419ddfa5161..88004d59322ee 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;
> +	}

[Severity: High]
Could calling sdw_amd_exit(*sdw_ctx) before clearing *sdw_ctx race with the
parent ACP interrupt handler and trigger a use-after-free?

The callers of sdw_amd_probe() (snd_acp63_probe() ->
create_acp63_platform_devs() -> amd_sdw_probe() in sound/soc/amd/ps/pci-ps.c,
and amd_sof_acp_probe() / amd_sof_acp7x_probe() -> amd_sof_sdw_probe() in
sound/soc/sof/amd/acp.c) enable ACP external interrupts and register their
shared hard IRQ handlers (acp63_irq_handler(), acp_irq_handler(), and
acp7x_irq_handler()) before passing &adata->sdw as sdw_ctx to sdw_amd_probe().

In sdw_amd_probe(), *sdw_ctx (adata->sdw) is assigned immediately before
sdw_amd_startup() runs. Inside sdw_amd_startup() -> amd_sdw_manager_start() in
drivers/soundwire/amd_manager.c, interrupts are unmasked before the fallible
manager enable step and before subsequent manager instances in the loop are
started:

drivers/soundwire/amd_manager.c:amd_sdw_manager_start() {
    ...
		amd_enable_sdw_interrupts(amd_manager);
		ret = amd_enable_sdw_manager(amd_manager);
		if (ret)
			return ret;
    ...
}

If amd_enable_sdw_manager() fails, or if manager 0 starts and manager 1 fails
in amd_sdw_manager_start(), sdw_amd_probe() calls sdw_amd_exit(*sdw_ctx) while
the parent PCI IRQ is active and *sdw_ctx (adata->sdw) is still non-NULL:

drivers/soundwire/amd_init.c:sdw_amd_exit() {
	sdw_amd_cleanup(ctx);
	kfree(ctx->peripherals);
	kfree(ctx);
}

Because sdw_amd_cleanup() unregisters and frees ctx->pdev[i] (and its devres
amd_manager) and kfree(ctx) frees the context before *sdw_ctx is set to NULL, a
concurrent hard IRQ handler on another CPU can observe the non-NULL adata->sdw
pointer and dereference freed ctx or ctx->pdev[i]->dev, or schedule
amd_sdw_irq_thread after amd_sdw_manager_remove() has already canceled the work
and freed amd_manager:

sound/soc/sof/amd/acp.c:acp7x_irq_handler() {
    ...
	if (adata->sdw) {
		for (instance = 0; instance < ACP7X_SDW_MAX_MANAGER_COUNT; instance++) {
			if (ext_intr_stat & acp7x_sof_sdw_ext_stat[instance]) {
				sof_acp7x_handle_sdw_manager_irq(sdev, adata, desc, instance,
								 acp7x_sof_sdw_ext_stat[instance]);
				irq_flag = 1;
			}
		}
	}
    ...
}

sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
    ...
	if (ext_intr_stat & ACP_SDW0_STAT) {
		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
		if (amd_manager)
			schedule_work(&amd_manager->amd_sdw_irq_thread);
		irq_flag = 1;
	}
    ...
}

Unlike the normal remove paths (which call free_irq() before sdw_amd_exit()),
the IRQ remains active here. Should sdw_amd_probe() hold ctx in a local
variable and only publish *sdw_ctx after sdw_amd_startup() succeeds, while also
ensuring any interrupts enabled during a partial sdw_amd_startup() are masked
and synchronized before sdw_amd_exit() frees the devices and context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026044507.2310694-1-Vijendar.Mukunda@amd.com?part=7

Re: [PATCH v4 07/13] soundwire: amd: fix ctx leak when sdw_amd_startup() fails

[Mukunda,Vijendar]

On 10/26/26 15:18, sashiko-bot@kernel.org wrote:
> [You don't often get email from sashiko-bot@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] soundwire: amd: race and UAF in sdw_amd_probe() error cleanup with active IRQ
> --
>
> commit 60bd17e72e211d96b5525ff2d9fb05330bb627b0
> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>
> soundwire: amd: fix ctx leak when sdw_amd_startup() fails
>
> This commit cleans up the SoundWire context and registered platform devices via
> sdw_amd_exit() and clears *sdw_ctx if sdw_amd_startup() fails during
> sdw_amd_probe().
>
>> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
>> index 8e419ddfa5161..88004d59322ee 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;
>> +     }
> [Severity: High]
> Could calling sdw_amd_exit(*sdw_ctx) before clearing *sdw_ctx race with the
> parent ACP interrupt handler and trigger a use-after-free?
++

SoundWire IRQs are only enabled inside amd_sdw_manager_start()
during sdw_amd_startup() via amd_enable_sdw_interrupts(). The
platform device probe (amd_sdw_manager_probe()) does not enable
any SoundWire interrupts — it only initializes data structures
and work items. So the ACP_SDW0_STAT / ACP_SDW1_STAT bits in
ACP_EXTERNAL_INTR_STAT cannot be set before sdw_amd_startup()
runs, and the IRQ handler will not reach the adata->sdw
dereference during sdw_amd_probe_controller().

On the failure path, sdw_amd_exit() -> sdw_amd_cleanup() ->
platform_device_unregister() triggers amd_sdw_manager_remove(),
which calls amd_disable_sdw_interrupts() followed by
cancel_work_sync() before freeing amd_manager. So any interrupt
enabled for a partially-started manager is masked before the
manager is freed.

That said, as a defensive improvement, NULL guards on adata->sdw
will be added to acp63_irq_handler() and acp_irq_handler()
before dereferencing adata->sdw->pdev[i].
>
> The callers of sdw_amd_probe() (snd_acp63_probe() ->
> create_acp63_platform_devs() -> amd_sdw_probe() in sound/soc/amd/ps/pci-ps.c,
> and amd_sof_acp_probe() / amd_sof_acp7x_probe() -> amd_sof_sdw_probe() in
> sound/soc/sof/amd/acp.c) enable ACP external interrupts and register their
> shared hard IRQ handlers (acp63_irq_handler(), acp_irq_handler(), and
> acp7x_irq_handler()) before passing &adata->sdw as sdw_ctx to sdw_amd_probe().
The ACP external IRQ handler (acp63_irq_handler) is registered
before sdw_amd_probe() is called. However, SoundWire-specific
status bits (ACP_SDW0_STAT, ACP_SDW1_STAT) in
ACP_EXTERNAL_INTR_STAT are only set when the SoundWire manager
is active on the bus. These bits cannot be asserted before
amd_enable_sdw_interrupts() runs inside amd_sdw_manager_start()
during sdw_amd_startup(). So the adata->sdw->pdev[i] dereference
path in the IRQ handler is not reachable before that point,
regardless of whether adata->sdw is NULL or non-NULL.
>
> In sdw_amd_probe(), *sdw_ctx (adata->sdw) is assigned immediately before
> sdw_amd_startup() runs. Inside sdw_amd_startup() -> amd_sdw_manager_start() in
> drivers/soundwire/amd_manager.c, interrupts are unmasked before the fallible
> manager enable step and before subsequent manager instances in the loop are
> started:
>
> drivers/soundwire/amd_manager.c:amd_sdw_manager_start() {
>      ...
>                  amd_enable_sdw_interrupts(amd_manager);
>                  ret = amd_enable_sdw_manager(amd_manager);
>                  if (ret)
>                          return ret;
>      ...
> }
>
> If amd_enable_sdw_manager() fails, or if manager 0 starts and manager 1 fails
> in amd_sdw_manager_start(), sdw_amd_probe() calls sdw_amd_exit(*sdw_ctx) while
> the parent PCI IRQ is active and *sdw_ctx (adata->sdw) is still non-NULL:
>
> drivers/soundwire/amd_init.c:sdw_amd_exit() {
>          sdw_amd_cleanup(ctx);
>          kfree(ctx->peripherals);
>          kfree(ctx);
> }
Since SoundWire IRQs are not enabled until amd_sdw_manager_start()
runs inside sdw_amd_startup(), the window between *sdw_ctx being
set and startup beginning is safe — the IRQ handler cannot reach
the SDW processing path before that point.

If amd_enable_sdw_manager() fails after amd_enable_sdw_interrupts(),
the failure path calls sdw_amd_exit() -> sdw_amd_cleanup() ->
platform_device_unregister(), which triggers
amd_sdw_manager_remove(). That function calls
amd_disable_sdw_interrupts() before cancel_work_sync() and before
freeing amd_manager. So the interrupt is masked and all work
drained before teardown — no use-after-free can occur on this
path.

As a defensive improvement, a separate follow-up fix will update
sdw_amd_probe() to hold ctx in a local variable and only publish
*sdw_ctx after sdw_amd_startup() succeeds, making the lifetime
contract explicit and removing any ambiguity about pointer
visibility during the startup failure window.
>
> Because sdw_amd_cleanup() unregisters and frees ctx->pdev[i] (and its devres
> amd_manager) and kfree(ctx) frees the context before *sdw_ctx is set to NULL, a
> concurrent hard IRQ handler on another CPU can observe the non-NULL adata->sdw
> pointer and dereference freed ctx or ctx->pdev[i]->dev, or schedule
> amd_sdw_irq_thread after amd_sdw_manager_remove() has already canceled the work
> and freed amd_manager:
>
> sound/soc/sof/amd/acp.c:acp7x_irq_handler() {
>      ...
>          if (adata->sdw) {
>                  for (instance = 0; instance < ACP7X_SDW_MAX_MANAGER_COUNT; instance++) {
>                          if (ext_intr_stat & acp7x_sof_sdw_ext_stat[instance]) {
>                                  sof_acp7x_handle_sdw_manager_irq(sdev, adata, desc, instance,
>                                                                   acp7x_sof_sdw_ext_stat[instance]);
>                                  irq_flag = 1;
>                          }
>                  }
>          }
>      ...
> }
>
> sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
>      ...
>          if (ext_intr_stat & ACP_SDW0_STAT) {
>                  writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>                  amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
>                  if (amd_manager)
>                          schedule_work(&amd_manager->amd_sdw_irq_thread);
>                  irq_flag = 1;
>          }
>      ...
> }
Will address this missing NULL guard check for adata->sdw  as a follow 
up fix.
>
> Unlike the normal remove paths (which call free_irq() before sdw_amd_exit()),
> the IRQ remains active here. Should sdw_amd_probe() hold ctx in a local
> variable and only publish *sdw_ctx after sdw_amd_startup() succeeds, while also
> ensuring any interrupts enabled during a partial sdw_amd_startup() are masked
> and synchronized before sdw_amd_exit() frees the devices and context?
As mentioned above, Both points will be addressed:

1. sdw_amd_probe() will be updated to delay *sdw_ctx publication
    until after sdw_amd_startup() succeeds, as a defensive
    improvement making the lifetime contract explicit.

2. Interrupt masking before teardown is already handled by
    amd_sdw_manager_remove() (called via platform_device_unregister
    inside sdw_amd_cleanup()), which masks SoundWire interrupts
    via amd_disable_sdw_interrupts() and drains work via
    cancel_work_sync() before freeing amd_manager. No additional
    masking step is needed in sdw_amd_probe()'s error path.

3. NULL guards on adata->sdw will be added to
    acp63_irq_handler() and acp_irq_handler() for both SDW0 and
    SDW1 dereference sites as a defensive fix.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261026044507.2310694-1-Vijendar.Mukunda@amd.com?part=7

[PATCH v4 08/13] 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 ccd97bc6b757..8e1339ed44ad 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1452,7 +1452,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.53.0

[PATCH v4 09/13] 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 8e1339ed44ad..570a4f0c8372 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;
@@ -1332,7 +1326,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;
 	}
@@ -1376,7 +1371,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.53.0

[PATCH v4 10/13] 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 | 98 ++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 33 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 570a4f0c8372..a685bd7da52b 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1288,6 +1288,23 @@ static int __maybe_unused amd_pm_prepare(struct device *dev)
 	return 0;
 }
 
+static int __maybe_unused 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);
@@ -1303,22 +1320,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
@@ -1331,10 +1344,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;
 }
@@ -1353,21 +1375,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;
@@ -1376,7 +1394,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;
@@ -1388,6 +1411,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;
 }
@@ -1409,11 +1436,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));
 		/*
@@ -1427,11 +1452,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;
@@ -1457,10 +1480,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.53.0

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

[Vijendar Mukunda]

The CLK_STOP_MODE branches in amd_suspend() and amd_suspend_runtime()
did not close the ACP_EXTERNAL_INTR_CNTL delivery gate before draining
amd_sdw_irq_thread and amd_sdw_work. An ISR already running when the
drains completed could re-queue work after cancel_work_sync() returned,
racing amd_sdw_clock_stop().

In both CLK_STOP_MODE paths, amd_sdw_wake_enable() was also called
before amd_enable_sdw_interrupts(). Since amd_enable_sdw_interrupts()
writes the interrupt mask unconditionally, it clobbered the wake bit
set by amd_sdw_wake_enable().

For both amd_suspend() and amd_suspend_runtime():
 - CLK_STOP_MODE: add amd_disable_sdw_interrupts() and
   cancel_work_sync(irq_thread) before the work drain; reopen the gate
   with amd_enable_sdw_interrupts() after clock stop, then call
   amd_sdw_wake_enable() so the wake bit is not clobbered.
 - POWER_OFF_MODE: add amd_disable_sdw_interrupts() and
   cancel_work_sync(irq_thread) before the work drain; remove the
   redundant post-clock-stop amd_disable_sdw_interrupts() call.

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 | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index a685bd7da52b..d7289b840513 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1318,15 +1318,20 @@ 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);
 		if (ret)
 			return ret;
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
+		amd_enable_sdw_interrupts(amd_manager);
+		amd_sdw_wake_enable(amd_manager, false);
 	} 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);
@@ -1339,7 +1344,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;
@@ -1374,14 +1378,21 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 		return 0;
 	}
 	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
-		amd_sdw_wake_enable(amd_manager, true);
+		amd_disable_sdw_interrupts(amd_manager);
+		cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
+		cancel_work_sync(&amd_manager->amd_sdw_work);
 		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;
+		amd_enable_sdw_interrupts(amd_manager);
+		amd_sdw_wake_enable(amd_manager, true);
 	} 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)
@@ -1389,7 +1400,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.53.0

Re: [PATCH v4 11/13] soundwire: amd: fix interrupt gate and work drain ordering in PM ops

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] soundwire: amd: amd_enable_sdw_interrupts() in suspend fires stale status IRQs
- [Medium] soundwire: amd: missing IRQ sync before work drain in amd_suspend()
--

commit ecf45f9e24e7a7e6c5e9ff39ceefacd5dd89af26
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: fix interrupt gate and work drain ordering in PM ops

This commit updates amd_suspend() and amd_suspend_runtime() to disable
SoundWire interrupts and cancel amd_sdw_irq_thread before draining
amd_sdw_work and stopping the bus clock. In the AMD_SDW_CLK_STOP_MODE path,
it also re-enables SoundWire interrupts and configures wake enablement after
stopping the clock.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index a685bd7da52b1..d7289b840513b 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -1318,15 +1318,20 @@ 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);

[Severity: Medium]
Can an in-flight hard IRQ handler on another CPU still schedule
amd_sdw_irq_thread after cancel_work_sync() returns here (and in the
corresponding AMD_SDW_POWER_OFF_MODE and amd_suspend_runtime() paths)?

In sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() (as well as
acp_irq_handler() and acp7x_irq_handler() in sound/soc/sof/amd/acp.c), the
hard IRQ handler reads ACP_EXTERNAL_INTR_STAT and schedules
amd_sdw_irq_thread without holding acp_sdw_lock:

sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
	...
	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
	if (ext_intr_stat & ACP_SDW0_STAT) {
		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
		if (amd_manager)
			schedule_work(&amd_manager->amd_sdw_irq_thread);
		irq_flag = 1;
	}
	...
}

If the hard IRQ handler on CPU 1 has already read ACP_EXTERNAL_INTR_STAT
before CPU 0 masks ACP_EXTERNAL_INTR_CNTL in amd_disable_sdw_interrupts(),
CPU 0 can complete both cancel_work_sync() calls before CPU 1 invokes
schedule_work(&amd_manager->amd_sdw_irq_thread).

Because amd_disable_sdw_interrupts() only clears the mask registers and does
not clear ACP_SW_STATE_CHANGE_STATUS_0TO7 or
ACP_SW_STATE_CHANGE_STATUS_8TO11, the newly scheduled amd_sdw_irq_thread()
will see the non-zero status registers and schedule amd_sdw_work concurrently
with amd_sdw_clock_stop() and amd_disable_sdw_manager(). In addition, since
the post-clock-stop amd_disable_sdw_interrupts() call is removed from the
AMD_SDW_POWER_OFF_MODE path, a racing amd_sdw_update_slave_status_work() with
status[0] == SDW_SLAVE_ATTACHED would also write AMD_SDW_IRQ_MASK_0TO7 and
AMD_SDW_IRQ_MASK_8TO11 back to the mask registers.

Should the driver synchronize with the parent ACP IRQ or clear the status
registers after synchronizing before draining the work items?

> -		amd_sdw_wake_enable(amd_manager, false);
>  		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;
> +		amd_enable_sdw_interrupts(amd_manager);
> +		amd_sdw_wake_enable(amd_manager, false);

[Severity: High]
Could calling amd_enable_sdw_interrupts() right after amd_sdw_clock_stop()
here and in amd_suspend_runtime() re-trigger slave status interrupts while
the SoundWire bus clock is stopped?

When a SoundWire interrupt fires just before amd_disable_sdw_interrupts(),
the hard IRQ handler clears ACP_EXTERNAL_INTR_STAT and queues
amd_sdw_irq_thread, leaving ACP_SW_STATE_CHANGE_STATUS_0TO7 and
ACP_SW_STATE_CHANGE_STATUS_8TO11 non-zero until amd_sdw_irq_thread() runs to
completion:

drivers/soundwire/amd_manager.c:amd_sdw_irq_thread() {
	...
	status_change_8to11 = readl(amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
	status_change_0to7 = readl(amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);
	if (!status_change_0to7 && !status_change_8to11)
		return;
	...
	if (status_change_8to11 || status_change_0to7)
		schedule_work(&amd_manager->amd_sdw_work);
	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);
}

If cancel_work_sync(&amd_manager->amd_sdw_irq_thread) cancels that pending
work item before it executes, or if any slave status bits latch during
sdw_bus_prep_clk_stop() and sdw_bus_clk_stop() while the masks are zeroed,
ACP_SW_STATE_CHANGE_STATUS_0TO7 and ACP_SW_STATE_CHANGE_STATUS_8TO11 remain
non-zero after amd_sdw_clock_stop() sets clk_stopped = true.

Calling amd_enable_sdw_interrupts() at that point unmasks
ACP_EXTERNAL_INTR_CNTL, AMD_SDW_IRQ_MASK_0TO7, and AMD_SDW_IRQ_MASK_8TO11:

drivers/soundwire/amd_manager.c:amd_enable_sdw_interrupts() {
	...
	writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio +
		       ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
	writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio +
		       ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
	writel(AMD_SDW_IRQ_ERROR_MASK, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
}

Wouldn't unmasking those registers with stale bits still set in
ACP_SW_STATE_CHANGE_STATUS_0TO7 and ACP_SW_STATE_CHANGE_STATUS_8TO11
immediately fire an interrupt and schedule amd_sdw_irq_thread() and
amd_sdw_work while the link clock is stopped, causing MIPI command timeouts
in amd_sdw_update_slave_status_work()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026044507.2310694-1-Vijendar.Mukunda@amd.com?part=11

Re: [PATCH v4 11/13] soundwire: amd: fix interrupt gate and work drain ordering in PM ops

[Mukunda,Vijendar]

On 10/26/26 15:57, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] soundwire: amd: amd_enable_sdw_interrupts() in suspend fires stale status IRQs
> - [Medium] soundwire: amd: missing IRQ sync before work drain in amd_suspend()
> --
>
> commit ecf45f9e24e7a7e6c5e9ff39ceefacd5dd89af26
> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>
> soundwire: amd: fix interrupt gate and work drain ordering in PM ops
>
> This commit updates amd_suspend() and amd_suspend_runtime() to disable
> SoundWire interrupts and cancel amd_sdw_irq_thread before draining
> amd_sdw_work and stopping the bus clock. In the AMD_SDW_CLK_STOP_MODE path,
> it also re-enables SoundWire interrupts and configures wake enablement after
> stopping the clock.
>
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index a685bd7da52b1..d7289b840513b 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -1318,15 +1318,20 @@ 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);
> [Severity: Medium]
> Can an in-flight hard IRQ handler on another CPU still schedule
> amd_sdw_irq_thread after cancel_work_sync() returns here (and in the
> corresponding AMD_SDW_POWER_OFF_MODE and amd_suspend_runtime() paths)?
++

This is a valid concern. A follow-up fix will introduce a
software irq_enabled flag in amd_sdw_manager. The flag will be
cleared in amd_disable_sdw_interrupts() before writing to
ACP_EXTERNAL_INTR_CNTL, and checked in acp63_irq_handler()
before calling schedule_work():

     if (amd_manager && amd_manager->irq_enabled)
         schedule_work(&amd_manager->amd_sdw_irq_thread);

Clearing the flag before cancel_work_sync() ensures that any
IRQ handler already executing on another CPU reads
irq_enabled == false and skips schedule_work(), closing the
race window without requiring synchronize_irq().

The wake path is handled correctly. In the CLK_STOP_MODE
suspend path, amd_enable_sdw_interrupts() is called after
amd_sdw_clock_stop(), which restores irq_enabled = true. Wake
interrupts then schedule amd_sdw_irq_thread which calls
amd_sdw_process_wake_event() -> pm_request_resume() for the
resume sequence. In the remove path, amd_enable_sdw_interrupts()
is never called, so irq_enabled remains false permanently.
>
> In sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() (as well as
> acp_irq_handler() and acp7x_irq_handler() in sound/soc/sof/amd/acp.c), the
> hard IRQ handler reads ACP_EXTERNAL_INTR_STAT and schedules
> amd_sdw_irq_thread without holding acp_sdw_lock:
>
> sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
> 	...
> 	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> 	if (ext_intr_stat & ACP_SDW0_STAT) {
> 		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> 		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
> 		if (amd_manager)
> 			schedule_work(&amd_manager->amd_sdw_irq_thread);
> 		irq_flag = 1;
> 	}
> 	...
> }
>
> If the hard IRQ handler on CPU 1 has already read ACP_EXTERNAL_INTR_STAT
> before CPU 0 masks ACP_EXTERNAL_INTR_CNTL in amd_disable_sdw_interrupts(),
> CPU 0 can complete both cancel_work_sync() calls before CPU 1 invokes
> schedule_work(&amd_manager->amd_sdw_irq_thread).
>
> Because amd_disable_sdw_interrupts() only clears the mask registers and does
> not clear ACP_SW_STATE_CHANGE_STATUS_0TO7 or
> ACP_SW_STATE_CHANGE_STATUS_8TO11, the newly scheduled amd_sdw_irq_thread()
> will see the non-zero status registers and schedule amd_sdw_work concurrently
> with amd_sdw_clock_stop() and amd_disable_sdw_manager(). In addition, since
> the post-clock-stop amd_disable_sdw_interrupts() call is removed from the
> AMD_SDW_POWER_OFF_MODE path, a racing amd_sdw_update_slave_status_work() with
> status[0] == SDW_SLAVE_ATTACHED would also write AMD_SDW_IRQ_MASK_0TO7 and
> AMD_SDW_IRQ_MASK_8TO11 back to the mask registers.
>
> Should the driver synchronize with the parent ACP IRQ or clear the status
> registers after synchronizing before draining the work items?
>
>> -		amd_sdw_wake_enable(amd_manager, false);
>>   		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;
>> +		amd_enable_sdw_interrupts(amd_manager);
>> +		amd_sdw_wake_enable(amd_manager, false);
> [Severity: High]
> Could calling amd_enable_sdw_interrupts() right after amd_sdw_clock_stop()
> here and in amd_suspend_runtime() re-trigger slave status interrupts while
> the SoundWire bus clock is stopped?
>
> When a SoundWire interrupt fires just before amd_disable_sdw_interrupts(),
> the hard IRQ handler clears ACP_EXTERNAL_INTR_STAT and queues
> amd_sdw_irq_thread, leaving ACP_SW_STATE_CHANGE_STATUS_0TO7 and
> ACP_SW_STATE_CHANGE_STATUS_8TO11 non-zero until amd_sdw_irq_thread() runs to
> completion:
>
> drivers/soundwire/amd_manager.c:amd_sdw_irq_thread() {
> 	...
> 	status_change_8to11 = readl(amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
> 	status_change_0to7 = readl(amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);
> 	if (!status_change_0to7 && !status_change_8to11)
> 		return;
> 	...
> 	if (status_change_8to11 || status_change_0to7)
> 		schedule_work(&amd_manager->amd_sdw_work);
> 	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
> 	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);
> }
>
> If cancel_work_sync(&amd_manager->amd_sdw_irq_thread) cancels that pending
> work item before it executes, or if any slave status bits latch during
> sdw_bus_prep_clk_stop() and sdw_bus_clk_stop() while the masks are zeroed,
> ACP_SW_STATE_CHANGE_STATUS_0TO7 and ACP_SW_STATE_CHANGE_STATUS_8TO11 remain
> non-zero after amd_sdw_clock_stop() sets clk_stopped = true.
>
> Calling amd_enable_sdw_interrupts() at that point unmasks
> ACP_EXTERNAL_INTR_CNTL, AMD_SDW_IRQ_MASK_0TO7, and AMD_SDW_IRQ_MASK_8TO11:
>
> drivers/soundwire/amd_manager.c:amd_enable_sdw_interrupts() {
> 	...
> 	writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio +
> 		       ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
> 	writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio +
> 		       ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
> 	writel(AMD_SDW_IRQ_ERROR_MASK, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
> }
>
> Wouldn't unmasking those registers with stale bits still set in
> ACP_SW_STATE_CHANGE_STATUS_0TO7 and ACP_SW_STATE_CHANGE_STATUS_8TO11
> immediately fire an interrupt and schedule amd_sdw_irq_thread() and
> amd_sdw_work while the link clock is stopped, causing MIPI command timeouts
> in amd_sdw_update_slave_status_work()?
ACP_SW_STATE_CHANGE_STATUS_0TO7 and
ACP_SW_STATE_CHANGE_STATUS_8TO11 are internal SoundWire status
registers polled directly by amd_sdw_irq_thread(). Stale bits
in these registers do not set ACP_EXTERNAL_INTR_STAT or raise
a CPU interrupt when the masks are re-enabled.

Once the bus clock is stopped, no slave status interrupts can
be asserted — only wake interrupts remain active, which are
handled separately via amd_sdw_wake_enable(). Calling
amd_enable_sdw_interrupts() after amd_sdw_clock_stop() is
therefore safe — no spurious interrupt delivery or MIPI command
timeouts can result from stale status bits.
>

[PATCH v4 12/13] soundwire: amd: fix IRQ thread dropping STATUS_0TO7 on simultaneous wake

[Vijendar Mukunda]

When a wake interrupt and a device-state change arrive simultaneously,
the IRQ thread used an early return after detecting the wake bit,
silently discarding STATUS_0TO7.

When the manager is already at D0, pm_request_resume() is a no-op and
amd_resume_runtime() is never called, so the dropped ATTACHED event
cannot be recovered and jack detection stops working.

Remove the early return: call amd_sdw_process_wake_event() inside a
conditional block and fall through to the normal slave-status processing
path. Mask AMD_SDW_WAKE_STAT_MASK out of the local status_change_8to11
copy after handling the wake event so the remaining interrupt bits are
processed without re-entering the wake path.

Fixes: 66c878838f37 ("soundwire: amd: handle SoundWire wake enable interrupt")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index d7289b840513..16dd17f7a343 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1019,8 +1019,11 @@ static void amd_sdw_irq_thread(struct work_struct *work)
 
 	dev_dbg(amd_manager->dev, "[SDW%d] SDW INT: 0to7=0x%x, 8to11=0x%x\n",
 		amd_manager->instance, status_change_0to7, status_change_8to11);
-	if (status_change_8to11 & AMD_SDW_WAKE_STAT_MASK)
-		return amd_sdw_process_wake_event(amd_manager);
+
+	if (status_change_8to11 & AMD_SDW_WAKE_STAT_MASK) {
+		amd_sdw_process_wake_event(amd_manager);
+		status_change_8to11 &= ~AMD_SDW_WAKE_STAT_MASK;
+	}
 
 	if (status_change_8to11 & AMD_SDW_PREQ_INTR_STAT) {
 		amd_sdw_read_and_process_ping_status(amd_manager);
-- 
2.53.0

Re: [PATCH v4 12/13] soundwire: amd: fix IRQ thread dropping STATUS_0TO7 on simultaneous wake

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soundwire: amd: race with runtime resume in amd_sdw_irq_thread() on wake
--

commit 926ef4f8a65a29a6e1b8f7dfc94a0c8f49817776
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: fix IRQ thread dropping STATUS_0TO7 on simultaneous wake

This commit removes the early return in amd_sdw_irq_thread() when
handling a SoundWire wake interrupt and masks AMD_SDW_WAKE_STAT_MASK out
of status_change_8to11 so that simultaneous peripheral status changes are
processed instead of dropped.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index d7289b840513b..16dd17f7a343a 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -1019,8 +1019,11 @@ static void amd_sdw_irq_thread(struct work_struct *work)
>  
>  	dev_dbg(amd_manager->dev, "[SDW%d] SDW INT: 0to7=0x%x, 8to11=0x%x\n",
>  		amd_manager->instance, status_change_0to7, status_change_8to11);
> -	if (status_change_8to11 & AMD_SDW_WAKE_STAT_MASK)
> -		return amd_sdw_process_wake_event(amd_manager);
> +
> +	if (status_change_8to11 & AMD_SDW_WAKE_STAT_MASK) {
> +		amd_sdw_process_wake_event(amd_manager);
> +		status_change_8to11 &= ~AMD_SDW_WAKE_STAT_MASK;
> +	}
>  
>  	if (status_change_8to11 & AMD_SDW_PREQ_INTR_STAT) {
>  		amd_sdw_read_and_process_ping_status(amd_manager);

[Severity: High]
Can removing the early return here cause peripheral status processing to
race with runtime resume or execute while the bus clock is still stopped
when waking from runtime suspend?

In drivers/soundwire/amd_manager.c, amd_sdw_process_wake_event() only
queues an asynchronous resume via pm_request_resume():

static void amd_sdw_process_wake_event(struct amd_sdw_manager *amd_manager)
{
	dev_dbg(amd_manager->dev, "SoundWire Wake event reported\n");
	pm_request_resume(amd_manager->dev);
	writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance));
	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
}

When amd_sdw_irq_thread() runs with AMD_SDW_WAKE_STAT_MASK and peripheral
status bits set in status_change_0to7 or status_change_8to11 while
amd_manager->dev is runtime suspended or resuming, pm_request_resume()
returns immediately without waiting for amd_resume_runtime() to finish on
pm_wq.

Execution then falls through in amd_sdw_irq_thread() to either call
amd_sdw_read_and_process_ping_status() or call
amd_sdw_update_slave_status() and schedule amd_manager->amd_sdw_work
(amd_sdw_update_slave_status_work()), and then clears
ACP_SW_STATE_CHANGE_STATUS_8TO11 and ACP_SW_STATE_CHANGE_STATUS_0TO7:

	if (status_change_8to11 & AMD_SDW_PREQ_INTR_STAT) {
		amd_sdw_read_and_process_ping_status(amd_manager);
	} else {
		/* Check for the updated status on peripheral device */
		amd_sdw_update_slave_status(status_change_0to7, status_change_8to11, amd_manager);
	}
	if (status_change_8to11 || status_change_0to7)
		schedule_work(&amd_manager->amd_sdw_work);
	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);

Neither amd_sdw_read_and_process_ping_status() nor
amd_sdw_update_slave_status_work() synchronizes with runtime PM on
amd_manager->dev before accessing the bus or interrupt masks:

static void amd_sdw_update_slave_status_work(struct work_struct *work)
{
	...
	if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
		writel(0, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
		writel(0, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
	}

update_status:
	sdw_handle_slave_status(&amd_manager->bus, amd_manager->status);

In AMD_SDW_CLK_STOP_MODE, amd_suspend_runtime() stops the clock and keeps
interrupts enabled while clk_stopped is true, and amd_resume_runtime()
restarts the clock in amd_sdw_clock_stop_exit() before completing
sdw_bus_exit_clk_stop() and the D0 transition. In AMD_SDW_POWER_OFF_MODE,
amd_resume_runtime() resets slave status and reinitializes the manager
and interrupt masks.

Could amd_sdw_read_and_process_ping_status() or
amd_sdw_update_slave_status_work() issue immediate commands via
amd_sdw_send_cmd_get_resp() while the clock is still stopped (timing out
after the status registers have already been cleared) or race
concurrently with amd_resume_runtime()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026044507.2310694-1-Vijendar.Mukunda@amd.com?part=12

Re: [PATCH v4 12/13] soundwire: amd: fix IRQ thread dropping STATUS_0TO7 on simultaneous wake

[Mukunda,Vijendar]

On 10/26/26 16:12, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] soundwire: amd: race with runtime resume in amd_sdw_irq_thread() on wake
> --
>
> commit 926ef4f8a65a29a6e1b8f7dfc94a0c8f49817776
> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>
> soundwire: amd: fix IRQ thread dropping STATUS_0TO7 on simultaneous wake
>
> This commit removes the early return in amd_sdw_irq_thread() when
> handling a SoundWire wake interrupt and masks AMD_SDW_WAKE_STAT_MASK out
> of status_change_8to11 so that simultaneous peripheral status changes are
> processed instead of dropped.
>
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index d7289b840513b..16dd17f7a343a 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -1019,8 +1019,11 @@ static void amd_sdw_irq_thread(struct work_struct *work)
>>   
>>   	dev_dbg(amd_manager->dev, "[SDW%d] SDW INT: 0to7=0x%x, 8to11=0x%x\n",
>>   		amd_manager->instance, status_change_0to7, status_change_8to11);
>> -	if (status_change_8to11 & AMD_SDW_WAKE_STAT_MASK)
>> -		return amd_sdw_process_wake_event(amd_manager);
>> +
>> +	if (status_change_8to11 & AMD_SDW_WAKE_STAT_MASK) {
>> +		amd_sdw_process_wake_event(amd_manager);
>> +		status_change_8to11 &= ~AMD_SDW_WAKE_STAT_MASK;
>> +	}
>>   
>>   	if (status_change_8to11 & AMD_SDW_PREQ_INTR_STAT) {
>>   		amd_sdw_read_and_process_ping_status(amd_manager);
> [Severity: High]
> Can removing the early return here cause peripheral status processing to
> race with runtime resume or execute while the bus clock is still stopped
> when waking from runtime suspend?
++

The fall-through is intentional and safe. This fix handles the case
where a peripheral signals a status change while the bus is already
active — wake and status bits can arrive in the same interrupt cycle,
and the old early return silently dropped status_change_0to7.

In AMD_SDW_CLK_STOP_MODE and AMD_SDW_POWER_OFF_MODE,
amd_suspend_runtime() calls cancel_work_sync() on both
amd_sdw_irq_thread and amd_sdw_work before amd_sdw_clock_stop()
runs, so the irq_thread cannot be in flight while the clock is
stopped. After clock stop, only the wake interrupt is re-enabled.
ACP_SW_STATE_CHANGE_STATUS_0TO7 and AMD_SDW_PREQ_INTR_STAT are
set only by SoundWire bus transactions, which require the bus clock.
So when the wake interrupt fires from suspend, those bits are zero
and the fall-through path to status processing is not taken.
> In drivers/soundwire/amd_manager.c, amd_sdw_process_wake_event() only
> queues an asynchronous resume via pm_request_resume():
>
> static void amd_sdw_process_wake_event(struct amd_sdw_manager *amd_manager)
> {
> 	dev_dbg(amd_manager->dev, "SoundWire Wake event reported\n");
> 	pm_request_resume(amd_manager->dev);
> 	writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance));
> 	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
> }
>
> When amd_sdw_irq_thread() runs with AMD_SDW_WAKE_STAT_MASK and peripheral
> status bits set in status_change_0to7 or status_change_8to11 while
> amd_manager->dev is runtime suspended or resuming, pm_request_resume()
> returns immediately without waiting for amd_resume_runtime() to finish on
> pm_wq.
pm_request_resume() returning immediately is not a problem here.
The irq_thread cannot re-enter until the next interrupt, and the
next SoundWire interrupt only fires after amd_resume_runtime()
re-enables interrupts and brings the bus back up. There is no
window where status processing runs concurrently with an in-progress
resume.
> Execution then falls through in amd_sdw_irq_thread() to either call
> amd_sdw_read_and_process_ping_status() or call
> amd_sdw_update_slave_status() and schedule amd_manager->amd_sdw_work
> (amd_sdw_update_slave_status_work()), and then clears
> ACP_SW_STATE_CHANGE_STATUS_8TO11 and ACP_SW_STATE_CHANGE_STATUS_0TO7:
>
> 	if (status_change_8to11 & AMD_SDW_PREQ_INTR_STAT) {
> 		amd_sdw_read_and_process_ping_status(amd_manager);
> 	} else {
> 		/* Check for the updated status on peripheral device */
> 		amd_sdw_update_slave_status(status_change_0to7, status_change_8to11, amd_manager);
> 	}
> 	if (status_change_8to11 || status_change_0to7)
> 		schedule_work(&amd_manager->amd_sdw_work);
> 	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
> 	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);
The fall-through is only reachable when the bus is active. In
AMD_SDW_CLK_STOP_MODE, ACP_SW_STATE_CHANGE_STATUS_0TO7 and
AMD_SDW_PREQ_INTR_STAT are only set by SoundWire bus transactions,
which require the bus clock to be running. When the wake interrupt
fires from clock-stop suspend those bits are zero, so neither
amd_sdw_read_and_process_ping_status() nor amd_sdw_update_slave_
status() is reached.
> Neither amd_sdw_read_and_process_ping_status() nor
> amd_sdw_update_slave_status_work() synchronizes with runtime PM on
> amd_manager->dev before accessing the bus or interrupt masks:
>
> static void amd_sdw_update_slave_status_work(struct work_struct *work)
> {
> 	...
> 	if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
> 		writel(0, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
> 		writel(0, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
> 	}
>
> update_status:
> 	sdw_handle_slave_status(&amd_manager->bus, amd_manager->status);
Explicit runtime PM synchronization is not needed in these functions
because the suspend path in amd_suspend_runtime() calls
cancel_work_sync() on both amd_sdw_irq_thread and amd_sdw_work
before amd_sdw_clock_stop() runs:

     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_clock_stop(amd_manager);

This guarantees neither work item is in flight when the clock stops.
The bus is therefore always running when these functions execute.
>
> In AMD_SDW_CLK_STOP_MODE, amd_suspend_runtime() stops the clock and keeps
> interrupts enabled while clk_stopped is true, and amd_resume_runtime()
> restarts the clock in amd_sdw_clock_stop_exit() before completing
> sdw_bus_exit_clk_stop() and the D0 transition. In AMD_SDW_POWER_OFF_MODE,
> amd_resume_runtime() resets slave status and reinitializes the manager
> and interrupt masks.
In AMD_SDW_CLK_STOP_MODE, after the clock is stopped only the wake
interrupt is re-enabled via amd_sdw_wake_enable(). The
AMD_SDW_WAKE_STAT_MASK bit is set by the hardware wake-detection
circuit, independently of bus clock activity. Peripheral status bits
in ACP_SW_STATE_CHANGE_STATUS_0TO7 and AMD_SDW_PREQ_INTR_STAT
require the bus clock, so they will be zero when the wake fires.
After AMD_SDW_WAKE_STAT_MASK is cleared by amd_sdw_process_wake_
event(), status_change_8to11 becomes zero and amd_sdw_irq_thread()
returns without touching the bus.

In AMD_SDW_POWER_OFF_MODE, amd_resume_runtime() fully reinitializes
the manager — restoring the clock, calling amd_init_sdw_manager(),
and re-enabling interrupts — before returning. The irq_thread and
amd_sdw_work are cancelled in amd_suspend_runtime() before power-
off, so neither can run during resume. The next irq_thread execution
can only be scheduled by a new interrupt, which requires the bus to
be fully up — guaranteed by the time amd_resume_runtime() returns.

>
> Could amd_sdw_read_and_process_ping_status() or
> amd_sdw_update_slave_status_work() issue immediate commands via
> amd_sdw_send_cmd_get_resp() while the clock is still stopped (timing out
> after the status registers have already been cleared) or race
> concurrently with amd_resume_runtime()?
No. As explained above, amd_suspend_runtime() drains both work items
via cancel_work_sync() before stopping the clock, so
amd_sdw_read_and_process_ping_status() and
amd_sdw_update_slave_status_work() cannot execute while
clk_stopped == true.

Likewise, there is no concurrent race with amd_resume_runtime()
because the next interrupt (which would schedule the irq_thread)
only fires after amd_resume_runtime() has re-enabled interrupts and
restored the bus. The runtime PM framework serializes this: the
device must be fully resumed before new interrupts are delivered.

[PATCH v4 13/13] soundwire: amd: fix STATUS_8TO11 double-clear race in wake handler

[Vijendar Mukunda]

amd_sdw_process_wake_event() cleared ACP_SW_STATE_CHANGE_STATUS_8TO11,
and amd_sdw_irq_thread() cleared it again at the tail. A new interrupt
arriving between the two clears would be wiped before being read,
causing an event to be silently lost.

Remove the ACP_SW_STATE_CHANGE_STATUS_8TO11 clear from
amd_sdw_process_wake_event(), leaving the single tail clear in
amd_sdw_irq_thread() as the only write to that register per invocation.

Fixes: 66c878838f37 ("soundwire: amd: handle SoundWire wake enable interrupt")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 16dd17f7a343..38a9aa1af59a 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1002,7 +1002,6 @@ static void amd_sdw_process_wake_event(struct amd_sdw_manager *amd_manager)
 	dev_dbg(amd_manager->dev, "SoundWire Wake event reported\n");
 	pm_request_resume(amd_manager->dev);
 	writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance));
-	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
 }
 
 static void amd_sdw_irq_thread(struct work_struct *work)
-- 
2.53.0