Re: [PATCH v5 3/9] ASoC: qcom: lpass-rx-macro: Add shikra compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] sound/soc: data race on rx_mclk_users leads to premature MCLK disable
--

commit 3e23cc050d00484d829b0a64e250b4871e497cc8
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

ASoC: qcom: lpass-rx-macro: Add shikra compatible

This commit adds the shikra RX macro compatible data and hooks it into the
LPASS codec v4.1 flow. It reuses existing v2.5 register access helpers and
configures the FS counter to be bypassed before MCLK is enabled.

> diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c
> index c41eb8de61609..93ceee878b78a 100644
> --- a/sound/soc/codecs/lpass-rx-macro.c
> +++ b/sound/soc/codecs/lpass-rx-macro.c

[ ... ]

> @@ -2041,20 +2044,25 @@ static int rx_macro_mclk_enable(struct rx_macro *rx, bool mclk_enable)
>  
>  	if (mclk_enable) {
>  		if (rx->rx_mclk_users == 0) {
>  			regmap_update_bits(regmap, CDC_RX_CLK_RST_CTRL_MCLK_CONTROL,
>  					   CDC_RX_CLK_MCLK_EN_MASK |
>  					   CDC_RX_CLK_MCLK2_EN_MASK,
>  					   CDC_RX_CLK_MCLK_ENABLE |
>  					   CDC_RX_CLK_MCLK2_ENABLE);
>  			regmap_update_bits(regmap, CDC_RX_CLK_RST_CTRL_FS_CNT_CONTROL,
>  					   CDC_RX_FS_MCLK_CNT_CLR_MASK, 0x00);
> +
> +			if (rx->fs_cnt_bypass)
> +				regmap_set_bits(regmap, CDC_RX_CLK_RST_CTRL_FS_CNT_CONTROL,
> +						CDC_RX_FS_CNT_BYPASS);
> +
>  			regmap_update_bits(regmap, CDC_RX_CLK_RST_CTRL_FS_CNT_CONTROL,
>  					   CDC_RX_FS_MCLK_CNT_EN_MASK,
>  					   CDC_RX_FS_MCLK_CNT_ENABLE);
>  			regcache_mark_dirty(regmap);
>  			ret = regcache_sync(regmap);
>  			if (ret)
>  				return ret;
>  		}
>  		rx->rx_mclk_users++;

[Severity: Medium]
Could this function be susceptible to a data race on rx->rx_mclk_users?

It appears that rx_macro_mclk_enable() can be called from two different
concurrency domains without internal locking: swclk_gate_enable() and
swclk_gate_disable() (which execute under the clock framework's prepare_lock
mutex) and rx_macro_mclk_event() (which executes under ALSA DAPM's
dapm_mutex).

If the rx->rx_mclk_users counter is checked and modified non-atomically,
could concurrent stream start and clock enable events cause a lost increment?

If an increment is lost, when one stream later stops, rx_mclk_users could
drop to 0 prematurely, disabling the MCLK hardware while the other stream
is still actively relying on it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-add_shikra_support_v5-v5-0-fb9f0667e613@oss.qualcomm.com?part=3