On 2022-10-12 4:07 PM, 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.
Given no responses, perhaps there's something in my message to be improved?
