| Message ID | 1467730478-9696-17-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Dear Vinod On 5 July 2016 at 22:54, 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: Zhangfei Gao <zhangfei.gao@linaro.org> > Cc: Daniel Mack <zonque@gmail.com> > --- > drivers/dma/mmp_pdma.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c > index 56f1fd68b620..975bc4bf525b 100644 > --- a/drivers/dma/mmp_pdma.c > +++ b/drivers/dma/mmp_pdma.c > @@ -931,6 +931,23 @@ static void dma_do_tasklet(unsigned long data) > static int mmp_pdma_remove(struct platform_device *op) > { > struct mmp_pdma_device *pdev = platform_get_drvdata(op); > + struct mmp_pdma_phy *phy; > + int i, irq = 0, irq_num = 0; > + > + > + irq = platform_get_irq(op, 0); > + devm_free_irq(&op->dev, irq, pdev); > + > + for (i = 0; i < pdev->dma_channels; i++) { > + if (platform_get_irq(op, i) > 0) > + irq_num++; > + } > + > + for (i = 0; i < pdev->dma_channels; i++) { > + phy = &pdev->phy[i]; > + irq = (irq_num != pdev->dma_channels) ? 0 : platform_get_irq(op, i); > + devm_free_irq(&op->dev, irq, phy); > + } > Is it possible irq=0 is freed twice? Just browse the code, there are two cases. Documentation/devicetree/bindings/dma/mmp-dma.txt /* * Each channel has specific irq * ICU parse out irq channel from ICU register, * while DMA controller may not able to distinguish the irq channel * Using this method, interrupt-parent is required as demuxer * For example, pxa688 icu register 0x128, bit 0~15 is PDMA channel irq, * 18~21 is ADMA irq */ pdma: dma-controller@d4000000 { compatible = "marvell,pdma-1.0"; reg = <0xd4000000 0x10000>; interrupts = <0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15>; interrupt-parent = <&intcmux32>; #dma-channels = <16>; }; /* * One irq for all channels * Dmaengine driver (DMA controller) distinguish irq channel via * parsing internal register */ pdma: dma-controller@d4000000 { compatible = "marvell,pdma-1.0"; reg = <0xd4000000 0x10000>; interrupts = <47>; #dma-channels = <16>; }; if (irq_num != dma_channels) { // share one irq irq = platform_get_irq(op, 0); devm_free_irq(&op->dev, irq, pdev); } else { // each channel has specific irq for (i = 0; i < dma_channels; i++) { irq = platform_get_irq(op, i); phy = &pdev->phy[i]; devm_free_irq(&op->dev, irq, phy); } } Thanks Zhangfei -- 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 Fri, Jul 08, 2016 at 10:40:42AM +0800, Zhangfei Gao wrote: > Dear Vinod > > On 5 July 2016 at 22:54, 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: Zhangfei Gao <zhangfei.gao@linaro.org> > > Cc: Daniel Mack <zonque@gmail.com> > > --- > > drivers/dma/mmp_pdma.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c > > index 56f1fd68b620..975bc4bf525b 100644 > > --- a/drivers/dma/mmp_pdma.c > > +++ b/drivers/dma/mmp_pdma.c > > @@ -931,6 +931,23 @@ static void dma_do_tasklet(unsigned long data) > > static int mmp_pdma_remove(struct platform_device *op) > > { > > struct mmp_pdma_device *pdev = platform_get_drvdata(op); > > + struct mmp_pdma_phy *phy; > > + int i, irq = 0, irq_num = 0; > > + > > + > > + irq = platform_get_irq(op, 0); > > + devm_free_irq(&op->dev, irq, pdev); > > + > > + for (i = 0; i < pdev->dma_channels; i++) { > > + if (platform_get_irq(op, i) > 0) > > + irq_num++; > > + } > > + > > + for (i = 0; i < pdev->dma_channels; i++) { > > + phy = &pdev->phy[i]; > > + irq = (irq_num != pdev->dma_channels) ? 0 : platform_get_irq(op, i); > > + devm_free_irq(&op->dev, irq, phy); > > + } > > > Is it possible irq=0 is freed twice? Yes I see the point, we should do: for (i = 0; i < pdev->dma_channels; i++) { if (platform_get_irq(op, i) > 0) irq_num++; } if (irq_num != pdev->dma_channels) { irq = platform_get_irq(op, 0); devm_free_irq(&op->dev, irq, pdev); } else { for (i = 0; i < pdev->dma_channels; i++) { phy = &pdev->phy[i]; irq = (irq_num != pdev->dma_channels) ? 0 : platform_get_irq(op, i); devm_free_irq(&op->dev, irq, phy); } } Thanks
On 8 July 2016 at 11:51, Vinod Koul <vinod.koul@intel.com> wrote: > On Fri, Jul 08, 2016 at 10:40:42AM +0800, Zhangfei Gao wrote: >> Dear Vinod >> >> On 5 July 2016 at 22:54, 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: Zhangfei Gao <zhangfei.gao@linaro.org> >> > Cc: Daniel Mack <zonque@gmail.com> >> > --- >> > drivers/dma/mmp_pdma.c | 17 +++++++++++++++++ >> > 1 file changed, 17 insertions(+) >> > >> > diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c >> > index 56f1fd68b620..975bc4bf525b 100644 >> > --- a/drivers/dma/mmp_pdma.c >> > +++ b/drivers/dma/mmp_pdma.c >> > @@ -931,6 +931,23 @@ static void dma_do_tasklet(unsigned long data) >> > static int mmp_pdma_remove(struct platform_device *op) >> > { >> > struct mmp_pdma_device *pdev = platform_get_drvdata(op); >> > + struct mmp_pdma_phy *phy; >> > + int i, irq = 0, irq_num = 0; >> > + >> > + >> > + irq = platform_get_irq(op, 0); >> > + devm_free_irq(&op->dev, irq, pdev); >> > + >> > + for (i = 0; i < pdev->dma_channels; i++) { >> > + if (platform_get_irq(op, i) > 0) >> > + irq_num++; >> > + } >> > + >> > + for (i = 0; i < pdev->dma_channels; i++) { >> > + phy = &pdev->phy[i]; >> > + irq = (irq_num != pdev->dma_channels) ? 0 : platform_get_irq(op, i); >> > + devm_free_irq(&op->dev, irq, phy); >> > + } >> > >> Is it possible irq=0 is freed twice? > > Yes I see the point, we should do: > > for (i = 0; i < pdev->dma_channels; i++) { > if (platform_get_irq(op, i) > 0) > irq_num++; > } > > if (irq_num != pdev->dma_channels) { > irq = platform_get_irq(op, 0); > devm_free_irq(&op->dev, irq, pdev); > } > else { > for (i = 0; i < pdev->dma_channels; i++) { > phy = &pdev->phy[i]; > irq = (irq_num != pdev->dma_channels) ? 0 : platform_get_irq(op, i); here irq = platform_get_irq(op, i); should be OK. Thanks -- 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
Hi Vinod, On 07/05/2016 04:54 PM, Vinod Koul wrote: > dmaengine device should explicitly call devm_free_irq() when using > devm_reqister_irq(). You mean devm_request_irq(), right? The whole point about these devm_* functions is that the resources claimed by them are automatically freed on driver removal, so I wonder why it is necessary to call them explicitly for this driver. What am I missing? Thanks, Daniel > > 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: Zhangfei Gao <zhangfei.gao@linaro.org> > Cc: Daniel Mack <zonque@gmail.com> > --- > drivers/dma/mmp_pdma.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c > index 56f1fd68b620..975bc4bf525b 100644 > --- a/drivers/dma/mmp_pdma.c > +++ b/drivers/dma/mmp_pdma.c > @@ -931,6 +931,23 @@ static void dma_do_tasklet(unsigned long data) > static int mmp_pdma_remove(struct platform_device *op) > { > struct mmp_pdma_device *pdev = platform_get_drvdata(op); > + struct mmp_pdma_phy *phy; > + int i, irq = 0, irq_num = 0; > + > + > + irq = platform_get_irq(op, 0); > + devm_free_irq(&op->dev, irq, pdev); > + > + for (i = 0; i < pdev->dma_channels; i++) { > + if (platform_get_irq(op, i) > 0) > + irq_num++; > + } > + > + for (i = 0; i < pdev->dma_channels; i++) { > + phy = &pdev->phy[i]; > + irq = (irq_num != pdev->dma_channels) ? 0 : platform_get_irq(op, i); > + devm_free_irq(&op->dev, irq, phy); > + } > > dma_async_device_unregister(&pdev->device); > return 0; > -- 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 Fri, Jul 08, 2016 at 10:52:15AM +0200, Daniel Mack wrote: > Hi Vinod, > > On 07/05/2016 04:54 PM, Vinod Koul wrote: > > dmaengine device should explicitly call devm_free_irq() when using > > devm_reqister_irq(). > > You mean devm_request_irq(), right? Yes, thats for spotting :) > The whole point about these devm_* > functions is that the resources claimed by them are automatically freed > on driver removal, so I wonder why it is necessary to call them > explicitly for this driver. What am I missing? spurious irq's! I do not advise using devm_register_irq() and if someone does, I always ask them to explicitly freeup or disable in the .remove.
On 07/14/2016 07:52 AM, Vinod Koul wrote: > On Fri, Jul 08, 2016 at 10:52:15AM +0200, Daniel Mack wrote: >> Hi Vinod, >> >> On 07/05/2016 04:54 PM, Vinod Koul wrote: >>> dmaengine device should explicitly call devm_free_irq() when using >>> devm_reqister_irq(). >> >> You mean devm_request_irq(), right? > > Yes, thats for spotting :) > >> The whole point about these devm_* >> functions is that the resources claimed by them are automatically freed >> on driver removal, so I wonder why it is necessary to call them >> explicitly for this driver. What am I missing? > > spurious irq's! > > I do not advise using devm_register_irq() and if someone does, I always ask > them to explicitly freeup or disable in the .remove. > Ok, that's right, thanks for explaining. I wonder if it was a good idea to add a helper function for devres which disables all IRQs allocated with devm_request*_irq(), so it can be called from all drivers? Daniel -- 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/mmp_pdma.c b/drivers/dma/mmp_pdma.c index 56f1fd68b620..975bc4bf525b 100644 --- a/drivers/dma/mmp_pdma.c +++ b/drivers/dma/mmp_pdma.c @@ -931,6 +931,23 @@ static void dma_do_tasklet(unsigned long data) static int mmp_pdma_remove(struct platform_device *op) { struct mmp_pdma_device *pdev = platform_get_drvdata(op); + struct mmp_pdma_phy *phy; + int i, irq = 0, irq_num = 0; + + + irq = platform_get_irq(op, 0); + devm_free_irq(&op->dev, irq, pdev); + + for (i = 0; i < pdev->dma_channels; i++) { + if (platform_get_irq(op, i) > 0) + irq_num++; + } + + for (i = 0; i < pdev->dma_channels; i++) { + phy = &pdev->phy[i]; + irq = (irq_num != pdev->dma_channels) ? 0 : platform_get_irq(op, i); + devm_free_irq(&op->dev, irq, phy); + } dma_async_device_unregister(&pdev->device); return 0;
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: Zhangfei Gao <zhangfei.gao@linaro.org> Cc: Daniel Mack <zonque@gmail.com> --- drivers/dma/mmp_pdma.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)