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

[Jerome Brunet <jbrunet@baylibre.com>]

On mer. 10 juin 2026 at 10:32, sashiko-bot@kernel.org wrote:

> 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().

This is path the very few have tested since it only happens when
someone is playing around the routing kcontrol on a running
stream. Still it is supposed to work.

>
> 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.

Ok but the IF is important here.

A widget that failed PRE_PMU event should not receive a PRE_PMD one.
so it makes to unwind anything on error.

>
> 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?

2 things here:

1) This is a DPCM backend so no "period elapsed events" here. This would
happen on the FIFO frontends

2) The return code is a grey area here. There could be multiple
formatters attached, this is mostly why the error code is ignored, so a
working backend would continue to operate if another fail. That's the
idea at least. 

-- 
Jerome

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