Re: [PATCH 13/21] soundwire: amd: Check ping command response for timeout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] soundwire: amd: stale status[] processed on ping timeout
--

commit 390668deca85bb859cd47cec7a3a11161bb84e4d
Author: Lee Jones <lee@kernel.org>

soundwire: amd: Check ping command response for timeout

This commit adds checks for -ETIMEDOUT after calling
amd_sdw_send_cmd_get_resp() in amd_sdw_read_and_process_ping_status() and
amd_sdw_read_ping_status() so that a timeout error code is not decoded as
peripheral status bits.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index a3316efdf8ac2..a8220235a2ae3 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -448,6 +448,8 @@ static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_man
>  	mutex_lock(&amd_manager->bus.msg_lock);
>  	response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0);
>  	mutex_unlock(&amd_manager->bus.msg_lock);
> +	if (response == -ETIMEDOUT)
> +		return;

[Severity: Medium]
When amd_sdw_send_cmd_get_resp() times out here, returning early without
propagating an error leaves amd_manager->status[] containing stale status
values while callers proceed as if the status array was refreshed.

In drivers/soundwire/amd_manager.c, amd_sdw_update_slave_status_work()
relies on amd_sdw_read_and_process_ping_status() to overwrite
amd_manager->status[] before jumping back to update_status:

	if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
		if (retry_count++ < SDW_MAX_DEVICES) {
			writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio +
			       ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
			writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio +
			       ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
			amd_sdw_read_and_process_ping_status(amd_manager);
			goto update_status;
		}
		...
	}

Because sdw_handle_slave_status() only reads amd_manager->status[] and
does not clear status[0], a ping timeout leaves status[0] set to
SDW_SLAVE_ATTACHED (along with any other stale entries in status[]),
causing the loop to repeatedly call sdw_handle_slave_status() with stale
peripheral status up to SDW_MAX_DEVICES times.

Similarly, in amd_sdw_irq_thread():

	if (status_change_8to11 & AMD_SDW_PREQ_INTR_STAT) {
		amd_sdw_read_and_process_ping_status(amd_manager);
	} else {
		/* Check for the updated status on peripheral device */
		amd_sdw_update_slave_status(status_change_0to7, status_change_8to11, amd_manager);
	}
	if (status_change_8to11 || status_change_0to7)
		schedule_work(&amd_manager->amd_sdw_work);

If the ping command times out when handling AMD_SDW_PREQ_INTR_STAT,
amd_sdw_work is still scheduled and will process stale status values left
in amd_manager->status[] from a previous interrupt.

Should amd_sdw_read_and_process_ping_status() return an error code so that
its callers can avoid reprocessing stale amd_manager->status[] entries on
timeout?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030104840.3361217-1-lee@kernel.org?part=13