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?
> +
Useless blank line.
> + }
> +
> + 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.
> }
>
> @@ -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.
> +
> + 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
