On Wed, Jul 06, 2016 at 08:52:12PM +0200, Robert Jarzmik wrote: > Vinod Koul <vinod.koul@xxxxxxxxx> writes: > > > dmaengine device should explicitly call devm_free_irq() when using > > devm_reqister_irq(). > > > > The irq is still ON when devices remove is executed and irq should be > > quiesced before remove is completed. > > Hi Vinod, > > I understand this patch, but I'd like to know if this is the best fix. > > Let me explain how I see a race in remove() path : > - as long as there is one dma channel requested, ie. dma_chan_get() but no > dma_chan_put(), the try_module_get() of dma_chan_get() prevents the remove() > routine from running > - when the last channel is released, ie. the last dma_chan_put() is called, if > there is a running DMA, a "mess-up" window appears > => this is because pxa_dma doesn't implement device_synchronise() > => this is where I think an improvement could be made > - if dmaengine_synchronize() is implemented, and the last descriptor is > finished, the DMA irq cannot fire anymore on pxa (phy_disable() done on all > phys implies DMA irq cannot fire anymore). > > So I'd rather have pxad_device.slave.dmaengine_synchronize() implemented to > solve the race issue, and I do volunteer for that. Please do, patch is always welcome :) > Now there might be something I'm not seeing that you've observed on other dma > IPs, so I'd like to know if I'm not missing another usecase where a race might > appear. Suprious irqs. In case your irq fires somehow when you are not prepared can least to nasty crashes... > PS: I'm not very proud of myself about for loop and the interrupts allocation, > so seeing it another time in the release routine is a bit painful :) yeah had to so it for few ones :( -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
