Re: [PATCH v1 2/7] ASoC: codecs: ES8389: Modify volatile_register

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete volatile register whitelist causes state loss on suspend/resume.
- [High] Ignored `regmap_read()` return values lead to the use of uninitialized stack variables.
- [High] Unbalanced clock prepare/enable and disable/unprepare calls due to manual bias level manipulation.
--

commit 13fa26be72cc2a7540f3f86a2ef766aae985561e
Author: Zhang Yi <zhangyi@everest-semi.com>

ASoC: codecs: ES8389: Modify volatile_register

Mark some registers that are not volatile as false
And modified the logic for cache_bypass during 8389_resume.

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index 449d9574b03a8..be7a36f17083f 100644
> --- a/sound/soc/codecs/es8389.c
> +++ b/sound/soc/codecs/es8389.c
> @@ -50,10 +50,29 @@ static const char * const es8389_core_supplies[] = {
>  static bool es8389_volatile_register(struct device *dev,
>  			unsigned int reg)
>  {
> -	if ((reg  <= 0xff))
> -		return true;
> -	else
> +	switch (reg) {
> +	case ES8389_ADCL_VOL:
> +	case ES8389_ADCR_VOL:
> +	case ES8389_MIC1_GAIN:
> +	case ES8389_MIC2_GAIN:
> +	case ES8389_DACL_VOL:
> +	case ES8389_DACR_VOL:
> +	case ES8389_ALC_ON:
> +	case ES8389_ALC_CTL:
> +	case ES8389_ALC_TARGET:
> +	case ES8389_ALC_GAIN:
> +	case ES8389_ADC_MUTE:
> +	case ES8389_OSR_VOL:
> +	case ES8389_DAC_INV:
> +	case ES8389_MIX_VOL:
> +	case ES8389_DAC_MIX:
> +	case ES8389_ADC_RESET:
> +	case ES8389_ADC_MODE:
> +	case ES8389_DMIC_EN:
>  		return false;
> +	default:
> +		return true;
> +	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but will this incomplete whitelist
cause state loss across suspend/resume cycles?

Because the default case marks all unlisted registers as volatile, critical
configuration registers like ES8389_ADC_HPF2, ES8389_MASTER_MODE, and
ES8389_CLK_DIV1 are omitted from the cache.

Since regcache_sync() skips volatile registers, won't es8389_init() just
overwrite these with default values during resume, discarding any active
user configurations?

[ ... ]

> @@ -861,13 +880,13 @@ static int es8389_resume(struct snd_soc_component *component)
>  	regcache_cache_only(es8389->regmap, false);
>  	regcache_cache_bypass(es8389->regmap, true);
>  	regmap_read(es8389->regmap, ES8389_RESET, &regv);

[Severity: High]
This is a pre-existing issue, but does this code risk branching on an
uninitialized variable?

If regmap_read() fails during early resume, regv will contain uninitialized
stack memory. The subsequent check against 0xff will yield unpredictable
results, potentially skipping the required initialization.

The same issue appears to exist in es8389_init() when reading
ES8389_MAX_REGISTER into the local variable reg before assigning it to
es8389->version. Could we check the return values of these reads?

> -	regcache_cache_bypass(es8389->regmap, false);
>  
>  	if (regv == 0xff)
>  		es8389_init(component);
>  	else
>  		es8389_set_bias_level(component, SND_SOC_BIAS_ON);

[Severity: High]
This is a pre-existing issue, but does calling es8389_set_bias_level()
directly from the resume and suspend handlers conflict with the ASoC DAPM
framework?

Since DAPM independently transitions bias levels and triggers the
registered set_bias_level callback, it seems this direct call would cause
clk_prepare_enable() to run multiple times on resume, leaking the clock
reference count.

Similarly, during suspend, wouldn't redundant calls to
clk_disable_unprepare() cause a clock underflow warning?

>  
> +	regcache_cache_bypass(es8389->regmap, false);
>  	regcache_sync(es8389->regmap);
>  
>  	return 0;

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