Re: [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] If `axg_tdm_formatter_attach()` fails to enable the formatter (e.g. when trying to catch up to an already running stream), it returns an error and skips `list_add_tail()`. However, the DAPM framework will log the error but still mark the widget as powered up. Later, during teardown or routing changes, DAPM will call `axg_tdm_formatter_dettach()`, which blindly executes `list_del(&formatter->list)`. Since the list head is zero-initialized by `devm_kzalloc` and was never added to the list, `list_del()` will dereference NULL pointers, causing a kernel crash.
- [High] The return value of `axg_tdm_stream_start()` is completely ignored in `axg_tdm_iface_trigger()`. If the stream fails to start (for example, due to a failure in `axg_tdm_formatter_enable()`), `axg_tdm_stream_start()` correctly returns an error code. However, `axg_tdm_iface_trigger()` drops this error and unconditionally returns 0.
--

commit d9e23c7e7ade1061d83943c614a45fe952dec33d
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks

Clean up the code using guard() for mutex locks.
Merely code refactoring, and no behavior change.

> diff --git a/sound/soc/meson/axg-tdm-formatter.c b/sound/soc/meson/axg-tdm-formatter.c
> --- a/sound/soc/meson/axg-tdm-formatter.c
> +++ b/sound/soc/meson/axg-tdm-formatter.c
> @@ -157,20 +157,19 @@ static int axg_tdm_formatter_attach(struct axg_tdm_formatter *formatter)
>  	struct axg_tdm_stream *ts = formatter->stream;
>  	int ret = 0;
>  
> -	mutex_lock(&ts->lock);
> +	guard(mutex)(&ts->lock);
>  
>  	/* Catch up if the stream is already running when we attach */
>  	if (ts->ready) {
>  		ret = axg_tdm_formatter_enable(formatter);
>  		if (ret) {
>  			pr_err("failed to enable formatter\n");
> -			goto out;
> +			return ret;
>  		}
>  	}
>  
>  	list_add_tail(&formatter->list, &ts->formatter_list);
> -out:
> -	mutex_unlock(&ts->lock);
> +
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but if axg_tdm_formatter_enable() fails,
this error path returns early and skips list_add_tail().

If the DAPM framework logs the error but still marks the widget as powered
up, it might later call axg_tdm_formatter_dettach() during teardown or
routing changes.

Since axg_tdm_formatter_dettach() blindly executes:

    scoped_guard(mutex, &ts->lock)
        list_del(&formatter->list);

Does this result in a NULL pointer dereference because the list head is
zero-initialized and was never actually added to the list?

[ ... ]
> @@ -330,7 +328,7 @@ int axg_tdm_stream_start(struct axg_tdm_stream *ts)
>  	struct axg_tdm_formatter *formatter;
>  	int ret = 0;
>  
> -	mutex_lock(&ts->lock);
> +	guard(mutex)(&ts->lock);
>  	ts->ready = true;
>  
>  	/* Start all the formatters attached to the stream */
> @@ -338,12 +336,10 @@ int axg_tdm_stream_start(struct axg_tdm_stream *ts)
>  		ret = axg_tdm_formatter_enable(formatter);
>  		if (ret) {
>  			pr_err("failed to start tdm stream\n");
> -			goto out;
> +			return ret;
>  		}
>  	}
>  
> -out:
> -	mutex_unlock(&ts->lock);
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the
return value of axg_tdm_stream_start() to be lost?

Looking at axg_tdm_iface_trigger() in sound/soc/meson/axg-tdm-interface.c:

    switch (cmd) {
    case SNDRV_PCM_TRIGGER_START:
    case SNDRV_PCM_TRIGGER_RESUME:
    case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
        axg_tdm_stream_start(ts);
        break;

If axg_tdm_stream_start() returns an error code here, the error isn't
propagated back to ALSA. Does this mask the initialization failure and
cause the framework to silently fail, leaving applications to hang while
waiting for period elapsed events?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610102153.83367-1-phucduc.bui@gmail.com?part=1

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