Re: [PATCH v0 3/7] ASoC: codecs: ES8389: Modify the clock table

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Bitwise logic in get_coeff() incorrectly treats dmic_sel=0 and dvdd_vol=0 as wildcards, shadowing specific clock configurations and breaking newly added entries.
- [High] Regulator reference leak on component removal.
- [High] Use of uninitialized memory if regmap_read fails in es8389_pcm_hw_params.
- [High] Use of uninitialized memory if regmap_read fails in es8389_resume.
- [High] es8389_pcm_hw_params silently ignores missing clock coefficients.
--

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

ASoC: codecs: ES8389: Modify the clock table

Updated the configuration for certain frequencies in the es8389 codec.

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index 638eb1f0462ea..cd8284da8c62c 100644
> --- a/sound/soc/codecs/es8389.c
> +++ b/sound/soc/codecs/es8389.c
> @@ -434,52 +434,54 @@ static const struct _coeff_div  coeff_div[] = {
[ ... ]
>  	{128, 24576000, 192000, 0x00, 0x50, 0x00, 0xC0, 0x18, 0xC1, 0x81, 0xC0, 0x00, 0x8F, 0x7F, 0xBF, 0xC0, 0x3F, 0x7F, 0x80, 0x12, 0xC0, 0x3F, 0xF9, 0x3F, 2, 2},
> +	{64, 12288000, 192000, 0x00, 0x41, 0x00, 0xC0, 0x18, 0xC1, 0x80, 0x00, 0x00, 0x8F, 0x7F, 0xEF, 0xC0, 0x7F, 0x7F, 0x80, 0x12, 0xC0, 0x3F, 0xF9, 0x3F, 1, 0},
>  };
>  
>  static inline int get_coeff(u8 vddd, u8 dmic, int mclk, int rate)

[Severity: High]
This is a pre-existing issue, but modifying this table highlights a bug
in get_coeff() that affects how these entries are matched.

Does the bitwise logic in get_coeff() incorrectly treat dmic_sel=0 and
dvdd_vol=0 as wildcards?

sound/soc/codecs/es8389.c:get_coeff() {
    ...
    vddd_det |= ~(coeff_div[i].dvdd_vol % 2) & 0x01;
    dmic_det |= ~(coeff_div[i].dmic_sel % 2) & 0x01;

    if (vddd_det && dmic_det)
        return i;
    ...
}

This unconditionally evaluates to 1 when dmic_sel or dvdd_vol is 0,
turning 0 into a wildcard instead of an exact match. This can shadow
specific clock configurations and render subsequent array entries with the
same rate/mclk unreachable.

[Severity: High]
This is a pre-existing issue, but could we fix a regulator reference leak
on component removal?

In es8389_probe(), regulators are retrieved and manually enabled, but the
corresponding es8389_remove() component teardown function does not disable
them:

sound/soc/codecs/es8389.c:es8389_remove() {
    struct es8389_private *es8389 = snd_soc_component_get_drvdata(component);
    ...
    regmap_write(es8389->regmap, ES8389_ISO_CTL, 0xC1);
    regmap_write(es8389->regmap, ES8389_PULL_DOWN, 0x00);
}

Does this leak the regulator references when the I2C driver is unbound or
the module is removed?

[Severity: High]
This is a pre-existing issue, but does this code use uninitialized memory
if regmap_read() fails?

sound/soc/codecs/es8389.c:es8389_pcm_hw_params() {
    ...
    regmap_read(es8389->regmap, ES8389_DMIC_EN, &regv);
    dmic_enable = regv >> 7 & 0x01;
    ...
}

The return value of regmap_read() is ignored. If the I2C read fails,
regv contains uninitialized stack memory, causing dmic_enable to be randomly
set and potentially leading to invalid clock configurations.

[Severity: High]
This is a pre-existing issue, but does this code use uninitialized memory
if regmap_read() fails upon resuming?

sound/soc/codecs/es8389.c:es8389_resume() {
    ...
    regmap_read(es8389->regmap, ES8389_RESET, &regv);
    regcache_cache_bypass(es8389->regmap, false);

    if (regv == 0xff)
    ...
}

Similar to hw_params, if the hardware is unresponsive and the read fails,
regv contains uninitialized stack memory, potentially causing unpredictable
branching.

[Severity: High]
This is a pre-existing issue, but does es8389_pcm_hw_params() silently ignore
missing clock coefficients?

sound/soc/codecs/es8389.c:es8389_pcm_hw_params() {
    ...
    } else {
        dev_warn(component->dev, "Clock coefficients do not match");
    }

    return 0;
}

If get_coeff() fails to find a valid clock configuration, this function logs a
warning but still returns 0 (success). Can this mislead the ALSA core into
running audio with a misconfigured codec clock tree?

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