Re: [PATCH v5 10/25] 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 to consider:
- [High] soundwire: amd: unpolled IMM_RES_VALID clear in
  amd_sdw_send_cmd_get_resp()
--

commit 89df36f0150dc3c96912a2aa097eb2d1d5dc0e6d
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 a warning, write
*response, and return 0 when polling for AMD_SDW_IMM_RES_VALID to clear
times out after reading the response registers.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index f2ed0490aa82d..579b12946c0ca 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -384,9 +384,12 @@ static int amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lo
>  	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) {
> -		dev_err(amd_manager->dev, "SDW%x cmd status retry failed\n",
> -			amd_manager->instance);
> -		return ret;
> +		/* Response already latched above; timeout here is non-fatal.
> +		 * The residual IMM_RES_VALID bit will be cleared by the bad-state
> +		 * guard on the next invocation, provided IMM_CMD_BUSY is not also stuck.
> +		 */
> +		dev_warn(amd_manager->dev, "SDW%x cmd status retry failed\n",
> +			 amd_manager->instance);
>  	}
>  	*response = ((u64)upper_resp << 32) | lower_resp;
>  	return 0;

[Severity: High]
Can returning 0 when AMD_SDW_IMM_RES_VALID fails to clear cause the next
command to read stale response registers from ACP_SW_IMM_RESP_UPPER_WORD
and ACP_SW_IMM_RESP_LOWER_QWORD?

When a caller such as amd_sdw_xfer_msg() issues back-to-back commands, the
next call to amd_sdw_send_cmd_get_resp() hits the bad-state guard, which
writes AMD_SDW_IMM_RES_VALID without polling for it to clear before
issuing the new command and polling for (sts & AMD_SDW_IMM_RES_VALID):

drivers/soundwire/amd_manager.c:amd_sdw_send_cmd_get_resp() {
    ...
	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 readl_poll_timeout() reads ACP_SW_IMM_CMD_STS immediately before
sleeping and the clear handshake is asynchronous, the residual
AMD_SDW_IMM_RES_VALID bit can still be set on the first read, causing
amd_sdw_send_cmd_get_resp() to return the previous command's response
before the new SoundWire frame finishes on the bus.  Follow-up commit
"soundwire: amd: clarify bad-state guard poll omission in IMM_RES_VALID
path" documents the missing poll in the bad-state guard, but does not
prevent the subsequent response poll from matching the un-cleared bit.

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