Re: [PATCH] ASoC: codecs: madera: release runtime PM ref on FLL config failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ASoC: madera: broken error handling and return value in madera_fllhj_enable()
--

commit 0527a87963a29c49669e1565c1faf7d3cc1ad756
Author: Wentao Liang <vulab@iscas.ac.cn>

ASoC: codecs: madera: release runtime PM ref on FLL config failure

This commit releases the runtime PM reference and skips waiting for FLL lock
when madera_fllhj_apply() fails in madera_fllhj_enable() while the FLL was
previously disabled.

> diff --git a/sound/soc/codecs/madera.c b/sound/soc/codecs/madera.c
> index 58255844b4df3..2f6256f8e87b2 100644
> --- a/sound/soc/codecs/madera.c
> +++ b/sound/soc/codecs/madera.c
> @@ -4608,8 +4608,17 @@ static int madera_fllhj_enable(struct madera_fll *fll)
>  			   MADERA_FLL1_HOLD_MASK,
>  			   0);
>  
> -	if (!already_enabled)
> -		madera_wait_for_fll(fll, true);
> +	/*
> +	 * If configuring the FLL failed it is left disabled, so a later
> +	 * madera_fllhj_disable() will not see the ENA bit set and will not
> +	 * drop the runtime PM reference taken above.
> +	 */
> +	if (!already_enabled) {
> +		if (ret)
> +			pm_runtime_put_autosuspend(madera->dev);
> +		else
> +			madera_wait_for_fll(fll, true);
> +	}
>  
>  	return 0;
>  }

[Severity: High]
When madera_fllhj_apply() fails in sound/soc/codecs/madera.c, execution
jumps to the out label before calling pm_runtime_put_autosuspend() and
returning:

sound/soc/codecs/madera.c:madera_fllhj_enable() {
	...
	ret = madera_fllhj_apply(fll, fll->ref_freq);
	if (ret) {
		madera_fll_err(fll, "Failed to set FLL: %d\n", ret);
		goto out;
	}
	...
out:
	regmap_update_bits(madera->regmap,
			   fll->base + MADERA_FLL_CONTROL_11_OFFS,
			   MADERA_FLL1_LOCKDET_MASK,
			   MADERA_FLL1_LOCKDET_MASK);

	regmap_update_bits(madera->regmap,
			   fll->base + MADERA_FLL_CONTROL_2_OFFS,
			   MADERA_FLL1_CTRL_UPD_MASK,
			   MADERA_FLL1_CTRL_UPD_MASK);

	/* Release the hold so that flln locks to external frequency */
	regmap_update_bits(madera->regmap,
			   fll->base + MADERA_FLL_CONTROL_1_OFFS,
			   MADERA_FLL1_HOLD_MASK,
			   0);
}

As documented in madera_fllhj_disable(), a disabled FLL requires
MADERA_FLL1_LOCKDET_MASK = 0, MADERA_FLL1_HOLD_MASK = 1, and
MADERA_FLL1_CTRL_UPD_MASK = 0 across runtime suspend and resume so that
regcache_sync() does not latch CTRL_UPD before writing out all FLL
registers:

sound/soc/codecs/madera.c:madera_fllhj_disable() {
	...
	/* ctrl_up gates the writes to all the fll's registers, setting it to 0
	 * here ensures that after a runtime suspend/resume cycle when one
	 * enables the fll then ctrl_up is the last bit that is configured
	 * by the fll enable code rather than the cache sync operation which
	 * would have updated it much earlier before writing out all fll
	 * registers
	 */
	regmap_update_bits(madera->regmap,
			   fll->base + MADERA_FLL_CONTROL_2_OFFS,
			   MADERA_FLL1_CTRL_UPD_MASK, 0);
}

Does dropping the runtime PM reference at the end of madera_fllhj_enable()
allow the device to runtime-suspend while MADERA_FLL1_CTRL_UPD_MASK = 1,
MADERA_FLL1_LOCKDET_MASK = 1, and MADERA_FLL1_HOLD_MASK = 0 remain set in
the regcache for a disabled FLL?

Additionally, madera_fllhj_enable() still returns 0 instead of ret at the
end of the function. Because cs47l92_set_fll() -> madera_fllhj_set_refclk()
stores fll->fout before calling madera_fllhj_enable():

sound/soc/codecs/madera.c:madera_fllhj_set_refclk() {
	...
	fll->ref_src = source;
	fll->ref_freq = fin;
	fll->fout = fout;

	if (fout)
		ret = madera_fllhj_enable(fll);
	else
		madera_fllhj_disable(fll);

	return ret;
}

shouldn't madera_fllhj_enable() avoid setting LOCKDET/CTRL_UPD or clearing
HOLD on error and return ret so the caller does not see success with
fll->fout left non-zero while the FLL hardware is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104022814.1484284-1-vulab@iscas.ac.cn?part=1