Re: [alsa-devel] [question about DPCM] dpcm_be_disconnect race condition
On Wed, Aug 27, 2014 at 04:15:00PM +0800, Qiao Zhou wrote:
Hi Liam, Mark
I'm developing audio with DPCM on one phone platform, and met one kernel panic issue related with dpcm_be_connect. Could you please help to suggest?
Adding Liam.
Taking audio playback for example:
- User space writes all the data into audio buffer. When the blocking write
returns, it closes audio path(which updates dapm and triggers soc_dpcm_runtime_update). 2. When user space closes audio path, kernel audio driver(DMA) is still transferring data from audio buffer to audio peripheral. So DMA interrupt will come, and the interrupt handler runs on different cpu comparing to which the task to close audio path runs on. 3. Due to system schedule, below case may happen.
cpu0 cpu1soc_dapm_mixer_update_power -> | DMA interrupt -> soc_dpcm_runtime_update -> | snd_pcm_period_elapsed -> dpcm_be_disconnect(old) -> | snd_pcm_update_hw_ptr0 -> __________________________ | xrun -> | | | snd_pcm_stop -> |list_del(&dpcm->list_be) | | dpcm_fe_dai_trigger -> |list_del(&dpcm->list_fe) | | dpcm_be_dai_trigger -> |kfree(dpcm); | | list_for_each_entry(dpcm.. |_________________________| |
On CPU1, list_for_each_entry(dpcm, &fe->dpcm[stream].be_clients, list_be) will get dpcm and corresponding be, but there is no protection against the dpcm_be_disconnect on cpu0. Related dpcm for the BE may be invalid/freed when cpu1 tries to access it.
Should we require user space to make sure audio transfer ends before closing audio path to avoid such issue? I'm not sure if I missed anything or used it wrongly.
Well, it should do that in general if it wants to make sure that the audio actually gets played but the kernel should protect against this. We should be holding a lock whenever we update the lists.
Thanks in advance!
Best Regards Qiao
On 08/28/2014 02:52 AM, Mark Brown wrote:
On Wed, Aug 27, 2014 at 04:15:00PM +0800, Qiao Zhou wrote:
Hi Liam, Mark
I'm developing audio with DPCM on one phone platform, and met one kernel panic issue related with dpcm_be_connect. Could you please help to suggest?
Adding Liam.
Taking audio playback for example:
- User space writes all the data into audio buffer. When the blocking write
returns, it closes audio path(which updates dapm and triggers soc_dpcm_runtime_update). 2. When user space closes audio path, kernel audio driver(DMA) is still transferring data from audio buffer to audio peripheral. So DMA interrupt will come, and the interrupt handler runs on different cpu comparing to which the task to close audio path runs on. 3. Due to system schedule, below case may happen.
cpu0 cpu1soc_dapm_mixer_update_power -> | DMA interrupt -> soc_dpcm_runtime_update -> | snd_pcm_period_elapsed -> dpcm_be_disconnect(old) -> | snd_pcm_update_hw_ptr0 -> __________________________ | xrun -> | | | snd_pcm_stop -> |list_del(&dpcm->list_be) | | dpcm_fe_dai_trigger -> |list_del(&dpcm->list_fe) | | dpcm_be_dai_trigger -> |kfree(dpcm); | | list_for_each_entry(dpcm.. |_________________________| |
On CPU1, list_for_each_entry(dpcm, &fe->dpcm[stream].be_clients, list_be) will get dpcm and corresponding be, but there is no protection against the dpcm_be_disconnect on cpu0. Related dpcm for the BE may be invalid/freed when cpu1 tries to access it.
Should we require user space to make sure audio transfer ends before closing audio path to avoid such issue? I'm not sure if I missed anything or used it wrongly.
Well, it should do that in general if it wants to make sure that the audio actually gets played but the kernel should protect against this. We should be holding a lock whenever we update the lists.
Thanks a lot for your suggestion! Firstly we'll ask user space app to apply this rule currently to avoid such issue. Currently it holds card->mutex in mutex_lock_nested whenever the list is updated. Here list_for_each_entry in snd_pcm_stop may run in interrupt context(the bottom half of tasklet), so it can not use this mutex simply. Is there any other way to protect it?
Thanks in advance!
Best Regards Qiao
On Thu, Aug 28, 2014 at 10:07:14AM +0800, Qiao Zhou wrote:
On 08/28/2014 02:52 AM, Mark Brown wrote:
On Wed, Aug 27, 2014 at 04:15:00PM +0800, Qiao Zhou wrote:
Well, it should do that in general if it wants to make sure that the audio actually gets played but the kernel should protect against this. We should be holding a lock whenever we update the lists.
Thanks a lot for your suggestion! Firstly we'll ask user space app to apply this rule currently to avoid such issue. Currently it holds card->mutex in mutex_lock_nested whenever the list is updated. Here list_for_each_entry in snd_pcm_stop may run in interrupt context(the bottom half of tasklet), so it can not use this mutex simply. Is there any other way to protect it?
Right, that's a problem. We'd need to either have a finer grained spin lock (which we'd doubtless end up needing to hold for far too long so probably won't work) or defer the list update to a workqueue or other non-interrupt context which gets tricky. Hrm...
participants (2)
-
Mark Brown -
Qiao Zhou