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?