| Message ID | 56BC6C94.1060409@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/11/2016 12:12 PM, Peter Ujfalusi wrote: > On 02/11/2016 11:41 AM, Lars-Peter Clausen wrote: >> On 02/11/2016 10:08 AM, Peter Ujfalusi wrote: >>> We need the callback to support the dmaengine_terminate_sync(). >>> >>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> >> Looks good, but I noticed a slight race condition in >> edma_completion_handler(). You need to fetch echan->desc while holding the >> vchan.lock. Otherwise this can race against terminate_all() and the callback >> might get scheduled even though terminate_all() completed and then there is >> a race where the synchronize() operation could be called before the callback >> gets scheduled, which means it doesn't do its intended job. Highly unlikely >> to happen, but theoretically possible. > > Right, actually I had (have) another series fixing the very same race in a > different way - patching the terminate_all (series attached). > We have seen race with RT kernel on uniprocessor setup. > > The tasklet_kill after the terminate_all will execute the scheduled task > unconditionally, so the vchan_complete() will run after we have terminated the > channel, which might be not what we want. > I have also seen a race condition as explained in the first patch. The only > way I was able to fix that by using the attached patches. Unfortunately I can > not test RT with mainline yet, so I'm not 100% sure if by using the > dmaengine_terminate_sync() in drivers will fix the issue. > Yes, dmaengine_terminate_sync() is supposed to fix the same issue. One of the problems when implementing this was that e.g. for audio it might happen that we terminate the transfer from within the tasklet callback itself. In that case doing tasklet_disable() will deadlock since it will wait until the tasklet has finished from within the tasklet. This is why the synchronize API has two primitives. Terminate and synchronize, so you can split them if necessary. The only thing you need to make sure is that the implementation of synchronize() is correct. In the EDMA case echan->desc is read without holding a lock which still keeps the race condition open. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From 061b50dd88f65f352afaee7d4418bbef1a9a5e35 Mon Sep 17 00:00:00 2001 From: Peter Ujfalusi <peter.ujfalusi@ti.com> Date: Wed, 10 Feb 2016 08:56:01 +0200 Subject: [RFC 3/3] dmaengine: edma: Prevent race between vchan_complete() and terminate_all Implement protection against vchan_complete() calling the client callback after the channel has been terminated. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- drivers/dma/edma.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c index 290e1a721c5b..edbf3a5d04b7 100644 --- a/drivers/dma/edma.c +++ b/drivers/dma/edma.c @@ -842,7 +842,9 @@ static int edma_terminate_all(struct dma_chan *chan) unsigned long flags; LIST_HEAD(head); + tasklet_disable(&echan->vchan.task); spin_lock_irqsave(&echan->vchan.lock, flags); + vchan_terminate(&echan->vchan); /* * Stop DMA activity: we assume the callback will not be called @@ -865,6 +867,7 @@ static int edma_terminate_all(struct dma_chan *chan) vchan_get_all_descriptors(&echan->vchan, &head); spin_unlock_irqrestore(&echan->vchan.lock, flags); vchan_dma_desc_free_list(&echan->vchan, &head); + tasklet_enable(&echan->vchan.task); return 0; } -- 2.7.1