Re: [PATCH v4 12/13] soundwire: amd: fix IRQ thread dropping STATUS_0TO7 on simultaneous wake

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soundwire: amd: race with runtime resume in amd_sdw_irq_thread() on wake
--

commit 926ef4f8a65a29a6e1b8f7dfc94a0c8f49817776
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: fix IRQ thread dropping STATUS_0TO7 on simultaneous wake

This commit removes the early return in amd_sdw_irq_thread() when
handling a SoundWire wake interrupt and masks AMD_SDW_WAKE_STAT_MASK out
of status_change_8to11 so that simultaneous peripheral status changes are
processed instead of dropped.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index d7289b840513b..16dd17f7a343a 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -1019,8 +1019,11 @@ static void amd_sdw_irq_thread(struct work_struct *work)
>  
>  	dev_dbg(amd_manager->dev, "[SDW%d] SDW INT: 0to7=0x%x, 8to11=0x%x\n",
>  		amd_manager->instance, status_change_0to7, status_change_8to11);
> -	if (status_change_8to11 & AMD_SDW_WAKE_STAT_MASK)
> -		return amd_sdw_process_wake_event(amd_manager);
> +
> +	if (status_change_8to11 & AMD_SDW_WAKE_STAT_MASK) {
> +		amd_sdw_process_wake_event(amd_manager);
> +		status_change_8to11 &= ~AMD_SDW_WAKE_STAT_MASK;
> +	}
>  
>  	if (status_change_8to11 & AMD_SDW_PREQ_INTR_STAT) {
>  		amd_sdw_read_and_process_ping_status(amd_manager);

[Severity: High]
Can removing the early return here cause peripheral status processing to
race with runtime resume or execute while the bus clock is still stopped
when waking from runtime suspend?

In drivers/soundwire/amd_manager.c, amd_sdw_process_wake_event() only
queues an asynchronous resume via pm_request_resume():

static void amd_sdw_process_wake_event(struct amd_sdw_manager *amd_manager)
{
	dev_dbg(amd_manager->dev, "SoundWire Wake event reported\n");
	pm_request_resume(amd_manager->dev);
	writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance));
	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
}

When amd_sdw_irq_thread() runs with AMD_SDW_WAKE_STAT_MASK and peripheral
status bits set in status_change_0to7 or status_change_8to11 while
amd_manager->dev is runtime suspended or resuming, pm_request_resume()
returns immediately without waiting for amd_resume_runtime() to finish on
pm_wq.

Execution then falls through in amd_sdw_irq_thread() to either call
amd_sdw_read_and_process_ping_status() or call
amd_sdw_update_slave_status() and schedule amd_manager->amd_sdw_work
(amd_sdw_update_slave_status_work()), and then clears
ACP_SW_STATE_CHANGE_STATUS_8TO11 and ACP_SW_STATE_CHANGE_STATUS_0TO7:

	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);
	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);

Neither amd_sdw_read_and_process_ping_status() nor
amd_sdw_update_slave_status_work() synchronizes with runtime PM on
amd_manager->dev before accessing the bus or interrupt masks:

static void amd_sdw_update_slave_status_work(struct work_struct *work)
{
	...
	if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
		writel(0, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
		writel(0, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
	}

update_status:
	sdw_handle_slave_status(&amd_manager->bus, amd_manager->status);

In AMD_SDW_CLK_STOP_MODE, amd_suspend_runtime() stops the clock and keeps
interrupts enabled while clk_stopped is true, and amd_resume_runtime()
restarts the clock in amd_sdw_clock_stop_exit() before completing
sdw_bus_exit_clk_stop() and the D0 transition. In AMD_SDW_POWER_OFF_MODE,
amd_resume_runtime() resets slave status and reinitializes the manager
and interrupt masks.

Could amd_sdw_read_and_process_ping_status() or
amd_sdw_update_slave_status_work() issue immediate commands via
amd_sdw_send_cmd_get_resp() while the clock is still stopped (timing out
after the status registers have already been cleared) or race
concurrently with amd_resume_runtime()?

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