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

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

On 9/14/26 23:00, Pierre-Louis Bossart wrote:
> On 9/14/26 08:07, Mukunda,Vijendar wrote:
>>
>> 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.
> My take on error handling is to avoid partially functional setups. Keep
> 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.
Thanks for the suggestion Pierre.  I considered moving pm_runtime_enable()
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.