Re: [PATCH v2 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: broken S20_LE/S24_LE constraints in
  sdca_asoc_set_constraints()
- [Medium] ASoC: SDCA: zero constraint mask in
  sdca_asoc_set_constraints() allows 1..32
--

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

ASoC: SDCA: Add port constraints into sdca_asoc_set_constraints()

This commit adds SoundWire port channel and word-length constraints to
sdca_asoc_set_constraints() for both Input and Output Terminals.

> diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c
> index be1b4c0075796..d1b40c6ccae2b 100644
> --- a/sound/soc/sdca/sdca_asoc.c
> +++ b/sound/soc/sdca/sdca_asoc.c
[ ... ]
> @@ -1507,9 +1510,57 @@ static int get_cluster_mask(struct device *dev, struct sdca_function_data *funct
>  	return 0;
>  }
>  
> +static int get_port_masks(struct sdw_slave *sdw, unsigned int port,
> +			  struct snd_pcm_substream *substream,
> +			  unsigned int *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 |= BIT(prop[i].words[j] - 1);
> +		} else {
> +			*word_mask = GENMASK(MIN(31, prop[i].max_word - 1),
> +					     MAX(1, prop[i].min_word) - 1);
> +		}
> +
> +		/* 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);
> +		}
> +
> +		return 0;
> +	}
> +
> +	return -ENODEV;
> +}
> +
>  /**
>   * sdca_asoc_set_constraints - constrain channels available on a DAI
>   * @dev: Pointer to the device, used for error messages.
> + * @sdw: Pointer to the SoundWire peripheral device.
>   * @regmap: Pointer to the Function register map.
>   * @function: Pointer to the Function information.
>   * @substream: Pointer to the PCM substream.
> @@ -1519,42 +1570,73 @@ static int get_cluster_mask(struct device *dev, struct sdca_function_data *funct
>   *
>   * Return: Returns zero on success, and a negative error code on failure.
>   */
> -int sdca_asoc_set_constraints(struct device *dev, struct regmap *regmap,
> +int sdca_asoc_set_constraints(struct device *dev, struct sdw_slave *sdw,
> +			      struct regmap *regmap,
>  			      struct sdca_function_data *function,
>  			      struct snd_pcm_substream *substream,
>  			      struct snd_soc_dai *dai)
>  {
> -	static const unsigned int channel_list[] = {
> +	static const unsigned int constraint_list[] = {
>  		 1,  2,  3,  4,  5,  6,  7,  8,  9, 10, 11, 12, 13, 14, 15, 16,
>  		17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32,
>  	};
>  	struct sdca_asoc_constraints *constraints = snd_soc_dai_to_priv(dai);
>  	struct sdca_entity *entity = &function->entities[dai->id];
> +	unsigned int port_ch_mask, port_width_mask;
>  	unsigned int cluster_ch_mask;
> +	int port;
>  	int ret;
>  
> -	static_assert(ARRAY_SIZE(channel_list) == SDCA_MAX_CHANNEL_COUNT);
> +	static_assert(ARRAY_SIZE(constraint_list) <= sizeof(port_width_mask) * BITS_PER_BYTE);
> +	static_assert(ARRAY_SIZE(constraint_list) == SDCA_MAX_CHANNEL_COUNT);
>  	static_assert(sizeof(cluster_ch_mask) * BITS_PER_BYTE >= SDCA_MAX_CHANNEL_COUNT);
>  
> -	if (entity->type != SDCA_ENTITY_TYPE_IT)
> -		return 0;
> +	/* FIXME: Currently only a single unshared port is supported */
> +	port = sdca_asoc_get_port(dev, regmap, function, dai);
> +	if (port < 0)
> +		return port;
>  
> -	ret = get_cluster_mask(dev, function, entity, &cluster_ch_mask);
> -	if (ret)
> +	ret = get_port_masks(sdw, port, substream, &port_width_mask, &port_ch_mask);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: no properties for port %d\n", entity->label, port);
>  		return ret;
> +	}
> +
> +	/* FIXME: OTs should inherit cluster information */
> +	if (entity->type == SDCA_ENTITY_TYPE_IT) {
> +		ret = get_cluster_mask(dev, function, entity, &cluster_ch_mask);
> +		if (ret)
> +			return ret;
> +	} else {
> +		cluster_ch_mask = UINT_MAX;
> +	}
> +
> +	constraints->channels.count = ARRAY_SIZE(constraint_list);
> +	constraints->channels.list = constraint_list;
> +	constraints->channels.mask = cluster_ch_mask & port_ch_mask;
>  
> -	dev_dbg(dev, "%s: set channel constraint mask: %#x\n",
> -		entity->label, cluster_ch_mask);
> +	constraints->width.count = ARRAY_SIZE(constraint_list);
> +	constraints->width.list = constraint_list;
> +	constraints->width.mask = port_width_mask;

[Severity: Medium]
What happens if cluster_ch_mask & port_ch_mask (or port_width_mask) is 0?

In snd_interval_list(), a mask of 0 means no bitmask filter is applied, so
passing a zero mask to snd_pcm_hw_constraint_list() allows all values
1..32 in constraint_list instead of rejecting the empty set.  For example,
in sound/soc/codecs/wcd9378-sdca.c, entity IT 131 uses Cluster 3
(num_channels = 3, cluster_ch_mask = 0x4) while its port DP7 (sink[1] in
wcd9378_sdca_read_prop()) has min_ch = 1 and max_ch = 1 (port_ch_mask =
0x1), resulting in constraints->channels.mask == 0 and leaving channels
1..32 unconstrained during PCM refinement.

>  
> -	constraints->channels.count = ARRAY_SIZE(channel_list);
> -	constraints->channels.list = channel_list;
> -	constraints->channels.mask = cluster_ch_mask;
> +	dev_dbg(dev, "%s: set constraint masks, channel: %#x, width: %#x\n",
> +		entity->label, constraints->channels.mask, constraints->width.mask);
>  
>  	ret = snd_pcm_hw_constraint_list(substream->runtime, 0,
>  					 SNDRV_PCM_HW_PARAM_CHANNELS,
>  					 &constraints->channels);
>  	if (ret) {
> -		dev_err(dev, "%s: failed to add constraint: %d\n", entity->label, ret);
> +		dev_err(dev, "%s: failed to add channel constraint: %d\n",
> +			entity->label, ret);
> +		return ret;
> +	}
> +
> +	ret = snd_pcm_hw_constraint_list(substream->runtime, 0,
> +					 SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
> +					 &constraints->width);

[Severity: High]
Does constraining SNDRV_PCM_HW_PARAM_SAMPLE_BITS with port_width_mask
break SNDRV_PCM_FORMAT_S20_LE and SNDRV_PCM_FORMAT_S24_LE?

In sound/core/pcm_native.c, snd_pcm_hw_rule_format() and
snd_pcm_hw_rule_sample_bits() relate SNDRV_PCM_HW_PARAM_FORMAT and
SNDRV_PCM_HW_PARAM_SAMPLE_BITS using snd_pcm_format_physical_width(),
which is 32 bits for both S20_LE and S24_LE (the formats populated by
width_find_mask()).  However, get_port_masks() builds port_width_mask from
SoundWire port word lengths, which represent the sample width (e.g. 16,
20, 24).

When a SoundWire port supports up to 24-bit words (for example max_word =
24 or words = { 16, 24 }), bit 31 (32 bits) is not set in port_width_mask,
so constraining SNDRV_PCM_HW_PARAM_SAMPLE_BITS eliminates S20_LE and
S24_LE.  Conversely, if a port supports 16-bit and 32-bit words but not
20-bit or 24-bit words, allowing 32 in SNDRV_PCM_HW_PARAM_SAMPLE_BITS
still permits S20_LE and S24_LE.

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