Re: [PATCH v5 4/4] ASoC: airoha: Add AFE and machine driver for Airoha AN7581

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Use of uninitialized `dev` pointer and zero-sized allocation for `afe->irqs` and `afe->memif` causes a kernel panic on probe.
- [High] Potential NULL pointer dereference in `mtk_dai_etdm_config` if `set_fmt` is not called.
- [Medium] Unconditional `devm_kzalloc` in `mtk_dai_etdm_set_fmt` leads to a memory leak on repeated calls.
- [Medium] Device tree node reference leak on the success path in `an7581_wm8960_machine_probe`.
- [Medium] IRQ handler inappropriately returns `IRQ_HANDLED` when no interrupts match.
- [Medium] Clock inversion flags are successfully parsed but completely ignored by hardware configuration.
- [Medium] The IRQ handler clears the interrupt status flag after processing the event, potentially discarding new interrupts.
- [Medium] The machine driver modifies a globally defined static `snd_soc_dai_link` array during probe.
--

--- Patch [4]: [PATCH v5 4/4] ASoC: airoha: Add AFE and machine driver for Airoha AN7581 ---
commit c3c67d6bafbbabd1b6c6e75b8c29e6aac51229d2
Author: Christian Marangi <ansuelsmth@gmail.com>

ASoC: airoha: Add AFE and machine driver for Airoha AN7581

This adds support for the Sound system present on the Airoha AN7581 SoC,
based on the mediatek AFE drivers. It also includes the machine driver
to create an actual sound card for the AFE.

> diff --git a/sound/soc/mediatek/an7581/an7581-afe-pcm.c b/sound/soc/mediatek/an7581/an7581-afe-pcm.c
> new file mode 100644
> --- /dev/null
> +++ b/sound/soc/mediatek/an7581/an7581-afe-pcm.c
> @@ -0,0 +1,517 @@
> [ ... ]
> +static irqreturn_t an7581_afe_irq_handler(int irq_id, void *dev)
> +{
> [ ... ]
> +	for (i = 0; i < AN7581_MEMIF_NUM; i++) {
> +		struct mtk_base_afe_memif *memif = &afe->memif[i];
> +
> +		if (!memif->substream)
> +			continue;
> +
> +		if (memif->irq_usage < 0)
> +			continue;
> +
> +		irq_desc = &an7581_memif_irq_descs[i];
> +		if (status & irq_desc->status_bit)
> +			snd_pcm_period_elapsed(memif->substream);
> +	}
> +
> +	for (i = 0; i < AN7581_MEMIF_NUM; i++) {
> +		struct regmap *irq_regmap;
> +
> +		irq_desc = &an7581_memif_irq_descs[i];
> +		if (!(status & irq_desc->status_bit))
> +			continue;
> +
> +		irq_regmap = afe->irqs[irq_desc->irq].regmap;
> +		regmap_set_bits(irq_regmap, irq_desc->clear_reg,
> +				BIT(AFE_IRQ_CLR_SHIFT));

[Severity: Medium]
By clearing the interrupt status flag after calling the potentially
time-consuming snd_pcm_period_elapsed(), is there a risk of inadvertently
acknowledging and dropping a new interrupt that fired during the handler's
execution?

> +		regmap_clear_bits(irq_regmap, irq_desc->clear_reg,
> +				  BIT(AFE_IRQ_CLR_SHIFT));
> +
> +		regmap_set_bits(irq_regmap, irq_desc->clear_reg,
> +				BIT(AFE_IRQ_MISS_FLG_CLR_SHIFT));
> +		regmap_clear_bits(irq_regmap, irq_desc->clear_reg,
> +				  BIT(AFE_IRQ_MISS_FLG_CLR_SHIFT));
> +	}
> +
> +	return IRQ_HANDLED;

[Severity: Medium]
Should this return IRQ_NONE if no interrupt flags were actually matched in
the status register? Returning IRQ_HANDLED unconditionally might prevent
the IRQ subsystem from detecting a spurious interrupt storm.

> +}
> [ ... ]
> +static int an7581_afe_pcm_dev_probe(struct platform_device *pdev)
> +{
> +	struct an7581_afe_private *afe_priv;
> +	struct reset_control *reset;
> +	struct mtk_base_afe *afe;
> +	struct device *dev;
> +	int i, irq_id, ret;
> +	void *base;
> +
> +	afe = devm_kzalloc(&pdev->dev, sizeof(*afe), GFP_KERNEL);
> +	if (!afe)
> +		return -ENOMEM;
> +
> +	afe->irqs = devm_kcalloc(dev, afe->irqs_size, sizeof(*afe->irqs),
> +				 GFP_KERNEL);

[Severity: High]
Is it safe to use the dev pointer here before it is initialized? It looks
like dev gets assigned later in this function.

Also, since afe was just allocated with devm_kzalloc(), wouldn't
afe->irqs_size be 0? Passing a size of 0 to devm_kcalloc() will return
ZERO_SIZE_PTR, which could cause a panic when it is dereferenced later
during initialization.

> +	if (!afe->irqs)
> +		return -ENOMEM;
> [ ... ]
> diff --git a/sound/soc/mediatek/an7581/an7581-dai-etdm.c b/sound/soc/mediatek/an7581/an7581-dai-etdm.c
> new file mode 100644
> --- /dev/null
> +++ b/sound/soc/mediatek/an7581/an7581-dai-etdm.c
> @@ -0,0 +1,433 @@
> [ ... ]
> +static int mtk_dai_etdm_config(struct mtk_base_afe *afe,
> +			       struct snd_pcm_hw_params *params,
> +			       struct snd_soc_dai *dai,
> +			       int stream)
> +{
> +	struct an7581_afe_private *afe_priv = afe->platform_priv;
> +	struct mtk_dai_etdm_priv *etdm_data = afe_priv->dai_priv[dai->id];
> [ ... ]
> +	/* CON0 */
> +	mask = ETDM_SLAVE_MODE | ETDM_BIT_LEN | ETDM_WRD_LEN |
> +	       ETDM_FMT | ETDM_CH_NUM;
> +	val = FIELD_PREP(ETDM_BIT_LEN, params_width(params) - 1) |
> +	      FIELD_PREP(ETDM_WRD_LEN, get_etdm_wlen(bit_width) - 1) |
> +	      FIELD_PREP(ETDM_FMT, etdm_data->format) |

[Severity: High]
Is it possible for mtk_dai_etdm_config() to be called without set_fmt
having been called first? If so, could dereferencing etdm_data->format
here result in a NULL pointer dereference, as etdm_data is only allocated
in mtk_dai_etdm_set_fmt()?

> +	      FIELD_PREP(ETDM_CH_NUM,
> +			 get_etdm_ch_fixup(params_channels(params)) - 1);
> +	if (etdm_data->slave_mode)
> +		val |= ETDM_SLAVE_MODE;

[Severity: Medium]
Does the hardware configuration ignore the etdm_data->bck_inv and
etdm_data->lrck_inv flags? It appears they are parsed from the format 
mask in mtk_dai_etdm_set_fmt() but never actually applied to any hardware
registers here.

> [ ... ]
> +static int mtk_dai_etdm_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct an7581_afe_private *afe_priv = afe->platform_priv;
> +	struct mtk_dai_etdm_priv *etdm_data;
> +	void *priv_data;
> +
> +	priv_data = devm_kzalloc(afe->dev, sizeof(struct mtk_dai_etdm_priv),
> +				 GFP_KERNEL);

[Severity: Medium]
Could this unconditionally allocate memory on every call, potentially
leaking the previous allocation if set_fmt is called multiple times by
ALSA core or machine drivers?

> +	if (!priv_data)
> +		return -ENOMEM;
> +
> +	afe_priv->dai_priv[dai->id] = priv_data;
> +	etdm_data = afe_priv->dai_priv[dai->id];
> [ ... ]
> diff --git a/sound/soc/mediatek/an7581/an7581-wm8960.c b/sound/soc/mediatek/an7581/an7581-wm8960.c
> new file mode 100644
> --- /dev/null
> +++ b/sound/soc/mediatek/an7581/an7581-wm8960.c
> @@ -0,0 +1,161 @@
> [ ... ]
> +static int an7581_wm8960_machine_probe(struct platform_device *pdev)
> +{
> [ ... ]
> +	for_each_card_prelinks(card, i, dai_link) {
> +		struct snd_soc_dai_link_component *platform;
> +
> +		dai_link->num_platforms = 2;
> +		for_each_link_platforms(dai_link, d, platform) {
> +			if (platform->name)
> +				continue;
> +			platform->of_node = platform_dai_node;

[Severity: Medium]
Could modifying the globally defined static an7581_wm8960_dai_links
array here cause stale pointers to be left behind across module reloads
or unbind/bind cycles?

> +		}
> +	}
> [ ... ]
> +	ret = devm_snd_soc_register_card(&pdev->dev, card);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "%s snd_soc_register_card fail\n", __func__);
> +		goto err_of_node_put;
> +	}
> +
> +	return 0;
> +
> +err_of_node_put:
> +	of_node_put(platform_dai_node);
> +	of_node_put(codec_dai_node);
> +	return ret;
> +}

[Severity: Medium]
Does returning 0 early in the success path skip the of_node_put() calls, 
leaking the device tree node references acquired earlier via 
of_parse_phandle()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807200936.201827-1-ansuelsmth@gmail.com?part=4