[PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability

Vinod Koul Wrote:

Sent: Monday, July 25, 2011 7:48 PM
To: Russell King - ARM Linux
Cc: vinod.koul at intel.com; Boojin Kim; 'Kukjin Kim'; 'Jassi Brar';
'Grant Likely'; linux-samsung-soc at vger.kernel.org; 'Mark Brown'; 'Dan
Williams'; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability

On Mon, 2011-07-25 at 11:36 +0100, Russell King - ARM Linux wrote:

quoted

On Mon, Jul 25, 2011 at 07:31:45PM +0900, Boojin Kim wrote:

quoted

quoted

On Mon, Jul 25, 2011 at 10:28:22AM +0900, Boojin Kim wrote:

quoted

+static void pl330_tasklet_cyclic(unsigned long data)
+{
+	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
+	struct dma_pl330_desc *desc, *_dt;
+	unsigned long flags;
+	LIST_HEAD(list);
+
+	spin_lock_irqsave(&pch->lock, flags);

...

quoted

+			callback = desc->txd.callback;
+			if (callback)
+				callback(desc->txd.callback_param);

On this again - what if the callback wants to terminate the DMA

activity

quoted

quoted

quoted

because there's no more audio data to be sent/received from the

device?

quoted

quoted

Do you mean what is happened if the callback() is called after

channel is

quoted

quoted

terminated ?
Or What is happened if Callback() calls 'dma_release_channel()' to

terminate

quoted

quoted

DMA?

No.  I mean what if the callback wants to call

dmaengine_terminate_all().
you are supposed to drop the lock here, that way callback can call any
DMA API, otherwise it will result in deadlock.
This make me wonder you haven't read the documentation at all, please
ensure you have read Documentation/dmaengine.txt before next posting

I found the deadlock problem that you post. I will fix it and read the 
document again.

quoted

quoted

quoted

quoted

+	if (!pch->cyclic_task) {
+		pch->cyclic_task =
+			kmalloc(sizeof(struct tasklet_struct),

GFP_KERNEL);

quoted

quoted

quoted

quoted

+		tasklet_init(pch->cyclic_task,
+			pl330_tasklet_cyclic, (unsigned int)pch);

Here you allocate memory for the cyclic task.  Above you set this

pointer

quoted

quoted

quoted

to NULL.  That sounds like a memory leak to me.  Why are you

kmallocing

quoted

quoted

quoted

this memory - why can't it be part of the dma_pl330_chan

structure?  It's

quoted

quoted

quoted

only 28 bytes.

It's my mistake. I should have been free of the memory.

And the reason why I use kmalloc for 'cyclic_task' is following.
This memory size for 'cyclic_tasklet' is the 896 bytes ( = the

number of

quoted

quoted

channel * sizeof(struct tasklet_struct)= 32*28) for each DMAC. And

This

quoted

quoted

memory size is increased according to the number of DMAC.
And Samsung has the DMAC that is dedicated for Mem-to-Mem

operation. If I

quoted

quoted

make 'cyclic_task' be part of dma_pl330_chan, this DMAC that is

dedicated

quoted

quoted

for Mem-to-Mem operation should hold unused data.
So, I think it's loss that all dma channels hold own 'cyclic_task'.

Could you re-use the tasklet that already exists?

--
~Vinod Koul
Intel Corp.