Re: [PATCH v5 22/25] 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
issues to consider:
- [Medium] soundwire: amd: unserialized hard IRQ race with work drain in
  amd_suspend()
- [Low] soundwire: amd: commit message describes prior patch revision in
  PM ops
--

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

> soundwire: amd: fix interrupt gate and work drain ordering in PM ops
> 
> The CLK_STOP_MODE branches in amd_suspend() and amd_suspend_runtime()
> did not close the ACP_EXTERNAL_INTR_CNTL delivery gate before draining
> amd_sdw_irq_thread and amd_sdw_work. An ISR already running when the
> drains completed could re-queue work after cancel_work_sync() returned,
> racing amd_sdw_clock_stop().
> 
> In both CLK_STOP_MODE paths, amd_sdw_wake_enable() was also called
> before amd_enable_sdw_interrupts(). Since amd_enable_sdw_interrupts()
> writes the interrupt mask unconditionally, it clobbered the wake bit
> set by amd_sdw_wake_enable().
> 
> For both amd_suspend() and amd_suspend_runtime():
>  - CLK_STOP_MODE: add amd_disable_sdw_interrupts() and
>    cancel_work_sync(irq_thread) before the work drain; reopen the gate
>    with amd_enable_sdw_interrupts() after clock stop, then call
>    amd_sdw_wake_enable() so the wake bit is not clobbered.
>  - POWER_OFF_MODE: add amd_disable_sdw_interrupts() and
>    cancel_work_sync(irq_thread) before the work drain; remove the
>    redundant post-clock-stop amd_disable_sdw_interrupts() call.
> 
> Fixes: 86a4371b7697 ("soundwire: amd: fix for handling slave alerts after link is down")
> Reported-by: Lee Jones <lee@kernel.org>
> Closes: https://sashiko.dev/#/bug/linux-3b774ace-00cf-41e6-82eb-85cd43f5e1d6
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

[Severity: Low]
Does the commit message describe an intermediate revision of this series
rather than the parent commit?

In the parent commit, amd_suspend_runtime() did not drain
amd_sdw_irq_thread or amd_sdw_work at all (this patch adds both
cancel_work_sync() calls there), and neither CLK_STOP_MODE path called
amd_enable_sdw_interrupts() before this patch.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index 2b38b9bf97f50..4e151f7350c96 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -1437,15 +1437,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);
> -		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: Medium]
Can an in-flight parent ACP hard IRQ handler still queue
amd_sdw_irq_thread after cancel_work_sync() returns in amd_suspend() and
amd_suspend_runtime()?

None of the parent ACP hard IRQ handlers (such as acp63_irq_handler() in
sound/soc/amd/ps/pci-ps.c) check amd_manager->interrupt_enabled or acquire
acp_sdw_lock before calling schedule_work(&amd_manager->amd_sdw_irq_thread):

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);
    ...
}

If the hard IRQ fires just before amd_disable_sdw_interrupts() masks
ACP_EXTERNAL_INTR_CNTL, the two contexts can interleave as follows:

CPU0                                  CPU1 (<hardirq>)
amd_suspend()
                                      acp63_irq_handler()
                                        readl(... + ACP_EXTERNAL_INTR_STAT)
  amd_disable_sdw_interrupts()
    interrupt_enabled = false
    amd_updatel(ACP_EXTERNAL_INTR_CNTL, ...)
  cancel_work_sync(&amd_sdw_irq_thread)
  cancel_work_sync(&amd_sdw_work)
  amd_sdw_clock_stop()
  amd_enable_sdw_interrupts()
    interrupt_enabled = true
                                        schedule_work(&amd_sdw_irq_thread)
                                      amd_sdw_irq_thread()
                                        schedule_work(&amd_sdw_work)
                                          <-- runs while clock is stopped

Even in AMD_SDW_POWER_OFF_MODE where interrupt_enabled remains false,
amd_sdw_irq_thread() still reads SoundWire MMIO registers and calls
amd_sdw_read_and_process_ping_status() without checking interrupt_enabled,
racing with amd_sdw_clock_stop(), amd_disable_sdw_manager(), and D3 entry.
In AMD_SDW_CLK_STOP_MODE, calling amd_enable_sdw_interrupts() right after
amd_sdw_clock_stop() also sets interrupt_enabled back to true while the
clock is stopped, allowing a late amd_sdw_irq_thread() to schedule
amd_sdw_work and trigger MIPI command timeouts (-ETIMEDOUT).

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