On 2022-10-28 3:38 PM, Takashi Iwai wrote:
On Wed, 12 Oct 2022 16:07:49 +0200,
Cezary Rojewski wrote:
Hello,
Writing with question regarding dpcm_set_fe_update_state() function
which is part of sound/soc/soc-pcm.c file and has been introduced with
commit "ASoC: dpcm: Fix race between FE/BE updates and trigger" [1].
The part that concerns me is the invocation of
dpcm_fe_dai_do_trigger() regardless of the actual state given DPCM is
in (actual state, not the DPCM_UPDATE_XX). The conditional invocation
of said _trigger() and addition of .trigger_pending field are here to
address a race where PCM state is being modified from multiple
locations simultaneously, at least judging by the commit's
description.
Note that the dpcm_set_fe_update_state() is called from all the
dai-ops i.e.: startup, shutdown, hw_params, prepare and hw_free.
Now, given that knowledge, we could end up in scenario where
dpcm_fe_dai_do_trigger() is invoked as a part of
dpcm_fe_dai_hw_free(). dpcm_set_fe_update_state() is called there
twice, once with DPCM_UPDATE_FE and once with DPCM_UPDATE_NO. The
second case is the more interesting one since it's called **after**
->hw_free() callback is invoked for all the DAIs.
dpcm_fe_dai_hw_free()
dpcm_set_fe_update_state(DPCM_UPDATE_FE) // fine
(...)
dpcm_be_dai_hw_free() // data allocated in hw_params
// is freed here
(...)
dpcm_set_fe_update_state(DPCM_UPDATE_NO) // not fine
The last is *not fine* if the .trigger_pending is not a zero, and can
lead to panic as code used during ->trigger() is often manipulating
data allocated during ->hw_params() but that data has just been freed
with ->hw_free().
Is what I'm looking at a bug? Or, perhaps there's something I'm
missing in the picture. From my study, it seems that only
dpcm_fe_dai_prepare() is a safe place for calling
dpcm_fe_dai_do_trigger() - once .trigger_pending is taken into
account. Any input is much appreciated.
First off, each prepare, hw_params, hw_free, etc call is protected by
a mutex, so they can't be run simultaneously. And the commit was only
about the (atomic) trigger call during those operations (which may be
delayed if happening during other operations). So, the case you
suggested in the above can't happen in reality; PCM core won't fire
such trigger.
Of course, if you observe a race by fuzzing or such, then it'd be
worth for investigating further, though.
Hello,
First things first - thank you so much for the answer. One bug less on
Intel's side :D
Did not manage to connect the dots myself, logic around
dpcm_set_fe_upadte_state() is not straightforward one. And yes, I've
managed to get race going, but perhaps only because of an unguarded
snd_pcm_stop() in the avs-driver i.e. called without stream-lock held.
The scenario is quite complicated, requires several ongoing streams,
AudioDSP exception and then firmware-recovery attempt.
Will see if something still pops up.
Until we meet again!
Czarek
