Re: [PATCH 1/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:

New issues:
- [High] The regmap cache for non-volatile registers is overwritten with hardcoded default values during resume if the device lost power, neutralizing the patch's fix.

Pre-existing issues:
- [High] Critical configuration registers modified during stream setup are marked as volatile, causing them to be lost upon suspend/resume.
- [High] The variable `regv` is evaluated uninitialized if `regmap_read` fails during resume.
--

commit 146c46405cf59e3e0362628efb42fd4f52cff71a
Author: Zhang Yi <zhangyi@everest-semi.com>

ASoC: codecs: ES8389: Modify volatile_register

This patch modifies es8389_volatile_register() to mark a specific set of 
registers as non-volatile, enabling regcache for them. All other registers 
now default to being volatile.

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index 449d9574b03a8..9c816491648c7 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_ADC_HPF1:
> +	case ES8389_ADC_HPF2:

[ ... ]

> +	case ES8389_ADC_RESET:
>  		return false;
> +	default:
> +		return true;

[Severity: High]
This is a pre-existing issue, but does this fall-through prevent critical
configuration registers from being restored by regcache_sync()?

Critical configuration registers modified during stream setup like
ES8389_DMIC_EN, ES8389_ADC_MODE, ES8389_ADC_FORMAT_MUTE, and ES8389_ADC_OSR
remain uncached.

Since the ALSA framework does not re-invoke hw_params on resume, these
registers must be cached and restored by regcache_sync(). Because they are
volatile, active audio streams will fail or lose routing after a
suspend/resume cycle.

> +	}
>  }

[Severity: High]
By newly marking these registers as non-volatile, does this cause the
regcache to be overwritten with defaults during a power-loss resume?

In sound/soc/codecs/es8389.c:es8389_resume(), if the device lost power
(regv == 0xff), es8389_init() is called without regcache_cache_bypass being
enabled:

    regmap_read(es8389->regmap, ES8389_RESET, &regv);
    regcache_cache_bypass(es8389->regmap, false);

    if (regv == 0xff)
        es8389_init(component);

Inside es8389_init(), direct regmap_write() calls are made:

    regmap_write(es8389->regmap, ES8389_ADC_HPF1, 0x64);

Because bypass is disabled, these writes clobber the newly added regmap cache
with hardcoded default values. The subsequent regcache_sync() then syncs these
defaults instead of the user's prior settings, breaking state restoration.

[Severity: High]
This is a pre-existing issue, but can regv be evaluated uninitialized if the
regmap_read fails during resume?

In sound/soc/codecs/es8389.c:es8389_resume():

    regmap_read(es8389->regmap, ES8389_RESET, &regv);
    regcache_cache_bypass(es8389->regmap, false);

    if (regv == 0xff)

If regmap_read() fails (which can occur during resume due to bus readiness
issues), regv contains uninitialized stack memory. The subsequent evaluation
against 0xff produces undefined behavior, potentially skipping necessary
device initialization and leading to silent failure.

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