Re: [PATCH v3 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 9 potential issue(s) to consider:
- [High] IRQ handler is requested before its required data structures are allocated, leading to a potential NULL pointer dereference.
- [High] The interrupt handler fails to clear all active interrupts when multiple fire simultaneously, causing an interrupt storm and duplicate ALSA state updates.
- [High] Potential NULL pointer dereference in `mtk_dai_etdm_config()` if the `.set_fmt` callback is not invoked.
- [High] Device-specific regmaps are stored in a static global array, breaking multi-instance support.
- [Medium] Unconditional memory allocation in the `.set_fmt` callback causes a memory leak.
- [Medium] Device tree node references are leaked on successful probe in the machine driver.
- [Medium] Clock inversion settings parsed in `.set_fmt` are silently ignored during hardware configuration.
- [Medium] Incorrect constraint applied to `SNDRV_PCM_HW_PARAM_PERIOD_SIZE`, forcing excessive latency.
- [Medium] Hardware enablement state and atomic user counters are updated non-atomically, leading to a TOCTOU race condition.
--

commit 7488a2b7823cb2f6f02588d00f40f601dc2dcb3a
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.

> 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..d6c3fc09951c5
> --- /dev/null
> +++ b/sound/soc/mediatek/an7581/an7581-afe-pcm.c

[ ... ]

> +static struct mtk_base_irq_data irq_data[AN7581_IRQ_NUM] = {
> +	[AN7581_IRQ_0] = {
> +		.id = AN7581_IRQ_0,

[Severity: High]
Does this break support for multiple AFE hardware instances?

The device-specific regmap pointers are written directly into this static
global array during an7581_afe_pcm_dev_probe(), which would cause the first
device's IRQ handler to access the second device's memory-mapped registers.

[ ... ]

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

[Severity: Medium]
Will this enforce excessive latency?

The constraint is applied to SNDRV_PCM_HW_PARAM_PERIOD_SIZE, which is
measured in frames. At an 8000 Hz sample rate, a minimum of 0x2000 (8192)
frames enforces a minimum period latency of over 1 second. Was the
intention to constrain SNDRV_PCM_HW_PARAM_PERIOD_BYTES instead?

[ ... ]

> +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 = irq_data[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]
Can this cause an interrupt storm and duplicate ALSA state updates?

If both AFE_IRQ_STS_PLAY and AFE_IRQ_STS_RECORD are set in the status
register, the if/else block only selects AN7581_MEMIF_DL1. As a result, only
the playback interrupt is cleared, leaving the capture interrupt active.

The loop then calls snd_pcm_period_elapsed() for both streams. Since the
capture interrupt isn't cleared, the IRQ handler will immediately re-trigger,
observe the capture interrupt bit, clear it, and call
snd_pcm_period_elapsed() a second time for the same capture period.

[ ... ]

> +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;
> +
[ ... ]
> +	/* request irq */
> +	irq_id = platform_get_irq(pdev, 0);
> +	if (irq_id < 0)
> +		return irq_id;
> +
> +	ret = devm_request_irq(dev, irq_id, an7581_afe_irq_handler,
> +			       IRQF_TRIGGER_NONE, "asys-isr", (void *)afe);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request irq for asys-isr\n");
> +
> +	/* init memif */
> +	afe->memif_size = AN7581_MEMIF_NUM;
> +	afe->memif = devm_kcalloc(dev, afe->memif_size, sizeof(*afe->memif),
> +				  GFP_KERNEL);

[Severity: High]
Could this result in a NULL pointer dereference?

The IRQ handler is requested before afe->memif is allocated. If an interrupt
triggers immediately (e.g., from pending bootloader state or a shared IRQ
line), an7581_afe_irq_handler() will execute and unconditionally iterate
over afe->memif. Accessing &afe->memif[i] when the pointer is NULL will
cause a panic.

> 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..1022bedea9911
> --- /dev/null
> +++ b/sound/soc/mediatek/an7581/an7581-dai-etdm.c

[ ... ]

> +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));
> +
> +	atomic_inc(&afe_priv->users);
> +
> +	return 0;
> +}
> +
> +static void mtk_dai_etdm_shutdown(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;
> +
> +	if (atomic_dec_and_test(&afe_priv->users))
> +		regmap_clear_bits(afe->regmap, AFE_DAC_CON0,
> +				  BIT(AFE_AFE_ENABLE_SHIFT));
> +}

[Severity: Medium]
Could this introduce a race condition?

The hardware enablement state and atomic user counters are updated
non-atomically. If multiple streams are opened/closed concurrently, thread A
could increment the atomic counter immediately after thread B decrements it
to 0 but before thread B disables the hardware. Thread B will then disable
the hardware while thread A considers it successfully enabled, leaving the
hardware incorrectly powered off.

[ ... ]

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

[Severity: High]
Can this cause a NULL pointer dereference?

The etdm_data pointer is dynamically allocated and assigned in
mtk_dai_etdm_set_fmt(). If a machine driver does not define a dai_fmt and
the .set_fmt callback is never invoked, etdm_data will remain NULL. When an
audio stream is started, this will unconditionally dereference etdm_data,
causing a panic.

[Severity: Medium]
Are the clock inversion settings being ignored?

The .set_fmt callback parses SND_SOC_DAIFMT_INV_MASK and populates
etdm_data->bck_inv and etdm_data->lrck_inv. However, these fields are never
read or programmed into the hardware registers here in mtk_dai_etdm_config(),
meaning any requested phase inversions are silently ignored.

[ ... ]

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

[Severity: Medium]
Does this cause a memory leak?

A new mtk_dai_etdm_priv struct is allocated via devm_kzalloc() on every
invocation of mtk_dai_etdm_set_fmt(). Since snd_soc_dai_set_fmt() can be
called multiple times during the device lifecycle (e.g., from hw_params or
userspace topologies), each call leaks the previously allocated memory until
the device is unbound.

> diff --git a/sound/soc/mediatek/an7581/an7581-wm8960.c b/sound/soc/mediatek/an7581/an7581-wm8960.c
> new file mode 100644
> index 0000000000000..4e60e112018cc
> --- /dev/null
> +++ b/sound/soc/mediatek/an7581/an7581-wm8960.c

[ ... ]

> +static int an7581_wm8960_machine_probe(struct platform_device *pdev)
> +{
> +	struct device_node *platform_dai_node, *codec_dai_node;
> +	struct snd_soc_card *card = &an7581_wm8960_card;
> +	struct device_node *platform, *codec;
> +	struct snd_soc_dai_link *dai_link;
> +	int ret, i;
> +
> +	card->dev = &pdev->dev;
> +
> +	platform = of_get_child_by_name(pdev->dev.of_node, "platform");
> +
> +	if (platform) {
> +		platform_dai_node = of_parse_phandle(platform, "sound-dai", 0);
> +		of_node_put(platform);
> +
> +		if (!platform_dai_node) {
> +			dev_err(&pdev->dev, "Failed to parse platform/sound-dai property\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		dev_err(&pdev->dev, "Property 'platform' missing or invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	for_each_card_prelinks(card, i, dai_link) {
> +		if (dai_link->platforms->name)
> +			continue;
> +		dai_link->platforms->of_node = platform_dai_node;
> +	}
> +
> +	codec = of_get_child_by_name(pdev->dev.of_node, "codec");
> +
> +	if (codec) {
> +		codec_dai_node = of_parse_phandle(codec, "sound-dai", 0);
> +		of_node_put(codec);
> +
> +		if (!codec_dai_node) {
> +			of_node_put(platform_dai_node);
> +			dev_err(&pdev->dev, "Failed to parse codec/sound-dai property\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		of_node_put(platform_dai_node);
> +		dev_err(&pdev->dev, "Property 'codec' missing or invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	for_each_card_prelinks(card, i, dai_link) {
> +		if (dai_link->codecs->name)
> +			continue;
> +		dai_link->codecs->of_node = codec_dai_node;
> +	}
> +
> +	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;
> +
> +err_of_node_put:
> +	of_node_put(platform_dai_node);
> +	of_node_put(codec_dai_node);
> +	return ret;
> +}

[Severity: Medium]
Are device tree node references leaked here on successful probe?

The of_parse_phandle() function increments the reference count of the
returned nodes for platform_dai_node and codec_dai_node. While the error
paths correctly call of_node_put(), the success path returns 0 without
ever dropping these references. Since there is no .remove callback or
devres action to clean them up, the OF nodes are permanently leaked.

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