Re: [PATCH 3/8] soundwire: amd: fix work drain ordering and pm_runtime guard in remove path
From: Mukunda,Vijendar
Date: Tue Sep 15 2026 - 00:44:26 EST
On 9/14/26 23:00, Pierre-Louis Bossart wrote:
On 9/14/26 08:07, Mukunda,Vijendar wrote:Thanks for the suggestion Pierre. I considered moving pm_runtime_enable()
My take on error handling is to avoid partially functional setups. Keep
On 9/14/26 01:35, Pierre-Louis Bossart wrote:
On 9/10/26 21:00, Vijendar Mukunda wrote:Thanks for the comments. I agree that it is preferable to keep each patch
amd_sdw_manager_remove() cancelled amd_sdw_work but notThe alternative is to do a pm_runtime_enable() in the probe(), and later
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.
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?
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.
things simple, fail big and fail early. Well-intended concealment
schemes will introduce more problems, e.g. if the link for the right amp
fails the left one might work, but users will complain about left-only
sounds...
That said, I am not going to lay on the tracks if this is the design you
want for your IP.
into probe(), but I do not think that is safe given how the AMD SoundWire
driver is structured.
The current placement in amd_sdw_manager_start() follows the same model
used by the Intel SoundWire driver, where runtime PM is enabled only
after the hardware has been powered up and fully initialized.
More importantly, pm_runtime_set_active() requires the hardware to be in
a known operational state. For AMD, that is only true after
acp_sdw_clk_init_ctrl(), acp_init_sdw_manager(),
acp_enable_sdw_interrupts(), acp_enable_sdw_manager(), and
acp_sdw_set_frameshape() have all completed successfully. Calling
pm_runtime_set_active() from probe() would advertise the device as
active before any of this initialization has occurred.
Enabling runtime PM in probe() would also create a race window between
probe() and sdw_amd_startup(). During that window, the PM core could
invoke the runtime suspend callback, which accesses SoundWire manager
registers and performs clock-stop sequences. Since the hardware has not
yet been initialized, those register accesses would occur on an
uninitialized manager.
The failure path that motivated this change is also a real scenario.
sdw_amd_startup() iterates over all manager instances. If one instance
successfully completes startup and another fails later, the cleanup path
must handle a mix of initialized and non-initialized managers. The
pm_runtime_enabled() check added here ensures that
pm_runtime_disable() is only called for instances that actually reached
the point where runtime PM was enabled.
The probe/startup split is intentional and follows the existing
SoundWire subsystem design. Hardware bring-up is deferred until startup,
and runtime PM is enabled only after the manager is known to be fully
operational. Since the runtime PM callbacks directly access hardware
registers, allowing them to run before startup completes would be
unsafe.
Finally, moving pm_runtime_enable() into probe() would separate it from
pm_runtime_set_active(). The current ordering of
pm_runtime_set_active() followed by pm_runtime_enable() is the standard
runtime PM pattern and avoids additional synchronization requirements.
This design is not new. The placement of pm_runtime_enable() inside
amd_sdw_manager_start() was introduced by commit 81ff58ff71ad
("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")
and has been part of the upstream kernel since v6.4.
In summary, moving pm_runtime_enable() to probe() would expose runtime
PM callbacks before the SoundWire manager is initialized, creating a
real race between probe() and startup. Keeping it in
amd_sdw_manager_start() satisfies the requirements of
pm_runtime_set_active() and makes the pm_runtime_enabled() guard in the
remove path both correct and necessary.
We will split the patch and push the pm_runtime guard change separately.