Re: [PATCH v3 3/3] ASoC: SDCA: Add port constraints into sdca_asoc_set_constraints()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] ASoC: SDCA: zero word_mask in get_port_masks() when wordlengths
  are unset
- [Medium] ASoC: SDCA: 1-channel port_ch_mask in get_port_masks() when
  min_ch/max_ch are 0
--

commit 8d09b518756cef71f9c60a48b6568b96a8c9556b
Author: Charles Keepax <ckeepax@opensource.cirrus.com>

ASoC: SDCA: Add port constraints into sdca_asoc_set_constraints()

This commit updates sdca_asoc_set_constraints() to apply channel count and
sample format constraints derived from the SoundWire data port properties.

> diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c
> index be1b4c0075796..ccbb2c4b10267 100644
> --- a/sound/soc/sdca/sdca_asoc.c
> +++ b/sound/soc/sdca/sdca_asoc.c

[ ... ]

> @@ -1507,9 +1510,86 @@ static int get_cluster_mask(struct device *dev, struct sdca_function_data *funct
>  	return 0;
>  }
>  
> +static u64 get_word_mask(unsigned int width)
> +{
> +	switch (width) {
> +	case 8:
> +		return SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_U8;
> +	case 16:
> +		return SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |
> +		       SNDRV_PCM_FMTBIT_U16_LE | SNDRV_PCM_FMTBIT_U16_BE;
> +	case 18:
> +		return SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_U18_3LE |
> +		       SNDRV_PCM_FMTBIT_S18_3BE | SNDRV_PCM_FMTBIT_U18_3BE;
> +	case 20:
> +		return SNDRV_PCM_FMTBIT_S20_LE | SNDRV_PCM_FMTBIT_U20_LE |
> +		       SNDRV_PCM_FMTBIT_S20_BE | SNDRV_PCM_FMTBIT_U20_BE |
> +		       SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_U20_3LE |
> +		       SNDRV_PCM_FMTBIT_S20_3BE | SNDRV_PCM_FMTBIT_U20_3BE;
> +	case 24:
> +		return SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |
> +		       SNDRV_PCM_FMTBIT_U24_LE | SNDRV_PCM_FMTBIT_U24_BE |
> +		       SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_U24_3LE |
> +		       SNDRV_PCM_FMTBIT_S24_3BE | SNDRV_PCM_FMTBIT_U24_3BE;
> +	case 32:
> +		return SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE |
> +		       SNDRV_PCM_FMTBIT_U32_LE | SNDRV_PCM_FMTBIT_U32_BE;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int get_port_masks(struct sdw_slave *sdw, unsigned int port,
> +			  struct snd_pcm_substream *substream,
> +			  u64 *word_mask, unsigned int *ch_mask)
> +{
> +	struct sdw_dpn_prop *prop;
> +	unsigned int nprop;
> +	int i, j;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		prop = sdw->prop.sink_dpn_prop;
> +		nprop = hweight32(sdw->prop.sink_ports);
> +	} else {
> +		prop = sdw->prop.src_dpn_prop;
> +		nprop = hweight32(sdw->prop.source_ports);
> +	}
> +
> +	*word_mask = 0;
> +	*ch_mask = 0;
> +
> +	for (i = 0; i < nprop; i++) {
> +		if (prop[i].num != port)
> +			continue;
> +
> +		if (prop[i].num_words) {
> +			for (j = 0; j < prop[i].num_words; j++)
> +				*word_mask |= get_word_mask(prop[i].words[j]);
> +		} else {
> +			for (j = prop[i].min_word; j <= prop[i].max_word; j++)
> +				*word_mask |= get_word_mask(j);
> +		}

[Severity: High]
Does get_port_masks() leave *word_mask as 0 when a port does not specify
wordlength properties?

The MIPI DisCo wordlength properties are optional, and drivers such as
wcd9378_sdca_read_prop() leave num_words, min_word, and max_word as 0.
When all three are 0, the else loop runs once with j = 0, get_word_mask(0)
returns 0, and sdca_asoc_set_constraints() fails PCM startup with -EINVAL
because constraints->width is 0.

> +
> +		/* Only handle the number of channels for constraints. */
> +		if (prop[i].num_ch_combinations) {
> +			for (j = 0; j < prop[i].num_ch_combinations; j++)
> +				*ch_mask |= BIT(hweight32(prop[i].ch_combinations[j]) - 1);
> +		} else if (prop[i].num_channels) {
> +			*ch_mask = GENMASK(prop[i].num_channels - 1, 0);
> +		} else {
> +			*ch_mask = GENMASK(prop[i].max_ch - prop[i].min_ch, 0);
> +		}

[Severity: Medium]
As reported by Mark Brown on v2 [1], does this restrict *ch_mask to a
single channel (GENMASK(0, 0) == 0x1) when the optional channel properties
(num_ch_combinations, num_channels, max_ch, and min_ch) are all 0?  That
constrains the DAI to 1 channel instead of leaving the port channel count
unconstrained, and causes sdca_asoc_set_constraints() to fail with -EINVAL
if the terminal cluster only supports multi-channel configurations.

[1] https://lore.kernel.org/r/179413707633.3630016.13654136728294242171.b4-review@b4

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109155106.3727597-1-ckeepax@opensource.cirrus.com?part=3