Re: [PATCH v4 2/2] dmaengine: add a driver for AMBA AXI NBPF DMAC IP cores

On Fri, 25 Jul 2014, Vinod Koul wrote:

On Fri, Jul 25, 2014 at 05:00:53PM +0200, Guennadi Liakhovetski wrote:

quoted

Hi Vinod,

On Fri, 25 Jul 2014, Vinod Koul wrote:

Thanks for your review. However, I find the following a bit odd. As you 
remember, you already reviewed v2 of this driver:

http://www.spinics.net/lists/dmaengine/msg00880.html

and provided your comments to it, which I addressed in versions 3 and 4. 
Code, that you're commenting on here, hasn't (significantly) changed since 
v1. During your v2 review it didn't seem offending to you, now it does. 
Does this mean, that if / once I fix these issues, your next review might 
find some more _old_ code snippets, that you decide aren't appropriate?

This isn't the first time such a thing is happening with various reviewers 
and patch submitters. I think a reasonable approach is to "trust your own 
review." Once I've reviewed a piece of code and found it ok, I _normally_ 
won't ask a patch author to modify code, that didn't change since my 
previous review. Simple. Of course, sometimes it does happen, that the 
first review skips some important flaws, but then I consider that my 
fault, if I didn't find them during the first round and try to find a 
solution to minimise the damage to the author. Now, to specific comments.

I agree you can blame me for not spotting them earlier and yes criticism is
fair. But then overall goal is to ensure that code is better, so even if
comment comes late we should accept it.

Apparently, our approaches differ somewhat here. I explained already what 
I normally do in such cases.

quoted

quoted

On Sat, Jul 19, 2014 at 12:48:51PM +0200, Guennadi Liakhovetski wrote:

quoted

This patch adds a driver for NBPF DMAC IP cores from Renesas, designed for
the AMBA AXI bus.

quoted

+struct nbpf_desc {
+	struct dma_async_tx_descriptor async_tx;
+	bool user_wait;

sounds odd?

Maybe it's not the best name, I can gladly try to improve it, but I'm sure 
I'm not the best "namer," so, the same can be said about more or less 
every identifier in all my code - it could be improved. However, I don't 
think it's critical enough to delay mainlining until the next kernel 
version?

No it is not critical at all!

quoted

quoted

quoted

+static int nbpf_chan_probe(struct nbpf_device *nbpf, int n)
+{
+	struct dma_device *dma_dev = &nbpf->dma_dev;
+	struct nbpf_channel *chan = nbpf->chan + n;
+	int ret;
+
+	chan->nbpf = nbpf;
+	chan->base = nbpf->base + NBPF_REG_CHAN_OFFSET + NBPF_REG_CHAN_SIZE * n;
+	INIT_LIST_HEAD(&chan->desc_page);
+	spin_lock_init(&chan->lock);
+	chan->dma_chan.device = dma_dev;
+	dma_cookie_init(&chan->dma_chan);
+	nbpf_chan_prepare_default(chan);
+
+	dev_dbg(dma_dev->dev, "%s(): channel %d: -> %p\n", __func__, n, chan->base);
+
+	snprintf(chan->name, sizeof(chan->name), "nbpf %d", n);
+
+	ret = devm_request_threaded_irq(dma_dev->dev, chan->irq,
+			nbpf_chan_irq, nbpf_chan_irqt, IRQF_SHARED,
+			chan->name, chan);

devm_request_irq and friends arent apt here. You are in .remove and get an
irq, what prevents that race?

How is this your comment different for DMA drivers from any other drivers? 
Of course you have to make sure no more interrupts are arriving once in 
.remove() you lost the ability to handle IRQs. So, AFAIU, it's always a 
per-case study: you have to look at .remove() and consider, what happens 
if IRQ hits at any point inside it - if at all possible. Besides, DMA 
engine drivers are additionally protected by incremented module 
use-counts, once a channel is in use? As soon as a channel is released 
your driver has to make sure no more IRQs are occurring. Besides, there's 
a dmaengine_get()... So, I think, we really need to first find a race 
possibility, and then fix it. Besides, you could try

grep -rl devm_request_irq drivers/dma/*.c

And please also see there were regression on few of them, recently dw was
fixed for this case. Similarly few other drivers were fixed for their
behaviour in tasklet kill, syncronizing the irq and freeing up behaviour.

So now back to this driver, since you are using devm_ the irq is not freed,
so how does it prevent race.

How is it not freed? Of course it is freed, but later - after .release() 
has completed.

As for races - AFAIU, no interrupts should be coming after all DMA 
channels have been freed, and .release() will not be called as long as any 
channels are in use. However, I think, there might be a chance for a race 
if the user would try to free a DMA channel before all descriptors have 
completed and without calling TERMINATE_ALL. I think this would be an 
abnormal behaviour - usually you either wait for all descriptors to 
complete or terminate any remaining ones, but if such a buggy DMA slave 
driver would be used, and if .device_free_chan_resources() gets called 
(on a different CPU core) after a hard IRQ has completed and before the 
IRQ thread has run (this would be the same if a tasklet was used in the 
driver instead of an IRQ thread), i.e.

CPU core #0		CPU core #1

DMA IRQ
schedule IRQ thread
			.device_free_chan_resources()
IRQ thread:
nbpf_chan_irqt()

then there can be a problem, because .device_free_chan_resources() would 
free descriptor pages and the IRQ thread would try to use them.

Again, I consider that case to be a buggy DMA slave and even then such a 
race would be very unlikely, but theoretically it might be possible. The 
fix can be as simple as calling nbpf_chan_idle() from 
nbpf_free_chan_resources(). Would you like a new version with this single 
change or would such an incremental patch suffice?

If we do ensure that, then yes driver is fine
and can be merged, otherwise needs a fix

quoted

quoted

You need to run free_irq and synchronize_irq()
to ensure everything is proper before freeing up.

Also why threaded_irq when dmaengine API mandates you to use tasklet? The
callback is supposed to be invoked from a tasklet.

As for this one. Yes, I'm aware, that most other dmaengine drivers use a 
tasklet for bottom half IRQ processing. But is there a real requirement 
for that? This driver has been written in the end of last year and has 
been in use since then - no problems. As long as proper locking is used, 
an IRQ thread seems to work just fine here. And allows us to leverage the 
threaded-IRQ API instead of inventing own bottom-half processing.

But if there's a _real_ essential reason to use a tasklet here, I can 
switch, np. Does it really have to be done now or would an incremental 
patch suffice?

The API mandates this for a reason. I think we can add this later as it
doesn't break the functionality.

What was that reason? Is it that the network stack is using a tasklet and 
to make DMA usable for network TCP copy offloading? But that's only 
available with INTEL_IOATDMA and FSL_DMA and is broken anyway?

config NET_DMA
        bool "Network: TCP receive copy offload"
        depends on DMA_ENGINE && NET
        default (INTEL_IOATDMA || FSL_DMA)
        depends on BROKEN

Thanks
Guennadi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html