03.01.2020 11:16, Michał Mirosław пишет: > On Thu, Jan 02, 2020 at 06:09:45PM +0300, Dmitry Osipenko wrote: >> 30.12.2019 23:50, Michał Mirosław пишет: >>> On Mon, Dec 30, 2019 at 09:45:55PM +0100, Michał Mirosław wrote: >>>> On Sat, Dec 28, 2019 at 11:46:36PM +0300, Dmitry Osipenko wrote: >>>>> It's unsafe to check the channel's "busy" state without taking a lock, >>>>> it is also unsafe to assume that tasklet isn't in-fly. >>>> >>>> 'in-flight'. Also, the patch seems to have two independent bug-fixes >>>> in it. Second one doesn't look right, at least not without an explanation. >>>> >>>> First: >>>> >>>>> - if (tdc->busy) >>>>> - tegra_dma_terminate_all(dc); >>>>> + tegra_dma_terminate_all(dc); >>>> >>>> Second: >>>> >>>>> + tasklet_kill(&tdc->tasklet); >>> >>> BTW, maybe you can convert the code to threaded interrupt handler and >>> just get rid of the tasklet instead of fixing it? >> >> This shouldn't bring much benefit because the the code's logic won't be >> changed since we will still have to use the threaded ISR part as the >> bottom-half and then IRQ API doesn't provide a nice way to synchronize >> interrupt's execution, while tasklet_kill() is a nice way to sync it. > > What about synchronize_irq()? Good point! I totally forgot about it. The only difference between tasklet and threaded ISR should be that hardware interrupt is masked during of the threaded ISR execution, but at quick glance it shouldn't be a problem. BTW, I'm now thinking that the current code is wrong by accumulating callbacks count in ISR if callback's execution takes too much time, not sure that it's something what DMA clients expect to happen, will try to verify that. It also will be nice to get rid of the free list since it only complicates code without any real benefits, I actually checked that kmalloc doesn't introduce any noticeable latency at all. I'll probably defer the above changes for now, leaving them for 5.7, otherwise it could be a bit too many changes for this patchset (hopefully it will get into 5.6). > BTW, does tegra_dma_terminate_all() prevent further interrupts that might > cause the tasklet to be scheduled again? Yes, it should prevent further interrupts because it stops hardware and clears interrupt status, thus in a worst case ISR could emit "Interrupt already served status" message.
