Re: [PATCH v3 2/7] ASoC: codec: pcm1681: Add optional SCK clock and runtime PM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] snd: soc: pcm1681: Devres teardown ordering bug leaves component accessible with gated clock
--

commit 0a50ee85ef797e0816bf4c1d1aa2f3b04e25ef3c
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

ASoC: codec: pcm1681: Add optional SCK clock and runtime PM support

This commit adds support for an optional sck clock and runtime PM to the
PCM1681 codec driver. It enables the clock before accessing the device, waits
for a reset sequence, and uses runtime PM autosuspend to manage the clock state
alongside regmap caching.

> diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c
> index 60fdbe5c4e054..a1ccf1edf4819 100644
> --- a/sound/soc/codecs/pcm1681.c
> +++ b/sound/soc/codecs/pcm1681.c
[ ... ]
>  static int pcm1681_i2c_probe(struct i2c_client *client)
>  {
[ ... ]
> +	ret = devm_snd_soc_register_component(dev,
> +					      &soc_component_dev_pcm1681,
> +					      &pcm1681_dai, 1);
> +	if (ret) {
> +		dev_err(dev, "Failed to register component: %d\n", ret);
> +		goto err_pm;
> +	}
> +
> +	return 0;
> +
> +err_pm:
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
> +	if (!pm_runtime_status_suspended(dev))
> +		clk_disable_unprepare(priv->sck);
> +	pm_runtime_set_suspended(dev);
> +
> +	return ret;
>  }
>  
> +static void pcm1681_i2c_remove(struct i2c_client *client)
> +{
> +	struct pcm1681_private *priv = i2c_get_clientdata(client);
> +	struct device *dev = &client->dev;
> +
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
> +	/* Runtime PM may already have gated the clock */
> +	if (!pm_runtime_status_suspended(dev))
> +		clk_disable_unprepare(priv->sck);

[Severity: High]
Does this devres teardown ordering leave the component accessible while its
clock is gated?

In pcm1681_i2c_probe() in sound/soc/codecs/pcm1681.c, the component is
registered via devm_snd_soc_register_component(), which defers unregistration
until after pcm1681_i2c_remove() completes. However, pcm1681_i2c_remove()
manually disables runtime PM and gates the sck clock.

When the ASoC core later unregisters the component during the devres cleanup
phase, it may attempt to power down DAPM widgets or mute the codec, resulting
in physical regmap_write() calls. Since the clock is already gated and the
regmap cache is not marked as cache-only here, this regression could cause
these transactions to fail, potentially leading to I2C timeouts, bus hangs, or
kernel warnings.

Should devm_pm_runtime_enable() or a custom devm action be used for PM and
clock cleanup to ensure proper ordering, or should the component registration be
changed to the non-devres variant?

> +	pm_runtime_set_suspended(dev);
> +}

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