Re: [PATCH v1 4/7] ASoC: codecs: ES8389: Modify the clock table

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Flawed clock coefficient matching logic makes `dmic_sel = 0` and `dvdd_vol = 0` act as wildcards, leading to incorrect hardware configuration and dead code.
- [High] Unchecked `regmap_read()` return value leads to the use of uninitialized stack memory for hardware configuration.
--

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

ASoC: codecs: ES8389: Modify the clock table

Updated the configuration for certain frequencies
If get_coeff does not find a value that matches the condition, return -EINVAL

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index 886069be1535f..647d9d62820e6 100644
> --- a/sound/soc/codecs/es8389.c
> +++ b/sound/soc/codecs/es8389.c
[ ... ]
> @@ -434,52 +434,54 @@ static const struct _coeff_div  coeff_div[] = {
[ ... ]
>  	{256, 24576000, 96000, 0x00, 0x40, 0x00, 0xC0, 0x10, 0xC1, 0x80, 0xC0, 0x00, 0x9F, 0x7F, 0xBF, 0xC0, 0x7F, 0x7F, 0x80, 0x12, 0xC0, 0x35, 0x91, 0x28, 2, 2},
>  	{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},

[Severity: High]
This is a pre-existing issue, but will the wildcard matching logic in
get_coeff() cause this new entry to match incorrectly?

In get_coeff(), the wildcard logic uses modulo 2:

    dmic_det |= ~(coeff_div[i].dmic_sel % 2) & 0x01;

For dmic_sel = 0, 0 % 2 == 0, which forces dmic_det to 1 regardless of the
requested dmic state. This makes 0 behave exactly like the 2 wildcard. As a
result, entries intending to strictly match dmic = 0 (such as this newly
added entry) will incorrectly match any DMIC state.

Could this also cause existing entries like `{... 2, 0}` to mask subsequent
`{... 2, 1}` entries, resulting in unreachable dead code and incorrect
hardware coefficients being applied?

>  };
>  
>  static inline int get_coeff(u8 vddd, u8 dmic, int mclk, int rate)
[ ... ]
> @@ -657,7 +659,8 @@ static int es8389_pcm_hw_params(struct snd_pcm_substream *substream,
>  		regmap_write(es8389->regmap, ES8389_SYSTEM18, coeff_div[coeff].Reg0x18);
>  		regmap_write(es8389->regmap, ES8389_SYSTEM19, coeff_div[coeff].Reg0x19);
>  	} else {
> -		dev_warn(component->dev, "Clock coefficients do not match");
> +		dev_err(component->dev, "Clock coefficients do not match");
> +		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but could the lack of error checking on
regmap_read() lead to the use of uninitialized stack memory for hardware
configuration?

Earlier in es8389_pcm_hw_params(), the driver calls regmap_read() without
validating its return value:

    regmap_read(es8389->regmap, ES8389_DMIC_EN, &regv);
    dmic_enable = regv >> 7 & 0x01;

If the underlying communication fails, the local variable regv remains
uninitialized. This stack memory is then immediately evaluated when selecting
coefficients. Could this cause the driver to select and apply random,
non-deterministic clock coefficients to the hardware?

>  	}
>  
>  	return 0;
>  }

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