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

[Pierre-Louis Bossart]

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.