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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <s5hmtnavisi.wl-tiwai@suse.de>
Date:   Fri, 15 Oct 2021 09:39:09 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Sameer Pujar <spujar@...dia.com>
Cc:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        <alsa-devel@...a-project.org>, <broonie@...nel.org>,
        <vkoul@...nel.org>, Gyeongtaek Lee <gt82.lee@...sung.com>,
        Peter Ujfalusi <peter.ujfalusi@...ux.intel.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v3 05/13] ASoC: soc-pcm: align BE 'atomicity' with that of the FE

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