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

[Mukunda,Vijendar]

On 9/14/26 01:35, Pierre-Louis Bossart wrote:
> 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?
Thanks for the comments. I agree that it is preferable to keep each patch
focused, but in this case both changes are part of the same remove-path
cleanup bug and are tightly coupled.

I do not think moving pm_runtime_enable() to probe() is the right fix
here. In this driver, runtime PM is intentionally enabled only at the end
of amd_sdw_manager_start(), after the fallible hardware bring-up sequence.
If one instance succeeds and the next fails before pm_runtime_enable(), the
remove path can still run for the partially started instance. In that
scenario, an unconditional pm_runtime_disable() is incorrect because there
was no matching enable for that instance.

This driver is multi-instance: the controller loops over each link and
starts them independently. The PM state and workqueue state are per
instance, so the guard in remove() is needed to avoid an unbalanced PM
state from the partial-start failure path. Moving the enable into probe()
would also change the device lifecycle semantics by enabling PM before the
hardware bring-up sequence completes, which is broader than the actual bug
being fixed.

The work-drain ordering change is part of the same teardown correctness
issue: amd_sdw_irq_thread() can requeue amd_sdw_work, so the interrupt
source must be masked before draining both work items. That is a remove-
path issue and not something that belongs in probe().

I kept the patch scoped to the actual cleanup bug instead of restructuring
the startup lifecycle. If needed, I can split the PM guard and the work-
drain ordering into separate patches, but they are both required for the
same failure mode.


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