Re: [question about DPCM] dpcm_be_disconnect race condition

Re: [question about DPCM] dpcm_be_disconnect race condition

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2171 bytes --]

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:
> 1. 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                                      cpu1
> 
> soc_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
> 

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [question about DPCM] dpcm_be_disconnect race condition

[Qiao Zhou]

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:
>> 1. 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                                      cpu1
>>
>> soc_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
>>
-- 

Best Regards
Qiao

Re: [question about DPCM] dpcm_be_disconnect race condition

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1021 bytes --]

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...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]