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

[Valerio Setti]

On 11/8/26 10:01 PM, Martin Blumenstingl wrote:
> Hi Valerio,
>
> On Wed, Oct 21, 2026 at 11:52 PM Valerio Setti <vsetti@xxxxxxxxxxxx> 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