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