[alsa-devel] issue of shdma-base library

[Guennadi Liakhovetski]

Subject: [PATCH] ASoC: fsi: don't reschedule DMA from an atomic context

shdma doesn't support transfer re-scheduling or triggering from callbacks 
or from atomic context. The fsi driver issues DMA transfers from a tasklet 
context, which is a bug. To fix it convert tasklet to a work.

Reported-by: Do Q.Thang <dq-thang at jinso.co.jp>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski at gmx.de>
---

Thang-san, Please, check, whether the attached patch fixes your issues. It 
seems to fix them for me.

Mark, please, wait with committing this patch until the guys confirm, it 
really helps. It then will also have to go to 3.6-stable.

Thanks
Guennadi

On Tue, 2 Oct 2012, Do Q.Thang wrote:

> Hi Guennadi-san,
> 
> I'm testing uptream kernel (v3.6-rc3 ~) on the kzm-a9-gt & armadillo-800eva
> board.
> I found 2 issues with the sh-dma driver while testing audio playback.
> 
> issue 1: this log appeared while playback with the aplay, then aplay was
> stopped.
> 
> ------- log -----------------------------
> # aplay audio_s16le_44100.wav
>  Playing WAVE 'audio_s16le_44100.wav' : Signed 16 bit Little Endian, Rate
> 44100 Hz, Stereo
>  aplay: pcm_write:1710: write error: Input/output error
> -----------------------------------------
> 
> issue 2: this log appeared while playback with the aplay, then aplay was
> stopped.
> ( this issue isn't appear on the kzm-a9-gt board only )
> 
> ------- log -----------------------------
> # aplay audio_s16le_44100.wav
>  Playing WAVE 'audio.wav' : Signed 16 bit Little Endian, Rate 44100 Hz, Stereo
>  BUG: scheduling while atomic: kworker/1:1/273/0x00000103
>  aplay: pcm_write:1710: write error: Input/output error
> -----------------------------------------
> 
> I think these issues are introduced by "dmaengine:add an shdma-base library"
> commit.
> 
> ---------- commit -----------------------
> commit 9a7b8e002e331d0599127f16613c32f425a14f2c
> Author: Guennadi Liakhovetski <g.liakhovetski at gmx.de>
> Date:   Wed May 9 17:09:13 2012 +0200
> 
>     dmaengine: add an shdma-base library
> 
>     This patch extracts code from shdma.c, that does not directly deal with
>     hardware implementation details and can be re-used with diverse DMA
>     controller variants, found on SH-based SoCs.
> 
>     Signed-off-by: Guennadi Liakhovetski <g.liakhovetski at gmx.de>
>     Cc: Sascha Hauer <s.hauer at pengutronix.de>
>     Signed-off-by: Vinod Koul <vinod.koul at linux.intel.com>
> ------------------------------------------
> 
> About issue 1: I found that
> The fsi.c uses fsi_dma_do_tasklet() to setup dma-transfer.This is a function
> of a tasklet.
> When one dma-transfer is finished shdma_chan_ld_cleanup() is called.In this
> function
> __ld_cleanup() is called to cleanup the schan->ld_queue. In behavior of
> __ld_cleanup()
> if descriptor is the last chunk its callback is called, then its node is
> cleanup in the
> next call of __ld_cleanup().In callback of the last descriptor,
> fsi_dma_do_tasklet() is
> scheduled then is called after the shdma_chan_ld_cleanup() is finished.
> But when this issue appear, shdma_chan_ld_cleanup() is called while
> shdma_chan_ld_cleanup()
> is not finished.At that time, schan->ld_queue is not empty, so in
> shdma_tx_submit()
> schan->pm_state is set to SHDMA_PM_PENDING, and then dma-transfer isn't
> started in
> shdma_issue_pending().
> 
> About issue 2:
> I found that this issue is appear when pm_runtime_barrier() is running.
> This issue is not appear when disable CONFIG_PM_RUNTIME config.
> 
> What do you think about these issues?
> 
> Best regards
> ---
> Thang

diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
index 0540408..1bb0d58c 100644
--- a/sound/soc/sh/fsi.c
+++ b/sound/soc/sh/fsi.c
@@ -20,6 +20,7 @@
 #include <linux/sh_dma.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/workqueue.h>
 #include <sound/soc.h>
 #include <sound/sh_fsi.h>
 
@@ -223,7 +224,7 @@ struct fsi_stream {
 	 */
 	struct dma_chan		*chan;
 	struct sh_dmae_slave	slave; /* see fsi_handler_init() */
-	struct tasklet_struct	tasklet;
+	struct work_struct	work;
 	dma_addr_t		dma;
 };
 
@@ -1085,9 +1086,9 @@ static void fsi_dma_complete(void *data)
 	snd_pcm_period_elapsed(io->substream);
 }
 
-static void fsi_dma_do_tasklet(unsigned long data)
+static void fsi_dma_do_work(struct work_struct *work)
 {
-	struct fsi_stream *io = (struct fsi_stream *)data;
+	struct fsi_stream *io = container_of(work, struct fsi_stream, work);
 	struct fsi_priv *fsi = fsi_stream_to_priv(io);
 	struct snd_soc_dai *dai;
 	struct dma_async_tx_descriptor *desc;
@@ -1129,7 +1130,7 @@ static void fsi_dma_do_tasklet(unsigned long data)
 	 * FIXME
 	 *
 	 * In DMAEngine case, codec and FSI cannot be started simultaneously
-	 * since FSI is using tasklet.
+	 * since FSI is using the scheduler work queue.
 	 * Therefore, in capture case, probably FSI FIFO will have got
 	 * overflow error in this point.
 	 * in that case, DMA cannot start transfer until error was cleared.
@@ -1153,7 +1154,7 @@ static bool fsi_dma_filter(struct dma_chan *chan, void *param)
 
 static int fsi_dma_transfer(struct fsi_priv *fsi, struct fsi_stream *io)
 {
-	tasklet_schedule(&io->tasklet);
+	schedule_work(&io->work);
 
 	return 0;
 }
@@ -1195,14 +1196,14 @@ static int fsi_dma_probe(struct fsi_priv *fsi, struct fsi_stream *io, struct dev
 		return fsi_stream_probe(fsi, dev);
 	}
 
-	tasklet_init(&io->tasklet, fsi_dma_do_tasklet, (unsigned long)io);
+	INIT_WORK(&io->work, fsi_dma_do_work);
 
 	return 0;
 }
 
 static int fsi_dma_remove(struct fsi_priv *fsi, struct fsi_stream *io)
 {
-	tasklet_kill(&io->tasklet);
+	cancel_work_sync(&io->work);
 
 	fsi_stream_stop(fsi, io);