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

["Mukunda,Vijendar" <vijendar.mukunda@amd.com>]

On 10/26/26 15:57, sashiko-bot@kernel.org wrote:
> 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)?
++

This is a valid concern. A follow-up fix will introduce a
software irq_enabled flag in amd_sdw_manager. The flag will be
cleared in amd_disable_sdw_interrupts() before writing to
ACP_EXTERNAL_INTR_CNTL, and checked in acp63_irq_handler()
before calling schedule_work():

     if (amd_manager && amd_manager->irq_enabled)
         schedule_work(&amd_manager->amd_sdw_irq_thread);

Clearing the flag before cancel_work_sync() ensures that any
IRQ handler already executing on another CPU reads
irq_enabled == false and skips schedule_work(), closing the
race window without requiring synchronize_irq().

The wake path is handled correctly. In the CLK_STOP_MODE
suspend path, amd_enable_sdw_interrupts() is called after
amd_sdw_clock_stop(), which restores irq_enabled = true. Wake
interrupts then schedule amd_sdw_irq_thread which calls
amd_sdw_process_wake_event() -> pm_request_resume() for the
resume sequence. In the remove path, amd_enable_sdw_interrupts()
is never called, so irq_enabled remains false permanently.
>
> 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()?
ACP_SW_STATE_CHANGE_STATUS_0TO7 and
ACP_SW_STATE_CHANGE_STATUS_8TO11 are internal SoundWire status
registers polled directly by amd_sdw_irq_thread(). Stale bits
in these registers do not set ACP_EXTERNAL_INTR_STAT or raise
a CPU interrupt when the masks are re-enabled.

Once the bus clock is stopped, no slave status interrupts can
be asserted — only wake interrupts remain active, which are
handled separately via amd_sdw_wake_enable(). Calling
amd_enable_sdw_interrupts() after amd_sdw_clock_stop() is
therefore safe — no spurious interrupt delivery or MIPI command
timeouts can result from stale status bits.
>