Re: [PATCH 1/8] soundwire: amd: fix SDW command timeout return value handling
From: Mukunda,Vijendar
Date: Sat Sep 12 2026 - 05:06:50 EST
On 9/11/26 23:18, Mario Limonciello wrote:
On 9/10/26 14:00, Vijendar Mukunda wrote:
amd_sdw_send_cmd_get_resp() returned u64 but was returning -ETIMEDOUT
(a negative int) on readl_poll_timeout() failures, which silently
widens to a large u64 value.
Change the return type to int and pass the raw u64 response through an
out-parameter. Timeout is detected directly from readl_poll_timeout()
return value.
Update all callers: amd_program_scp_addr(), _amd_sdw_xfer_msg(),
amd_sdw_read_and_process_ping_status(), and amd_sdw_read_ping_status()
to check the return value and handle timeout explicitly. In
amd_sdw_read_and_process_ping_status(), the mutex unlock is also moved
to after amd_sdw_process_ping_status() so the lock is held across the
full status processing step.
Fixes: d8f48fbdfd9a ("soundwire: amd: Add support for AMD Manager driver")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
---
drivers/soundwire/amd_manager.c | 56 +++++++++++++++++++--------------
1 file changed, 33 insertions(+), 23 deletions(-)
diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index a3316efdf8ac..fa3f4e797edd 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -250,10 +250,9 @@ static void amd_sdw_ctl_word_prep(u32 *lower_word, u32 *upper_word, struct sdw_m
*lower_word = lower_data;
}
-static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lower_data,
- u32 upper_data)
+static int amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lower_data,
+ u32 upper_data, u64 *response)
{
- u64 resp;
u32 lower_resp, upper_resp;
u32 sts;
int ret;
@@ -291,9 +290,8 @@ static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lo
amd_manager->instance);
return ret;
}
- resp = upper_resp;
- resp = (resp << 32) | lower_resp;
- return resp;
+ *response = ((u64)upper_resp << 32) | lower_resp;
+ return 0;
}
static enum sdw_command_response
@@ -309,19 +307,22 @@ amd_program_scp_addr(struct amd_sdw_manager *amd_manager, struct sdw_msg *msg)
scp_msg.buf = &msg->addr_page1;
scp_msg.flags = SDW_MSG_FLAG_WRITE;
amd_sdw_ctl_word_prep(&lower_data, &upper_data, &scp_msg, 0);
- response_buf[0] = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data);
+ if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data, &response_buf[0])) {
+ dev_err_ratelimited(amd_manager->dev,
+ "SCP_addrpage command timeout for Slave %d\n", msg->dev_num);
+ return SDW_CMD_TIMEOUT;
+ }
scp_msg.addr = SDW_SCP_ADDRPAGE2;
scp_msg.buf = &msg->addr_page2;
amd_sdw_ctl_word_prep(&lower_data, &upper_data, &scp_msg, 0);
- response_buf[1] = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data);
+ if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data, &response_buf[1])) {
+ dev_err_ratelimited(amd_manager->dev,
+ "SCP_addrpage command timeout for Slave %d\n", msg->dev_num);
+ return SDW_CMD_TIMEOUT;
+ }
for (index = 0; index < 2; index++) {
- if (response_buf[index] == -ETIMEDOUT) {
- dev_err_ratelimited(amd_manager->dev,
- "SCP_addrpage command timeout for Slave %d\n",
- msg->dev_num);
- return SDW_CMD_TIMEOUT;
- } else if (!(response_buf[index] & AMD_SDW_MCP_RESP_ACK)) {
+ if (!(response_buf[index] & AMD_SDW_MCP_RESP_ACK)) {
if (response_buf[index] & AMD_SDW_MCP_RESP_NACK) {
dev_err_ratelimited(amd_manager->dev,
"SCP_addrpage NACKed for Slave %d\n",
@@ -366,11 +367,7 @@ static enum sdw_command_response amd_sdw_fill_msg_resp(struct amd_sdw_manager *a
if (msg->flags == SDW_MSG_FLAG_READ)
msg->buf[offset] = FIELD_GET(AMD_SDW_MCP_RESP_RDATA, response);
} else {
- if (response == -ETIMEDOUT) {
- dev_err_ratelimited(amd_manager->dev, "command timeout for Slave %d\n",
- msg->dev_num);
- return SDW_CMD_TIMEOUT;
- } else if (response & AMD_SDW_MCP_RESP_NACK) {
+ if (response & AMD_SDW_MCP_RESP_NACK) {
dev_err_ratelimited(amd_manager->dev,
"command response NACK received for Slave %d\n",
msg->dev_num);
@@ -390,7 +387,11 @@ static unsigned int _amd_sdw_xfer_msg(struct amd_sdw_manager *amd_manager, struc
u32 upper_data = 0, lower_data = 0;
amd_sdw_ctl_word_prep(&lower_data, &upper_data, msg, cmd_offset);
- response = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data);
+ if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data, &response)) {
+ dev_err_ratelimited(amd_manager->dev, "command timeout for Slave %d\n",
+ msg->dev_num);
+ return SDW_CMD_TIMEOUT;
+ }
return amd_sdw_fill_msg_resp(amd_manager, msg, response, cmd_offset);
}
@@ -446,9 +447,14 @@ static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_man
u64 response;
mutex_lock(&amd_manager->bus.msg_lock);
guard(mutex) might work better here so you don't need to cover every exit path with a mutex_unlock() manually.
Thanks for the suggestion. I agree that guard(mutex) would simplify
the locking and make the error paths cleaner.
For this patch, I wanted to keep the change focused on fixing the
-ETIMEDOUT handling issue and avoid mixing it with unrelated cleanup.
The driver currently uses explicit mutex_lock() / mutex_unlock() pairs
throughout amd_manager.c, so introducing guard(mutex) in a single
function would make the locking style inconsistent.
I'll plan to follow up with a separate cleanup patch that converts the
eligible mutex-protected paths to guard(mutex) consistently across the
driver. Keeping that work separate should make both the bug fix and the
cleanup easier to review and bisect.
- response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0);
- mutex_unlock(&amd_manager->bus.msg_lock);
+ if (amd_sdw_send_cmd_get_resp(amd_manager, 0, 0, &response)) {
+ mutex_unlock(&amd_manager->bus.msg_lock);
+ dev_err_ratelimited(amd_manager->dev, "SDW%x ping status timeout\n",
+ amd_manager->instance);
+ return;
+ }
amd_sdw_process_ping_status(response, amd_manager);
+ mutex_unlock(&amd_manager->bus.msg_lock);
}
static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
@@ -457,7 +463,11 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
u64 response;
u32 slave_stat;
- response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0);
+ if (amd_sdw_send_cmd_get_resp(amd_manager, 0, 0, &response)) {
+ dev_err_ratelimited(amd_manager->dev, "SDW%x ping status timeout\n",
+ amd_manager->instance);
+ return 0;
+ }
/* slave status from ping response */
slave_stat = FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_0_3, response);
slave_stat |= FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_4_11, response) << 8;