Re: [PATCH v4 05/13] soundwire: amd: fix work drain ordering in remove path

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

On 10/26/26 14:57, sashiko-bot@kernel.org wrote:
> [You don't often get email from sashiko-bot@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] soundwire: amd: race with in-flight IRQ handler in amd_sdw_manager_remove()
> --
>
> commit 25a684110adde4056bf6250565965402e67408ca
> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>
> soundwire: amd: fix work drain ordering in remove path
>
> This commit reorders work cancellation in amd_sdw_manager_remove() so that
> interrupts are disabled before calling cancel_work_sync() on
> amd_sdw_irq_thread and amd_sdw_work.
>
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index 5e9d228d09d06..a82714cc2f6c4 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -1169,8 +1169,9 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
>>        int ret;
>>
>>        pm_runtime_disable(&pdev->dev);
>> -     cancel_work_sync(&amd_manager->amd_sdw_work);
>>        amd_disable_sdw_interrupts(amd_manager);
>> +     cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
>> +     cancel_work_sync(&amd_manager->amd_sdw_work);
> [Severity: High]
> Can an in-flight parent ACP IRQ handler still schedule amd_sdw_irq_thread
> after cancel_work_sync() returns, causing a use-after-free of amd_manager
> and its MMIO regions?
++

The ACP IP has a dedicated interrupt line and does not share it
with any other IP. Once amd_disable_sdw_interrupts() clears the
enable bit in ACP_EXTERNAL_INTR_CNTL, the hardware cannot assert
the interrupt line again for SoundWire events.

However, a narrow race exists where an IRQ handler already
executing on another CPU has obtained amd_manager via
dev_get_drvdata() but has not yet called schedule_work() when
both cancel_work_sync() calls complete on the remove path.

A follow-up fix will introduce a software irq_enabled flag in
amd_sdw_manager, cleared in amd_disable_sdw_interrupts() before
writing to ACP_EXTERNAL_INTR_CNTL and restored in
amd_enable_sdw_interrupts(). The IRQ handler will check this
flag before calling schedule_work():

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

In the remove path, amd_enable_sdw_interrupts() is never called
after amd_disable_sdw_interrupts(), so irq_enabled stays false
and the race is closed.
>
> The commit message states:
>
>    An IRQ handler already in flight may still queue amd_sdw_irq_thread, so
>    cancel_work_sync() is used to drain both amd_sdw_irq_thread and any
>    amd_sdw_work items it may have scheduled. This fully quiesces the driver
>    workqueues, making synchronize_irq() unnecessary.
>
> However, cancel_work_sync() only cancels a work item that is already
> pending or waits for one that is currently executing at the moment
> cancel_work_sync() checks it, and re-enables the work item before
> returning. Unlike disable_work_sync(), it does not block future
> schedule_work() calls, nor does it wait for an in-flight hardirq handler on
> another CPU that has not yet called schedule_work().
This is acknowledged. The irq_enabled software flag introduced
by the follow-up fix addresses this. Clearing the flag in
amd_disable_sdw_interrupts() before cancel_work_sync() runs
ensures that an IRQ handler already executing on another CPU
reads irq_enabled == false and skips schedule_work(), preventing
any re-queue after the drain completes.

The wake path, which is handled inside amd_sdw_irq_thread()
via amd_sdw_process_wake_event() -> pm_request_resume(), is
not affected. In the CLK_STOP_MODE suspend path,
amd_enable_sdw_interrupts() is called after cancel_work_sync()
to restore irq_enabled = true, allowing wake interrupts to
re-schedule amd_sdw_irq_thread for the resume sequence. In
the remove path, amd_enable_sdw_interrupts() is never called,
so irq_enabled remains false permanently.

> In addition, amd_disable_sdw_interrupts() only clears the enable mask in
> ACP_EXTERNAL_INTR_CNTL via a posted write without clearing already-latched
> status bits in ACP_EXTERNAL_INTR_STAT:
The IRQ handler clears ACP_EXTERNAL_INTR_STAT before calling
schedule_work():

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

Any latched status bit is therefore cleared by the IRQ handler
itself before work is queued. Since the ACP IP has a dedicated
interrupt line, once the source is masked no new interrupt can
fire to re-set those bits. The irq_enabled flag in the follow-up
fix closes the remaining narrow window for any handler already
in flight at the moment of masking.
> drivers/soundwire/amd_manager.c:amd_disable_sdw_interrupts() {
>          mutex_lock(amd_manager->acp_sdw_lock);
>          irq_mask = sdw_manager_reg_mask_array[amd_manager->instance];
>          amd_updatel(amd_manager->acp_mmio, ACP_EXTERNAL_INTR_CNTL(amd_manager->instance),
>                      irq_mask, 0);
>          mutex_unlock(amd_manager->acp_sdw_lock);
> ...
> }
>
> Meanwhile, the parent IRQ handlers such as acp63_irq_handler() in
> sound/soc/amd/ps/pci-ps.c and acp_irq_handler() in sound/soc/sof/amd/acp.c
> check ACP_EXTERNAL_INTR_STAT and call schedule_work():
>
> 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 acp63_irq_handler() or acp_irq_handler() is executing on CPU 1 during
> device unbind, module unload, or probe error unwind, and has read
> ACP_EXTERNAL_INTR_STAT and dev_get_drvdata() just before CPU 0 runs
> amd_disable_sdw_interrupts() and both cancel_work_sync() calls, both
> cancel_work_sync() calls will observe idle work items and return
> immediately.
>
> CPU 1 can then call schedule_work(&amd_manager->amd_sdw_irq_thread),
> queuing amd_sdw_irq_thread (and subsequently amd_sdw_work) to run
> concurrently with sdw_bus_master_delete(), amd_disable_sdw_manager(), and
> the devres release of amd_manager and its ioremapped mmio/acp_mmio regions.
>
> Should this use disable_work_sync() instead of cancel_work_sync() (or
> synchronize with the parent IRQ after masking and clearing pending status)
> so that an in-flight IRQ handler cannot re-queue the work after the drain
> completes?

Neither disable_work_sync() nor synchronize_irq() is needed.
A follow-up fix will introduce the irq_enabled software flag in
amd_sdw_manager, tied to amd_enable_sdw_interrupts() and
amd_disable_sdw_interrupts(). Clearing the flag before
cancel_work_sync() in the remove path ensures that any IRQ
handler already executing on another CPU skips schedule_work()
when it checks irq_enabled. This closes the race without
requiring disable_work_sync() or synchronize_irq().

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