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

From: Takashi Iwai <tiwai@suse.de>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: alsa-devel@alsa-project.org,
	Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
	Sameer Pujar <spujar@nvidia.com>,
	vkoul@kernel.org, broonie@kernel.org,
	Gyeongtaek Lee <gt82.lee@samsung.com>,
	Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Subject: Re: [RFC PATCH v2 0/5] ASoC: soc-pcm: fix trigger race conditions with shared BE
Date: Fri, 08 Oct 2021 16:51:17 +0200	[thread overview]
Message-ID: <s5hczof7eoq.wl-tiwai@suse.de> (raw)
In-Reply-To: <e9340874-320a-8fc6-f3a4-9cf77f85db25@linux.intel.com>

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