Re: [PATCH v2 5/7] ASoC: codecs: ES8389: Modify the initial configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Regulators enabled in component probe are never disabled, leading to a refcount leak on module removal.
- [High] Unbalanced clock prepare/enable calls in bias level handling cause clock refcount underflows.
- [High] The return value of `regmap_read` is ignored, leading to the use of uninitialized stack variables if the I2C read fails.
- [High] If the codec was reset during suspend, es8389_resume re-initializes it but fails to turn on the clocks and bias.
--

commit 15d74449bde5fe0d61c65b106c592477686df171
Author: Zhang Yi <zhangyi@everest-semi.com>

ASoC: codecs: ES8389: Modify the initial configuration

This commit modifies the initial configuration for the ES8389 codec by
updating the values written to ES8389_ISO_CTL and ES8389_OSC_CLK during
initialization.

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index dc3410459f39d..09257687bb84c 100644
> --- a/sound/soc/codecs/es8389.c
> +++ b/sound/soc/codecs/es8389.c
> @@ -793,7 +793,7 @@ static void es8389_init(struct snd_soc_component *component)
>  
>  	regmap_read(es8389->regmap, ES8389_MAX_REGISTER, &reg);
>  	es8389->version = reg;

[Severity: High]
This is a pre-existing issue, but what happens if regmap_read() fails here?

If the underlying I2C transfer fails, regmap_read() returns an error and
leaves reg uninitialized. This uninitialized stack variable is then blindly
assigned to es8389->version.

This same pattern occurs in multiple places, such as es8389_resume():

sound/soc/codecs/es8389.c:es8389_resume() {
    ...
	regmap_read(es8389->regmap, ES8389_RESET, &regv);

	if (regv == 0xff)
    ...
}

and es8389_mute():

sound/soc/codecs/es8389.c:es8389_mute() {
    ...
		regmap_read(es8389->regmap, ES8389_CSM_STATE1, &regv);
		if (regv != ES8389_STATE_ON) {
    ...
}

Should the return value of regmap_read() be checked to prevent using
uninitialized stack variables and causing random device state
misconfigurations?

> -	regmap_write(es8389->regmap, ES8389_ISO_CTL, 0x00);
> +	regmap_write(es8389->regmap, ES8389_ISO_CTL, 0x56);

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this driver leak regulator references
on module removal?

In es8389_probe(), the core regulators are enabled:

sound/soc/codecs/es8389.c:es8389_probe() {
    ...
	ret = regulator_bulk_enable(ARRAY_SIZE(es8389_core_supplies),
				    es8389->core_supply);
    ...
}

However, es8389_remove() fails to call regulator_bulk_disable():

sound/soc/codecs/es8389.c:es8389_remove() {
	struct es8389_private *es8389 = snd_soc_component_get_drvdata(component);

	regmap_write(es8389->regmap, ES8389_MASTER_MODE, 0x28);
    ...
}

When the component is unbound, the devres framework will free the regulator
structure while its enable count is unbalanced. Does this cause a regulator
core warning and a permanent resource leak?


[Severity: High]
This is a pre-existing issue, but do we have unbalanced clock prepare/enable
calls in the bias level handling?

es8389_set_bias_level() unconditionally disables mclk in the standby case:

sound/soc/codecs/es8389.c:es8389_set_bias_level() {
    ...
	case SND_SOC_BIAS_STANDBY:
        ...
		clk_disable_unprepare(es8389->mclk);
		break;
    ...
}

Then in es8389_suspend(), the driver explicitly forces a standby transition:

sound/soc/codecs/es8389.c:es8389_suspend() {
    ...
	es8389_set_bias_level(component, SND_SOC_BIAS_STANDBY);
    ...
}

If the ASoC core had already transitioned the bias to standby prior to
component suspend, does this force a double-disable of the clock and cause a
clock underflow?


[Severity: High]
This is a pre-existing issue, but does es8389_resume() correctly bring the
device out of standby?

If the codec was reset during suspend, it re-initializes the codec via
es8389_init():

sound/soc/codecs/es8389.c:es8389_resume() {
    ...
	if (regv == 0xff)
		es8389_init(component);
	else
		es8389_set_bias_level(component, SND_SOC_BIAS_ON);
    ...
}

However, es8389_init() only configures registers without transitioning the
bias state or enabling mclk. By skipping the es8389_set_bias_level(...,
SND_SOC_BIAS_ON) call, does the device remain unclocked and in a powered-off
bias state?

If so, a subsequent ASoC state transition to standby might try to unprepare
the already disabled clock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618082842.2778-1-zhangyi@everest-semi.com?part=5