Re: [RFC PATCH v2 0/5] ASoC: soc-pcm: fix trigger race conditions with shared BE

[Takashi Iwai <tiwai@suse.de>]

On Fri, 08 Oct 2021 16:41:59 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> >>>> I think the only solution is to follow the example of the PCM case,
> >>>> where the type of lock depends on the FE types, with the assumption that
> >>>> there are no mixed atomic/non-atomic FE configurations.
> >>>
> >>> Yes, and I guess we can simply replace those all card->dpcm_lock with
> >>> FE's stream lock.  It'll solve the atomicity problem, too, and the FE
> >>> stream lock can be applied outside the loop of dpcm_be_disconnect()
> >>> gracefully.
> >>>
> >>> And, this should solve the race with dpcm_be_dai_trigger() as well,
> >>> because it's called from dpcm_fe_dai_trigger() that is called already
> >>> inside the FE's stream lock held by PCM core.  A PoC is something like
> >>> below.  (I replaced the superfluous *_irqsave with *_irq there)
> >>
> >> No I don't think so. The code starts from an FE and loops for all the
> >> BEs connected to that FE, but we want to serialize at the BE level! we
> >> really need a dpcm lock at the card level, not the FE/stream level.
> > 
> > The FE lock prevents the race between dpcm_be_dai_trigger() and
> > dpcm_be_disconnect(), i.e. the problem Gyeongtaek showed.
> > 
> > The race among concurrent dpcm_be_dai_trigger() calls itself can be
> > addressed by BE stream locks as suggested earlier.
> 
> I am not following your line of thought Takashi.
> 
> dpcm_be_disconnect already uses a spin_lock around
> 
> list_del(&dpcm->list_be);
> list_del(&dpcm->list_fe);
> 
> and in some other places, are you suggesting we change those to the FE lock?

Basically yes.

> Otherwise, I understood your proposal as using three locks (existing
> spinlock, FE lock, BE lock) to deal with DPCM. If the existing spinlock
> and FE lock are combined, we'd still have two locks.

Stream locks are more fine-grained, hence more efficient :)
The card-level spinlock is superfluous and it can go away.

> I was suggesting we use only one ;-)

Basically we need to protect two things:
- The BE links
- The concurrent accesses to BEs
The former belongs to each FE that holds the links, and the FE stream
lock would cover.  The latter is rather a per-BE business.

An oft-seen risk of multiple locks is deadlocking, but in this case,
as long as we keep the lock order FE->BE, nothing wrong can happen.


Takashi