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

[Takashi Iwai]

On Mon, 11 Oct 2021 22:06:51 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> >>> 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.
> >>
> >> famous last words  "nothing wrong can happen." :-)
> >>
> >> I already added a helper to do this FE lock, I can easily replace the
> >> implementation to remove the spin_lock and use the FE PCM lock.
> >> we might even add the lock in the definition of for_each_dpcm_be() to
> >> avoid misses.
> >>
> >> Let me try this out today, thanks for the suggestions.
> > 
> > well, it's not successful at all...
> > 
> > When I replace the existing dpcm_lock with the FE PCM lock as you
> > suggested, without any additional changes, speaker-test produces valid
> > audio on the endpoints, but if I try a Ctrl-C or limit the number of
> > loops with the '-l' option, I hear an endless loop on the same buffer
> > and I have to power cycle my test device to stop the sound.
> > 
> > See 2 patches attached, the first patch with the introduction of the
> > helper works fine, the second with the use of the FE PCM lock doesn't.
> > In hindsight I am glad I worked on minimal patches, one after the other,
> > to identify problems.
> > 
> > And when I add the BE lock, then nothing happens. Device stuck and no
> > audio...
> > 
> > There must be something we're missing related to the locking...
> 
> And indeed there's a deadlock!
> 
> snd_pcm_period_elapsed() takes the FE pcm stream lock, and will call
> snd_pcm_trigger.

Indeed, this would deadlock.

> So if we also take the pcm stream lock in the BE
> trigger, there's a conceptual deadlock: we painted ourselves in a corner
> by using the same lock twice.
> 
> Takashi, are you really sure we should protect these for_each_dpcm_be()
> loops with the same pcm lock?

The call within the FE lock is done only in dpcm_dai_trigger_fe_be(),
and this should call dpcm_be_dai_trigger() as is.  In other places,
the calls are without FE lock, hence they can take the lock,
e.g. create a variant dpcm_dai_trigger_fe_be_lock().

> it seems like asking for trouble to
> revisit the ALSA core just to walking through a list of BEs? Would you
> object to changing dpcm_lock as I suggested, but not interfering with
> stream handling?

That would work, too, it's just a pity to degrade the fine-grained
locks that have been already taken into global locks...


Takashi