Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Barry Song]

Hi Arnd,

Thanks for review and good comments.

2011/9/21 Arnd Bergmann <arnd@xxxxxxxx>:
> Hi Barry,
>
> I just looked at the driver again and stumbled over a potential race:
>
> On Friday 16 September 2011, Barry Song wrote:
>> +
>> +/* Execute all queued DMA descriptors */
>> +static void sirfsoc_dma_execute(struct sirfsoc_dma_chan *schan)
>> +{
>> + Â Â Â struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(&schan->chan);
>> + Â Â Â int cid = schan->chan.chan_id;
>> + Â Â Â struct sirfsoc_dma_desc *sdesc = NULL;
>> +
>> + Â Â Â sdesc = list_first_entry(&schan->queued, struct sirfsoc_dma_desc,
>> + Â Â Â Â Â Â Â node);
>> + Â Â Â /* Move the first queued descriptor to active list */
>> + Â Â Â list_move_tail(&schan->queued, &schan->active);
>> +
>> + Â Â Â /* 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) |
>> + Â Â Â Â Â Â Â (schan->direction << 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_relaxed(schan->addr >> 2, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_ADDR);
>> +}
>
> I think you need to add a memory write barrier somewhere in here, because
> writel_relaxed() does not flush out the CPUs write buffers, unlike writel().
>
> Theoretically, you might be starting a DMA that reads from coherent memory
> but the data is still stuck in the CPU. I assume that the last writel_relaxed()
> is the access that actually starts the DMA, so it should be airtight once you
> replace that with writel().

yes. ARM_DMA_MEM_BUFFERABLE is forced on for CPU_V7 like primaII. we
used __raw_writel or writel_relaxed
before and haven't gotten any bug reported until now. anyway, actually
i need  the IO barrier.

>
>> +/* Interrupt handler */
>> +static irqreturn_t sirfsoc_dma_irq(int irq, void *data)
>> +{
>> + Â Â Â struct sirfsoc_dma *sdma = data;
>> + Â Â Â struct sirfsoc_dma_chan *schan;
>> + Â Â Â u32 is;
>> + Â Â Â int ch;
>> +
>> + Â Â Â is = readl_relaxed(sdma->base + SIRFSOC_DMA_CH_INT);
>> + Â Â Â while ((ch = fls(is) - 1) >= 0) {
>> + Â Â Â Â Â Â Â is &= ~(1 << ch);
>> + Â Â Â Â Â Â Â writel_relaxed(1 << ch, sdma->base + SIRFSOC_DMA_CH_INT);
>> + Â Â Â Â Â Â Â schan = &sdma->channels[ch];
>> +
>> + Â Â Â Â Â Â Â spin_lock(&schan->lock);
>> +
>> + Â Â Â Â Â Â Â /* Execute queued descriptors */
>> + Â Â Â Â Â Â Â list_splice_tail_init(&schan->active, &schan->completed);
>> + Â Â Â Â Â Â Â if (!list_empty(&schan->queued))
>> + Â Â Â Â Â Â Â Â Â Â Â sirfsoc_dma_execute(schan);
>> +
>> + Â Â Â Â Â Â Â spin_unlock(&schan->lock);
>> + Â Â Â }
>
> Similarly, readl_relaxed() does might not force in inbound DMA to be
> completed, causing you to call the tasklet before the data is visible
> to the CPU. While your hardware might have better guarantees, the
> API you are using does not. It should be find when you replace the
> first read_relaxed with readl() here.
>
> Â Â Â ÂArnd
>
-barry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/