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

[sashiko-bot@kernel.org]

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)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027083840.28813-1-peter.ujfalusi@linux.intel.com?part=3