| Message ID | 1394089610-3314-2-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ccc7aad04c95 |
| Headers | show |
On Thu, 6 Mar 2014, Vinod Koul wrote: > As discussed in [1] the tasklet_disable is not a proper function for teardown. > The driver also uses correct method of tasklet_kill. So remove tasklet_disable > > [1]: http://lwn.net/Articles/588457/ > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > --- > drivers/dma/at_hdmac.c | 1 - > 1 files changed, 0 insertions(+), 1 deletions(-) > > diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c > index e2c04dc..c13a3bb 100644 > --- a/drivers/dma/at_hdmac.c > +++ b/drivers/dma/at_hdmac.c > @@ -1569,7 +1569,6 @@ static int at_dma_remove(struct platform_device *pdev) > > /* Disable interrupts */ > atc_disable_chan_irq(atdma, chan->chan_id); > - tasklet_disable(&atchan->tasklet); > > tasklet_kill(&atchan->tasklet); > list_del(&chan->device_node); So you disable the irq at the device level, but what makes sure that the last interrupt on that line has completed and no further scheduling of the tasklet can happen? See [1] :) Thanks, tglx -- 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 Thu, Mar 06, 2014 at 12:26:38PM +0100, Thomas Gleixner wrote: > > > On Thu, 6 Mar 2014, Vinod Koul wrote: > > > As discussed in [1] the tasklet_disable is not a proper function for teardown. > > The driver also uses correct method of tasklet_kill. So remove tasklet_disable > > > > [1]: http://lwn.net/Articles/588457/ > > > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > --- > > drivers/dma/at_hdmac.c | 1 - > > 1 files changed, 0 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c > > index e2c04dc..c13a3bb 100644 > > --- a/drivers/dma/at_hdmac.c > > +++ b/drivers/dma/at_hdmac.c > > @@ -1569,7 +1569,6 @@ static int at_dma_remove(struct platform_device *pdev) > > > > /* Disable interrupts */ > > atc_disable_chan_irq(atdma, chan->chan_id); > > - tasklet_disable(&atchan->tasklet); > > > > tasklet_kill(&atchan->tasklet); > > list_del(&chan->device_node); > > So you disable the irq at the device level, but what makes sure that > the last interrupt on that line has completed and no further > scheduling of the tasklet can happen? See [1] :) Yes I missed that will fix in v2
On Thu, Mar 06, 2014 at 12:26:38PM +0100, Thomas Gleixner wrote: > > > On Thu, 6 Mar 2014, Vinod Koul wrote: > > > As discussed in [1] the tasklet_disable is not a proper function for teardown. > > The driver also uses correct method of tasklet_kill. So remove tasklet_disable > > > > [1]: http://lwn.net/Articles/588457/ > > > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > --- > > drivers/dma/at_hdmac.c | 1 - > > 1 files changed, 0 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c > > index e2c04dc..c13a3bb 100644 > > --- a/drivers/dma/at_hdmac.c > > +++ b/drivers/dma/at_hdmac.c > > @@ -1569,7 +1569,6 @@ static int at_dma_remove(struct platform_device *pdev) > > > > /* Disable interrupts */ > > atc_disable_chan_irq(atdma, chan->chan_id); > > - tasklet_disable(&atchan->tasklet); > > > > tasklet_kill(&atchan->tasklet); > > list_del(&chan->device_node); > > So you disable the irq at the device level, but what makes sure that > the last interrupt on that line has completed and no further > scheduling of the tasklet can happen? See [1] :) Okay the irq is freed here. The free_irq() will ensure that the last interrupt has completed. At least that's what documentation says "The function does not return until any executing interrupts for this IRQ have completed" and if i read the code correctly it does so as well. So wouldn't that take care of no further scheduling of tasklet here or I missed something? Then, the tasklet_kill will ensure the scheduled ones are completed as well
On Fri, 7 Mar 2014, Vinod Koul wrote: > On Thu, Mar 06, 2014 at 12:26:38PM +0100, Thomas Gleixner wrote: > > > > > > On Thu, 6 Mar 2014, Vinod Koul wrote: > > > > > As discussed in [1] the tasklet_disable is not a proper function for teardown. > > > The driver also uses correct method of tasklet_kill. So remove tasklet_disable > > > > > > [1]: http://lwn.net/Articles/588457/ > > > > > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > --- > > > drivers/dma/at_hdmac.c | 1 - > > > 1 files changed, 0 insertions(+), 1 deletions(-) > > > > > > diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c > > > index e2c04dc..c13a3bb 100644 > > > --- a/drivers/dma/at_hdmac.c > > > +++ b/drivers/dma/at_hdmac.c > > > @@ -1569,7 +1569,6 @@ static int at_dma_remove(struct platform_device *pdev) > > > > > > /* Disable interrupts */ > > > atc_disable_chan_irq(atdma, chan->chan_id); > > > - tasklet_disable(&atchan->tasklet); > > > > > > tasklet_kill(&atchan->tasklet); > > > list_del(&chan->device_node); > > > > So you disable the irq at the device level, but what makes sure that > > the last interrupt on that line has completed and no further > > scheduling of the tasklet can happen? See [1] :) > Okay the irq is freed here. The free_irq() will ensure that the last interrupt has > completed. At least that's what documentation says "The function does not return > until any executing interrupts for this IRQ have completed" and if i read the > code correctly it does so as well. > > So wouldn't that take care of no further scheduling of tasklet here or I missed > something? > > Then, the tasklet_kill will ensure the scheduled ones are completed as well I was just asking. The free_irq() does the trick, yes. Thanks, tglx -- 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, Mar 07, 2014 at 07:15:12PM +0100, Thomas Gleixner wrote: > On Fri, 7 Mar 2014, Vinod Koul wrote: > > > On Thu, Mar 06, 2014 at 12:26:38PM +0100, Thomas Gleixner wrote: > > > > > > > > > On Thu, 6 Mar 2014, Vinod Koul wrote: > > > > > > > As discussed in [1] the tasklet_disable is not a proper function for teardown. > > > > The driver also uses correct method of tasklet_kill. So remove tasklet_disable > > > > > > > > [1]: http://lwn.net/Articles/588457/ > > > > > > > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > > --- > > > > drivers/dma/at_hdmac.c | 1 - > > > > 1 files changed, 0 insertions(+), 1 deletions(-) > > > > > > > > diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c > > > > index e2c04dc..c13a3bb 100644 > > > > --- a/drivers/dma/at_hdmac.c > > > > +++ b/drivers/dma/at_hdmac.c > > > > @@ -1569,7 +1569,6 @@ static int at_dma_remove(struct platform_device *pdev) > > > > > > > > /* Disable interrupts */ > > > > atc_disable_chan_irq(atdma, chan->chan_id); > > > > - tasklet_disable(&atchan->tasklet); > > > > > > > > tasklet_kill(&atchan->tasklet); > > > > list_del(&chan->device_node); > > > > > > So you disable the irq at the device level, but what makes sure that > > > the last interrupt on that line has completed and no further > > > scheduling of the tasklet can happen? See [1] :) > > Okay the irq is freed here. The free_irq() will ensure that the last interrupt has > > completed. At least that's what documentation says "The function does not return > > until any executing interrupts for this IRQ have completed" and if i read the > > code correctly it does so as well. > > > > So wouldn't that take care of no further scheduling of tasklet here or I missed > > something? > > > > Then, the tasklet_kill will ensure the scheduled ones are completed as well > > I was just asking. The free_irq() does the trick, yes. Okay thanks. So juts need to fix the second driver, will that right away!
diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c index e2c04dc..c13a3bb 100644 --- a/drivers/dma/at_hdmac.c +++ b/drivers/dma/at_hdmac.c @@ -1569,7 +1569,6 @@ static int at_dma_remove(struct platform_device *pdev) /* Disable interrupts */ atc_disable_chan_irq(atdma, chan->chan_id); - tasklet_disable(&atchan->tasklet); tasklet_kill(&atchan->tasklet); list_del(&chan->device_node);
As discussed in [1] the tasklet_disable is not a proper function for teardown. The driver also uses correct method of tasklet_kill. So remove tasklet_disable [1]: http://lwn.net/Articles/588457/ Reported-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Vinod Koul <vinod.koul@intel.com> --- drivers/dma/at_hdmac.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)