Re: async between dmaengine_pcm_dma_complete and snd_pcm_release

[Qiao Zhou <zhouqiao@marvell.com>]

On 10/09/2013 06:53 PM, Takashi Iwai wrote:
> At Wed, 09 Oct 2013 10:30:14 +0200,
> Lars-Peter Clausen wrote:
>>
>> On 10/09/2013 10:19 AM, Lars-Peter Clausen wrote:
>>> On 10/09/2013 09:29 AM, Qiao Zhou wrote:
>>>> Hi Mark, Liam, Jaroslav, Takashi
>>>>
>>>> I met an issue in which kernel panic appears in dmaengine_pcm_dma_complete
>>>> function on a quad-core system. The dmaengine_pcm_dma_complete is running
>>>> core0, while snd_pcm_release has already been executed on core1, due to in
>>>> low memory stress oom killer kills the audio thread to release some memory.
>>>>
>>>> snd_pcm_release frees the runtime parameters, and runtime is used in
>>>> dmaengine_pcm_dma_complete, which is a callback from tasklet in dmaengine.
>>>> In current audio driver, we can't promise that dmaengine_pcm_dma_complete is
>>>> not executed after snd_pcm_release on multi cores. Maybe we should add some
>>>> protection. Do you have any suggestion?
>>>>
>>>> I have tried to apply below workaround, which can fix the panic, but I'm not
>>>> confident it's proper. Need your comment and better suggestion.
>>>
>>> I think this is a general problem with your dmaengine driver, nothing audio
>>> specific. If the callback is able to run after dmaengine_terminate_all() has
>>> returned successfully there is a bug in the dmaengine driver. You need to
>>> make sure that none of the callbacks is called after terminate_all() has
>>> finished and you probably also have to make sure that the tasklet has
>>> completed, if it is running at the same time as the call to
>>> dmaengine_terminate_all().
>>
>> On the other hand that last part could get tricky as the
>> dmaengine_terminate_all() might be call from within the callback.
>
> Basically you'd need a sync for the termination (in this case
> something like tasklet_kill()) to be called at hw_free and prepare
> where you can do schedule.  dmaengine_terminate_all() can't sync for
> termination by itself, as it's called in the trigger PCM callback
> inside a spinlock.
Yes, I tried tasklet_kill() in terminate_all() but it didn't help due to 
inside a spinlock. I'll try to implement in a better place as you 
suggest. thanks.
>
>
> Takashi
>
>>
>>>
>>> - Lars
>>>
>>>
>>>>
>>>>
>>>>  From d568a88e8f66ee21d44324bdfb48d2a3106cf0d1 Mon Sep 17 00:00:00 2001
>>>> From: Qiao Zhou <zhouqiao@marvell.com>
>>>> Date: Wed, 9 Oct 2013 15:24:29 +0800
>>>> Subject: [PATCH] ASoC: soc-dmaengine: add mutex to protect runtime param
>>>>
>>>> under SMP arch, the dmaengine_pcm_dma_complete callback has the
>>>> chance to run on one cpu while at the same time, the substream is
>>>> released on another cpu. thus it may access param which is already
>>>> freed. we need to add mutes to protect such access, and check PCM
>>>> availability before using it.
>>>>
>>>> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com>
>>>> ---
>>>>   sound/soc/soc-dmaengine-pcm.c |   11 ++++++++++-
>>>>   1 files changed, 10 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c
>>>> index 111b7d9..5917029 100644
>>>> --- a/sound/soc/soc-dmaengine-pcm.c
>>>> +++ b/sound/soc/soc-dmaengine-pcm.c
>>>> @@ -125,13 +125,22 @@ EXPORT_SYMBOL_GPL(snd_hwparams_to_dma_slave_config);
>>>>   static void dmaengine_pcm_dma_complete(void *arg)
>>>>   {
>>>>       struct snd_pcm_substream *substream = arg;
>>>> -    struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
>>>> +    struct dmaengine_pcm_runtime_data *prtd;
>>>> +
>>>> +    mutex_lock(&substream->pcm->open_mutex);
>>>> +    if (!substream || !substream->runtime) {
>>>> +        mutex_unlock(&substream->pcm->open_mutex);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    prtd = substream_to_prtd(substream);
>>>>
>>>>       prtd->pos += snd_pcm_lib_period_bytes(substream);
>>>>       if (prtd->pos >= snd_pcm_lib_buffer_bytes(substream))
>>>>           prtd->pos = 0;
>>>>
>>>>       snd_pcm_period_elapsed(substream);
>>>> +    mutex_unlock(&substream->pcm->open_mutex);
>>>>   }
>>>>
>>>>   static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream
>>>> *substream)
>>>
>>> _______________________________________________
>>> Alsa-devel mailing list
>>> Alsa-devel@alsa-project.org
>>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>>
>>


-- 

Best Regards
Qiao