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

From: Takashi Iwai <tiwai@suse.de>
To: Sameer Pujar <spujar@nvidia.com>
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	<alsa-devel@alsa-project.org>, <broonie@kernel.org>,
	<vkoul@kernel.org>, Gyeongtaek Lee <gt82.lee@samsung.com>,
	Peter Ujfalusi <peter.ujfalusi@linux.intel.com>,
	Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v3 05/13] ASoC: soc-pcm: align BE 'atomicity' with that of the FE
Date: Fri, 15 Oct 2021 09:39:09 +0200	[thread overview]
Message-ID: <s5hmtnavisi.wl-tiwai@suse.de> (raw)
In-Reply-To: <2847a6d1-d97f-4161-c8b6-03672cf6645c@nvidia.com>

On Fri, 15 Oct 2021 08:24:41 +0200,
Sameer Pujar wrote:
> 
> 
> 
> On 10/13/2021 8:00 PM, Pierre-Louis Bossart wrote:
> > Since the flow for DPCM is based on taking a lock for the FE first, we
> > need to make sure during the connection between a BE and an FE that
> > they both use the same 'atomicity', otherwise we may sleep in atomic
> > context.
> >
> > If the FE is nonatomic, this patch forces the BE to be nonatomic as
> > well. That should have no negative impact since the BE 'inherits' the
> > FE properties.
> >
> > However, if the FE is atomic and the BE is not, then the configuration
> > is flagged as invalid.
> 
> In normal PCM, atomicity seems to apply only for trigger(). Other
> callbacks like prepare, hw_params are executed in non-atomic
> context. So when 'nonatomic' flag is false, still it is possible to
> sleep in a prepare or hw_param callback and this is true for FE as
> well. So I am not sure if atomicity is applicable as a whole even for
> FE.
> 
> 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
- 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.

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.

Also, BE-vs-BE race can be protected by taking a BE lock inside
dpcm_be_dai_trigger().

Takashi