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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Critical configuration registers are incorrectly marked as volatile, causing permanent state loss on suspend/resume.
- [High] Failure to check the return value of `regmap_read` leads to branching on and evaluating uninitialized stack variables.
--

commit 963a77368701e6888ce20ffbda8712f190e260d3
Author: Zhang Yi <zhangyi@everest-semi.com>

ASoC: codecs: ES8389: Modify volatile_register

This patch updates the es8389_volatile_register function to explicitly mark
certain registers as non-volatile, falling back to treating others as
volatile. It also adjusts the timing of when regcache_cache_bypass is
disabled in the es8389_resume function.

> 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 is a pre-existing issue, but does defaulting to true for all other
registers omit key configuration registers from the cache?

Because volatile registers are skipped by regcache_sync() during resume,
registers such as ES8389_MASTER_MODE, TDM settings, and clock dividers will
not be restored if they are treated as volatile. Could this break the audio
formatting and cause playback or capture to fail after a suspend and resume
cycle?

[ ... ]

> @@ -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);
> -	regcache_cache_bypass(es8389->regmap, false);
>  
>  	if (regv == 0xff)

[Severity: High]
This is a pre-existing issue, but does the lack of a return value check on
regmap_read() leave regv uninitialized if the I2C read fails?

If the read fails, evaluating the uninitialized stack variable regv will
unpredictably dictate whether a full hardware initialization is performed.

This same pattern appears to exist in other places as well:

sound/soc/codecs/es8389.c:es8389_init() {
    ...
    regmap_read(es8389->regmap, ES8389_MAX_REGISTER, &reg);
    es8389->version = reg;
    ...
}

and

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

Could these lead to inconsistent hardware states or silent audio failures?

>  		es8389_init(component);
>  	else
>  		es8389_set_bias_level(component, SND_SOC_BIAS_ON);
>  
> +	regcache_cache_bypass(es8389->regmap, false);
>  	regcache_sync(es8389->regmap);
>  
>  	return 0;

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