Re: [PATCH 3/3] ASoC: hdac_hda: Balance the runtime PM disable in codec remove

["Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>]

On 27/10/2026 11:12, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] ASoC: hdac_hda: codec stays powered in D0 on re-probe due to RPM_SUSPENDED state
> --
> 
> commit fc19b5dab0635a3fee4ca467d334fb76459c8230
> Author: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> 
> ASoC: hdac_hda: Balance the runtime PM disable in codec remove
> 
> This patch re-enables runtime PM at the end of hdac_hda_codec_remove() to
> balance its earlier pm_runtime_disable() call so that runtime PM does not
> remain disabled after snd_hda_codec_register() runs on a subsequent probe.
> 
>> diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
>> index cae5711e979c3..bd4b113401a8d 100644
>> --- a/sound/soc/codecs/hdac_hda.c
>> +++ b/sound/soc/codecs/hdac_hda.c
>> @@ -554,6 +554,7 @@ static void hdac_hda_codec_remove(struct snd_soc_component *component)
>>  		return;
>>  	}
>>  
>> +	/* prevent runtime PM callbacks while the link and the codec are torn down */
>>  	pm_runtime_disable(&hdev->dev);
>>  	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
>>  
>> @@ -561,6 +562,9 @@ static void hdac_hda_codec_remove(struct snd_soc_component *component)
>>  		driver->ops->remove(codec);
>>  
>>  	snd_hda_codec_cleanup_for_unbind(codec);
>> +
>> +	/* Balances the disable above, cleanup_for_unbind keeps runtime PM disabled */
>> +	pm_runtime_enable(&hdev->dev);
>>  }
> 
> [Severity: Medium]
> Does the codec hardware still stay powered in D0 after an unbind and re-bind
> cycle even with this balanced pm_runtime_enable() call?
> 
> On initial device creation, snd_hdac_device_init() sets runtime_status to
> RPM_ACTIVE while runtime PM is disabled (disable_depth == 1). However, when
> hdac_hda_codec_remove() finishes, runtime_status is left as RPM_SUSPENDED:
> either the codec was already runtime-suspended before an ASoC card unbind,
> or on a codec driver unbind __device_release_driver() calls
> device_unbind_cleanup() -> pm_runtime_reinit(), which sees
> !pm_runtime_enabled(dev) and calls pm_runtime_set_suspended(dev):
> 
> drivers/base/power/runtime.c:pm_runtime_reinit() {
> 	if (pm_runtime_enabled(dev))
> 		return;
> 
> 	if (dev->power.runtime_status == RPM_ACTIVE)
> 		pm_runtime_set_suspended(dev);
>     ...
> }
> 
> When the codec is probed again, hdac_hda_codec_probe() enters with
> disable_depth == 1, usage_count == 1, and runtime_status == RPM_SUSPENDED.
> It acquires the HDA link via snd_hdac_ext_bus_link_get() and calls
> snd_hda_codec_device_new(), which powers the hardware up to AC_PWRST_D0 and
> calls pm_runtime_forbid(). Because runtime PM is still disabled at that
> point, rpm_resume() returns -EACCES and leaves runtime_status as
> RPM_SUSPENDED.
> 
> At the end of hdac_hda_codec_probe(), it assumes runtime_status is still
> RPM_ACTIVE from initial device creation and never calls
> pm_runtime_set_active(&hdev->dev):
> 
> sound/soc/codecs/hdac_hda.c:hdac_hda_codec_probe() {
>     ...
> 	/*
> 	 * hdac_device core already sets the state to active and calls
> 	 * get_noresume. So enable runtime and set the device to suspend.
> 	 * pm_runtime_enable is also called during codec registration
> 	 */
> 	pm_runtime_put(&hdev->dev);
> 	pm_runtime_suspend(&hdev->dev);
> }
> 
> When snd_hda_codec_register() subsequently runs, it enables runtime PM and
> calls snd_hda_power_down():
> 
> sound/hda/common/codec.c:snd_hda_codec_register() {
>     ...
> 	if (device_is_registered(hda_codec_dev(codec))) {
> 		snd_hda_codec_display_power(codec, true);
> 		pm_runtime_enable(hda_codec_dev(codec));
> 		/* it was powered up in snd_hda_codec_new(), now all done */
> 		snd_hda_power_down(codec);
>     ...
> }
> 
> Because dev->power.runtime_status is already RPM_SUSPENDED, rpm_suspend()
> inside snd_hda_power_down() returns 1 immediately without invoking
> hda_codec_runtime_suspend() or scheduling autosuspend. Doesn't this leave the
> codec hardware powered in AC_PWRST_D0, the HDA link reference from
> hdac_hda_codec_probe() held, and the display power reference from
> snd_hda_codec_register() held, unless pm_runtime_set_active(&hdev->dev) is
> also called in hdac_hda_codec_probe() (similar to hda_codec_probe_complete()
> in sound/soc/codecs/hda.c)?

Yes, this is valid, I will add the pm_runtime_set_active() call.
Now tested on device and it is reflecting correct state across the
expanded test matrix.

> 

-- 
Péter