Re: [PATCH v3 5/7] ASoC: codec: pcm1681: Enable the PCM1681 supplies

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] asoc: devres ordering bug leaves component active after remove disables clocks
--

commit 7a5eea3c05ab4c0d2ec17ff354d7cd0e31ecd47b
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>
Subject: ASoC: codec: pcm1681: Enable the PCM1681 supplies

This commit adds regulator support for the PCM1681 DAC's analog and digital
supplies. It uses devm_regulator_bulk_get_enable() during probe to ensure
power is enabled before touching clocks or regmap.

> diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c
> index 3675b2236d663..6002da3a56669 100644
> --- a/sound/soc/codecs/pcm1681.c
> +++ b/sound/soc/codecs/pcm1681.c
[ ... ]
> @@ -347,6 +352,13 @@ static int pcm1681_i2c_probe(struct i2c_client *client)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	ret = devm_regulator_bulk_get_enable(dev,
> +					     ARRAY_SIZE(pcm1681_supply_names),
> +					     pcm1681_supply_names);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable supplies\n");
> +

[Severity: High]
Does adding this regulator to the devres stack interact poorly with the device
teardown sequence?

The probe function registers the component using
devm_snd_soc_register_component(), meaning it will be unregistered
automatically by devres after the driver's remove() callback completes.

However, in pcm1681_i2c_remove(), the driver explicitly tears down runtime
PM and forcefully disables the hardware clock before devres gets a chance
to unregister the component:

sound/soc/codecs/pcm1681.c:pcm1681_i2c_remove() {
	...
	pm_runtime_disable(dev);
	/* Runtime PM may already have gated the clock */
	if (!pm_runtime_status_suspended(dev))
		clk_disable_unprepare(priv->sck);
	...
}

This creates a window where the ASoC component is still fully registered and
active in ALSA, but its hardware clock is disabled and runtime PM is
permanently deactivated.

If ALSA or userspace accesses the component (e.g., reading or writing mixer
controls like pcm1681_put_deemph) during this window, the component will
attempt I2C register accesses without the required system clock or runtime PM
support. This can lead to kernel crashes, I2C bus hangs, or silent register
corruption.

While the current patch adds a devm_regulator action to the devres stack,
modifying the overall teardown sequence, this ordering window leaves the
component vulnerable during remove. Should the driver avoid devres for
component registration or reorder the manual cleanup to prevent this?

>  	priv->sck = devm_clk_get_optional(dev, "sck");
>  	if (IS_ERR(priv->sck))
>  		return dev_err_probe(dev, PTR_ERR(priv->sck),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-nord-asoc-driver-support-v3-v3-0-0c1897f21ccf@oss.qualcomm.com?part=5