All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks
@ 2026-06-10 10:21 ` phucduc.bui
  0 siblings, 0 replies; 10+ messages in thread
From: phucduc.bui @ 2026-06-10 10:21 UTC (permalink / raw)
  To: Mark Brown, Jerome Brunet
  Cc: Liam Girdwood, Neil Armstrong, Kevin Hilman, Martin Blumenstingl,
	Jaroslav Kysela, Takashi Iwai, linux-sound, linux-arm-kernel,
	linux-amlogic, linux-kernel, bui duc phuc

From: bui duc phuc <phucduc.bui@gmail.com>

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

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/meson/axg-tdm-formatter.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/sound/soc/meson/axg-tdm-formatter.c b/sound/soc/meson/axg-tdm-formatter.c
index f451e4dce442..a6ba401104d5 100644
--- 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;
 }
 
@@ -178,9 +177,8 @@ static void axg_tdm_formatter_dettach(struct axg_tdm_formatter *formatter)
 {
 	struct axg_tdm_stream *ts = formatter->stream;
 
-	mutex_lock(&ts->lock);
-	list_del(&formatter->list);
-	mutex_unlock(&ts->lock);
+	scoped_guard(mutex, &ts->lock)
+		list_del(&formatter->list);
 
 	axg_tdm_formatter_disable(formatter);
 }
@@ -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;
 }
 EXPORT_SYMBOL_GPL(axg_tdm_stream_start);
@@ -352,15 +348,13 @@ void axg_tdm_stream_stop(struct axg_tdm_stream *ts)
 {
 	struct axg_tdm_formatter *formatter;
 
-	mutex_lock(&ts->lock);
+	guard(mutex)(&ts->lock);
 	ts->ready = false;
 
 	/* Stop all the formatters attached to the stream */
 	list_for_each_entry(formatter, &ts->formatter_list, list) {
 		axg_tdm_formatter_disable(formatter);
 	}
-
-	mutex_unlock(&ts->lock);
 }
 EXPORT_SYMBOL_GPL(axg_tdm_stream_stop);
 
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks
@ 2026-06-10 10:21 ` phucduc.bui
  0 siblings, 0 replies; 10+ messages in thread
From: phucduc.bui @ 2026-06-10 10:21 UTC (permalink / raw)
  To: Mark Brown, Jerome Brunet
  Cc: Liam Girdwood, Neil Armstrong, Kevin Hilman, Martin Blumenstingl,
	Jaroslav Kysela, Takashi Iwai, linux-sound, linux-arm-kernel,
	linux-amlogic, linux-kernel, bui duc phuc

From: bui duc phuc <phucduc.bui@gmail.com>

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

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/meson/axg-tdm-formatter.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/sound/soc/meson/axg-tdm-formatter.c b/sound/soc/meson/axg-tdm-formatter.c
index f451e4dce442..a6ba401104d5 100644
--- 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;
 }
 
@@ -178,9 +177,8 @@ static void axg_tdm_formatter_dettach(struct axg_tdm_formatter *formatter)
 {
 	struct axg_tdm_stream *ts = formatter->stream;
 
-	mutex_lock(&ts->lock);
-	list_del(&formatter->list);
-	mutex_unlock(&ts->lock);
+	scoped_guard(mutex, &ts->lock)
+		list_del(&formatter->list);
 
 	axg_tdm_formatter_disable(formatter);
 }
@@ -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;
 }
 EXPORT_SYMBOL_GPL(axg_tdm_stream_start);
@@ -352,15 +348,13 @@ void axg_tdm_stream_stop(struct axg_tdm_stream *ts)
 {
 	struct axg_tdm_formatter *formatter;
 
-	mutex_lock(&ts->lock);
+	guard(mutex)(&ts->lock);
 	ts->ready = false;
 
 	/* Stop all the formatters attached to the stream */
 	list_for_each_entry(formatter, &ts->formatter_list, list) {
 		axg_tdm_formatter_disable(formatter);
 	}
-
-	mutex_unlock(&ts->lock);
 }
 EXPORT_SYMBOL_GPL(axg_tdm_stream_stop);
 
-- 
2.43.0


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks
  2026-06-10 10:21 ` phucduc.bui
  (?)
@ 2026-06-10 10:32 ` sashiko-bot
  2026-06-10 13:11   ` Jerome Brunet
  -1 siblings, 1 reply; 10+ messages in thread
From: sashiko-bot @ 2026-06-10 10:32 UTC (permalink / raw)
  To: phucduc.bui; +Cc: neil.armstrong, linux-amlogic

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks
  2026-06-10 10:21 ` phucduc.bui
@ 2026-06-10 12:54   ` Jerome Brunet
  -1 siblings, 0 replies; 10+ messages in thread
From: Jerome Brunet @ 2026-06-10 12:54 UTC (permalink / raw)
  To: phucduc.bui
  Cc: Mark Brown, Liam Girdwood, Neil Armstrong, Kevin Hilman,
	Martin Blumenstingl, Jaroslav Kysela, Takashi Iwai, linux-sound,
	linux-arm-kernel, linux-amlogic, linux-kernel

On mer. 10 juin 2026 at 17:21, phucduc.bui@gmail.com wrote:

> From: bui duc phuc <phucduc.bui@gmail.com>
>
> Clean up the code using guard() for mutex locks.
> Merely code refactoring, and no behavior change.

I suppose it is OK but it does not seem to really clean anything and
make the code easier to follow in that instance, from my perspective at
least.

If there is policy to systematically use guard() whenever
possible then OK, otherwise it seems unnecessary.

>
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  sound/soc/meson/axg-tdm-formatter.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/sound/soc/meson/axg-tdm-formatter.c b/sound/soc/meson/axg-tdm-formatter.c
> index f451e4dce442..a6ba401104d5 100644
> --- 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;
>  }
>  
> @@ -178,9 +177,8 @@ static void axg_tdm_formatter_dettach(struct axg_tdm_formatter *formatter)
>  {
>  	struct axg_tdm_stream *ts = formatter->stream;
>  
> -	mutex_lock(&ts->lock);
> -	list_del(&formatter->list);
> -	mutex_unlock(&ts->lock);
> +	scoped_guard(mutex, &ts->lock)
> +		list_del(&formatter->list);
>  
>  	axg_tdm_formatter_disable(formatter);
>  }
> @@ -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;
>  }
>  EXPORT_SYMBOL_GPL(axg_tdm_stream_start);
> @@ -352,15 +348,13 @@ void axg_tdm_stream_stop(struct axg_tdm_stream *ts)
>  {
>  	struct axg_tdm_formatter *formatter;
>  
> -	mutex_lock(&ts->lock);
> +	guard(mutex)(&ts->lock);
>  	ts->ready = false;
>  
>  	/* Stop all the formatters attached to the stream */
>  	list_for_each_entry(formatter, &ts->formatter_list, list) {
>  		axg_tdm_formatter_disable(formatter);
>  	}
> -
> -	mutex_unlock(&ts->lock);
>  }
>  EXPORT_SYMBOL_GPL(axg_tdm_stream_stop);

-- 
Jerome

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks
@ 2026-06-10 12:54   ` Jerome Brunet
  0 siblings, 0 replies; 10+ messages in thread
From: Jerome Brunet @ 2026-06-10 12:54 UTC (permalink / raw)
  To: phucduc.bui
  Cc: Mark Brown, Liam Girdwood, Neil Armstrong, Kevin Hilman,
	Martin Blumenstingl, Jaroslav Kysela, Takashi Iwai, linux-sound,
	linux-arm-kernel, linux-amlogic, linux-kernel

On mer. 10 juin 2026 at 17:21, phucduc.bui@gmail.com wrote:

> From: bui duc phuc <phucduc.bui@gmail.com>
>
> Clean up the code using guard() for mutex locks.
> Merely code refactoring, and no behavior change.

I suppose it is OK but it does not seem to really clean anything and
make the code easier to follow in that instance, from my perspective at
least.

If there is policy to systematically use guard() whenever
possible then OK, otherwise it seems unnecessary.

>
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  sound/soc/meson/axg-tdm-formatter.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/sound/soc/meson/axg-tdm-formatter.c b/sound/soc/meson/axg-tdm-formatter.c
> index f451e4dce442..a6ba401104d5 100644
> --- 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;
>  }
>  
> @@ -178,9 +177,8 @@ static void axg_tdm_formatter_dettach(struct axg_tdm_formatter *formatter)
>  {
>  	struct axg_tdm_stream *ts = formatter->stream;
>  
> -	mutex_lock(&ts->lock);
> -	list_del(&formatter->list);
> -	mutex_unlock(&ts->lock);
> +	scoped_guard(mutex, &ts->lock)
> +		list_del(&formatter->list);
>  
>  	axg_tdm_formatter_disable(formatter);
>  }
> @@ -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;
>  }
>  EXPORT_SYMBOL_GPL(axg_tdm_stream_start);
> @@ -352,15 +348,13 @@ void axg_tdm_stream_stop(struct axg_tdm_stream *ts)
>  {
>  	struct axg_tdm_formatter *formatter;
>  
> -	mutex_lock(&ts->lock);
> +	guard(mutex)(&ts->lock);
>  	ts->ready = false;
>  
>  	/* Stop all the formatters attached to the stream */
>  	list_for_each_entry(formatter, &ts->formatter_list, list) {
>  		axg_tdm_formatter_disable(formatter);
>  	}
> -
> -	mutex_unlock(&ts->lock);
>  }
>  EXPORT_SYMBOL_GPL(axg_tdm_stream_stop);

-- 
Jerome

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks
  2026-06-10 10:32 ` sashiko-bot
@ 2026-06-10 13:11   ` Jerome Brunet
  0 siblings, 0 replies; 10+ messages in thread
From: Jerome Brunet @ 2026-06-10 13:11 UTC (permalink / raw)
  To: sashiko-bot; +Cc: phucduc.bui, sashiko-reviews, neil.armstrong, linux-amlogic

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks
  2026-06-10 12:54   ` Jerome Brunet
@ 2026-06-10 15:46     ` Mark Brown
  -1 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2026-06-10 15:46 UTC (permalink / raw)
  To: Jerome Brunet
  Cc: phucduc.bui, Liam Girdwood, Neil Armstrong, Kevin Hilman,
	Martin Blumenstingl, Jaroslav Kysela, Takashi Iwai, linux-sound,
	linux-arm-kernel, linux-amlogic, linux-kernel


[-- Attachment #1.1: Type: text/plain, Size: 608 bytes --]

On Wed, Jun 10, 2026 at 02:54:08PM +0200, Jerome Brunet wrote:
> On mer. 10 juin 2026 at 17:21, phucduc.bui@gmail.com wrote:

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

> I suppose it is OK but it does not seem to really clean anything and
> make the code easier to follow in that instance, from my perspective at
> least.

> If there is policy to systematically use guard() whenever
> possible then OK, otherwise it seems unnecessary.

I don't know about policy but there's definitely people who are keen on
converting things to this pattern.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks
@ 2026-06-10 15:46     ` Mark Brown
  0 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2026-06-10 15:46 UTC (permalink / raw)
  To: Jerome Brunet
  Cc: phucduc.bui, Liam Girdwood, Neil Armstrong, Kevin Hilman,
	Martin Blumenstingl, Jaroslav Kysela, Takashi Iwai, linux-sound,
	linux-arm-kernel, linux-amlogic, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

On Wed, Jun 10, 2026 at 02:54:08PM +0200, Jerome Brunet wrote:
> On mer. 10 juin 2026 at 17:21, phucduc.bui@gmail.com wrote:

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

> I suppose it is OK but it does not seem to really clean anything and
> make the code easier to follow in that instance, from my perspective at
> least.

> If there is policy to systematically use guard() whenever
> possible then OK, otherwise it seems unnecessary.

I don't know about policy but there's definitely people who are keen on
converting things to this pattern.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks
  2026-06-10 12:54   ` Jerome Brunet
@ 2026-06-10 16:27     ` Bui Duc Phuc
  -1 siblings, 0 replies; 10+ messages in thread
From: Bui Duc Phuc @ 2026-06-10 16:27 UTC (permalink / raw)
  To: Jerome Brunet
  Cc: Mark Brown, Liam Girdwood, Neil Armstrong, Kevin Hilman,
	Martin Blumenstingl, Jaroslav Kysela, Takashi Iwai, linux-sound,
	linux-arm-kernel, linux-amlogic, linux-kernel

Hi Jerome,

Thank you for your feedback,

>
> I suppose it is OK but it does not seem to really clean anything and
> make the code easier to follow in that instance, from my perspective at
> least.
>
> If there is policy to systematically use guard() whenever
> possible then OK, otherwise it seems unnecessary.
>

I have noticed that guard() has been adopted in several subsystems.
Since this appears to be the only place in the Meson ASoC code currently using
mutex_lock()/mutex_unlock(), I converted it for consistency with the
newer style.

Going forward, should new Meson ASoC code use guard(), or should it continue
using the traditional mutex_lock()/mutex_unlock() pattern?

Best regards,
Phuc

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks
@ 2026-06-10 16:27     ` Bui Duc Phuc
  0 siblings, 0 replies; 10+ messages in thread
From: Bui Duc Phuc @ 2026-06-10 16:27 UTC (permalink / raw)
  To: Jerome Brunet
  Cc: Mark Brown, Liam Girdwood, Neil Armstrong, Kevin Hilman,
	Martin Blumenstingl, Jaroslav Kysela, Takashi Iwai, linux-sound,
	linux-arm-kernel, linux-amlogic, linux-kernel

Hi Jerome,

Thank you for your feedback,

>
> I suppose it is OK but it does not seem to really clean anything and
> make the code easier to follow in that instance, from my perspective at
> least.
>
> If there is policy to systematically use guard() whenever
> possible then OK, otherwise it seems unnecessary.
>

I have noticed that guard() has been adopted in several subsystems.
Since this appears to be the only place in the Meson ASoC code currently using
mutex_lock()/mutex_unlock(), I converted it for consistency with the
newer style.

Going forward, should new Meson ASoC code use guard(), or should it continue
using the traditional mutex_lock()/mutex_unlock() pattern?

Best regards,
Phuc

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-06-10 16:27 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-10 10:21 [PATCH] ASoC: meson: axg-tdm-formatter: Use guard() for mutex locks phucduc.bui
2026-06-10 10:21 ` phucduc.bui
2026-06-10 10:32 ` sashiko-bot
2026-06-10 13:11   ` Jerome Brunet
2026-06-10 12:54 ` Jerome Brunet
2026-06-10 12:54   ` Jerome Brunet
2026-06-10 15:46   ` Mark Brown
2026-06-10 15:46     ` Mark Brown
2026-06-10 16:27   ` Bui Duc Phuc
2026-06-10 16:27     ` Bui Duc Phuc

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.