Re: async between dmaengine_pcm_dma_complete and snd_pcm_release

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

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

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