Re: [PATCH v2 3/6] ASoC: meson: add AUDIN driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] sound/soc/meson: period drift in timer callback due to incorrect last_pos update
- [Medium] sound/soc/meson: global state pollution due to mutating static widget array
- [Medium] sound/soc/meson: stack overflow in recursive DAPM graph traversal without cycle detection
--

commit cc303f24603b3290227ada8353fd36a74cf1a83e
Author: Valerio Setti <vsetti@baylibre.com>

ASoC: meson: add AUDIN driver

AUDIN provides audio input support to the Amlogic GX based platforms
(GXBB/GXL). It adds support for I2S data formatter through a widget
and instantiates each of the FIFOs as component's DAI.

> diff --git a/sound/soc/meson/audin-fifo.c b/sound/soc/meson/audin-fifo.c
> new file mode 100644
> index 0000000000000..39ca65e3cee9b
> --- /dev/null
> +++ b/sound/soc/meson/audin-fifo.c

[ ... ]

> +static enum hrtimer_restart audin_fifo_timer_cb(struct hrtimer *timer)
> +{
> +	struct audin_fifo_dai_data *data =
> +		container_of(timer, struct audin_fifo_dai_data, polling_timer);
> +	struct snd_pcm_runtime *runtime = data->substream->runtime;
> +	unsigned int curr_pos, delta, sleep_time;
> +
> +	if (!READ_ONCE(data->running))
> +		return HRTIMER_NORESTART;
> +
> +	curr_pos = audin_fifo_pos(data->dai, runtime);
> +	delta = (curr_pos >= data->last_pos) ?
> +		curr_pos - data->last_pos :
> +		(runtime->dma_bytes - data->last_pos) + curr_pos;
> +
> +	/* Report only when the period is completed. */
> +	if (delta >= data->period_bytes) {
> +		data->last_pos = curr_pos;

[Severity: Medium]
Does setting data->last_pos directly to curr_pos introduce cumulative period 
drift? 

Because the hrtimer callback might be delayed, curr_pos might be past the
exact period boundary. Updating the base to curr_pos locks in these 
scheduling delays, which could cause the reported hw_ptr to desynchronize 
from actual hardware DMA and lead to audio stuttering or xruns.

Should data->last_pos be incremented by exact multiples of data->period_bytes 
instead?

> +		delta = delta % data->period_bytes;
> +		snd_pcm_period_elapsed(data->substream);
> +	}

[ ... ]

> diff --git a/sound/soc/meson/audin-formatter-i2s.c b/sound/soc/meson/audin-formatter-i2s.c
> new file mode 100644
> index 0000000000000..8bbb5cdb52dc8
> --- /dev/null
> +++ b/sound/soc/meson/audin-formatter-i2s.c

[ ... ]

> +static struct snd_soc_dai *
> +audin_formatter_i2s_get_be(struct snd_soc_dapm_widget *w)
> +{
> +	struct snd_soc_dapm_path *p;
> +	struct snd_soc_dai *be;
> +
> +	snd_soc_dapm_widget_for_each_source_path(w, p) {
> +		if (!p->connect)
> +			continue;
> +
> +		if (p->source->id == snd_soc_dapm_dai_out)
> +			return (struct snd_soc_dai *)p->source->priv;
> +
> +		be = audin_formatter_i2s_get_be(p->source);

[Severity: Medium]
Can this recursive call lead to a kernel stack overflow if there is a cycle 
in the DAPM graph?

The traversal using snd_soc_dapm_widget_for_each_source_path() does not 
appear to track visited nodes or limit the recursion depth. A malformed 
device tree audio-routing or an unexpected userspace mixer configuration that 
creates a cycle could trigger infinite recursion when evaluating the graph.

> +		if (be)
> +			return be;
> +	}
> +
> +	return NULL;
> +}

[ ... ]

> diff --git a/sound/soc/meson/audin.c b/sound/soc/meson/audin.c
> new file mode 100644
> index 0000000000000..ec024267ac29c
> --- /dev/null
> +++ b/sound/soc/meson/audin.c

[ ... ]

> +static struct snd_soc_dapm_widget audin_dapm_widgets[] = {
> +	[AUDIN_WIDGET_I2S_FORMATTER] =
> +		SND_SOC_DAPM_PGA_E("I2S Formatter", SND_SOC_NOPM, 0, 0, NULL, 0,
> +				   gx_formatter_event,
> +				   (SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD)),

[ ... ]

> +static int audin_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	void __iomem *mmio;
> +	struct regmap *regmap;
> +	struct clk *clk;
> +	int ret;

[ ... ]

> +	ret = gx_formatter_create(dev, &audin_dapm_widgets[AUDIN_WIDGET_I2S_FORMATTER],
> +				  &audin_formatter_i2s_drv, regmap);

[Severity: Medium]
Is it safe to mutate the static global audin_dapm_widgets array during 
device probe?

Looking at gx_formatter_create(), it allocates memory with devm_kzalloc() 
and assigns it to the widget's priv pointer. If the probe defers, fails 
later, or if the device is unbound, the devm-managed memory is freed but 
the static global template retains a dangling pointer.

This could lead to a use-after-free or memory corruption if multiple device 
instances are probed, as they would be sharing and overwriting the same 
static widget state.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to allocate formatter\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917-audin-v2-0-38de4bc1de34@baylibre.com?part=3

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic