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