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

[Mario Limonciello <mario.limonciello@amd.com>]

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

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

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