Re: [PATCH] ASoC: meson: gx-formatter: change stream lock from mutex to spinlock

[Valerio Setti <vsetti@baylibre.com>]

On 11/8/26 10:01 PM, Martin Blumenstingl wrote:
> Hi Valerio,
> 
> On Wed, Oct 21, 2026 at 11:52 PM Valerio Setti <vsetti@baylibre.com> wrote:
> [...]
>> @@ -85,9 +85,9 @@ static void gx_formatter_detach(struct gx_formatter *formatter)
>>          if (!ts)
>>                  return;
>>
>> -       mutex_lock(&ts->lock);
>> +       spin_lock_irq(&ts->lock);
>>          list_del(&formatter->list);
>> -       mutex_unlock(&ts->lock);
>> +       spin_unlock_irq(&ts->lock);
>>
>>          gx_formatter_disable(formatter);
> The documentation below states that ->ops->disable is called with the lock held.
> gx_stream_stop() does call it with the locks held but
> gx_formatter_detach here doesn't.
> 
> It doesn't seem to be problematic yet because aiu-formatter-i2s.c
> doesn't register a ->disable callback.
> Can you please still look into whether this is a problem?
> 
> Also for a future patch (not this bugfix anymore) it would be great to
> have lockdep_assert_held validate that the lock has been taken (inside
> ->enable, ->disable and ->prepare).
> 
> [...]
>>   struct gx_formatter_ops {
>>          struct gx_stream *(*get_stream)(struct snd_soc_dapm_widget *w);
>> +       /*
>> +        * Following functions are called with gx_stream->lock held and local
>> +        * IRQs disabled so they must not sleep.
>> +        */
>>          void (*enable)(struct regmap *map);
>>          void (*disable)(struct regmap *map);
>>          int (*prepare)(struct regmap *map,

Hi Martin.

Oh, good catch indeed.

As you correctly pointed out that's not a problem today because the only 
formatter currently present in the tree doesn't implement the 
'disable()' callback. However the upcoming AUDIN support [1] will use 
that callback, so the question might remain.

Technically in 'gx_formatter_detach' the formatter is first removed from 
the formatter list (with spinlock held) before being disabled. This 
means that other start/stop actions cannot be called on that formatter.

So even though this is ideally safe, I think that moving 
'gx_formatter_disable' under the spinlock guard is better: all the 
register access are MMIO (not sleeping) and then the behavior would be 
symmetrical with 'gx_formatter_attach'.

Thanks also for the suggestion about 'lockdep_assert_held'.

I will send an updated version with these problem fixed.

Thanks for the review!

-- 
Valerio