| Message ID | 1470221967-19350-1-git-send-email-fabien.lahoudere@collabora.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 08/03/2016 12:59 PM, Fabien Lahoudere wrote: > From: Hannu Koivisto <hannu.koivisto@vincit.fi> > > dma_rx_callback() may see NULL dma_chan_rx if DMA interrupt [1] occurs a > moment[2] before imx_uart_dma_exit() sets it to NULL. imx_uart_dma_exit() > calls dmaengine_terminate_all() and dma_release_channel() but neither of > those prevent the callback being called after they have returned. A similar > problem has been discussed by ALSA developers > (http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067239.html) > and it was pointed out that dmaengine_terminate_all() might be called from > the callback, so we cannot call tasklet_kill() in imx-sdma's code called by > dmaengine_terminate_all(). > > Hopefully it doesn't make sense to call dma_release_channel() from the > callback, so instead of adding synchronization to imx serial driver, we add > tasklet_kill() call to sdma_free_chan_resources(). While most DMA drivers > don't do that, there is one example that does: pl330. > > [1] It schedules sdma_tasklet, which again calls the dma_rx_callback. > [2] I tested this by scheduling the sdma tasklet as far as right before the > imx_stop_tx() call in imx_shutdown() and the problem occurred. > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> I'd prefer that the driver implements the new synchronization API[1]. This is a more generic approach and covers of all cases of this race condition. If the synchronize() callback is implemented the core will automatically make sure that the channel is synchronized when it is freed. - Lars [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=b36f09c3c441a6e59eab9315032e7d546571de3f -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/03/2016 01:19 PM, Lars-Peter Clausen wrote: > On 08/03/2016 12:59 PM, Fabien Lahoudere wrote: >> From: Hannu Koivisto <hannu.koivisto@vincit.fi> >> >> dma_rx_callback() may see NULL dma_chan_rx if DMA interrupt [1] occurs a >> moment[2] before imx_uart_dma_exit() sets it to NULL. imx_uart_dma_exit() >> calls dmaengine_terminate_all() and dma_release_channel() but neither of >> those prevent the callback being called after they have returned. A similar >> problem has been discussed by ALSA developers >> (http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067239.html) >> and it was pointed out that dmaengine_terminate_all() might be called from >> the callback, so we cannot call tasklet_kill() in imx-sdma's code called by >> dmaengine_terminate_all(). >> >> Hopefully it doesn't make sense to call dma_release_channel() from the >> callback, so instead of adding synchronization to imx serial driver, we add >> tasklet_kill() call to sdma_free_chan_resources(). While most DMA drivers >> don't do that, there is one example that does: pl330. >> >> [1] It schedules sdma_tasklet, which again calls the dma_rx_callback. >> [2] I tested this by scheduling the sdma tasklet as far as right before the >> imx_stop_tx() call in imx_shutdown() and the problem occurred. >> >> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> > > I'd prefer that the driver implements the new synchronization API[1]. This > is a more generic approach and covers of all cases of this race condition. > > If the synchronize() callback is implemented the core will automatically > make sure that the channel is synchronized when it is freed. Looking at the driver it also seems that just calling tasklet_kill() is not enough. The tasklet_schedule() in the sdma_int_handler() is not synchronized to anything. So if the interrupt triggers just at the right time it might re-schedule the tasklet after tasklet_kill() has been called. Especially if the tasklet_kill() runs on a different CPU. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index 46c9027..da86792 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -1135,6 +1135,8 @@ static void sdma_free_chan_resources(struct dma_chan *chan) sdma_disable_channel(chan); + tasklet_kill(&sdmac->tasklet); + if (sdmac->event_id0) sdma_event_disable(sdmac, sdmac->event_id0); if (sdmac->event_id1)