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

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

New issues:
- [Medium] Potential I2C communication failure due to missing stabilization delay after enabling regulators.

Pre-existing issues:
- [Critical] Use-After-Free of snd_soc_component in the IRQ handler due to devres teardown order.
- [High] Regmap cache desynchronization due to pre-existing hardware reset defeating the patch's premise.
--

--- Patch [2]: [PATCH v2 2/2] ASoC: codecs: es8316: Add regulator support ---
commit 3daba0da576ac350b6f022776265341a98e4d7f5
Author: Hongyang Zhao <[email protected]>

ASoC: codecs: es8316: Add regulator support
    
ES8316 has separate AVDD, CPVDD, DVDD and PVDD supply inputs.

> Request and enable the supplies during I2C probe, before initializing
> the regmap. Keep them enabled for the lifetime of the I2C device so the
> regmap cannot access an unpowered device and its cache remains
> synchronized if the ASoC component is unbound and rebound.

[Severity: High]
This isn't a bug introduced by this patch, but does the pre-existing hardware
reset in es8316_probe() defeat the premise of keeping the regmap synchronized?

When the component is unbound and rebound, es8316_probe() executes:

sound/soc/codecs/es8316.c:es8316_probe() {
    ...
    snd_soc_component_write(component, ES8316_RESET, 0x3f);
    ...
}

This wipes the physical chip state without marking the regcache dirty.
Consequently, subsequent ALSA control writes matching the stale cache might be
dropped, leading to silent audio or incorrect settings.

> Signed-off-by: Hongyang Zhao <[email protected]>

> diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
> index 87f331868dc7f..24ec8b211cdba 100644
> --- a/sound/soc/codecs/es8316.c
> +++ b/sound/soc/codecs/es8316.c
[ ... ]
> @@ -866,6 +874,11 @@ static int es8316_i2c_probe(struct i2c_client *i2c_client)
>  
>  	i2c_set_clientdata(i2c_client, es8316);
>  
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(es8316_supply_names),
> +					     es8316_supply_names);

[Severity: Medium]
Will this lack of stabilization delay cause I2C communication failures?

If the ES8316 requires a delay after power is applied before its I2C state
machine is ready, the first I2C write during the component probe could
receive a NACK. Might it be safer to add a small post-power-on delay here?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "unable to enable supplies\n");
> +

[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free risk during
devres teardown?

During module removal or I2C device unbind, devres tears down resources in
reverse order of allocation. The IRQ is requested later in this function
before the component memory is allocated via devres.

If the hardware asserts the IRQ in the narrow window after the component
is freed but before the IRQ is disabled and freed, es8316_irq() will
dereference the freed component pointer:

sound/soc/codecs/es8316.c:es8316_irq() {
    ...
    dev_dbg(comp->dev, "gpio flags %#04x\n", flags);
    ...
}

Can this lead to memory corruption if the handler executes late?

>  	es8316->regmap = devm_regmap_init_i2c(i2c_client, &es8316_regmap);
>  	if (IS_ERR(es8316->regmap))
>  		return PTR_ERR(es8316->regmap);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812-es8316-regulator-next-20260722-v2-0-e7078bc9bc9c@thundersoft.com?part=2