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

["Mukunda,Vijendar" <vijendar.mukunda@amd.com>]

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

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

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

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