Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331768 > unrolled thread

[PATCH v2] dmaengine: edma: Implement device_synchronize callback

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] dmaengine: edma: Implement device_synchronize callback Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-02-11 10:20 +0100
    Re: [PATCH v2] dmaengine: edma: Implement device_synchronize callback Lars-Peter Clausen <lars@metafoo.de> - 2016-02-11 10:50 +0100
      Re: [PATCH v2] dmaengine: edma: Implement device_synchronize callback Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-02-11 12:20 +0100
        Re: [PATCH v2] dmaengine: edma: Implement device_synchronize callback Lars-Peter Clausen <lars@metafoo.de> - 2016-02-11 13:40 +0100
    Re: [PATCH v2] dmaengine: edma: Implement device_synchronize callback Vinod Koul <vinod.koul@intel.com> - 2016-02-22 03:50 +0100

#1331768 — [PATCH v2] dmaengine: edma: Implement device_synchronize callback

We need the callback to support the dmaengine_terminate_sync().

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 2dac314a2d7a..290e1a721c5b 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -869,6 +869,13 @@ static int edma_terminate_all(struct dma_chan *chan)
 	return 0;
 }
 
+static void edma_synchronize(struct dma_chan *chan)
+{
+	struct edma_chan *echan = to_edma_chan(chan);
+
+	vchan_synchronize(&echan->vchan);
+}
+
 static int edma_slave_config(struct dma_chan *chan,
 	struct dma_slave_config *cfg)
 {
@@ -1808,6 +1815,7 @@ static void edma_dma_init(struct edma_cc *ecc, bool legacy_mode)
 	s_ddev->device_pause = edma_dma_pause;
 	s_ddev->device_resume = edma_dma_resume;
 	s_ddev->device_terminate_all = edma_terminate_all;
+	s_ddev->device_synchronize = edma_synchronize;
 
 	s_ddev->src_addr_widths = EDMA_DMA_BUSWIDTHS;
 	s_ddev->dst_addr_widths = EDMA_DMA_BUSWIDTHS;
@@ -1833,6 +1841,7 @@ static void edma_dma_init(struct edma_cc *ecc, bool legacy_mode)
 		m_ddev->device_pause = edma_dma_pause;
 		m_ddev->device_resume = edma_dma_resume;
 		m_ddev->device_terminate_all = edma_terminate_all;
+		m_ddev->device_synchronize = edma_synchronize;
 
 		m_ddev->src_addr_widths = EDMA_DMA_BUSWIDTHS;
 		m_ddev->dst_addr_widths = EDMA_DMA_BUSWIDTHS;
-- 
2.7.1

[toc] | [next] | [standalone]

#1331791

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.

[toc] | [prev] | [next] | [standalone]

#1331824

[Multipart message — attachments visible in raw view] — view raw

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.

-- 
Péter

[toc] | [prev] | [next] | [standalone]

#1331917

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

[toc] | [prev] | [next] | [standalone]

#1339082

On Thu, Feb 11, 2016 at 11:08:42AM +0200, Peter Ujfalusi wrote:
> We need the callback to support the dmaengine_terminate_sync().

Applied, thanks

-- 
~Vinod

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web