Re: [PATCH 3/8] soundwire: amd: fix work drain ordering and pm_runtime guard in remove path

[Pierre-Louis Bossart]

On 9/10/26 21:00, Vijendar Mukunda wrote:
> amd_sdw_manager_remove() cancelled amd_sdw_work but not
> amd_sdw_irq_thread. Since amd_sdw_irq_thread() calls
> schedule_work(&amd_sdw_work), an in-flight irq_thread item can
> re-queue amd_sdw_work after its cancel returns, defeating the
> cancellation.
> 
> Fix by calling amd_disable_sdw_interrupts() first to quiesce the
> hardware IRQ source, then cancel_work_sync() for amd_sdw_irq_thread,
> then cancel_work_sync() for amd_sdw_work. The existing
> cancel_work_sync(amd_sdw_work) is also moved to after
> amd_disable_sdw_interrupts() so that any work item queued between the
> old cancel position and the interrupt disable cannot escape draining.
> 
> synchronize_irq() is deliberately not used before the
> cancel_work_sync() calls. Once SoundWire interrupts are masked, no new
> IRQ deliveries can occur. 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.
> 
> Also guard pm_runtime_disable() so it is only called when runtime PM
> was actually enabled. amd_sdw_manager_start() calls pm_runtime_enable()
> only at the very end, after several fallible hardware init steps. If
> sdw_amd_startup() fails mid-loop (one manager started, the next fails
> before pm_runtime_enable()), sdw_amd_exit() triggers
> platform_device_unregister() for all managers. Calling
> pm_runtime_disable() on the partially-started manager finds
> disable_depth already at its initial value of 1, silently increments it
> to 2 and returns without a warning, so a later pm_runtime_enable() would
> only bring it back to 1 and leave runtime PM disabled. Use
> pm_runtime_enabled() to skip the call when it was never paired with an
> enable.

The alternative is to do a pm_runtime_enable() in the probe(), and later
a pm_runtime_set_active().

That way if the probe is successful, then the remove() will always deal
a balanced enable.

Maybe only put a single 'fix' per patch?
> Fixes: f93b697ed98e ("soundwire: amd: cancel pending slave status handling workqueue during remove sequence")
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
> ---
>  drivers/soundwire/amd_manager.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index a57b59609bfe..bbe1e73ed255 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -1172,9 +1172,11 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
>  	struct amd_sdw_manager *amd_manager = dev_get_drvdata(&pdev->dev);
>  	int ret;
>  
> -	pm_runtime_disable(&pdev->dev);
> -	cancel_work_sync(&amd_manager->amd_sdw_work);
> +	if (pm_runtime_enabled(&pdev->dev))
> +		pm_runtime_disable(&pdev->dev);
>  	amd_disable_sdw_interrupts(amd_manager);
> +	cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
> +	cancel_work_sync(&amd_manager->amd_sdw_work);
>  	sdw_bus_master_delete(&amd_manager->bus);
>  	ret = amd_disable_sdw_manager(amd_manager);
>  	if (ret)