Re: [PATCH 1/4] ASoC: meson: gx: add gx-formatter and gx-interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unhandled error paths during widget power-up leave the formatter in an incomplete state, causing deterministic kernel panics during power-down.
- [High] Missing rollback of enabled formatters in `gx_stream_start` failure path leads to hardware state leaks.
--

commit 37f14cf52933f802dadd48fb50c3876ad1e0e2c5
Author: Valerio Setti <vsetti@baylibre.com>
Subject: ASoC: meson: gx: add gx-formatter and gx-interface

This commit adds basic formatter and interface blocks to shape I2S in GX 
devices. It introduces gx_formatter_create() to allow existing AIU code to
utilize the formatter without devicetree modifications.

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

[ ... ]

> +static int gx_formatter_power_up(struct gx_formatter *formatter,
> +				      struct snd_soc_dapm_widget *w)
> +{
> +	struct gx_stream *ts = formatter->drv->ops->get_stream(w);
> +	int ret;
> +
> +	/*
> +	 * If we don't get a stream at this stage, it would mean that the
> +	 * widget is powering up but is not attached to any backend DAI.
> +	 * It should not happen, ever !
> +	 */
> +	if (WARN_ON(!ts))
> +		return -ENODEV;

Will this cause a NULL pointer dereference later if the warning triggers?

Since ASoC DAPM core ignores error codes returned by widget PRE_PMU events,
the widget will still be considered powered up and the PRE_PMD event will
eventually execute when the stream stops.

If this returns -ENODEV here, formatter->stream remains NULL. When PRE_PMD
triggers gx_formatter_power_down() and then gx_formatter_detach(), the
detach function will attempt to lock ts->lock where ts is NULL.

> +
> +	formatter->stream = ts;
> +	ret = gx_formatter_attach(formatter);
> +	if (ret)
> +		return ret;

Could this error path also lead to a crash during the PRE_PMD teardown?

If gx_formatter_attach() fails, it skips adding formatter->list to the
stream's formatter_list. Later, when the stream stops and PRE_PMD is invoked,
gx_formatter_detach() unconditionally calls list_del(&formatter->list).

Since the list was never initialized with INIT_LIST_HEAD() or added to the
list, calling list_del() on it will dereference uninitialized pointers.

> +
> +	return 0;
> +}

[ ... ]

> +int gx_stream_start(struct gx_stream *ts)
> +{
> +	struct gx_formatter *formatter;
> +	int ret = 0;
> +
> +	mutex_lock(&ts->lock);
> +
> +	/* Start all the formatters attached to the stream */
> +	list_for_each_entry(formatter, &ts->formatter_list, list) {
> +		ret = gx_formatter_enable(formatter);
> +		if (ret) {
> +			pr_err("failed to start tdm stream\n");
> +			goto out;

Do we need to disable the formatters that were already successfully enabled
in previous iterations of this loop before returning?

The ALSA core does not automatically invoke the STOP trigger when a START
trigger fails. Returning an error here without rolling back the earlier
formatters could leave the hardware clocks and registers permanently enabled,
resulting in leaked resources.

> +		}
> +	}
> +
> +	ts->ready = true;
> +
> +out:
> +	mutex_unlock(&ts->lock);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-reshape-aiu-as-axg-v1-0-53b457784ff3@baylibre.com?part=1

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