Re: async between dmaengine_pcm_dma_complete and snd_pcm_release

[Lars-Peter Clausen <lars@metafoo.de>]

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.

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