Re: [PATCH 18/21] soundwire: amd: Synchronise parent IRQ and disable work on manager removal

[Mukunda,Vijendar]

On 10/30/26 16:16, Lee Jones wrote:
> [You don't often get email from lee@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> In the ACP top-half interrupt handlers, amd_manager is retrieved via
> dev_get_drvdata() and used to schedule amd_sdw_irq_thread when
> non-NULL.  If an interrupt is already in flight on another CPU when
> amd_sdw_manager_remove() runs, the handler can call
> schedule_work(&amd_manager->amd_sdw_irq_thread) after cancel_work_sync()
> returns and after devm frees amd_manager.
>
> Disable SoundWire interrupts, synchronise with the parent PCI device
> IRQ, disable both manager work items with disable_work_sync(), and mask
> interrupts again in case amd_sdw_update_slave_status_work() was running.
>
> Fixes: f93b697ed98e ("soundwire: amd: cancel pending slave status handling workqueue during remove sequence")
> Closes: https://sashiko.dev/#/bug/linux-ab458b2e-f505-4a41-8238-a93ceb5d9c77
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Lee Jones <lee@xxxxxxxxxx>
> ---
>   drivers/soundwire/amd_manager.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index 1d8a08bda261..f39f4f04db7e 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -8,10 +8,12 @@
>   #include <linux/completion.h>
>   #include <linux/cleanup.h>
>   #include <linux/device.h>
> +#include <linux/interrupt.h>
>   #include <linux/io.h>
>   #include <linux/jiffies.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> +#include <linux/pci.h>
>   #include <linux/slab.h>
>   #include <linux/soundwire/sdw.h>
>   #include <linux/soundwire/sdw_registers.h>
> @@ -1199,8 +1201,11 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
>          pm_runtime_get_sync(&pdev->dev);
>          pm_runtime_disable(&pdev->dev);
>          pm_runtime_put_noidle(&pdev->dev);
> -       cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
> -       cancel_work_sync(&amd_manager->amd_sdw_work);
> +       amd_disable_sdw_interrupts(amd_manager);
> +       if (pdev->dev.parent && dev_is_pci(pdev->dev.parent))
> +               synchronize_irq(to_pci_dev(pdev->dev.parent)->irq);
> +       disable_work_sync(&amd_manager->amd_sdw_irq_thread);
> +       disable_work_sync(&amd_manager->amd_sdw_work);
++

The synchronize_irq() approach is incorrect for this hardware. The
ACP IP uses a dedicated line-based interrupt. Using
to_pci_dev(pdev->dev.parent)->irq gives the parent PCI device's
primary IRQ, which is not the SoundWire interrupt line. Synchronizing
the wrong IRQ does not ensure the ACP SoundWire top-half handler has
completed on another CPU.

Additionally, disable_work_sync() is not available in older stable
kernels, making the Cc: stable tag unworkable without further
backport effort.

The double call to amd_disable_sdw_interrupts() is also unjustified —
the work handler does not re-enable interrupts.

We have addressed this race in our upcoming v4 patch series using an
interrupt_enabled flag checked in the top-half before scheduling
work. This is the standard pattern for this type of race and has been
validated on the hardware. Please drop this patch.
>          amd_disable_sdw_interrupts(amd_manager);
>          sdw_bus_master_delete(&amd_manager->bus);
>          ret = amd_disable_sdw_manager(amd_manager);
> --
> 2.56.0.317.g454f8302e1-goog