Re: Question regarding delayed trigger in dpcm_set_fe_update_state()

[Cezary Rojewski <cezary.rojewski@intel.com>]

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?