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
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
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
