Re: [PATCH v2 v2 2/2] soundwire: qcom: Add set_channel_map support to SWR master DAI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] soundwire: qcom: lock ordering inversion in qcom_swrm_port_enable()
- [High] soundwire: qcom: zero slot masks in qcom_swrm_set_channel_map() clobber ports
- [High] soundwire: qcom: single-direction qcom_swrm_set_channel_map() clobbers other direction
--

commit c509f8b3c2506c16cc2d00c47ddc8257189902cf
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

soundwire: qcom: Add set_channel_map support to SWR master DAI

This commit adds .set_channel_map support to the Qualcomm SoundWire master
DAI so the ASoC layer can configure per-port TX and RX channel masks used by
qcom_swrm_port_enable().

> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 147e55b7a600b..c411c3628dee4 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c

[ ... ]

> @@ -1188,12 +1192,28 @@ static int qcom_swrm_port_enable(struct sdw_bus *bus,
>  {
>  	u32 reg;
>  	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	struct qcom_swrm_port_config *pcfg;
>  	u32 val;
>  	u32 offset = ctrl->reg_layout[SWRM_OFFSET_DP_PORT_CTRL_BANK];
>  
>  	reg = SWRM_DPn_PORT_CTRL_BANK(offset, enable_ch->port_num, bank);
>  
>  	ctrl->reg_read(ctrl, reg, &val);
> +	pcfg = &ctrl->pconfig[enable_ch->port_num];
> +
> +	mutex_lock(&ctrl->port_lock);

[Severity: High]
Does acquiring ctrl->port_lock inside qcom_swrm_port_enable() cause a lock
ordering inversion with ctrl->bus.bus_lock?

During hw_params, qcom_swrm_hw_params() -> qcom_swrm_stream_alloc_ports()
acquires ctrl->port_lock and then calls sdw_stream_add_master(), which locks
bus->bus_lock:

drivers/soundwire/qcom.c:qcom_swrm_stream_alloc_ports() {
	guard(mutex)(&ctrl->port_lock);
	...
	sdw_stream_add_master(&ctrl->bus, &sconfig, pconfig,
			      nports, stream);
}

During stream enable and disable, sdw_enable_stream() and sdw_disable_stream()
acquire bus->bus_lock via sdw_acquire_bus_lock() before calling into
bus->port_ops->dpn_port_enable_ch(), which invokes qcom_swrm_port_enable() and
acquires ctrl->port_lock in the reverse order (bus->bus_lock ->
ctrl->port_lock).

> +	if (enable_ch->port_num <= ctrl->num_dout_ports) {
> +		if (pcfg->rx_ch_map_valid)
> +			enable_ch->ch_mask = pcfg->rx_ch_mask;
> +		else if (pcfg->tx_ch_map_valid)
> +			enable_ch->ch_mask = pcfg->tx_ch_mask;
> +	} else {
> +		if (pcfg->tx_ch_map_valid)
> +			enable_ch->ch_mask = pcfg->tx_ch_mask;
> +		else if (pcfg->rx_ch_map_valid)
> +			enable_ch->ch_mask = pcfg->rx_ch_mask;
> +	}
> +	mutex_unlock(&ctrl->port_lock);
>  
>  	if (enable_ch->enable)
>  		val |= (enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
> @@ -1413,6 +1433,39 @@ static void *qcom_swrm_get_sdw_stream(struct snd_soc_dai *dai, int direction)
>  	return ctrl->sruntime[dai->id];
>  }
>  
> +static int qcom_swrm_set_channel_map(struct snd_soc_dai *dai,
> +				     unsigned int tx_num, const unsigned int *tx_slot,
> +				     unsigned int rx_num, const unsigned int *rx_slot)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> +	unsigned int max_ports = ctrl->nports;
> +	unsigned int i;
> +
> +	if ((tx_num && !tx_slot) || (rx_num && !rx_slot))
> +		return -EINVAL;
> +
> +	mutex_lock(&ctrl->port_lock);
> +	for (i = 1; i <= max_ports; i++) {
> +		ctrl->pconfig[i].tx_ch_map_valid = false;
> +		ctrl->pconfig[i].rx_ch_map_valid = false;

[Severity: High]
As reported by Sashiko on v1 [1], when qcom_swrm_set_channel_map() is called
for a single stream direction, does unconditionally resetting both
tx_ch_map_valid and rx_ch_map_valid to false clobber the opposite direction's
channel map?

In qcom_snd_sdw_startup(), playback calls snd_soc_dai_set_channel_map() with
tx_num = 0 and rx_num = SDW_MAX_PORTS, whereas capture calls it with
tx_num = SDW_MAX_PORTS and rx_num = 0. Because all DAIs on the controller
share ctrl->pconfig[], starting a playback stream clears tx_ch_map_valid across
all ports and sets rx_ch_map_valid = true, while starting a capture stream
clears rx_ch_map_valid and sets tx_ch_map_valid = true.

In addition, because qcom_swrm_port_enable() falls back to the opposite
direction's mask when a port's own direction flag is false:

	if (enable_ch->port_num <= ctrl->num_dout_ports) {
		if (pcfg->rx_ch_map_valid)
			enable_ch->ch_mask = pcfg->rx_ch_mask;
		else if (pcfg->tx_ch_map_valid)
			enable_ch->ch_mask = pcfg->tx_ch_mask;
	} else {
		if (pcfg->tx_ch_map_valid)
			enable_ch->ch_mask = pcfg->tx_ch_mask;
		else if (pcfg->rx_ch_map_valid)
			enable_ch->ch_mask = pcfg->rx_ch_mask;
	}

won't DIN (TX) ports end up using pcfg->rx_ch_mask after a playback
set_channel_map() call, and DOUT (RX) ports end up using pcfg->tx_ch_mask
after a capture set_channel_map() call?

> +
> +		/* TX setup */
> +		if (tx_slot && i < tx_num) {
> +			ctrl->pconfig[i].tx_ch_mask = tx_slot[i];
> +			ctrl->pconfig[i].tx_ch_map_valid = true;
> +		}
> +
> +		/* RX setup */
> +		if (rx_slot && i < rx_num) {
> +			ctrl->pconfig[i].rx_ch_mask = rx_slot[i];
> +			ctrl->pconfig[i].rx_ch_map_valid = true;
> +		}

[Severity: High]
As reported by Sashiko on v1 [2], if tx_slot[i] or rx_slot[i] is 0 for an
unused or unmapped port in the sparse channel map array, will marking
tx_ch_map_valid or rx_ch_map_valid as true cause qcom_swrm_port_enable() to
override enable_ch->ch_mask with 0?

Codec callbacks such as wcd937x_get_channel_map() and pm4125_get_channel_map()
populate a 15-element (SDW_MAX_PORTS) sparse array where unmapped or inactive
master ports have slot[i] == 0:

sound/soc/codecs/wcd937x.c:wcd937x_get_channel_map() {
	for (i = 0; i < SDW_MAX_PORTS; i++)
		rx_slot[i] = wcd->master_channel_map[i];

	*rx_num = i;
}

Since max_ports <= 14 < SDW_MAX_PORTS, i < tx_num or i < rx_num is true for
every port 1..max_ports. Any port whose slot entry is 0 gets tx_ch_mask = 0 or
rx_ch_mask = 0 with the valid flag set to true, so qcom_swrm_port_enable() will
overwrite the stream-provided enable_ch->ch_mask with 0 instead of falling
back to the stream's channel mask.

[1] https://lore.kernel.org/r/20260629141633.86657-3-mohammad.rafi.shaik@oss.qualcomm.com
[2] https://lore.kernel.org/r/20260629141633.86657-3-mohammad.rafi.shaik@oss.qualcomm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-soundwire_channel_map-v2-0-5efe2021552a@oss.qualcomm.com?part=2