Russell King - ARM Linux Wrote:

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

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

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

device?

quoted

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

channel is

quoted

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

terminate

quoted

DMA?

No.  I mean what if the callback wants to call
dmaengine_terminate_all().

I found the deadlock problem that you and Vinod post. I will fix it.

quoted

quoted

quoted

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

GFP_KERNEL);

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

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

kmallocing

quoted

quoted

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

It's

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

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

This

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

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

dedicated

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?

Yes, It can be re-used. But, This tasklet should be initialized on each
request by using 'tasklet_init()' because it needs the own channel as its
parameter.