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

[sashiko-bot@kernel.org]

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