| Message ID | 1467730478-9696-25-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Vinod Koul <vinod.koul@intel.com> 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. 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. Please tell me your thoughts. Cheers. -- Robert 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 :) -- 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 Wed, Jul 06, 2016 at 08:52:12PM +0200, Robert Jarzmik wrote: > Vinod Koul <vinod.koul@intel.com> 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 :(
diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c index e756a30ccba2..1a1c515e8120 100644 --- a/drivers/dma/pxa_dma.c +++ b/drivers/dma/pxa_dma.c @@ -1279,11 +1279,33 @@ static void pxad_free_channels(struct dma_device *dmadev) } } +static void pxad_free_irq(struct platform_device *op, struct pxad_device *pdev) +{ + struct pxad_phy *phy; + int nr_irq = 0, irq, irq0, i; + + irq0 = platform_get_irq(op, 0); + for (i = 0; i < pdev->nr_chans; i++) + if (platform_get_irq(op, i) > 0) + nr_irq++; + + for (i = 0; i < pdev->nr_chans; i++) { + phy = &pdev->phys[i]; + irq = platform_get_irq(op, i); + if ((nr_irq > 1) && (irq > 0)) + devm_free_irq(&op->dev, irq, phy); + + if ((nr_irq == 1) && (i == 0)) + devm_free_irq(&op->dev, irq0, pdev); + } +} + static int pxad_remove(struct platform_device *op) { struct pxad_device *pdev = platform_get_drvdata(op); pxad_cleanup_debugfs(pdev); + pxad_free_irq(op, pdev); pxad_free_channels(&pdev->slave); dma_async_device_unregister(&pdev->slave); return 0;
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. Signed-off-by: Vinod Koul <vinod.koul@intel.com> Cc: Robert Jarzmik <robert.jarzmik@free.fr> --- drivers/dma/pxa_dma.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)