[PATCH 0/2] Add SoundWire port constraints into SDCA stream

[PATCH 0/2] Add SoundWire port constraints into SDCA stream

[Charles Keepax]

SDCA adds constraints onto the DAI based on the DisCo properties for the
Input/Output Terminal. As the SoundWire port for a Terminal can be
selected it is possible for the SoundWire port to support fewer options
than the Terminal. These patches add the SoundWire port properties into
the constraints for the DAI startup to cover these situations.

Charles Keepax (2):
  ASoC: SDCA: Factor out helper for creating mask of Cluster channels
  ASoC: SDCA: Add port constraints into sdca_asoc_set_constraints()

 include/sound/sdca_asoc.h            |   4 +-
 sound/soc/sdca/sdca_asoc.c           | 176 ++++++++++++++++++++-------
 sound/soc/sdca/sdca_class_function.c |   3 +-
 3 files changed, 140 insertions(+), 43 deletions(-)

-- 
2.47.3

[PATCH 1/2] ASoC: SDCA: Factor out helper for creating mask of Cluster channels

[Charles Keepax]

Factor out the code that builds a mask of the allowed channels from the
Cluster information. This is slightly cleaner and prepares for
additional constraints to be added to sdca_asoc_set_constraints().

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/sdca/sdca_asoc.c | 64 +++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 25 deletions(-)

diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c
index ccc579f878ff1..b801cb92cc3db 100644
--- a/sound/soc/sdca/sdca_asoc.c
+++ b/sound/soc/sdca/sdca_asoc.c
@@ -1434,6 +1434,37 @@ int sdca_asoc_populate_component(struct device *dev,
 }
 EXPORT_SYMBOL_NS(sdca_asoc_populate_component, "SND_SOC_SDCA");
 
+static int get_cluster_mask(struct device *dev, struct sdca_function_data *function,
+			    struct sdca_entity *entity, unsigned int *ch_mask)
+{
+	struct sdca_control_range *range;
+	struct sdca_control *control;
+	int i;
+
+	control = sdca_selector_find_control(dev, entity, SDCA_CTL_IT_CLUSTERINDEX);
+	if (!control)
+		return -EINVAL;
+
+	range = sdca_control_find_range(dev, entity, control, SDCA_CLUSTER_NCOLS, 0);
+	if (!range)
+		return -EINVAL;
+
+	*ch_mask = 0;
+
+	for (i = 0; i < range->rows; i++) {
+		int clusterid = sdca_range(range, SDCA_CLUSTER_CLUSTERID, i);
+		struct sdca_cluster *cluster;
+
+		cluster = sdca_id_find_cluster(dev, function, clusterid);
+		if (!cluster)
+			return -ENODEV;
+
+		*ch_mask |= (1 << (cluster->num_channels - 1));
+	}
+
+	return 0;
+}
+
 /**
  * sdca_asoc_set_constraints - constrain channels available on a DAI
  * @dev: Pointer to the device, used for error messages.
@@ -1457,38 +1488,21 @@ int sdca_asoc_set_constraints(struct device *dev, struct regmap *regmap,
 	};
 	struct sdca_entity *entity = &function->entities[dai->id];
 	struct snd_pcm_hw_constraint_list *constraint;
-	struct sdca_control_range *range;
-	struct sdca_control *control;
-	unsigned int channel_mask = 0;
-	int i, ret;
+	unsigned int cluster_ch_mask;
+	int ret;
 
 	static_assert(ARRAY_SIZE(channel_list) == SDCA_MAX_CHANNEL_COUNT);
-	static_assert(sizeof(channel_mask) * BITS_PER_BYTE >= 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;
 
-	control = sdca_selector_find_control(dev, entity, SDCA_CTL_IT_CLUSTERINDEX);
-	if (!control)
-		return -EINVAL;
-
-	range = sdca_control_find_range(dev, entity, control, SDCA_CLUSTER_NCOLS, 0);
-	if (!range)
-		return -EINVAL;
-
-	for (i = 0; i < range->rows; i++) {
-		int clusterid = sdca_range(range, SDCA_CLUSTER_CLUSTERID, i);
-		struct sdca_cluster *cluster;
-
-		cluster = sdca_id_find_cluster(dev, function, clusterid);
-		if (!cluster)
-			return -ENODEV;
-
-		channel_mask |= (1 << (cluster->num_channels - 1));
-	}
+	ret = get_cluster_mask(dev, function, entity, &cluster_ch_mask);
+	if (ret)
+		return ret;
 
 	dev_dbg(dev, "%s: set channel constraint mask: %#x\n",
-		entity->label, channel_mask);
+		entity->label, cluster_ch_mask);
 
 	constraint = kzalloc_obj(*constraint);
 	if (!constraint)
@@ -1496,7 +1510,7 @@ int sdca_asoc_set_constraints(struct device *dev, struct regmap *regmap,
 
 	constraint->count = ARRAY_SIZE(channel_list);
 	constraint->list = channel_list;
-	constraint->mask = channel_mask;
+	constraint->mask = cluster_ch_mask;
 
 	ret = snd_pcm_hw_constraint_list(substream->runtime, 0,
 					 SNDRV_PCM_HW_PARAM_CHANNELS,
-- 
2.47.3

[PATCH 2/2] ASoC: SDCA: Add port constraints into sdca_asoc_set_constraints()

[Charles Keepax]

Currently the SDCA code constrains the params based on the DisCo for
the SDCA Terminal used. However, it ignores the properties of the
SoundWire port, which in some cases may further constrain the allowed
params. Add additional constraints to apply these restrictions.

Some minor additional refactoring is required since port constraints
should be applied for both Input and Output Terminals, whereas
currently Cluster constraints are only handled for Input Terminals.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/sdca_asoc.h            |   4 +-
 sound/soc/sdca/sdca_asoc.c           | 122 ++++++++++++++++++++++-----
 sound/soc/sdca/sdca_class_function.c |   3 +-
 3 files changed, 106 insertions(+), 23 deletions(-)

diff --git a/include/sound/sdca_asoc.h b/include/sound/sdca_asoc.h
index 599a72f335153..965ca5ce06a77 100644
--- a/include/sound/sdca_asoc.h
+++ b/include/sound/sdca_asoc.h
@@ -12,6 +12,7 @@
 
 struct device;
 struct regmap;
+struct sdw_slave;
 struct sdca_function_data;
 struct sdca_pde_delay;
 struct snd_ctl_elem_value;
@@ -89,7 +90,8 @@ int sdca_asoc_populate_rate_format(struct device *dev,
 				struct sdca_entity *entity,
 				struct snd_soc_pcm_stream *stream);
 
-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);
diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c
index b801cb92cc3db..4a96ec5d11743 100644
--- a/sound/soc/sdca/sdca_asoc.c
+++ b/sound/soc/sdca/sdca_asoc.c
@@ -13,11 +13,13 @@
 #include <linux/delay.h>
 #include <linux/dev_printk.h>
 #include <linux/device.h>
+#include <linux/limits.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/overflow.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
+#include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_registers.h>
 #include <linux/string_helpers.h>
 #include <linux/types.h>
@@ -1465,9 +1467,56 @@ 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->min_word && prop->max_word) {
+			*word_mask = GENMASK(prop->min_word - 1, prop->max_word - 1);
+		} else {
+			for (j = 0; j < prop->num_words; j++)
+				*word_mask |= (1 << (prop->words[j] - 1));
+		}
+
+		/* Only handle the number of channels for constraints. */
+		if (prop->num_ch_combinations) {
+			for (j = 0; j < prop->num_ch_combinations; j++)
+				*ch_mask |= (1 << (hweight32(prop->ch_combinations[j]) - 1));
+		} else if (prop->min_ch && prop->max_ch) {
+			*ch_mask = GENMASK(prop->min_ch - 1, prop->max_ch - 1);
+		} else {
+			*ch_mask = GENMASK(0, prop->num_channels - 1);
+		}
+
+		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.
@@ -1477,51 +1526,82 @@ 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_entity *entity = &function->entities[dai->id];
-	struct snd_pcm_hw_constraint_list *constraint;
+	struct snd_pcm_hw_constraint_list *constraints;
+	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) == 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;
+	}
 
-	dev_dbg(dev, "%s: set channel constraint mask: %#x\n",
-		entity->label, cluster_ch_mask);
+	/* 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;
+	}
 
-	constraint = kzalloc_obj(*constraint);
-	if (!constraint)
+	constraints = kzalloc_objs(*constraints, 2);
+	if (!constraints)
 		return -ENOMEM;
 
-	constraint->count = ARRAY_SIZE(channel_list);
-	constraint->list = channel_list;
-	constraint->mask = cluster_ch_mask;
+	constraints[0].count = ARRAY_SIZE(constraint_list);
+	constraints[0].list = constraint_list;
+	constraints[0].mask = cluster_ch_mask & port_ch_mask;
+
+	constraints[1].count = ARRAY_SIZE(constraint_list);
+	constraints[1].list = constraint_list;
+	constraints[1].mask = port_width_mask;
+
+	dev_dbg(dev, "%s: set constraint masks, channel: %#x, width: %#x\n",
+		entity->label, constraints[0].mask, constraints[1].mask);
 
 	ret = snd_pcm_hw_constraint_list(substream->runtime, 0,
 					 SNDRV_PCM_HW_PARAM_CHANNELS,
-					 constraint);
+					 &constraints[0]);
 	if (ret) {
-		dev_err(dev, "%s: failed to add constraint: %d\n", entity->label, ret);
-		kfree(constraint);
+		dev_err(dev, "%s: failed to add channel constraint: %d\n",
+			entity->label, ret);
+		kfree(constraints);
+		return ret;
+	}
+
+	ret = snd_pcm_hw_constraint_list(substream->runtime, 0,
+					 SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
+					 &constraints[1]);
+	if (ret) {
+		dev_err(dev, "%s: failed to add word constraint: %d\n",
+			entity->label, ret);
+		kfree(constraints);
 		return ret;
 	}
 
-	snd_soc_dai_set_priv(dai, constraint);
+	snd_soc_dai_set_priv(dai, constraints);
 
 	return 0;
 }
@@ -1537,9 +1617,9 @@ EXPORT_SYMBOL_NS(sdca_asoc_set_constraints, "SND_SOC_SDCA");
 void sdca_asoc_free_constraints(struct snd_pcm_substream *substream,
 				struct snd_soc_dai *dai)
 {
-	struct snd_pcm_hw_constraint_list *constraint = snd_soc_dai_to_priv(dai);
+	struct snd_pcm_hw_constraint_list *constraints = snd_soc_dai_to_priv(dai);
 
-	kfree(constraint);
+	kfree(constraints);
 }
 EXPORT_SYMBOL_NS(sdca_asoc_free_constraints, "SND_SOC_SDCA");
 
diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
index 13b70f0f0dbf7..d7fba834601e6 100644
--- a/sound/soc/sdca/sdca_class_function.c
+++ b/sound/soc/sdca/sdca_class_function.c
@@ -121,8 +121,9 @@ static int class_function_startup(struct snd_pcm_substream *substream,
 				  struct snd_soc_dai *dai)
 {
 	struct class_function_drv *drv = snd_soc_component_get_drvdata(dai->component);
+	struct sdw_slave *sdw = dev_to_sdw_dev(drv->dev->parent);
 
-	return sdca_asoc_set_constraints(drv->dev, drv->regmap, drv->function,
+	return sdca_asoc_set_constraints(drv->dev, sdw, drv->regmap, drv->function,
 					 substream, dai);
 }
 
-- 
2.47.3

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] ASoC: SDCA: wrong array index in get_port_masks()
- [High] ASoC: SDCA: reversed GENMASK() arguments in get_port_masks()
- [High] ASoC: SDCA: u32 underflow and zeroed masks in get_port_masks()
- [High] ASoC: SDCA: use-after-free of hw_constraint_list in sdca_asoc_set_constraints()
- [Medium] ASoC: SDCA: zero channel mask in sdca_asoc_set_constraints() removes constraints
--

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

ASoC: SDCA: Add port constraints into sdca_asoc_set_constraints()

This commit adds SoundWire data port channel count and sample bit width
constraints into 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 b801cb92cc3db..4a96ec5d11743 100644
> --- a/sound/soc/sdca/sdca_asoc.c
> +++ b/sound/soc/sdca/sdca_asoc.c

[ ... ]

> @@ -1465,9 +1467,56 @@ 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->min_word && prop->max_word) {
> +			*word_mask = GENMASK(prop->min_word - 1, prop->max_word - 1);
> +		} else {
> +			for (j = 0; j < prop->num_words; j++)
> +				*word_mask |= (1 << (prop->words[j] - 1));
> +		}
> +
> +		/* Only handle the number of channels for constraints. */
> +		if (prop->num_ch_combinations) {
> +			for (j = 0; j < prop->num_ch_combinations; j++)
> +				*ch_mask |= (1 << (hweight32(prop->ch_combinations[j]) - 1));
> +		} else if (prop->min_ch && prop->max_ch) {
> +			*ch_mask = GENMASK(prop->min_ch - 1, prop->max_ch - 1);
> +		} else {
> +			*ch_mask = GENMASK(0, prop->num_channels - 1);
> +		}

[Severity: High]
In get_port_masks(), after matching prop[i].num == port, all of the
subsequent field accesses dereference prop (i.e. prop[0]) instead of
prop[i].

When a peripheral has multiple SoundWire data ports and the matched port
is at index i > 0 of sink_dpn_prop or src_dpn_prop, won't this compute
word_mask and ch_mask from the first port's properties rather than the
matched port?

[Severity: High]
Are the arguments to GENMASK() reversed in get_port_masks()?

GENMASK(h, l) takes the high bit position first and the low bit position
second:

  GENMASK(prop[i].max_word - 1, prop[i].min_word - 1)
  GENMASK(prop[i].max_ch - 1, prop[i].min_ch - 1)
  GENMASK(prop[i].num_channels - 1, 0)

With the low bit passed as h and the high bit passed as l, whenever
min_word < max_word, min_ch < max_ch, or num_channels > 1, the shift masks
have no overlapping bits and evaluate to 0.

[Severity: High]
What happens in get_port_masks() when optional data port channel or
wordlength properties are omitted?

In MIPI DisCo for SoundWire (sdw_slave_read_dpn()) and in SDCA peripheral
drivers such as wcd9378_sdca_read_prop(), data port channel properties
(num_ch_combinations, min_ch/max_ch, num_channels) and wordlength
properties (min_word/max_word, num_words) are optional.

When num_ch_combinations, min_ch/max_ch, and num_channels are all 0,
prop->num_channels - 1 underflows u32 to 0xffffffff inside GENMASK(),
causing a shift-out-of-bounds and leaving *ch_mask as 0. That zero
port_ch_mask then zeroes out cluster_ch_mask & port_ch_mask in
sdca_asoc_set_constraints().

> +
> +		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.
> @@ -1477,51 +1526,82 @@ 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_entity *entity = &function->entities[dai->id];
> -	struct snd_pcm_hw_constraint_list *constraint;
> +	struct snd_pcm_hw_constraint_list *constraints;
> +	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) == 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;
> +	}
>  
> -	dev_dbg(dev, "%s: set channel constraint mask: %#x\n",
> -		entity->label, cluster_ch_mask);
> +	/* 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;
> +	}
>  
> -	constraint = kzalloc_obj(*constraint);
> -	if (!constraint)
> +	constraints = kzalloc_objs(*constraints, 2);
> +	if (!constraints)
>  		return -ENOMEM;
>  
> -	constraint->count = ARRAY_SIZE(channel_list);
> -	constraint->list = channel_list;
> -	constraint->mask = cluster_ch_mask;
> +	constraints[0].count = ARRAY_SIZE(constraint_list);
> +	constraints[0].list = constraint_list;
> +	constraints[0].mask = cluster_ch_mask & port_ch_mask;

[Severity: Medium]
If cluster_ch_mask & port_ch_mask evaluates to 0 in
sdca_asoc_set_constraints() (for example, if the cluster and port channel
masks have no common channel counts), constraints[0].mask is set to 0 and
passed to snd_pcm_hw_constraint_list().

In sound/core/pcm_lib.c, snd_interval_list() treats mask == 0 as meaning no
mask filter should be applied:

	for (k = 0; k < count; k++) {
		if (mask && !(mask & (1 << k)))
			continue;

Won't a zero mask cause snd_interval_list() to permit all 1..32 channels in
constraint_list rather than rejecting the empty intersection?

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

[Severity: High]
Can freeing constraints here (or in sdca_asoc_free_constraints()) leave a
dangling pointer in substream->runtime->hw_constraints.rules?

When the first snd_pcm_hw_constraint_list() call succeeds,
snd_pcm_hw_rule_add() stores &constraints[0] in rule->private inside
substream->runtime->hw_constraints.rules.

If the second snd_pcm_hw_constraint_list() call fails, kfree(constraints)
frees the array while &constraints[0] remains registered in
runtime->hw_constraints.rules. Similarly, on DPCM SoundWire setups where
dpcm_be_dai_startup() aliases be_substream->runtime = fe_substream->runtime,
if a backend startup rolls back or the backend shuts down via
dpcm_be_dai_shutdown() -> sdca_asoc_free_constraints() while the frontend
substream remains open, subsequent hw_refine or hw_params calls on the
frontend will invoke snd_pcm_hw_rule_list() and dereference the freed
constraints memory.

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

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

[Charles Keepax]

On Thu, Nov 05, 2026 at 01:49:52PM +0000, Charles Keepax wrote:
> Currently the SDCA code constrains the params based on the DisCo for
> the SDCA Terminal used. However, it ignores the properties of the
> SoundWire port, which in some cases may further constrain the allowed
> params. Add additional constraints to apply these restrictions.
> 
> Some minor additional refactoring is required since port constraints
> should be applied for both Input and Output Terminals, whereas
> currently Cluster constraints are only handled for Input Terminals.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---

Some Sashiko comments to fix up on this one too.

Thanks,
Charles

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

[Pierre-Louis Bossart]

> +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);
> +	}

The definition of those DisCo properties was done with separate masks
for source and sinks, but it's allowed to have the same bit set for
source and sink if the port supports bi-directional transfers.

I don't think it impacts the way the constraints are added, since you
use substream->stream direction to disambiguate, but I thought I'd ask
the question.

> -	if (entity->type != SDCA_ENTITY_TYPE_IT)
> -		return 0;
> +	/* FIXME: Currently only a single unshared port is supported */

I must admit I have no idea how we'd handle ports that can be connected
to multiple terminals. It's allowed in the spec, even between Functions
which are then no longer independent, and I was told it was due to
hardware restrictions, but still I hope we never have to deal with this...

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

[Charles Keepax]

On Thu, Nov 05, 2026 at 08:49:43PM +0100, Pierre-Louis Bossart wrote:
> 
> > +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);
> > +	}
> 
> The definition of those DisCo properties was done with separate masks
> for source and sinks, but it's allowed to have the same bit set for
> source and sink if the port supports bi-directional transfers.
> 
> I don't think it impacts the way the constraints are added, since you
> use substream->stream direction to disambiguate, but I thought I'd ask
> the question.

Yeah I think this mostly all works out. ALSA separates the capture
and playback streams, and this stuff is called from the DAI ops.

> 
> > -	if (entity->type != SDCA_ENTITY_TYPE_IT)
> > -		return 0;
> > +	/* FIXME: Currently only a single unshared port is supported */
> 
> I must admit I have no idea how we'd handle ports that can be connected
> to multiple terminals. It's allowed in the spec, even between Functions
> which are then no longer independent, and I was told it was due to
> hardware restrictions, but still I hope we never have to deal with this...

lol yeah me too. For now I am mostly just putting these FIXME's
in at all the places where this is going to become relevant. I
must confess I haven't reviewed this part of the spec in detail.
It feels like sharing a couple of ports for a single terminal is
probably tractable if a little annoying. But yeah, if we got as
far as sharing a port across functions that seems like a total
nightmare. We would probably need to do a bit of re-architecting
and add a port abstraction at the device level, but hopefully
people are wise enough to not build hardware like that.

Thanks,
Charles

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

[Pierre-Louis Bossart]

>>> -	if (entity->type != SDCA_ENTITY_TYPE_IT)
>>> -		return 0;
>>> +	/* FIXME: Currently only a single unshared port is supported */
>>
>> I must admit I have no idea how we'd handle ports that can be connected
>> to multiple terminals. It's allowed in the spec, even between Functions
>> which are then no longer independent, and I was told it was due to
>> hardware restrictions, but still I hope we never have to deal with this...
> 
> lol yeah me too. For now I am mostly just putting these FIXME's
> in at all the places where this is going to become relevant. I
> must confess I haven't reviewed this part of the spec in detail.
> It feels like sharing a couple of ports for a single terminal is
> probably tractable if a little annoying. But yeah, if we got as
> far as sharing a port across functions that seems like a total
> nightmare. We would probably need to do a bit of re-architecting
> and add a port abstraction at the device level, but hopefully
> people are wise enough to not build hardware like that.
I re-read the spec and I think the main usage for the Terminal to ports
mapping is bandwidth management, possibly across multiple lanes. But
that's a 1:N terminal to port mapping, not M:1 or M:N. It's complicated
but not earth-shattering. Also the mapping is supposed to be pre-made in
DisCo tables, not something that requires much intelligence in drivers -
until the tables are wrong but that's a different story.

I didn't see any wording on sharing ports across functions, I am 100%
sure this was talked about in the WG but maybe it didn't make it in the
final 1.0 version. That said it's not described as forbidden either so a
well-intended creative hardware designer will probably make that
"optimization" at some point...