| Message ID | 1467730478-9696-12-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Jul 05, 2016 at 08:24:17PM +0530, Vinod Koul wrote: > 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: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Linus Walleij <linus.walleij@linaro.org> > --- > +static void imxdma_free_irq(struct platform_device *pdev, struct imxdma_engine *imxdma) > +{ > + int i; > + > + if (is_imx1_dma(imxdma)) { > + devm_free_irq(&pdev->dev, imxdma->irq, imxdma); > + devm_free_irq(&pdev->dev, imxdma->irq_err, imxdma); > + } > + > + for (i = 0; i < IMX_DMA_CHANNELS; i++) { > + struct imxdma_channel *imxdmac = &imxdma->channel[i]; > + > + if (!is_imx1_dma(imxdma)) > + devm_free_irq(&pdev->dev, imxdmac->irq, imxdma); > + } > +} > + > static int imxdma_remove(struct platform_device *pdev) > { > struct imxdma_engine *imxdma = platform_get_drvdata(pdev); > > + imxdma_free_irq(pdev, imxdma); > + I don't think this is a good idea for various reasons. When it's not safe to rely on the interrupt being released by devm later then shouldn't we explicitly release the interrupt in the probe error path aswell? Then if we don't let devm release the irq what's the point in using devm_request_irq at all? Also what's the case in which we have to explicitly release the irq? The imx-dma driver explicitly disables the interrupts for deactivated channels and I assume that when the drivers remove() function is called every channel *is* deactivated. If it's not then we are in big trouble since then we must assume that also a DMA transfer might be ongoing which will surely lead to trouble when the driver is removed. I think that when this patch solves an issue we really have a problem elsewhere, including that most users of devm_request_irq are wrong and this function maybe shouldn't even exist. Sascha
On Wed, Jul 06, 2016 at 11:02:06AM +0200, Sascha Hauer wrote: > > static int imxdma_remove(struct platform_device *pdev) > > { > > struct imxdma_engine *imxdma = platform_get_drvdata(pdev); > > > > + imxdma_free_irq(pdev, imxdma); > > + > > I don't think this is a good idea for various reasons. > > When it's not safe to rely on the interrupt being released by devm later > then shouldn't we explicitly release the interrupt in the probe error > path aswell? Then if we don't let devm release the irq what's the point > in using devm_request_irq at all? IMHO devm_request_irq was not really a good idea, it helps, but is very easy to get it wrong, which seems to be the usual case. > Also what's the case in which we have to explicitly release the irq? The > imx-dma driver explicitly disables the interrupts for deactivated > channels and I assume that when the drivers remove() function is called > every channel *is* deactivated. If it's not then we are in big trouble > since then we must assume that also a DMA transfer might be ongoing which > will surely lead to trouble when the driver is removed. DMA transfer cannot be running at that time. That is not the point. Sure you may have set the controller to not fire again, but does it really protect you against spurious interrupts. The point is that, there is no guarantee, so we are better off freeing up explicitly so that it doesn't fire when remove is completed. Same is true for tasklets. Btw I would have been okay if the driver disabled irq in remove, few of them do that and I haven't tinkered those.. Thanks
On Wed, Jul 06, 2016 at 10:37:22PM +0530, Vinod Koul wrote: > On Wed, Jul 06, 2016 at 11:02:06AM +0200, Sascha Hauer wrote: > > > static int imxdma_remove(struct platform_device *pdev) > > > { > > > struct imxdma_engine *imxdma = platform_get_drvdata(pdev); > > > > > > + imxdma_free_irq(pdev, imxdma); > > > + > > > > I don't think this is a good idea for various reasons. > > > > When it's not safe to rely on the interrupt being released by devm later > > then shouldn't we explicitly release the interrupt in the probe error > > path aswell? Then if we don't let devm release the irq what's the point > > in using devm_request_irq at all? > > IMHO devm_request_irq was not really a good idea, it helps, but is very > easy to get it wrong, which seems to be the usual case. > > > Also what's the case in which we have to explicitly release the irq? The > > imx-dma driver explicitly disables the interrupts for deactivated > > channels and I assume that when the drivers remove() function is called > > every channel *is* deactivated. If it's not then we are in big trouble > > since then we must assume that also a DMA transfer might be ongoing which > > will surely lead to trouble when the driver is removed. > > DMA transfer cannot be running at that time. That is not the point. > > Sure you may have set the controller to not fire again, but does it > really protect you against spurious interrupts. > > The point is that, there is no guarantee, so we are better off freeing > up explicitly so that it doesn't fire when remove is completed. Same is > true for tasklets. > > Btw I would have been okay if the driver disabled irq in remove, few of > them do that and I haven't tinkered those.. Wouldn't it then be better to use disable_irq() instead of releasing it? This would make the intention clearer. Sascha
On Fri, Jul 08, 2016 at 07:34:23AM +0200, Sascha Hauer wrote: > On Wed, Jul 06, 2016 at 10:37:22PM +0530, Vinod Koul wrote: > > On Wed, Jul 06, 2016 at 11:02:06AM +0200, Sascha Hauer wrote: > > > > static int imxdma_remove(struct platform_device *pdev) > > > > { > > > > struct imxdma_engine *imxdma = platform_get_drvdata(pdev); > > > > > > > > + imxdma_free_irq(pdev, imxdma); > > > > + > > > > > > I don't think this is a good idea for various reasons. > > > > > > When it's not safe to rely on the interrupt being released by devm later > > > then shouldn't we explicitly release the interrupt in the probe error > > > path aswell? Then if we don't let devm release the irq what's the point > > > in using devm_request_irq at all? > > > > IMHO devm_request_irq was not really a good idea, it helps, but is very > > easy to get it wrong, which seems to be the usual case. > > > > > Also what's the case in which we have to explicitly release the irq? The > > > imx-dma driver explicitly disables the interrupts for deactivated > > > channels and I assume that when the drivers remove() function is called > > > every channel *is* deactivated. If it's not then we are in big trouble > > > since then we must assume that also a DMA transfer might be ongoing which > > > will surely lead to trouble when the driver is removed. > > > > DMA transfer cannot be running at that time. That is not the point. > > > > Sure you may have set the controller to not fire again, but does it > > really protect you against spurious interrupts. > > > > The point is that, there is no guarantee, so we are better off freeing > > up explicitly so that it doesn't fire when remove is completed. Same is > > true for tasklets. > > > > Btw I would have been okay if the driver disabled irq in remove, few of > > them do that and I haven't tinkered those.. > > Wouldn't it then be better to use disable_irq() instead of releasing it? > This would make the intention clearer. That would solve the problem as well. Either way it doesn't matter to me. Let me know your preference and I will change this accordingly Thanks
On Fri, Jul 08, 2016 at 01:13:05PM +0530, Vinod Koul wrote: > On Fri, Jul 08, 2016 at 07:34:23AM +0200, Sascha Hauer wrote: > > On Wed, Jul 06, 2016 at 10:37:22PM +0530, Vinod Koul wrote: > > > On Wed, Jul 06, 2016 at 11:02:06AM +0200, Sascha Hauer wrote: > > > > > static int imxdma_remove(struct platform_device *pdev) > > > > > { > > > > > struct imxdma_engine *imxdma = platform_get_drvdata(pdev); > > > > > > > > > > + imxdma_free_irq(pdev, imxdma); > > > > > + > > > > > > > > I don't think this is a good idea for various reasons. > > > > > > > > When it's not safe to rely on the interrupt being released by devm later > > > > then shouldn't we explicitly release the interrupt in the probe error > > > > path aswell? Then if we don't let devm release the irq what's the point > > > > in using devm_request_irq at all? > > > > > > IMHO devm_request_irq was not really a good idea, it helps, but is very > > > easy to get it wrong, which seems to be the usual case. > > > > > > > Also what's the case in which we have to explicitly release the irq? The > > > > imx-dma driver explicitly disables the interrupts for deactivated > > > > channels and I assume that when the drivers remove() function is called > > > > every channel *is* deactivated. If it's not then we are in big trouble > > > > since then we must assume that also a DMA transfer might be ongoing which > > > > will surely lead to trouble when the driver is removed. > > > > > > DMA transfer cannot be running at that time. That is not the point. > > > > > > Sure you may have set the controller to not fire again, but does it > > > really protect you against spurious interrupts. > > > > > > The point is that, there is no guarantee, so we are better off freeing > > > up explicitly so that it doesn't fire when remove is completed. Same is > > > true for tasklets. > > > > > > Btw I would have been okay if the driver disabled irq in remove, few of > > > them do that and I haven't tinkered those.. > > > > Wouldn't it then be better to use disable_irq() instead of releasing it? > > This would make the intention clearer. > > That would solve the problem as well. Either way it doesn't matter to me. > > Let me know your preference and I will change this accordingly I prefer disable_irq(). Regards Sascha
diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c index 48d85f8b95fe..aed0b77285da 100644 --- a/drivers/dma/imx-dma.c +++ b/drivers/dma/imx-dma.c @@ -167,6 +167,7 @@ struct imxdma_channel { u32 ccr_to_device; bool enabled_2d; int slot_2d; + unsigned int irq; }; enum imx_dma_type { @@ -186,6 +187,9 @@ struct imxdma_engine { struct imx_dma_2d_config slots_2d[IMX_DMA_2D_SLOTS]; struct imxdma_channel channel[IMX_DMA_CHANNELS]; enum imx_dma_type devtype; + unsigned int irq; + unsigned int irq_err; + }; struct imxdma_filter_data { @@ -1100,6 +1104,7 @@ static int __init imxdma_probe(struct platform_device *pdev) dev_warn(imxdma->dev, "Can't register IRQ for DMA\n"); goto disable_dma_ahb_clk; } + imxdma->irq = irq; irq_err = platform_get_irq(pdev, 1); if (irq_err < 0) { @@ -1113,6 +1118,7 @@ static int __init imxdma_probe(struct platform_device *pdev) dev_warn(imxdma->dev, "Can't register ERRIRQ for DMA\n"); goto disable_dma_ahb_clk; } + imxdma->irq_err = irq_err; } /* enable DMA module */ @@ -1150,6 +1156,8 @@ static int __init imxdma_probe(struct platform_device *pdev) irq + i, i); goto disable_dma_ahb_clk; } + + imxdmac->irq = irq + i; init_timer(&imxdmac->watchdog); imxdmac->watchdog.function = &imxdma_watchdog; imxdmac->watchdog.data = (unsigned long)imxdmac; @@ -1217,10 +1225,29 @@ disable_dma_ipg_clk: return ret; } +static void imxdma_free_irq(struct platform_device *pdev, struct imxdma_engine *imxdma) +{ + int i; + + if (is_imx1_dma(imxdma)) { + devm_free_irq(&pdev->dev, imxdma->irq, imxdma); + devm_free_irq(&pdev->dev, imxdma->irq_err, imxdma); + } + + for (i = 0; i < IMX_DMA_CHANNELS; i++) { + struct imxdma_channel *imxdmac = &imxdma->channel[i]; + + if (!is_imx1_dma(imxdma)) + devm_free_irq(&pdev->dev, imxdmac->irq, imxdma); + } +} + static int imxdma_remove(struct platform_device *pdev) { struct imxdma_engine *imxdma = platform_get_drvdata(pdev); + imxdma_free_irq(pdev, imxdma); + dma_async_device_unregister(&imxdma->dma_device); if (pdev->dev.of_node)
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: Sascha Hauer <s.hauer@pengutronix.de> Cc: Linus Walleij <linus.walleij@linaro.org> --- drivers/dma/imx-dma.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)