Re: [PATCH v9 04/10] ASoC: qcom: Add QAIF CPU DAI ops, regmap, DT parsing and platform init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt: missing upper bounds validation for TDM slot configuration from device tree
- [Medium] asoc: stopping one substream clears shared AIF loopback enable bit
- [Medium] asoc: silently ignores unsupported clock inversion requests for I2S format
--

commit 149e10572aefd12876aee55c367d4fb0489f0eeb
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>

ASoC: qcom: Add QAIF CPU DAI ops, regmap, DT parsing and platform init

Add the QAIF CPU DAI driver to support Qualcomm audio interface paths
used for CDC DMA and serial AIF audio links.

> diff --git a/sound/soc/qcom/qaif-cpu.c b/sound/soc/qcom/qaif-cpu.c
> new file mode 100644
> --- /dev/null
> +++ b/sound/soc/qcom/qaif-cpu.c
[ ... ]
> +static int qaif_aif_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
> +					 struct snd_pcm_hw_params *params,
> +					 struct snd_soc_dai *dai)
> +{
[ ... ]
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		ret = regmap_update_bits(map, QAIF_AUD_INTF_BIT_WIDTH_CFG_REG(idx),
> +					 QAIF_AIF_SLOT_WIDTH_TX_MASK |
> +					 QAIF_AIF_SAMPLE_WIDTH_TX_MASK,
> +					 QAIF_AIF_SLOT_WIDTH(slot_width) |

[Severity: Medium]
What happens here if an invalid device tree provides oversized values for
slot width (e.g., > 32) or frame bits (e.g., > 1024)?

It appears they are not validated against maximums in
of_qaif_parse_aif_intf_cfg() when reading dai-tdm-slot-width and
dai-tdm-slot-num.

Could this cause the unvalidated values to silently overflow the 5-bit
QAIF_AIF_SLOT_WIDTH_TX_MASK and 10-bit QAIF_AIF_BITS_PER_LANE_MASK?

[ ... ]
> +static int qaif_aif_cpu_daiops_trigger(struct snd_pcm_substream *substream,
> +				       int cmd, struct snd_soc_dai *dai)
> +{
[ ... ]
> +	cfg = &drvdata->aif_intf_cfg[idx];
> +	if (cfg->loopback_en)
> +		enable_bit = QAIF_AIF_CTL_ENABLE_BIT;
> +	else if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		enable_bit = QAIF_AIF_CTL_ENABLE_TX_BIT;
> +	else
> +		enable_bit = QAIF_AIF_CTL_ENABLE_RX_BIT;
> +
> +	switch (cmd) {
[ ... ]
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		ret = regmap_update_bits(drvdata->audio_qaif_map,
> +					 QAIF_AUD_INTF_CTL_REG(idx), enable_bit, 0);

[Severity: Medium]
If two streams are running concurrently in loopback mode (full-duplex)
and one stream stops, will this unconditionally clear the shared master
QAIF_AIF_CTL_ENABLE_BIT and accidentally break the remaining active stream?

[ ... ]
> +static int qaif_aif_cpu_daiops_set_fmt(struct snd_soc_dai *dai,
> +				       unsigned int fmt)
> +{
[ ... ]
> +	/*
> +	 * For DSP_A/DSP_B (TDM/PCM) modes, sync inversion is board-configurable
> +	 * via the NB_IF flag.  For I2S, invert_sync is fixed above and must
> +	 * not be overridden here.
> +	 */
> +	if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) != SND_SOC_DAIFMT_I2S) {
> +		switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> +		case SND_SOC_DAIFMT_NB_NF:
> +		case SND_SOC_DAIFMT_NB_IF:
> +			cfg->invert_sync = (fmt & SND_SOC_DAIFMT_INV_MASK) ==
> +					   SND_SOC_DAIFMT_NB_IF;
> +			break;
> +		default:
> +			dev_err(dai->dev, "bit clock inversion is not supported\n");
> +			return -EINVAL;
> +		}
> +	}

[Severity: Medium]
Does bypassing this validation block for I2S formats allow unsupported
clock inversions to be silently ignored?

If a machine driver requests an unsupported format inversion alongside
SND_SOC_DAIFMT_I2S, qaif_aif_cpu_daiops_set_fmt() will return success (0)
instead of -EINVAL.

Could this lead the machine driver to proceed with a mismatched hardware
state and corrupted audio?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-qaif-shikra-audio-v9-0-f3022e28f2fd@oss.qualcomm.com?part=4