linux-kernel - Re: [PATCH v3] dmaengine: add CSR SiRFprimaII DMAC driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 17 Oct 2011 21:59:05 +0530
From:	Vinod Koul <vinod.koul@...el.com>
To:	Barry Song <21cnbao@...il.com>
Cc:	Arnd Bergmann <arnd@...db.de>,
	Jassi Brar <jaswinder.singh@...aro.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	linux-kernel@...r.kernel.org, workgroup.linux@....com,
	Rongjun Ying <rongjun.ying@....com>,
	linux-arm-kernel@...ts.infradead.org,
	Barry Song <Barry.Song@....com>
Subject: Re: [PATCH v3] dmaengine: add CSR SiRFprimaII DMAC driver

On Mon, 2011-10-17 at 22:18 +0800, Barry Song wrote:
> 2011/10/17 Vinod Koul <vinod.koul@...el.com>:
> >> +
> >> +     /* Start the DMA transfer */
> >> +     writel_relaxed(sdesc->width, sdma->base + SIRFSOC_DMA_WIDTH_0 + cid * 4);
> >> +     writel_relaxed(cid | (schan->mode << SIRFSOC_DMA_MODE_CTRL_BIT) |
> >> +             (sdesc->dir << SIRFSOC_DMA_DIR_CTRL_BIT),
> >> +             sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_CTRL);
> >> +     writel_relaxed(sdesc->xlen, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_XLEN);
> >> +     writel_relaxed(sdesc->ylen, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_YLEN);
> >> +     writel_relaxed(readl_relaxed(sdma->base + SIRFSOC_DMA_INT_EN) | (1 << cid),
> >> +             sdma->base + SIRFSOC_DMA_INT_EN);
> >> +     writel(sdesc->addr >> 2, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_ADDR);
> >> +
> >> +     if (sdesc->cyclic) {
> >> +             writel((1 << cid) | 1 << (cid + 16) |
> >> +                     readl_relaxed(sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL),
> >> +                     sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
> >> +             schan->happened_cyclic = schan->completed_cyclic = 0;
> >> +     }
> > any reason why we have mixed use of writel_relaxes and writel?
> > Shouldn't all the DMA register writes be done only using writel?
> 
> Arnd comment this in v2.
Yes he certainly did, but for maintainability, it would really help to
explain what you are doing here.

> >
> >> +}
> >> +
> >> +
> >> +static struct dma_async_tx_descriptor *sirfsoc_dma_prep_interleaved(
> >> +     struct dma_chan *chan, struct dma_interleaved_template *xt)
> >> +{
> >> +     struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(chan);
> >> +     struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
> >> +     struct sirfsoc_dma_desc *sdesc = NULL;
> >> +     unsigned long iflags;
> >> +     int ret;
> >> +
> >> +     if ((xt->dir != MEM_TO_DEV) || (xt->dir != DEV_TO_MEM)) {
> >> +             ret = -EINVAL;
> >> +             goto err_dir;
> >> +     }
> >> +
> >> +     /* Get free descriptor */
> >> +     spin_lock_irqsave(&schan->lock, iflags);
> >> +     if (!list_empty(&schan->free)) {
> >> +             sdesc = list_first_entry(&schan->free, struct sirfsoc_dma_desc,
> >> +                     node);
> >> +             list_del(&sdesc->node);
> >> +     }
> >> +     spin_unlock_irqrestore(&schan->lock, iflags);
> >> +
> >> +     if (!sdesc) {
> >> +             /* try to free completed descriptors */
> >> +             sirfsoc_dma_process_completed(sdma);
> >> +             ret = 0;
> >> +             goto no_desc;
> >> +     }
> >> +
> >> +     /* Place descriptor in prepared list */
> >> +     spin_lock_irqsave(&schan->lock, iflags);
> >> +     if ((xt->frame_size == 1) && (xt->numf > 0)) {
> > what does this mean?
> >> +             sdesc->cyclic = 0;
> >> +             sdesc->xlen = xt->sgl[0].size / 4;
> >> +             sdesc->width = (xt->sgl[0].size + xt->sgl[0].icg) / 4;
> > whats so magical about 4?
> 
> the xlen and dma_width is in 4 byes boundary. xlen =1 means 4 bytes
> will be transferred every line.
I meant two things
 - why magical number 4, CSRXXX_DMA_WITH would have been much better
 - same for frame_size as 1

> > the error path handling in this function is totally non existent!!
> 
> sorry. i must have maken these idiot mistakens due to copying from the
> probe of drivers/dma/mpc512x_dma.c.
> 
> >> +
> >> +static int __devexit sirfsoc_dma_remove(struct platform_device *op)
> >> +{
> >> +     struct device *dev = &op->dev;
> >> +     struct sirfsoc_dma *sdma = dev_get_drvdata(dev);
> >> +
> >> +     dma_async_device_unregister(&sdma->dma);
> >> +     devm_free_irq(dev, sdma->irq, sdma);
> >> +     irq_dispose_mapping(sdma->irq);
> > who will free mem allocated in probe?
> 
> also due to copying from drivers/dma/mpc512x_dma.c. sorry.
Thanks, I will put these in my todo


-- 
~Vinod

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/