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

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

New issues:
- [High] The `cs35l45_sync_en_put` ALSA control put callback incorrectly returns 1 when unchanged, ignores regmap errors, and lacks locking.
- [Medium] Boolean ALSA controls in `cs35l45_sync_controls` are incorrectly named with the suffix ' Enable' instead of ' Switch'.

Pre-existing issues:
- [High] The `cs35l45_amplifier_mode_put` ALSA control updates the software shadow state (`cs35l45->amplifier_mode`) and performs its precondition check outside of the DAPM mutex.
--

--- Patch [2]: [PATCH v3 2/2] ASoC: cs35l45: Adds support for SYNC bus ---
commit ba724d28c232323128106bdc26fdc20dca80f285
Author: Ricardo Rivera-Matos <[email protected]>

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. Initial support only includes software (SW)
data packets.

> diff --git a/sound/soc/codecs/cs35l45.c b/sound/soc/codecs/cs35l45.c
> index a032bb23b4ac7..559fae09674cc 100644
> --- a/sound/soc/codecs/cs35l45.c
> +++ b/sound/soc/codecs/cs35l45.c

[ ... ]

> @@ -195,6 +196,38 @@ static int cs35l45_activate_ctl(struct snd_soc_component *component,
>  	return 0;
>  }
>  
> +static int cs35l45_sync_en_get(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);
> +
> +	ucontrol->value.integer.value[0] = cs35l45->sync_en;
> +
> +	return 0;
> +}
> +
> +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);
> +
> +	if (ucontrol->value.integer.value[0] == cs35l45->sync_en)
> +		return 1;

[Severity: High]
Does this incorrectly return 1 when the value is unchanged?

The ALSA kcontrol API expects 0 when the value is unchanged, so returning 1
will cause spurious change events to be broadcast to userspace.

> +
> +	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 = ucontrol->value.integer.value[0];
> +
> +	return 1;
> +}

[Severity: High]
Is it safe to ignore the return values from regmap_set_bits() and
regmap_clear_bits() here? 

If the I/O operation fails, the software shadow state will become
desynchronized from the actual hardware state.

Also, should this state update be protected by a lock? 

Concurrent writes via snd_ctl_elem_write() could trigger race conditions since
the state check and update are not atomic.

[ ... ]

> @@ -591,6 +624,18 @@ static SOC_ENUM_SINGLE_DECL(amplifier_mode_enum, SND_SOC_NOPM, 0,
>  static DECLARE_TLV_DB_SCALE(amp_gain_tlv, 1000, 300, 0);
>  static const DECLARE_TLV_DB_SCALE(cs35l45_dig_pcm_vol_tlv, -10225, 25, true);
>  
> +static const struct snd_kcontrol_new cs35l45_sync_controls[] = {
> +	SOC_SINGLE_BOOL_EXT("SYNC Enable", 0, cs35l45_sync_en_get, cs35l45_sync_en_put),
> +	SOC_SINGLE("SYNC LSW RX Enable", CS35L45_SYNC_TX_RX_ENABLES,
> +			CS35L45_SYNC_LSW_RX_EN_SHIFT, 1, 0),
> +	SOC_SINGLE("SYNC LSW TX Enable", CS35L45_SYNC_TX_RX_ENABLES,
> +			CS35L45_SYNC_LSW_TX_EN_SHIFT, 1, 0),
> +	SOC_SINGLE("SYNC SW RX Enable", CS35L45_SYNC_TX_RX_ENABLES,
> +			CS35L45_SYNC_SW_RX_EN_SHIFT, 1, 0),
> +	SOC_SINGLE("SYNC SW TX Enable", CS35L45_SYNC_TX_RX_ENABLES,
> +			CS35L45_SYNC_SW_TX_EN_SHIFT, 1, 0),
> +};

[Severity: Medium]
Do these boolean controls need to end in " Switch" instead of " Enable"?

ALSA control naming conventions mandate that boolean controls must end in
" Switch". Standard user-space audio layers (like PulseAudio and PipeWire)
rely on this to recognize them as toggles rather than exposing them as
raw integers.

[ ... ]

While reviewing the ALSA controls, I noticed a pre-existing issue in the
cs35l45_amplifier_mode_put() function:

sound/soc/codecs/cs35l45.c:cs35l45_amplifier_mode_put() {
    ...
    snd_soc_dapm_mutex_unlock(dapm);

    cs35l45->amplifier_mode = ucontrol->value.integer.value[0];

    return 1;
}

[Severity: High]
This is a pre-existing issue, but does cs35l45_amplifier_mode_put() update
the software shadow state outside of the DAPM mutex? 

If two threads execute the put callback concurrently with different target
modes, thread B could check the condition before thread A completes its
hardware update. This could lead to the shadow state becoming out of sync
with the hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2