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