Re: [PATCH v3 3/3] ASoC: qcom: x1e80100: Allow userspace WSA RX chmap override

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] ASoC: qcom: x1e80100: channel map truncation in
  x1e80100_snd_build_wsa_slots()
- [Medium] ASoC: qcom: x1e80100: 6-channel WSA playback fails in
  AudioReach prepare
- [Medium] ASoC: qcom: x1e80100: TOCTOU race on wsa_chmap in
  x1e80100_snd_build_wsa_slots()
--

commit 6a0b312ac836c9b0665e198b3ecffb015e2de776
Author: Abel Vesa <abel.vesa@oss.qualcomm.com>

ASoC: qcom: x1e80100: Allow userspace WSA RX chmap override

This commit adds writable ALSA channel map controls for the WSA RX
backends so userspace can override the default Q6 slot mapping, and
extends the WSA channel count up to six channels.

> diff --git a/sound/soc/qcom/x1e80100.c b/sound/soc/qcom/x1e80100.c
> index c81df41ace883..6a91b3581c756 100644
> --- a/sound/soc/qcom/x1e80100.c
> +++ b/sound/soc/qcom/x1e80100.c
> @@ -15,14 +15,117 @@
>  #include "qdsp6/q6dsp-common.h"
>  #include "sdw.h"
>  
> +#define X1E80100_WSA_MAX_CHANNELS	6
> +#define X1E80100_WSA_PORTS		2
> +
>  struct x1e80100_snd_data {
>  	bool stream_prepared[AFE_PORT_MAX];
>  	struct snd_soc_card *card;
>  	struct snd_soc_jack jack;
>  	struct snd_soc_jack dp_jack[8];
>  	bool jack_setup;
> +	/* Userspace-selected SNDRV_CHMAP_* positions, 0 means unset */
> +	unsigned int wsa_chmap[X1E80100_WSA_PORTS][X1E80100_WSA_MAX_CHANNELS];
>  };

[ ... ]

> +static int x1e80100_wsa_chmap_ctl_put(struct snd_kcontrol *kcontrol,
> +				      struct snd_ctl_elem_value *ucontrol)
> +{
> +	unsigned int *map = x1e80100_wsa_chmap_from_kcontrol(kcontrol);
> +	bool changed = false;
> +	int i;
> +
> +	if (!map)
> +		return -EINVAL;
> +
> +	for (i = 0; i < X1E80100_WSA_MAX_CHANNELS; i++) {
> +		unsigned int pos = ucontrol->value.integer.value[i];
> +
> +		if (pos && qcom_snd_chmap_to_q6(pos) < 0)
> +			return -EINVAL;
> +
> +		if (map[i] != pos)
> +			changed = true;
> +	}
> +
> +	if (!changed)
> +		return 0;
> +
> +	for (i = 0; i < X1E80100_WSA_MAX_CHANNELS; i++)
> +		map[i] = ucontrol->value.integer.value[i];
> +
> +	return 1;
> +}

[ ... ]

> @@ -113,6 +218,14 @@ static int x1e80100_snd_hw_map_channels(unsigned int *ch_map, int num)
>  		ch_map[2] = PCM_CHANNEL_FR;
>  		ch_map[3] = PCM_CHANNEL_RB;
>  		break;
> +	case 6:
> +		ch_map[0] = PCM_CHANNEL_FL;
> +		ch_map[1] = PCM_CHANNEL_FR;
> +		ch_map[2] = PCM_CHANNEL_LB;
> +		ch_map[3] = PCM_CHANNEL_RB;
> +		ch_map[4] = PCM_CHANNEL_FC;
> +		ch_map[5] = PCM_CHANNEL_LFE;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}

[Severity: Medium]
Can a 6-channel WSA playback stream succeed when prepared through
AudioReach?

x1e80100_be_hw_params_fixup() leaves the channel count unchanged for
WSA_CODEC_DMA_RX_0 and WSA_CODEC_DMA_RX_1, so a 6-channel stream runs with
num_channels == 6 on both the frontend (q6apm_dai_prepare()) and backend
(q6apm_lpass_dai_prepare()).  However, the AudioReach media format helpers
reject anything above 4 channels with -EINVAL, and
audioreach_set_default_channel_mapping() only populates 1, 2 and 4
channels:

sound/soc/qcom/qdsp6/audioreach.c:audioreach_pcm_set_media_format() {
    ...
	if (num_channels > 4) {
		dev_err(graph->dev, "Error: Invalid channels (%d)!\n", num_channels);
		return -EINVAL;
	}
    ...
}

As a result, preparing a 6-channel WSA stream fails with -EINVAL in
audioreach_shmem_set_media_format() and audioreach_pcm_set_media_format().

> @@ -120,19 +233,56 @@ static int x1e80100_snd_hw_map_channels(unsigned int *ch_map, int num)
>  	return 0;
>  }
>  
> +static int x1e80100_snd_build_wsa_slots(struct x1e80100_snd_data *data,
> +					unsigned int dai_id,
> +					unsigned int channels,
> +					unsigned int *rx_slot)
> +{
> +	unsigned int i, set = 0;
> +	unsigned int *user;
> +	int idx;
> +
> +	idx = x1e80100_wsa_port_index(dai_id);
> +	if (idx < 0)
> +		return idx;
> +
> +	if (channels > X1E80100_WSA_MAX_CHANNELS)
> +		return -EINVAL;
> +
> +	user = data->wsa_chmap[idx];
> +	for (i = 0; i < channels; i++)
> +		if (user[i])
> +			set++;
> +
> +	/* Fall back to the default map unless userspace set all channels */
> +	if (set != channels)
> +		return x1e80100_snd_default_wsa_slots(rx_slot, channels);

[Severity: Medium]
Does x1e80100_snd_build_wsa_slots() truncate a wider userspace channel map
when a stream with fewer channels is prepared?

The loop only counts non-zero entries in user[0 .. channels - 1] and does
not check whether user[channels .. X1E80100_WSA_MAX_CHANNELS - 1] are also
set.  If UCM configures a 4-channel speaker map such as [FL, RL, FR, RR,
0, 0] at card init and a 2-channel stereo stream is later played, set is 2
(matching channels), so x1e80100_snd_build_wsa_slots() applies the first
two entries [FL, RL] instead of falling back to
x1e80100_snd_default_wsa_slots(), routing both stereo channels to the left
speakers.  Conversely, x1e80100_wsa_chmap_ctl_put() accepts sparse maps
with zeroed slots below X1E80100_WSA_MAX_CHANNELS that
x1e80100_snd_build_wsa_slots() then silently ignores.

> +
> +	for (i = 0; i < channels; i++) {
> +		int q6 = qcom_snd_chmap_to_q6(user[i]);
> +
> +		if (q6 < 0)
> +			return q6;
> +		rx_slot[i] = q6;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Can a concurrent control write race with x1e80100_snd_build_wsa_slots() on
data->wsa_chmap[idx] and cause x1e80100_snd_prepare() to fail with
-EINVAL?

x1e80100_wsa_chmap_ctl_put() updates data->wsa_chmap[idx] under
controls_rwsem, while x1e80100_snd_prepare() reads it under pcm_mutex
with no shared lock:

CPU0                                  CPU1
x1e80100_snd_prepare()
  x1e80100_snd_build_wsa_slots()
    for (i = 0; i < channels; i++)
      if (user[i])
        set++;
    /* set == channels */
                                      x1e80100_wsa_chmap_ctl_put()
                                        map[i] = 0;
    qcom_snd_chmap_to_q6(user[i])
      --> returns -EINVAL

Because user[i] is re-read in the second loop, clearing the channel map to
0 (unset) or updating it concurrently can make qcom_snd_chmap_to_q6(0)
abort PCM prepare with -EINVAL instead of falling back to
x1e80100_snd_default_wsa_slots(), or program a torn mix of old and new
channel positions.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-b4-sound-qcom-x1e80100-allow-ucm-channel-map-v3-0-29e06613115f@oss.qualcomm.com?part=3