Re: [PATCH RFC v2 3/6] ASoC: qcom: sdm845: Use per-speaker RX masks for TDM slot assignment

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Potential NULL pointer dereference when calling strcmp() on an unchecked codec_dai->component->name_prefix.
- [High] Improper reuse of the speaker playback mask (`SPK_TDM_RX_MASK`) for the CPU capture mask, causing dropped speaker feedback.
--

commit fc9c3ae11318fad7d402ab1b0d3e9652106e6e97
Author: David Heidelberg <david@ixit.cz>

ASoC: qcom: sdm845: Use per-speaker RX masks for TDM slot assignment
    
Both Left and Right codec DAIs were passing the same SPK_TDM_RX_MASK,
both speakers ended up on slot 0, breaking the one speaker in
configuration such as on Pixel 3.

> diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c
> index d1cc825fc5cdc..59cfa3b26cd4d 100644
> --- a/sound/soc/qcom/sdm845.c
> +++ b/sound/soc/qcom/sdm845.c
> @@ -157,7 +157,7 @@ static int sdm845_tdm_snd_hw_params(struct snd_pcm_substream *substream,
>  		if (!strcmp(codec_dai->component->name_prefix, "Left")) {

[Severity: High]
This isn't a regression introduced by this patch, but can this cause a kernel
panic if a device tree is missing the optional sound-name-prefix property?

If a local user opens the ALSA PCM device, it triggers
sdm845_tdm_snd_hw_params() which unconditionally passes
codec_dai->component->name_prefix to strcmp().

If the property is missing, name_prefix will be NULL. Can this strcmp()
dereference the NULL pointer and cause a panic?

This same unchecked dereference also occurs during PCM startup:

sound/soc/qcom/sdm845.c:sdm845_snd_startup() {
    ...
    for_each_rtd_codec_dais(rtd, j, codec_dai) {
        if (!strcmp(codec_dai->component->name_prefix, "Left")) {
            ...
        if (!strcmp(codec_dai->component->name_prefix, "Right")) {
    ...
}

>  			ret = snd_soc_dai_set_tdm_slot(
>  					codec_dai, LEFT_SPK_TDM_TX_MASK,
> -					SPK_TDM_RX_MASK, NUM_TDM_SLOTS,
> +					LEFT_SPK_TDM_RX_MASK, NUM_TDM_SLOTS,
>  					slot_width);
>  			if (ret < 0) {
>  				dev_err(rtd->dev,
> @@ -169,7 +169,7 @@ static int sdm845_tdm_snd_hw_params(struct snd_pcm_substream *substream,
>  		if (!strcmp(codec_dai->component->name_prefix, "Right")) {

[Severity: High]
This is the same pre-existing regression as above, but does this code
similarly panic if the sound-name-prefix property is missing for the right
codec?

>  			ret = snd_soc_dai_set_tdm_slot(
>  					codec_dai, RIGHT_SPK_TDM_TX_MASK,
> -					SPK_TDM_RX_MASK, NUM_TDM_SLOTS,
> +					RIGHT_SPK_TDM_RX_MASK, NUM_TDM_SLOTS,
>  					slot_width);
>  			if (ret < 0) {
>  				dev_err(rtd->dev,

[Severity: High]
This is a pre-existing regression cemented by an earlier patch in this series,
but does the CPU DAI capture configuration miss the speaker TX slots?

When a capture stream is opened, sdm845_tdm_snd_hw_params() configures the
capture mask:

sound/soc/qcom/sdm845.c:sdm845_tdm_snd_hw_params() {
    ...
    } else {
        ret = snd_soc_dai_set_tdm_slot(cpu_dai,
                                       SPK_TDM_RX_MASK |
                                       MIC_TDM_RX_MASK, 0,
                                       NUM_TDM_SLOTS, slot_width);
    ...
}

Because it uses SPK_TDM_RX_MASK | MIC_TDM_RX_MASK, it captures from slots 0-3.

This completely ignores LEFT_SPK_TDM_TX_MASK and RIGHT_SPK_TDM_TX_MASK. Does
this break the TDM routing for speaker IV sense feedback data transmitted on
slots 4-7?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-pixel3-audio-v2-0-7c22ed915291@ixit.cz?part=3