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

[Pierre-Louis Bossart]

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

Sounds good, thanks for sharing the details.
My suggestion was based on what we did for SoundWire codecs, it may not
be 100% applicable on the host side - or it would create new problems.