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

[Boojin Kim]

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:
> > On Mon, Jul 25, 2011 at 07:31:45PM +0900, Boojin Kim wrote:
> > > > On Mon, Jul 25, 2011 at 10:28:22AM +0900, Boojin Kim wrote:
> > > > > +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);
> > > > ...
> > > > > +			callback = desc->txd.callback;
> > > > > +			if (callback)
> > > > > +				callback(desc->txd.callback_param);
> > > >
> > > > On this again - what if the callback wants to terminate the DMA
> activity
> > > > because there's no more audio data to be sent/received from the
> device?
> > >
> > > Do you mean what is happened if the callback() is called after
> channel is
> > > terminated ?
> > > Or What is happened if Callback() calls 'dma_release_channel()' to
> terminate
> > > 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.

> >
> > > > > +	if (!pch->cyclic_task) {
> > > > > +		pch->cyclic_task =
> > > > > +			kmalloc(sizeof(struct tasklet_struct),
> GFP_KERNEL);
> > > > > +		tasklet_init(pch->cyclic_task,
> > > > > +			pl330_tasklet_cyclic, (unsigned int)pch);
> > > >
> > > > Here you allocate memory for the cyclic task.  Above you set this
> pointer
> > > > to NULL.  That sounds like a memory leak to me.  Why are you
> kmallocing
> > > > this memory - why can't it be part of the dma_pl330_chan
> structure?  It's
> > > > 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
> > > channel * sizeof(struct tasklet_struct)= 32*28) for each DMAC. And
> This
> > > 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
> > > make 'cyclic_task' be part of dma_pl330_chan, this DMAC that is
> dedicated
> > > 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.