| Message ID | 1467730478-9696-2-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Jul 5, 2016 at 4:54 PM, Vinod Koul <vinod.koul@intel.com> 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: Linus Walleij <linus.walleij@linaro.org> Acked-by: Linus Walleij <linus.walleij@linaro.org> This patch makes me very uneasy. A spurious IRQ can always happen, so Isn't this problem existing on more or less the .remove() path of 95% if all drivers using devm_request_irq() in the entire kernel? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 05, 2016 at 11:10:22PM +0200, Linus Walleij wrote: > On Tue, Jul 5, 2016 at 4:54 PM, Vinod Koul <vinod.koul@intel.com> 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: Linus Walleij <linus.walleij@linaro.org> > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > This patch makes me very uneasy. A spurious IRQ can always happen, so > Isn't this problem existing on more or less the .remove() path of 95% > if all drivers > using devm_request_irq() in the entire kernel? Yes it is indeed a problem. IMHO we should never have done devm variant for irq APIs. I have been pushing back on drivers not freeing up irqs in .remove. Maybe we should have devm variants made deprecated and discourage the use of these.. Thanks
On 07/06/2016 06:31 PM, Vinod Koul wrote: > On Tue, Jul 05, 2016 at 11:10:22PM +0200, Linus Walleij wrote: >> On Tue, Jul 5, 2016 at 4:54 PM, Vinod Koul <vinod.koul@intel.com> 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: Linus Walleij <linus.walleij@linaro.org> >> >> Acked-by: Linus Walleij <linus.walleij@linaro.org> >> >> This patch makes me very uneasy. A spurious IRQ can always happen, so >> Isn't this problem existing on more or less the .remove() path of 95% >> if all drivers >> using devm_request_irq() in the entire kernel? > > Yes it is indeed a problem. IMHO we should never have done devm variant > for irq APIs. I have been pushing back on drivers not freeing up irqs in > .remove. > > Maybe we should have devm variants made deprecated and discourage the > use of these.. There are safe use-cases for devm_request_irq(), but I agree by default it should be avoided since it is really easy to get it wrong. In general any resource that is accessed in the interrupt handler needs to be available until after the IRQ has been freed. This is regardless of whether devm_ is used or not. The rule of thumb for the devm case is that it is only safe to use it if free_irq() is the last line in your remove function. E.g. static int probe(...) { ... dev_state_struct = kzalloc(); ... request_irq(...); ... return 0; } static int remove(...) { ... free_irq(...); kfree(dev_state_struct); return 0; } is not save to convert (assuming dev_state_struct is used in the IRQ handler) to devm_request_irq since that would change the order in which resources are released. Switching to devm would free the memory before the IRQ which can trigger a use after free. On the other hand static int probe(...) { ... dev_state_struct = devm_kzalloc(); ... request_irq(...); ... return 0; } static int remove(...) { ... free_irq(...); return 0; } is safe to switch to devm_request_irq() since it will not change the ordering since free_irq() is already the last instruction executed in the remove function. devm will make sure that deallocation happens in the opposite order of allocation. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 6, 2016 at 6:37 PM, Lars-Peter Clausen <lars@metafoo.de> wrote: > On the other hand > > static int probe(...) > { > ... > dev_state_struct = devm_kzalloc(); > ... > request_irq(...); > ... > return 0; > } > > > > static int remove(...) > { > ... > free_irq(...); > return 0; > } > > is safe to switch to devm_request_irq() since it will not change the > ordering since free_irq() is already the last instruction executed in the > remove function. devm will make sure that deallocation happens in the > opposite order of allocation. I think that the common pattern you will see is that this latter case actually only happens in the few cases where NULL is passed as data to the [devm_]request_irq() registration call, and the driver does not have a state container at all. I think that is quite uncommon and mostly happens in legacy code, but I may be wrong.... I think we have a big bunch of cleanups to do wrt this, in my book devm_request_irq() is considered harmful from now on, thanks for putting this in the spotlight. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/coh901318.c b/drivers/dma/coh901318.c index c1006165cea8..ba044d4c1d53 100644 --- a/drivers/dma/coh901318.c +++ b/drivers/dma/coh901318.c @@ -1280,6 +1280,7 @@ struct coh901318_desc { struct coh901318_base { struct device *dev; void __iomem *virtbase; + unsigned int irq; struct coh901318_pool pool; struct powersave pm; struct dma_device dma_slave; @@ -2680,6 +2681,8 @@ static int __init coh901318_probe(struct platform_device *pdev) if (err) return err; + base->irq = irq; + err = coh901318_pool_create(&base->pool, &pdev->dev, sizeof(struct coh901318_lli), 32); @@ -2760,6 +2763,8 @@ static int coh901318_remove(struct platform_device *pdev) { struct coh901318_base *base = platform_get_drvdata(pdev); + devm_free_irq(&pdev->dev, base->irq, base); + of_dma_controller_free(pdev->dev.of_node); dma_async_device_unregister(&base->dma_memcpy); dma_async_device_unregister(&base->dma_slave);
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: Linus Walleij <linus.walleij@linaro.org> --- drivers/dma/coh901318.c | 5 +++++ 1 file changed, 5 insertions(+)