Re: [PATCH v5 2/2] ASoC: cs35l45: Adds support for SYNC bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Hardware I/O errors in cs35l45_sync_en_put cause permanent state desynchronization.
--

commit 81445881eb3cef5b79a02d7516346532c3f3f907
Author: Ricardo Rivera-Matos <rriveram@opensource.cirrus.com>
ASoC: cs35l45: Adds support for SYNC bus

Adds support for the multidevice synchronization (SYNC) bus. The SYNC
bus is an opaque one wire bus that is shared between Cirrus devices.

> diff --git a/sound/soc/codecs/cs35l45.c b/sound/soc/codecs/cs35l45.c
> index a032bb23b4ac7..763cd83b5d6d8 100644
> --- a/sound/soc/codecs/cs35l45.c
> +++ b/sound/soc/codecs/cs35l45.c
[ ... ]
> @@ -195,6 +196,46 @@ static int cs35l45_activate_ctl(struct snd_soc_component *component,
[ ... ]
> +static int cs35l45_sync_en_put(struct snd_kcontrol *kcontrol,
> +				      struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct cs35l45_private *cs35l45 =
> +			snd_soc_component_get_drvdata(component);
> +	struct snd_soc_dapm_context *dapm =
> +			snd_soc_component_to_dapm(component);
> +
> +	snd_soc_dapm_mutex_lock(dapm);
> +
> +	if ((bool)ucontrol->value.integer.value[0] == cs35l45->sync_en) {
> +		snd_soc_dapm_mutex_unlock(dapm);
> +		return 0;
> +	}
> +
> +	if ((bool)ucontrol->value.integer.value[0])
> +		regmap_set_bits(cs35l45->regmap, CS35L45_BLOCK_ENABLES2, CS35L45_SYNC_EN_MASK);
> +	else
> +		regmap_clear_bits(cs35l45->regmap, CS35L45_BLOCK_ENABLES2, CS35L45_SYNC_EN_MASK);
> +
> +	cs35l45->sync_en = (bool)ucontrol->value.integer.value[0];

[Severity: Medium]
Could this lead to permanent state desynchronization if the hardware I/O fails?
The regmap API calls can fail (e.g., returning -EIO on I2C/SPI errors), but the
error is ignored here and the software cache (cs35l45->sync_en) is updated
unconditionally.

If this happens, the ALSA control state becomes desynchronized from the hardware.
Because of the fast-path check at the beginning of this function:

    if ((bool)ucontrol->value.integer.value[0] == cs35l45->sync_en) {
        ...

Any subsequent attempts from userspace to set the correct value will be dropped,
making the failure permanent until explicitly toggled to the opposite state and
back again. Should the return values of regmap_set_bits() and regmap_clear_bits()
be checked before updating the cached state?

> +
> +	snd_soc_dapm_mutex_unlock(dapm);
> +
> +	return 1;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-sound-for-next-cs35l45-sync-v4-v5-0-fe3feba709ff@opensource.cirrus.com?part=2