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
--
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
