RE: [PATCH 5/9] dmaengine: provide a common function for completing a dma descriptor

2012-04-23 Thread Boojin Kim
Russell King - ARM Linux wrote:
 Sent: Wednesday, March 07, 2012 7:35 AM
 To: Dan Williams; Vinod Koul
 Cc: Stephen Warren; Linus Walleij; Srinidhi Kasagar; Li Yang; 
 linuxppc-dev@lists.ozlabs.org;
 linux-arm-ker...@lists.infradead.org
 Subject: [PATCH 5/9] dmaengine: provide a common function for completing a 
 dma descriptor

 Provide a common function to do the cookie mechanics for completing
 a DMA descriptor.
Dear Russell,
I met a problem on DMA cyclic mode (DMA_CYCLIC) for sound playback.
Kernel BUG occurs during DMA transfer with DMA cyclic mode.
This patch makes the cookies into zero. But, cookies should be kept during 
cyclic mode
because cyclic mode re-uses the cookies.
So, Error occurs on BUG_ON(tx-cookie  DMA_MIN_COOKIE) lines on 
dma_cookie_complete().
Please see following error.
[ cut here ]
kernel BUG at drivers/dma/dmaengine.h:53!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0Tainted: GW (3.4.0-rc2-00596-gc7d7a63 #5)
PC is at pl330_tasklet+0x58c/0x59c
LR is at _raw_spin_lock_irqsave+0x10/0x14
pc : [c0238a84]lr : [c052a3f4]psr: 68000193
sp : c06efde0  ip :   fp : c06efe4c
r10:   r9 :   r8 : c06efe18
r7 : d881b414  r6 : d881b410  r5 : d881b450  r4 : 0003
r3 : d8814bcc  r2 : d8814b60  r1 :   r0 : d8814b60
Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c5387d  Table: 57eb806a  DAC: 0015

I think the completing a dma descriptor without setting zero to cookies is 
required for cyclic mode.
Do I make new macro or modify dma_cookie_complete() for it?

Thanks
Boojin.



 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 ---
  drivers/dma/amba-pl08x.c|2 +-
  drivers/dma/at_hdmac.c  |2 +-
  drivers/dma/coh901318.c |2 +-
  drivers/dma/dmaengine.h |   18 ++
  drivers/dma/dw_dmac.c   |2 +-
  drivers/dma/ep93xx_dma.c|2 +-
  drivers/dma/fsldma.c|2 +-
  drivers/dma/imx-dma.c   |2 +-
  drivers/dma/imx-sdma.c  |2 +-
  drivers/dma/intel_mid_dma.c |2 +-
  drivers/dma/ioat/dma.c  |3 +--
  drivers/dma/ioat/dma_v2.c   |3 +--
  drivers/dma/ioat/dma_v3.c   |3 +--
  drivers/dma/ipu/ipu_idmac.c |2 +-
  drivers/dma/mxs-dma.c   |2 +-
  drivers/dma/pl330.c |2 +-
  drivers/dma/ste_dma40.c |2 +-
  drivers/dma/timb_dma.c  |2 +-
  drivers/dma/txx9dmac.c  |2 +-
  19 files changed, 36 insertions(+), 21 deletions(-)

 diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
 index 5996386..f0888c1 100644
 --- a/drivers/dma/amba-pl08x.c
 +++ b/drivers/dma/amba-pl08x.c
 @@ -1540,7 +1540,7 @@ static void pl08x_tasklet(unsigned long data)

   if (txd) {
   /* Update last completed */
 - plchan-chan.completed_cookie = txd-tx.cookie;
 + dma_cookie_complete(txd-tx);
   }

   /* If a new descriptor is queued, set it up plchan-at is NULL here */
 diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
 index df47e7d..b282630 100644
 --- a/drivers/dma/at_hdmac.c
 +++ b/drivers/dma/at_hdmac.c
 @@ -249,7 +249,7 @@ atc_chain_complete(struct at_dma_chan *atchan, struct 
 at_desc *desc)
   dev_vdbg(chan2dev(atchan-chan_common),
   descriptor %u complete\n, txd-cookie);

 - atchan-chan_common.completed_cookie = txd-cookie;
 + dma_cookie_complete(txd);

   /* move children to free_list */
   list_splice_init(desc-tx_list, atchan-free_list);
 diff --git a/drivers/dma/coh901318.c b/drivers/dma/coh901318.c
 index 843a1a3..24837d7 100644
 --- a/drivers/dma/coh901318.c
 +++ b/drivers/dma/coh901318.c
 @@ -691,7 +691,7 @@ static void dma_tasklet(unsigned long data)
   callback_param = cohd_fin-desc.callback_param;

   /* sign this job as completed on the channel */
 - cohc-chan.completed_cookie = cohd_fin-desc.cookie;
 + dma_cookie_complete(cohd_fin-desc);

   /* release the lli allocation and remove the descriptor */
   coh901318_lli_free(cohc-base-pool, cohd_fin-lli);
 diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h
 index 7692c86..47e0997 100644
 --- a/drivers/dma/dmaengine.h
 +++ b/drivers/dma/dmaengine.h
 @@ -5,6 +5,7 @@
  #ifndef DMAENGINE_H
  #define DMAENGINE_H

 +#include linux/bug.h
  #include linux/dmaengine.h

  /**
 @@ -27,4 +28,21 @@ static inline dma_cookie_t dma_cookie_assign(struct 
 dma_async_tx_descriptor
 *tx)
   return cookie;
  }

 +/**
 + * dma_cookie_complete - complete a descriptor
 + * @tx: descriptor to complete
 + *
 + * Mark this descriptor complete by updating the channels completed
 + * cookie marker.  Zero the descriptors cookie to prevent accidental
 + * repeated completions.
 + *
 + * Note: caller is expected to hold a lock to prevent concurrency.
 + */
 +static inline void dma_cookie_complete(struct dma_async_tx_descriptor *tx)
 +{
 + BUG_ON(tx-cookie  DMA_MIN_COOKIE);
 

RE: [PATCH 5/9] dmaengine: provide a common function for completing a dma descriptor

2012-04-23 Thread Boojin Kim
Vinod Koul wrote:
 Sent: Monday, April 23, 2012 7:01 PM
 To: Russell King - ARM Linux
 Cc: 'Stephen Warren'; 'Linus Walleij'; 'Srinidhi Kasagar'; Boojin Kim; 'Dan 
 Williams'; 'Li Yang';
 linuxppc-dev@lists.ozlabs.org; linux-arm-ker...@lists.infradead.org
 Subject: Re: [PATCH 5/9] dmaengine: provide a common function for completing 
 a dma descriptor

 On Mon, 2012-04-23 at 10:50 +0100, Russell King - ARM Linux wrote:
  On Mon, Apr 23, 2012 at 06:40:06PM +0900, Boojin Kim wrote:
   I met a problem on DMA cyclic mode (DMA_CYCLIC) for sound playback.
   Kernel BUG occurs during DMA transfer with DMA cyclic mode.
   This patch makes the cookies into zero. But, cookies should be kept
   during cyclic mode because cyclic mode re-uses the cookies.
 
  The protection is there to prevent cookies being accidentally re-used.
  If you're running a cyclic transfer, even then you shouldn't be completing
  the same cookie time and time again - I think Vinod also concurs with this.
 Right :)
 I recently committed patch for imx-dma which doesn't mark the cyclic
 descriptor as complete. Descriptor represents a transaction and makes no
 sense to complete t if the transaction is still continuing.
Dear Vinod,
you already fixed it. :) thanks.
And I have other question. (Actually, It doesn't relate to this patch.)
I met the DMA probing fail problem on Linux 3.4.
It's because the return value on regulator_get() is changed
from ENODEV to EPROBE_DEFER in case not to supply a vcore regulator.
So, I try to change the check value about the return value of regulator_get()
in amba_get_enable_vcore()from ENODEV to EPROBE_DEFER.
How about it ? Do you already fix it too?

Thanks,
Boojin

 
  I think our preference is for cyclic transfers to entire remain uncompleted,
  or to get a new cookie each time they allegedly complete.
 No it is not complete. Cyclic never completes, it aborts when user
 wants. The notification interrupt is for updating the
 counters/notifying (timestamp/periods elapsed in sound), and shouldn't
 be used for anything else

 --
 ~Vinod


 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev