| Message ID | 20160706163550.GB12591@localhost (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On 2016-07-06 19:35, Vinod Koul wrote: > On Wed, Jul 06, 2016 at 09:47:50AM +0300, okaya@codeaurora.org wrote: >> On 2016-07-05 17:54, Vinod Koul wrote: >> >drivers should ensure that tasklets are killed, so that they can't be >> >run after driver remove is executed >> > >> >Signed-off-by: Vinod Koul <vinod.koul@intel.com> >> >Cc: Sinan Kaya <okaya@codeaurora.org> >> >--- >> > drivers/dma/qcom/hidma.c | 1 + >> > 1 file changed, 1 insertion(+) >> > >> >diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c >> >index 41b5c6dee713..b2374cd91e45 100644 >> >--- a/drivers/dma/qcom/hidma.c >> >+++ b/drivers/dma/qcom/hidma.c >> >@@ -708,6 +708,7 @@ static int hidma_remove(struct platform_device >> >*pdev) >> > pm_runtime_get_sync(dmadev->ddev.dev); >> > dma_async_device_unregister(&dmadev->ddev); >> > devm_free_irq(dmadev->ddev.dev, dmadev->irq, dmadev->lldev); >> >+ tasklet_kill(&dmadev->task); >> > hidma_debug_uninit(dmadev); >> > hidma_ll_uninit(dmadev->lldev); >> > hidma_free(dmadev); >> >> Acked-by: Sinan Kaya <okaya@codeaurora.org> >> >> Thanks for taking care of this. We also need one more tasklet_kill >> for rst_task in hidma_ll.c > > Looks like it it two? > > Am adding this one: > > diff --git a/drivers/dma/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c > index f3929001539b..e8323c8e23d5 100644 > --- a/drivers/dma/qcom/hidma_ll.c > +++ b/drivers/dma/qcom/hidma_ll.c > @@ -831,6 +831,8 @@ int hidma_ll_uninit(struct hidma_lldev *lldev) > > required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres; > tasklet_kill(&lldev->task); > + tasklet_kill(&lldev->rst_task); This is good > + tasklet_kill(&lldev->task); This is redundant. Exists above. > memset(lldev->trepool, 0, required_bytes); > lldev->trepool = NULL; > lldev->pending_tre_count = 0; > > > 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
On Wed, Jul 06, 2016 at 10:25:35PM +0300, okaya@codeaurora.org wrote: > On 2016-07-06 19:35, Vinod Koul wrote: > >On Wed, Jul 06, 2016 at 09:47:50AM +0300, okaya@codeaurora.org wrote: > >>On 2016-07-05 17:54, Vinod Koul wrote: > >>>drivers should ensure that tasklets are killed, so that they can't be > >>>run after driver remove is executed > >>> > >>>Signed-off-by: Vinod Koul <vinod.koul@intel.com> > >>>Cc: Sinan Kaya <okaya@codeaurora.org> > >>>--- > >>> drivers/dma/qcom/hidma.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>>diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c > >>>index 41b5c6dee713..b2374cd91e45 100644 > >>>--- a/drivers/dma/qcom/hidma.c > >>>+++ b/drivers/dma/qcom/hidma.c > >>>@@ -708,6 +708,7 @@ static int hidma_remove(struct platform_device > >>>*pdev) > >>> pm_runtime_get_sync(dmadev->ddev.dev); > >>> dma_async_device_unregister(&dmadev->ddev); > >>> devm_free_irq(dmadev->ddev.dev, dmadev->irq, dmadev->lldev); > >>>+ tasklet_kill(&dmadev->task); > >>> hidma_debug_uninit(dmadev); > >>> hidma_ll_uninit(dmadev->lldev); > >>> hidma_free(dmadev); > >> > >>Acked-by: Sinan Kaya <okaya@codeaurora.org> > >> > >>Thanks for taking care of this. We also need one more tasklet_kill > >>for rst_task in hidma_ll.c > > > >Looks like it it two? > > > >Am adding this one: > > > >diff --git a/drivers/dma/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c > >index f3929001539b..e8323c8e23d5 100644 > >--- a/drivers/dma/qcom/hidma_ll.c > >+++ b/drivers/dma/qcom/hidma_ll.c > >@@ -831,6 +831,8 @@ int hidma_ll_uninit(struct hidma_lldev *lldev) > > > > required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres; > > tasklet_kill(&lldev->task); > >+ tasklet_kill(&lldev->rst_task); > > This is good > > >+ tasklet_kill(&lldev->task); > > This is redundant. Exists above. Are you sure, looking at the code, we seem to initialize three tasklets, one in hidma and two in ll part. Both are different pointer allocated by these routines?
On 2016-07-08 06:44, Vinod Koul wrote: > On Wed, Jul 06, 2016 at 10:25:35PM +0300, okaya@codeaurora.org wrote: >> On 2016-07-06 19:35, Vinod Koul wrote: >> >On Wed, Jul 06, 2016 at 09:47:50AM +0300, okaya@codeaurora.org wrote: >> >>On 2016-07-05 17:54, Vinod Koul wrote: >> >>>drivers should ensure that tasklets are killed, so that they can't be >> >>>run after driver remove is executed >> >>> >> >>>Signed-off-by: Vinod Koul <vinod.koul@intel.com> >> >>>Cc: Sinan Kaya <okaya@codeaurora.org> >> >>>--- >> >>> drivers/dma/qcom/hidma.c | 1 + >> >>> 1 file changed, 1 insertion(+) >> >>> >> >>>diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c >> >>>index 41b5c6dee713..b2374cd91e45 100644 >> >>>--- a/drivers/dma/qcom/hidma.c >> >>>+++ b/drivers/dma/qcom/hidma.c >> >>>@@ -708,6 +708,7 @@ static int hidma_remove(struct platform_device >> >>>*pdev) >> >>> pm_runtime_get_sync(dmadev->ddev.dev); >> >>> dma_async_device_unregister(&dmadev->ddev); >> >>> devm_free_irq(dmadev->ddev.dev, dmadev->irq, dmadev->lldev); >> >>>+ tasklet_kill(&dmadev->task); >> >>> hidma_debug_uninit(dmadev); >> >>> hidma_ll_uninit(dmadev->lldev); >> >>> hidma_free(dmadev); >> >> >> >>Acked-by: Sinan Kaya <okaya@codeaurora.org> >> >> >> >>Thanks for taking care of this. We also need one more tasklet_kill >> >>for rst_task in hidma_ll.c >> > >> >Looks like it it two? >> > >> >Am adding this one: >> > >> >diff --git a/drivers/dma/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c >> >index f3929001539b..e8323c8e23d5 100644 >> >--- a/drivers/dma/qcom/hidma_ll.c >> >+++ b/drivers/dma/qcom/hidma_ll.c >> >@@ -831,6 +831,8 @@ int hidma_ll_uninit(struct hidma_lldev *lldev) >> > >> > required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres; >> > tasklet_kill(&lldev->task); >> >+ tasklet_kill(&lldev->rst_task); >> >> This is good >> >> >+ tasklet_kill(&lldev->task); >> >> This is redundant. Exists above. > > Are you sure, looking at the code, we seem to initialize three > tasklets, one > in hidma and two in ll part. Both are different pointer allocated by > these > routines? Sorry, I am on vacation and have been checking emails from webmail. You are right, there needs to be one tasklet_kill in hidma.c and two tasklet_kill in hidma_ll.c in total. AFAIS, your patch is adding one more tasklet_kill(&lldev->task); In hidma_ll.c. this already exists in hidma_ll.c It could be my poor webmail interface. -- 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/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c index f3929001539b..e8323c8e23d5 100644 --- a/drivers/dma/qcom/hidma_ll.c +++ b/drivers/dma/qcom/hidma_ll.c @@ -831,6 +831,8 @@ int hidma_ll_uninit(struct hidma_lldev *lldev) required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres; tasklet_kill(&lldev->task); + tasklet_kill(&lldev->rst_task); + tasklet_kill(&lldev->task); memset(lldev->trepool, 0, required_bytes); lldev->trepool = NULL; lldev->pending_tre_count = 0;