Re: [PATCH v3 4/9] 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 4 potential issue(s) to consider:
- [High] Shared AIF enable bit is improperly cleared during concurrent loopback stream stop.
- [High] Inconsistent frame clock inversion handling between set_fmt and DT parser breaks I2S.
- [Medium] Device Tree TDM slot parameters are not validated, causing silent truncation and hardware misconfiguration.
- [Low] Missing DAI type validation in DT parser allows CIF DAIs to overwrite AIF interface configurations.
--

--- Patch [4]: [PATCH v3 4/9] ASoC: qcom: Add QAIF CPU DAI ops, regmap, DT parsing and platform init ---
commit 11d4fc6e518b0a0de63f9a65aeca246850ab08f1
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>
Subject: ASoC: qcom: Add QAIF CPU DAI ops, regmap, DT parsing and platform init

This patch implements the CIF and AIF DAI operations for the QAIF CPU driver.
It configures the DMA paths, MI2S/TDM formats, registers the regmap, and
parses per-interface configuration from the device tree.

> diff --git a/sound/soc/qcom/qaif-cpu.c b/sound/soc/qcom/qaif-cpu.c
> new file mode 100644
> index 0000000000000..8792d01254a68
> --- /dev/null
> +++ b/sound/soc/qcom/qaif-cpu.c

[ ... ]

> +static int qaif_aif_cpu_daiops_trigger(struct snd_pcm_substream *substream,
> +				       int cmd, struct snd_soc_dai *dai)
> +{
> +	struct qaif_drv_data *drvdata = snd_soc_dai_get_drvdata(dai);
> +	const struct qaif_variant *v = drvdata->variant;
> +	const struct qaif_aif_config *cfg;
> +	unsigned int enable_bit;
> +	int idx, ret;
> +
> +	idx = v->get_dma_idx(dai->driver->id);
> +	if (idx < 0) {
> +		dev_err(dai->dev, "Invalid DMA index: %d\n", idx);
> +		return -EINVAL;
> +	}
> +
> +	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_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		ret = regmap_update_bits(drvdata->audio_qaif_map,
> +					 QAIF_AUD_INTF_CTL_REG(idx), enable_bit, enable_bit);
> +		if (ret)
> +			dev_err(dai->dev, "error setting AIF enable bit: %d\n", ret);
> +		return ret;
> +	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: High]
When loopback is enabled, enable_bit is assigned to the global
QAIF_AIF_CTL_ENABLE_BIT instead of a stream-specific bit.

If both streams are running concurrently in full-duplex, will stopping one
stream unconditionally clear this shared enable bit and disrupt the other
active stream?

[ ... ]

> +static int qaif_aif_cpu_daiops_set_fmt(struct snd_soc_dai *dai,
> +				       unsigned int fmt)
> +{
> +	struct qaif_drv_data *drvdata = snd_soc_dai_get_drvdata(dai);
> +	const struct qaif_variant *v = drvdata->variant;
> +	int idx = v->get_dma_idx(dai->driver->id);
> +	struct qaif_aif_config *cfg;
> +
> +	if (idx < 0)
> +		return -EINVAL;
> +
> +	cfg = &drvdata->aif_intf_cfg[idx];

[ ... ]

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

[Severity: High]
If a machine driver sets the format to SND_SOC_DAIFMT_I2S with
SND_SOC_DAIFMT_NB_NF, this code clears cfg->invert_sync to false.

However, the device tree parser hardcodes cfg->invert_sync to true for I2S.
Does this set_fmt override conflict with the hardware requirements established
during DT parsing and break the I2S setup?

[ ... ]

> +static int of_qaif_parse_aif_intf_cfg(struct device *dev,
> +				      struct qaif_drv_data *data)
> +{
> +	const struct qaif_variant *v = data->variant;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *intf_np;
> +	struct qaif_aif_config *cfg;
> +	int ret;
> +	u32 dai_id;
> +	int intf_idx;
> +	int num_interfaces = 0;
> +	u32 val_buf[QAIF_MAX_LANES];
> +	int j, n;
> +	unsigned long configured_intf_mask = 0;
> +
> +	for_each_child_of_node(np, intf_np) {
> +		if (!of_property_present(intf_np, "reg"))
> +			continue;
> +
> +		if (num_interfaces >= QAIF_MAX_AIF_CFG_CNT) {
> +			dev_warn(dev, "Too many AIF interfaces, limiting to %d\n",
> +				 QAIF_MAX_AIF_CFG_CNT);
> +			of_node_put(intf_np);
> +			break;
> +		}
> +
> +		ret = of_property_read_u32(intf_np, "reg", &dai_id);
> +		if (ret) {
> +			dev_err(dev, "Missing reg for interface %pOFn\n", intf_np);
> +			of_node_put(intf_np);
> +			return ret;
> +		}
> +
> +		intf_idx = v->get_dma_idx(dai_id);

[Severity: Low]
This loop iterates over all child nodes with a reg property and assumes they
are AIF DAIs.

If a malformed device tree includes a CIF DAI child node, could its ID
overlap with an AIF DAI index when passed to get_dma_idx(), causing array
corruption in drvdata->aif_intf_cfg or a false duplicate registration error?

[ ... ]

> +		if (!qaif_is_mi2s_port(dai_id)) {
> +			ret = of_property_read_u32(intf_np, "dai-tdm-slot-width",
> +						   &cfg->slot_width);
> +			if (ret || !cfg->slot_width) {
> +				dev_err(dev, "Missing/invalid dai-tdm-slot-width for %pOFn\n",
> +					intf_np);
> +				of_node_put(intf_np);
> +				return ret ? ret : -EINVAL;
> +			}
> +			ret = of_property_read_u32(intf_np, "dai-tdm-slot-num",
> +						   &cfg->slot_num);

[Severity: Medium]
The values from dai-tdm-slot-width and dai-tdm-slot-num are read directly
into cfg without bounds checking against hardware maximums.

In qaif_aif_cpu_daiops_hw_params(), the unvalidated slot_width is shifted
and masked:

qaif_aif_cpu_daiops_hw_params()
    ...
    QAIF_AIF_SLOT_WIDTH(slot_width) |
    ...

If a device tree specifies a width greater than 32, does this result in
silent bitwise truncation and hardware misconfiguration instead of returning
an error during probe?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824063627.3595610-1-harendra.gautam@oss.qualcomm.com?part=4