Re: [PATCH v4 11/13] soundwire: amd: fix interrupt gate and work drain ordering in PM ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] soundwire: amd: amd_enable_sdw_interrupts() in suspend fires stale status IRQs
- [Medium] soundwire: amd: missing IRQ sync before work drain in amd_suspend()
--

commit ecf45f9e24e7a7e6c5e9ff39ceefacd5dd89af26
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: fix interrupt gate and work drain ordering in PM ops

This commit updates amd_suspend() and amd_suspend_runtime() to disable
SoundWire interrupts and cancel amd_sdw_irq_thread before draining
amd_sdw_work and stopping the bus clock. In the AMD_SDW_CLK_STOP_MODE path,
it also re-enables SoundWire interrupts and configures wake enablement after
stopping the clock.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index a685bd7da52b1..d7289b840513b 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -1318,15 +1318,20 @@ static int __maybe_unused amd_suspend(struct device *dev)
>  	}
>  
>  	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
> +		amd_disable_sdw_interrupts(amd_manager);
> +		cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
>  		cancel_work_sync(&amd_manager->amd_sdw_work);

[Severity: Medium]
Can an in-flight hard IRQ handler on another CPU still schedule
amd_sdw_irq_thread after cancel_work_sync() returns here (and in the
corresponding AMD_SDW_POWER_OFF_MODE and amd_suspend_runtime() paths)?

In sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() (as well as
acp_irq_handler() and acp7x_irq_handler() in sound/soc/sof/amd/acp.c), the
hard IRQ handler reads ACP_EXTERNAL_INTR_STAT and schedules
amd_sdw_irq_thread without holding acp_sdw_lock:

sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
	...
	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
	if (ext_intr_stat & ACP_SDW0_STAT) {
		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
		if (amd_manager)
			schedule_work(&amd_manager->amd_sdw_irq_thread);
		irq_flag = 1;
	}
	...
}

If the hard IRQ handler on CPU 1 has already read ACP_EXTERNAL_INTR_STAT
before CPU 0 masks ACP_EXTERNAL_INTR_CNTL in amd_disable_sdw_interrupts(),
CPU 0 can complete both cancel_work_sync() calls before CPU 1 invokes
schedule_work(&amd_manager->amd_sdw_irq_thread).

Because amd_disable_sdw_interrupts() only clears the mask registers and does
not clear ACP_SW_STATE_CHANGE_STATUS_0TO7 or
ACP_SW_STATE_CHANGE_STATUS_8TO11, the newly scheduled amd_sdw_irq_thread()
will see the non-zero status registers and schedule amd_sdw_work concurrently
with amd_sdw_clock_stop() and amd_disable_sdw_manager(). In addition, since
the post-clock-stop amd_disable_sdw_interrupts() call is removed from the
AMD_SDW_POWER_OFF_MODE path, a racing amd_sdw_update_slave_status_work() with
status[0] == SDW_SLAVE_ATTACHED would also write AMD_SDW_IRQ_MASK_0TO7 and
AMD_SDW_IRQ_MASK_8TO11 back to the mask registers.

Should the driver synchronize with the parent ACP IRQ or clear the status
registers after synchronizing before draining the work items?

> -		amd_sdw_wake_enable(amd_manager, false);
>  		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
>  		if (ret)
>  			return ret;
>  		ret = amd_sdw_clock_stop(amd_manager);
>  		if (ret)
>  			return ret;
> +		amd_enable_sdw_interrupts(amd_manager);
> +		amd_sdw_wake_enable(amd_manager, false);

[Severity: High]
Could calling amd_enable_sdw_interrupts() right after amd_sdw_clock_stop()
here and in amd_suspend_runtime() re-trigger slave status interrupts while
the SoundWire bus clock is stopped?

When a SoundWire interrupt fires just before amd_disable_sdw_interrupts(),
the hard IRQ handler clears ACP_EXTERNAL_INTR_STAT and queues
amd_sdw_irq_thread, leaving ACP_SW_STATE_CHANGE_STATUS_0TO7 and
ACP_SW_STATE_CHANGE_STATUS_8TO11 non-zero until amd_sdw_irq_thread() runs to
completion:

drivers/soundwire/amd_manager.c:amd_sdw_irq_thread() {
	...
	status_change_8to11 = readl(amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
	status_change_0to7 = readl(amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);
	if (!status_change_0to7 && !status_change_8to11)
		return;
	...
	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);
}

If cancel_work_sync(&amd_manager->amd_sdw_irq_thread) cancels that pending
work item before it executes, or if any slave status bits latch during
sdw_bus_prep_clk_stop() and sdw_bus_clk_stop() while the masks are zeroed,
ACP_SW_STATE_CHANGE_STATUS_0TO7 and ACP_SW_STATE_CHANGE_STATUS_8TO11 remain
non-zero after amd_sdw_clock_stop() sets clk_stopped = true.

Calling amd_enable_sdw_interrupts() at that point unmasks
ACP_EXTERNAL_INTR_CNTL, AMD_SDW_IRQ_MASK_0TO7, and AMD_SDW_IRQ_MASK_8TO11:

drivers/soundwire/amd_manager.c:amd_enable_sdw_interrupts() {
	...
	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);
	writel(AMD_SDW_IRQ_ERROR_MASK, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
}

Wouldn't unmasking those registers with stale bits still set in
ACP_SW_STATE_CHANGE_STATUS_0TO7 and ACP_SW_STATE_CHANGE_STATUS_8TO11
immediately fire an interrupt and schedule amd_sdw_irq_thread() and
amd_sdw_work while the link clock is stopped, causing MIPI command timeouts
in amd_sdw_update_slave_status_work()?

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