Re: [PATCH RFC v2 5/6] ASoC: cs35l36: Implement set_tdm_slot to program RX and TX slots

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unmapped TDM channels are not disabled and retain their default slot assignments, leading to TDM slot collisions.
- [High] The new set_tdm_slot configures the RX TDM slot via a register that aliases an existing ALSA user control, introducing state corruption.
--

commit af0d8ef24a330447b6254f7aa04fa7142a333d1b
Author: David Heidelberg <david@ixit.cz>

ASoC: cs35l36: Implement set_tdm_slot to program RX and TX slots

This commit implements the set_tdm_slot DAI operation for the CS35L36 codec,
allowing machine drivers to program ASP RX and TX slots. It maps bits in the
provided RX and TX masks to consecutive codec channels, and sets hardware
defaults if no slots are passed.

> diff --git a/sound/soc/codecs/cs35l36.c b/sound/soc/codecs/cs35l36.c
> index 89645327945f1..4d35ca6291548 100644
> --- a/sound/soc/codecs/cs35l36.c
> +++ b/sound/soc/codecs/cs35l36.c

[ ... ]

> @@ -947,10 +947,54 @@ static const struct cs35l36_pll_config *cs35l36_get_clk_config(
>  	return NULL;
>  }
>  
> +static void cs35l36_mask_to_slots(struct cs35l36_private *cs35l36,
> +				  unsigned long mask, unsigned int base_reg,
> +				  unsigned int nchan)
> +{
> +	unsigned int chan = 0, shift;
> +	int slot;
> +
> +	/* Two 6-bit slot fields per register, at bits 0 and 16 */
> +	for_each_set_bit(slot, &mask, BITS_PER_TYPE(mask)) {
> +		if (chan == nchan) {
> +			dev_warn(cs35l36->dev,
> +				 "Too many slots in TDM mask: %lx\n", mask);
> +			return;
> +		}
> +
> +		shift = (chan % 2) * CS35L36_ASP_TX2_SLOT_SHIFT;
> +		regmap_update_bits(cs35l36->regmap, base_reg + (chan / 2) * 4,
> +				   CS35L36_ASP_RX1_SLOT_MASK << shift,
> +				   slot << shift);
> +		chan++;
> +	}

[Severity: High]
What happens to the remaining hardware channels if the provided mask has fewer
set bits than the total number of hardware channels?

The loop finishes processing once all set bits are consumed, which seems to
ignore the remaining hardware channels and leaves them in a potentially
dangerous active state on their default slots.

For example, if a machine driver passes a tx_mask with fewer set bits than the
total number of hardware channels (8) to map a subset of slots (e.g.,
tx_mask = 0x30 to use slots 4 and 5), this loop assigns those slots to the
first channels (ASPTX1 and ASPTX2). It terminates early without modifying the
remaining channels (ASPTX3..8), leaving them at their hardware default slots
(slots 2..7).

If DAPM later enables these unmapped channels (e.g., via the 'AMP Capture'
path), they will transmit on their default slots. In this example, ASPTX5
defaults to slot 4, which collides directly with the explicitly mapped ASPTX1
on slot 4.

> +}
> +
> +static int cs35l36_set_tdm_slot(struct snd_soc_dai *dai,
> +				unsigned int tx_mask, unsigned int rx_mask,
> +				int slots, int slot_width)
> +{
> +	struct cs35l36_private *cs35l36 =
> +			snd_soc_component_get_drvdata(dai->component);
> +
> +	/* Note: rx/tx is from point of view of the CPU end */
> +	if (!slots || !rx_mask)
> +		rx_mask = BIT(0);		/* ASPRX1 in slot 0 */
> +
> +	if (!slots || !tx_mask)
> +		tx_mask = GENMASK(7, 0);	/* ASPTX1..8 in slots 0..7 */
> +
> +	cs35l36_mask_to_slots(cs35l36, rx_mask, CS35L36_ASP_RX1_SLOT, 1);

[Severity: High]
Does configuring the RX TDM slot via CS35L36_ASP_RX1_SLOT alias an existing
ALSA user control?

The driver exposes bit 0 of this exact register to userspace as a DAPM audio
routing control ('Input Mux') here:

sound/soc/codecs/cs35l36.c:
    static SOC_ENUM_SINGLE_DECL(chansel_enum, CS35L36_ASP_RX1_SLOT, 0,
                                cs35l36_chan_text);

Because both the DAI configuration callback and an ALSA user control manage the
LSB of this register, will saving and restoring ALSA state (e.g., via
alsactl restore on boot) silently overwrite bit 0 of the TDM slot configured
by the machine driver?

For example, if the machine driver configures TDM slot 4 (binary 100), and
userspace restores 'RX2' (value 1), the slot will be corrupted to 5
(binary 101), breaking the machine driver's hardware configuration.

> +	cs35l36_mask_to_slots(cs35l36, tx_mask, CS35L36_ASP_TX1_TX2_SLOT, 8);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-pixel3-audio-v2-0-7c22ed915291@ixit.cz?part=5