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

[Boojin Kim]

Russell King - ARM Linux Wrote:

> Sent: Monday, July 25, 2011 6:28 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 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?

>
> > +
>
> Useless blank line.
I will address your comment.
>
> > +		}
> > +
> > +	spin_unlock_irqrestore(&pch->lock, flags);
> > +}
> > +
> >  static void pl330_tasklet(unsigned long data)
> >  {
> >  	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
> > @@ -227,6 +267,9 @@ static void dma_pl330_rqcb(void *token, enum
> pl330_op_err err)
> >
> >  	spin_unlock_irqrestore(&pch->lock, flags);
> >
> > +	if (pch->cyclic_task)
> > +		tasklet_schedule(pch->cyclic_task);
> > +	else
> >  	tasklet_schedule(&pch->task);
>
> Indentation error.
Sorry again.. I will address your comment.

>
> >  }
> >
> > @@ -316,6 +359,15 @@ static void pl330_free_chan_resources(struct
dma_chan
> *chan)
> >  	pl330_release_channel(pch->pl330_chid);
> >  	pch->pl330_chid = NULL;
> >
> > +	if (pch->cyclic) {
> > +		pch->cyclic = false;
> > +		list_splice_tail_init(&pch->work_list,
&pch->dmac->desc_pool);
> > +		if (pch->cyclic_task) {
> > +			tasklet_kill(pch->cyclic_task);
> > +			pch->cyclic_task = NULL;
> > +		}
> > +	}
> > +
> >  	spin_unlock_irqrestore(&pch->lock, flags);
> >  }
> >
> > @@ -547,6 +599,63 @@ static inline int get_burst_len(struct
dma_pl330_desc
> *desc, size_t len)
> >  	return burst_len;
> >  }
> >
> > +static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
> > +		struct dma_chan *chan, dma_addr_t dma_addr, size_t len,
> > +		size_t period_len, enum dma_data_direction direction)
> > +{
> > +	struct dma_pl330_desc *desc;
> > +	struct dma_pl330_chan *pch = to_pchan(chan);
> > +	struct dma_pl330_peri *peri = chan->private;
> > +	dma_addr_t dst;
> > +	dma_addr_t src;
> > +
> > +	pch = to_pchan(chan);
> > +	if (!pch)
> > +		return NULL;
>
> You've already done the to_pchan() thing when declaring pch.  You've
> already dereferenced 'chan', so there's no way that pch could be NULL
> here.
I will address your comment.

>
> > +
> > +	desc = pl330_get_desc(pch);
> > +	if (!desc) {
> > +		dev_err(pch->dmac->pif.dev, "%s:%d Unable to fetch desc\n",
> > +			__func__, __LINE__);
> > +		return NULL;
> > +	}
> > +
> > +	switch (direction) {
> > +	case DMA_TO_DEVICE:
> > +		desc->rqcfg.src_inc = 1;
> > +		desc->rqcfg.dst_inc = 0;
> > +		src = dma_addr;
> > +		dst = peri->fifo_addr;
> > +		break;
> > +	case DMA_FROM_DEVICE:
> > +		desc->rqcfg.src_inc = 0;
> > +		desc->rqcfg.dst_inc = 1;
> > +		src = peri->fifo_addr;
> > +		dst = dma_addr;
> > +		break;
> > +	default:
> > +		dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma direction\n",
> > +		__func__, __LINE__);
> > +		return NULL;
> > +	}
> > +
> > +	desc->rqcfg.brst_size = peri->burst_sz;
> > +	desc->rqcfg.brst_len = 1;
> > +
> > +	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'.