Re: [PATCH v1 3/7] ASoC: codecs: ES8389: Fix the issue about mclk_src

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing symmetric constraints for channels and sample bits allows concurrent streams to corrupt shared clock configuration.
- [High] Concurrent `hw_params` calls can race, leading to corrupted internal state and interleaved register writes.
--

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

ASoC: codecs: ES8389: Fix the issue about mclk_src
    
Fix the issue with incorrect modifications to mclk_src when configuring
the system to use MCLK from the SCLK pin. Changes the mclk_src member
from u8 to bool to better represent its purpose.

> diff --git a/sound/soc/codecs/es8389.c b/sound/soc/codecs/es8389.c
> index be7a36f17083f..886069be1535f 100644
> --- a/sound/soc/codecs/es8389.c
> +++ b/sound/soc/codecs/es8389.c

[ ... ]

> @@ -607,9 +607,9 @@ static int es8389_pcm_hw_params(struct snd_pcm_substream *substream,
>  	regmap_update_bits(es8389->regmap, ES8389_ADC_FORMAT_MUTE, ES8389_DATA_LEN_MASK, state);
>  	regmap_update_bits(es8389->regmap, ES8389_DAC_FORMAT_MUTE, ES8389_DATA_LEN_MASK, state);
>  
> -	if (es8389->mclk_src == ES8389_SCLK_PIN) {
> +	if (es8389->mclk_src) {
>  		regmap_update_bits(es8389->regmap, ES8389_MASTER_CLK,
> -					ES8389_MCLK_SOURCE, es8389->mclk_src);
> +					ES8389_MCLK_MASK, ES8389_MCLK_FROM_SCLK);
>  		es8389->sysclk = params_channels(params) * params_width(params) * params_rate(params);

[Severity: High]
This is a pre-existing issue, but I noticed the driver dynamically calculates
a shared internal clock (es8389->sysclk) based on the stream parameters here.

Since the es8389_dai definition doesn't set .symmetric_channels = 1 or
.symmetric_sample_bits = 1, couldn't user-space concurrently open playback
and capture streams with different channel counts or bit widths?

Would the second stream silently recalculate and overwrite the hardware clock
registers, breaking the already-running stream?

[Severity: High]
This is also a pre-existing issue, but does this function need locking to
protect against concurrent execution?

ALSA's PCM core does not serialize hw_params calls between separate playback
and capture substreams on the same DAI. If playback and capture streams
execute hw_params simultaneously, couldn't they race when updating shared
driver state like es8389->sysclk and es8389->vddd?

Later in this function, there's a sequence of about 20 non-atomic regmap_write
operations to clock control registers:

    regmap_write(es8389->regmap, ES8389_CLK_DIV1, coeff_div[coeff].Reg0x04);
    regmap_write(es8389->regmap, ES8389_CLK_MUL, coeff_div[coeff].Reg0x05);
    ...

Could concurrent execution interleave these hardware register writes and
potentially leave the codec's clocking tree in an invalid state?

>  	}
>

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