Re: [PATCH v4 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] The IRQ handler fails to clear the capture interrupt when playback and capture interrupts fire simultaneously, causing an interrupt storm.
- [High] A race condition between `mtk_dai_etdm_startup` and `mtk_dai_etdm_shutdown` can leave the AFE hardware disabled despite an active stream.
- [High] `mtk_dai_etdm_config()` unconditionally dereferences `etdm_data`, causing a NULL pointer dereference if `set_fmt` was never called.
- [Medium] Device node references acquired via `of_parse_phandle()` are leaked on the successful execution of the machine driver probe.
- [Medium] Memory leak in `mtk_dai_etdm_set_fmt()` due to unconditional `devm_kzalloc()` on every invocation.
- [Medium] Resource leak in the error path of the frontend startup callback.
- [Medium] Userspace can flood the kernel log by requesting an unsupported audio sample rate.
- [Medium] The driver parses clock inversion formats but ignores them during hardware configuration.
--

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

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

Add support for the Sound system present on Airoha AN7581 SoC. This is
based on the mediatek AFE drivers. Also add 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
> index 0000000000000..74f72e45d11dc
> --- /dev/null
> +++ b/sound/soc/mediatek/an7581/an7581-afe-pcm.c
> @@ -0,0 +1,529 @@

[ ... ]

> +unsigned int an7581_afe_rate_transform(struct device *dev, unsigned int rate)
> +{
> +	switch (rate) {

[ ... ]

> +	case 384000:
> +		return ARH_AFE_RATE_384K;
> +	default:
> +		dev_warn(dev, "%s(), rate %u invalid, using %d!!!\n",
> +			 __func__, rate, ARH_AFE_RATE_48K);

[Severity: Medium]
Could this un-ratelimited warning allow a local user to flood the kernel log?

If a user requests a valid ALSA rate like 64000 Hz via hw_params, it will fall
through to this default case. Calling this in a loop might spam dmesg and
cause a denial of service.

> +		return ARH_AFE_RATE_48K;
> +	}
> +}

[ ... ]

> +static int an7581_afe_fe_startup(struct snd_pcm_substream *substream,
> +				 struct snd_soc_dai *dai)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	int ret;
> +
> +	ret = mtk_afe_fe_startup(substream, dai);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +		ret = snd_pcm_hw_constraint_minmax(runtime,
> +						   SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
> +						   0x2000, UINT_MAX);
> +		if (ret < 0)
> +			dev_err(afe->dev, "hw_constraint_minmax failed\n");

[Severity: Medium]
Will this leak resources if snd_pcm_hw_constraint_minmax() fails? 

If this function returns a negative error code here, the ALSA core won't
invoke the shutdown callback, meaning mtk_afe_fe_shutdown() is skipped
and the resources acquired by mtk_afe_fe_startup() remain held.

> +	}
> +
> +	return ret;
> +}

[ ... ]

> +static irqreturn_t an7581_afe_irq_handler(int irq_id, void *dev)
> +{
> +	const struct an7581_memif_irq_desc *irq_desc;
> +	struct mtk_base_afe *afe = dev;
> +	struct regmap *irq_regmap;
> +	u32 clear_reg, status;
> +	int substream, i;
> +
> +	regmap_read(afe->regmap, AFE_IRQ_STS, &status);
> +	if (status & AFE_IRQ_STS_PLAY)
> +		substream = AN7581_MEMIF_DL1;
> +	else
> +		substream = AN7581_MEMIF_UL1;
> +
> +	irq_desc = &an7581_memif_irq_descs[substream];
> +	irq_regmap = afe->irqs[irq_desc->irq].regmap;
> +	clear_reg = irq_desc->clear_reg;
> +
> +	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);
> +	}
> +
> +	regmap_set_bits(irq_regmap, clear_reg,
> +			BIT(AFE_IRQ_CLR_SHIFT));

[Severity: High]
Does this logic leave capture interrupts un-cleared if playback and capture
interrupts fire simultaneously?

If both AFE_IRQ_STS_PLAY and AFE_IRQ_STS_RECORD are set in the status, the
initial branch statically assigns substream = AN7581_MEMIF_DL1 and clear_reg
to the DL1 clear register. While both ALSA streams will be notified, only
the DL1 interrupt is cleared here. The hardware would keep the capture
interrupt active, resulting in an interrupt storm upon exiting the ISR.

> +	regmap_clear_bits(irq_regmap, clear_reg,
> +			  BIT(AFE_IRQ_CLR_SHIFT));

[ ... ]

> diff --git a/sound/soc/mediatek/an7581/an7581-dai-etdm.c b/sound/soc/mediatek/an7581/an7581-dai-etdm.c
> new file mode 100644
> index 0000000000000..d7efd41becb17
> --- /dev/null
> +++ b/sound/soc/mediatek/an7581/an7581-dai-etdm.c
> @@ -0,0 +1,458 @@

[ ... ]

> +static int mtk_dai_etdm_startup(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct an7581_afe_private *afe_priv = afe->platform_priv;
> +
> +	regmap_set_bits(afe->regmap, AFE_DAC_CON0,
> +			BIT(AFE_AFE_ENABLE_SHIFT));
> +
> +	mutex_lock(&afe_priv->user_lock);
> +	afe_priv->users++;
> +	mutex_unlock(&afe_priv->user_lock);

[Severity: High]
Is there a race condition between mtk_dai_etdm_startup() and 
mtk_dai_etdm_shutdown() that could leave the hardware disabled while active?

By setting AFE_AFE_ENABLE_SHIFT before acquiring user_lock, if shutdown() is
executing concurrently for a different stream (e.g., stopping capture while
starting playback), shutdown() might acquire the lock, decrement users to 0,
and clear the enable bit right after startup() sets it. When startup()
eventually increments users, the driver thinks the hardware is active, but it
remains disabled.

> +
> +	return 0;
> +}

[ ... ]

> +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];
> +	unsigned int rate = params_rate(params);
> +	unsigned int etdm_rate = an7581_etdm_rate_transform(afe->dev, rate);
> +	unsigned int channels = params_channels(params);
> +	unsigned int bit_width = params_width(params);
> +	unsigned int wlen = get_etdm_wlen(bit_width);
> +	unsigned int val = 0, val1 = 0;
> +	unsigned int mask = 0, mask1 = 0;
> +
> +	dev_dbg(afe->dev, "%s(), stream %d, rate %u, bitwidth %u\n",
> +		__func__, stream, rate, bit_width);
> +
> +	/* CON0 */
> +	mask |= ETDM_SLAVE_MODE;
> +	if (etdm_data->slave_mode)
> +		val |= ETDM_SLAVE_MODE;

[Severity: High]
Could this result in a NULL pointer dereference if a machine driver omits
the .dai_fmt configuration?

If mtk_dai_etdm_set_fmt() is never called, etdm_data will remain NULL, and
dereferencing it here would crash the kernel.

[Severity: Medium]
Are the clock inversion flags from etdm_data intentionally ignored here?

In mtk_dai_etdm_set_fmt(), SND_SOC_DAIFMT_INV_MASK is parsed and sets
etdm_data->bck_inv and etdm_data->lrck_inv. However, they appear to be
unused during this hardware configuration step, meaning the IN_CLK_INV and
OUT_CLK_INV hardware registers are never updated to reflect the requested
inversion.

> +	mask |= ETDM_BIT_LEN;
> +	val |= FIELD_PREP(ETDM_BIT_LEN, bit_width - 1);

[ ... ]

> +static int mtk_dai_etdm_hw_params(struct snd_pcm_substream *substream,
> +				  struct snd_pcm_hw_params *params,
> +				  struct snd_soc_dai *dai)
> +{

[ ... ]

> +	switch (rate) {

[ ... ]

> +	case 352800:
> +	case 384000:
> +		mtk_dai_etdm_config(afe, params, dai, substream->stream);
> +		return 0;
> +	default:
> +		dev_err(afe->dev,
> +			"Sample rate %d invalid\n",
> +			rate);

[Severity: Medium]
Could this un-ratelimited error allow a local user to flood the kernel log?

If a user requests a valid ALSA rate like 64000 Hz, it falls through to this
default case, similarly to an7581_afe_rate_transform().

> +		return -EINVAL;
> +	}
> +}

[ ... ]

> +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;
> +
> +	switch (dai->id) {
> +	case AN7581_DAI_ETDM:
> +		break;
> +	default:
> +		dev_warn(afe->dev, "%s(), id %d not support\n",
> +			 __func__, dai->id);
> +		return -EINVAL;
> +	}
> +
> +	priv_data = devm_kzalloc(afe->dev, sizeof(struct mtk_dai_etdm_priv),
> +				 GFP_KERNEL);
> +	if (!priv_data)
> +		return -ENOMEM;
> +
> +	afe_priv->dai_priv[dai->id] = priv_data;

[Severity: Medium]
Does allocating priv_data unconditionally on every invocation leak memory?

The ASoC framework can call .set_fmt multiple times (e.g., dynamically from
userspace). Each call overwrites the dai_priv pointer, leaving the prior
devm_kzalloc() allocations leaked for the lifetime of the device.

> +	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
> index 0000000000000..03ea5732563cf
> --- /dev/null
> +++ b/sound/soc/mediatek/an7581/an7581-wm8960.c
> @@ -0,0 +1,161 @@

[ ... ]

> +static int an7581_wm8960_machine_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = snd_soc_of_parse_audio_routing(card, "audio-routing");
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to parse audio-routing: %d\n", ret);
> +		goto err_of_node_put;
> +	}
> +
> +	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;

[Severity: Medium]
Does returning here leak the references to the device nodes?

of_parse_phandle() increments the reference count of the returned nodes. By
returning successfully, it bypasses the of_node_put() calls in the error
block. Because the driver has no .remove callback or devm action to release
them upon device unbind, the references seem permanently leaked.

> +
> +err_of_node_put:
> +	of_node_put(platform_dai_node);
> +	of_node_put(codec_dai_node);
> +	return ret;
> +}

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