Re: [RFC PATCH v3 05/13] ASoC: soc-pcm: align BE 'atomicity' with that of the FE

[Takashi Iwai]

 On Fri, 15 Oct 2021 13:22:50 +0200,
Pierre-Louis Bossart wrote:
> 
> >> At this point it does not cause serious problems, but with subsequent
> >> patches (especially when patch 7/13 is picked) I see failures. Please
> >> refer to patch 7/13 thread for more details.
> >>
> >>
> >> I am wondering if it is possible to only use locks internally for DPCM
> >> state management and decouple BE callbacks from this, like normal PCMs
> >> do?
> > 
> > Actually the patch looks like an overkill by adding the FE stream lock
> > at every loop, and this caused the problem, AFAIU.
> > 
> > Basically we need to protect the link addition / deletion while the
> > list traversal (there is a need for protection of BE vs BE access
> > race, but that's a different code path).  For the normal cases, it
> > seems already protected by card->pcm_mutex, but the problem is the FE
> > trigger case.  It was attempted by dpcm_lock, but that failed because
> > it couldn't be applied properly there.
> > 
> > That said, what we'd need is only:
> > - Drop dpcm_lock codes once
> 
> I am not able to follow this sentence, what did you mean here?

Just remove all dpcm_lock usages one to replace with a new one.

> > - Put FE stream lock around dpcm_be_connect() and dpcm_be_disconnect()
> > 
> > That should suffice for the race at trigger.  The FE stream lock is
> > already taken at trigger callback, and the rest list addition /
> > deletion are called from different code paths without stream locks, so
> > the explicit FE stream lock is needed there.
> 
> I am not able to follow what you meant after "the rest". This sentence
> mentions the FE stream lock in two places, but the second is not clear
> to me.

The FE stream locks are necessary only two points: at adding and
deleting the BE in the link.  We used dpcm_lock in other places, but
those are superfluous or would make problem if converted to a FE
stream lock.

> > In addition, a lock around dpcm_show_state() might be needed to be
> > replaced with card->pcm_mutex, and we may need to revisit whether all
> > other paths take card->pcm_mutex.
> 
> What happens if we show the state while a trigger happens? That's my
> main concern with using two separate locks (pcm_mutex and FE stream
> lock) to protect the same list, there are still windows of time where
> the list is not protected.

With the proper use of mutex, the list itself is protected.
If we need to protect the concurrent access to each BE in the show
method, an additional BE lock is needed in that part.  But that's a
subtle issue, as the link traversal itself is protected by the mutex.

> same with the use of for_each_dpcm_be() in soc-compress, SH and FSL
> drivers, there's no other mutex there.
> 
> My approach might have been overkill in some places, but it's comprehensive.
> 
> 
> > Also, BE-vs-BE race can be protected by taking a BE lock inside
> > dpcm_be_dai_trigger().
> 
> that's what I did, no?

Right.

So what I wrote is like the patches below.  Those three should be
applicable on top of the latest Linus tree.  It's merely a PoC, and it
doesn't take the compress-offload usage into account at all, but this
should illustrate my idea.


Takashi

Attachment: 0001-ASoC-soc-pcm-align-BE-atomicity-with-that-of-the-FE.patch
Description: Binary data