Re: [PATCH 2/2] ASoC: codecs: es8316: Add regulator support

[sashiko-bot@kernel.org]

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

New issues:
- [High] The regmap lacks cache-only protection while regulators are disabled, allowing physical I2C access to an unpowered device.
- [High] Regmap cache becomes permanently out-of-sync with hardware after component unbind/rebind due to newly added unhandled power loss.

Pre-existing issues:
- [High] Unchecked return value of regmap_read() leads to usage of an uninitialized stack variable in the IRQ handler.
--

--- Patch [2]: [PATCH 2/2] ASoC: codecs: es8316: Add regulator support ---
commit 8810711bae01652cebd6d01e2ef62b85b877ad30
Author: Hongyang Zhao <hongyang.zhao@thundersoft.com>

ASoC: codecs: es8316: Add regulator support

ES8316 has separate AVDD, CPVDD, DVDD and PVDD supply inputs. Request the
supplies during I2C probe. Enable them before enabling MCLK and accessing
the device registers, and disable them on probe failure and component
removal.

> diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
> index 3abe77423f29b..904056b2569a5 100644
> --- a/sound/soc/codecs/es8316.c
> +++ b/sound/soc/codecs/es8316.c
[ ... ]
> @@ -767,10 +778,16 @@ static int es8316_probe(struct snd_soc_component *component)
>  	if (!es8316->mclk)
>  		dev_warn(component->dev, "assuming static mclk\n");
>  
> +	ret = regulator_bulk_enable(ES8316_NUM_SUPPLIES, es8316->supplies);
> +	if (ret) {
> +		dev_err(component->dev, "unable to enable supplies\n");
> +		return ret;
> +	}
> +

[Severity: High]
Does this leave the regmap cache permanently out-of-sync with the hardware
after a component unbind and rebind?

When the component is unbound in es8316_remove(), regulator_bulk_disable()
cuts power to the hardware, wiping the hardware register state. However, the
regmap cache survives because it is tied to the persistent i2c device. When
power is restored here during a rebind, the hardware resets to defaults, but
because regcache_mark_dirty() is not called, subsequent regmap updates that
match the stale cache will be skipped.

[ ... ]
> @@ -800,6 +822,7 @@ static void es8316_remove(struct snd_soc_component *component)
>  	struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);
>  
>  	clk_disable_unprepare(es8316->mclk);
> +	regulator_bulk_disable(ES8316_NUM_SUPPLIES, es8316->supplies);
>  }

[Severity: High]
Is the regmap missing cache-only protection while the regulators are
disabled?

The regmap is initialized in es8316_i2c_probe() and persists until the i2c
device is removed. When the hardware is powered down here, can a read via
the regmap debugfs interface attempt physical i2c accesses to the unpowered
device?

[Severity: High]
This isn't a bug introduced by this patch, but does es8316_irq() use an
uninitialized stack variable if the i2c read fails?

sound/soc/codecs/es8316.c:es8316_irq() {
	...
	unsigned int flags;

	mutex_lock(&es8316->lock);

	regmap_read(es8316->regmap, ES8316_GPIO_FLAG, &flags);
	if (flags == 0x00)
		goto out;
	...
}

If a transient i2c bus error causes regmap_read() to fail and return an
error code, flags remains uninitialized. Will the function then evaluate
garbage data from the stack to determine the jack status?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-es8316-regulator-next-20260722-v1-0-721f757879a0@thundersoft.com?part=2