Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 15 Oct 2021 09:39:09 +0200
Message-ID: <s5hmtnavisi.wl-tiwai@suse.de>
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
In-Reply-To: <2847a6d1-d97f-4161-c8b6-03672cf6645c@nvidia.com>
References: <20211013143050.244444-1-pierre-louis.bossart@linux.intel.com>
        <20211013143050.244444-6-pierre-louis.bossart@linux.intel.com>
        <2847a6d1-d97f-4161-c8b6-03672cf6645c@nvidia.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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