Re: issue of shdma-base library

Re: 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@jinso.co.jp>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@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@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@gmx.de>
>     Cc: Sascha Hauer <s.hauer@pengutronix.de>
>     Signed-off-by: Vinod Koul <vinod.koul@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);
 

Re: issue of shdma-base library

[Guennadi Liakhovetski]

Mark, could you take this patch or would you like me to resend it with 
"subject" in the actual mail subject line or does it also work from the 
mail body?

Thanks
Guennadi

On Thu, 4 Oct 2012, Do Q.Thang wrote:

> Hi Guennadi-san
> 
> Thank you for your hard work.
> 
> On 10/03/2012 09:33 PM, Guennadi Liakhovetski wrote:
> > 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@jinso.co.jp>
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> > Thang-san, Please, check, whether the attached patch fixes your issues. It
> > seems to fix them for me.
> 
> This patch also fix them for me.
> 
> Thanks
> ---
> Thang
> 
> > 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@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@gmx.de>
> > >      Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > >      Signed-off-by: Vinod Koul <vinod.koul@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);
> >   
> > 
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: issue of shdma-base library

[Mark Brown]

On Fri, Oct 05, 2012 at 11:18:56AM +0200, Guennadi Liakhovetski wrote:

> Mark, could you take this patch or would you like me to resend it with 
> "subject" in the actual mail subject line or does it also work from the 
> mail body?

Don't top post!

I'll get to it some time, though the fact that the subject line doesn't
look like a patch let alone a patch for a subsystem I maintain means
that it's not going to be found when I go looking for unrevewied
patches.

Re: issue of shdma-base library

[Mark Brown]

On Wed, Oct 03, 2012 at 02:33:50PM +0200, Guennadi Liakhovetski wrote:
> 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.

Applied, thanks.