[PATCH 1/3] input: do not use tasklet_disable before tasklet_kill

[PATCH 1/3] input: do not use tasklet_disable before tasklet_kill

[Xiaotian Feng]

If tasklet_disable() is called before related tasklet handled,
tasklet_kill will never be finished. tasklet_kill is enough.

Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> 
Cc: Tony Lindgren <tony@atomide.com>
Cc: Sourav Poddar <sourav.poddar@ti.com>
Cc: Josh <joshua.taylor0@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-input@vger.kernel.org
---
 drivers/input/keyboard/omap-keypad.c |    3 +--
 drivers/input/serio/hil_mlc.c        |    1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/omap-keypad.c b/drivers/input/keyboard/omap-keypad.c
index 4a5fcc8..6c52447 100644
--- a/drivers/input/keyboard/omap-keypad.c
+++ b/drivers/input/keyboard/omap-keypad.c
@@ -362,12 +362,11 @@ static int __devexit omap_kp_remove(struct platform_device *pdev)
 	struct omap_kp *omap_kp = platform_get_drvdata(pdev);
 
 	/* disable keypad interrupt handling */
-	tasklet_disable(&kp_tasklet);
+	tasklet_kill(&kp_tasklet);
 	omap_writew(1, OMAP1_MPUIO_BASE + OMAP_MPUIO_KBD_MASKIT);
 	free_irq(omap_kp->irq, omap_kp);
 
 	del_timer_sync(&omap_kp->timer);
-	tasklet_kill(&kp_tasklet);
 
 	/* unregister everything */
 	input_unregister_device(omap_kp->input);
diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
index bfd3865..7fc1700 100644
--- a/drivers/input/serio/hil_mlc.c
+++ b/drivers/input/serio/hil_mlc.c
@@ -1011,7 +1011,6 @@ static void __exit hil_mlc_exit(void)
 {
 	del_timer_sync(&hil_mlcs_kicker);
 
-	tasklet_disable(&hil_mlcs_tasklet);
 	tasklet_kill(&hil_mlcs_tasklet);
 }
 
-- 
1.7.9.5

[PATCH 2/3] usb: gadget: fsl_qe_udc: do not use tasklet_disable before tasklet_kill

[Xiaotian Feng]

If tasklet_disable() is called before related tasklet handled,
tasklet_kill will never be finished. tasklet_kill is enough.

Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
Cc: Li Yang <leoli@freescale.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
---
 drivers/usb/gadget/fsl_qe_udc.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
index b09452d..4ad3b82 100644
--- a/drivers/usb/gadget/fsl_qe_udc.c
+++ b/drivers/usb/gadget/fsl_qe_udc.c
@@ -2661,7 +2661,7 @@ static int __devexit qe_udc_remove(struct platform_device *ofdev)
 	usb_del_gadget_udc(&udc->gadget);
 
 	udc->done = &done;
-	tasklet_disable(&udc->rx_tasklet);
+	tasklet_kill(&udc->rx_tasklet);
 
 	if (udc->nullmap) {
 		dma_unmap_single(udc->gadget.dev.parent,
@@ -2698,8 +2698,6 @@ static int __devexit qe_udc_remove(struct platform_device *ofdev)
 	free_irq(udc->usb_irq, udc);
 	irq_dispose_mapping(udc->usb_irq);
 
-	tasklet_kill(&udc->rx_tasklet);
-
 	iounmap(udc->usb_regs);
 
 	device_unregister(&udc->gadget.dev);
-- 
1.7.9.5

[PATCH 2/3] usb: gadget: fsl_qe_udc: do not use tasklet_disable before tasklet_kill

[Xiaotian Feng]

If tasklet_disable() is called before related tasklet handled,
tasklet_kill will never be finished. tasklet_kill is enough.

Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
Cc: Li Yang <leoli@freescale.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
---
 drivers/usb/gadget/fsl_qe_udc.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
index b09452d..4ad3b82 100644
--- a/drivers/usb/gadget/fsl_qe_udc.c
+++ b/drivers/usb/gadget/fsl_qe_udc.c
@@ -2661,7 +2661,7 @@ static int __devexit qe_udc_remove(struct platform_device *ofdev)
 	usb_del_gadget_udc(&udc->gadget);
 
 	udc->done = &done;
-	tasklet_disable(&udc->rx_tasklet);
+	tasklet_kill(&udc->rx_tasklet);
 
 	if (udc->nullmap) {
 		dma_unmap_single(udc->gadget.dev.parent,
@@ -2698,8 +2698,6 @@ static int __devexit qe_udc_remove(struct platform_device *ofdev)
 	free_irq(udc->usb_irq, udc);
 	irq_dispose_mapping(udc->usb_irq);
 
-	tasklet_kill(&udc->rx_tasklet);
-
 	iounmap(udc->usb_regs);
 
 	device_unregister(&udc->gadget.dev);
-- 
1.7.9.5

Re: [PATCH 2/3] usb: gadget: fsl_qe_udc: do not use tasklet_disable before tasklet_kill

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]

On Wed, Oct 31, 2012 at 04:06:00PM +0800, Xiaotian Feng wrote:
> If tasklet_disable() is called before related tasklet handled,
> tasklet_kill will never be finished. tasklet_kill is enough.

how did you test this ? Why changing FSL driver instead of switching
over to chipidea which is supposed to be shared by every licensee of the
chipidea core ?

> Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
> Cc: Li Yang <leoli@freescale.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
>  drivers/usb/gadget/fsl_qe_udc.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> index b09452d..4ad3b82 100644
> --- a/drivers/usb/gadget/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/fsl_qe_udc.c
> @@ -2661,7 +2661,7 @@ static int __devexit qe_udc_remove(struct platform_device *ofdev)
>  	usb_del_gadget_udc(&udc->gadget);
>  
>  	udc->done = &done;
> -	tasklet_disable(&udc->rx_tasklet);
> +	tasklet_kill(&udc->rx_tasklet);
>  
>  	if (udc->nullmap) {
>  		dma_unmap_single(udc->gadget.dev.parent,
> @@ -2698,8 +2698,6 @@ static int __devexit qe_udc_remove(struct platform_device *ofdev)
>  	free_irq(udc->usb_irq, udc);
>  	irq_dispose_mapping(udc->usb_irq);
>  
> -	tasklet_kill(&udc->rx_tasklet);
> -
>  	iounmap(udc->usb_regs);
>  
>  	device_unregister(&udc->gadget.dev);
> -- 
> 1.7.9.5
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] usb: gadget: fsl_qe_udc: do not use tasklet_disable before tasklet_kill

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]

On Wed, Oct 31, 2012 at 04:06:00PM +0800, Xiaotian Feng wrote:
> If tasklet_disable() is called before related tasklet handled,
> tasklet_kill will never be finished. tasklet_kill is enough.

how did you test this ? Why changing FSL driver instead of switching
over to chipidea which is supposed to be shared by every licensee of the
chipidea core ?

> Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
> Cc: Li Yang <leoli@freescale.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
>  drivers/usb/gadget/fsl_qe_udc.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> index b09452d..4ad3b82 100644
> --- a/drivers/usb/gadget/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/fsl_qe_udc.c
> @@ -2661,7 +2661,7 @@ static int __devexit qe_udc_remove(struct platform_device *ofdev)
>  	usb_del_gadget_udc(&udc->gadget);
>  
>  	udc->done = &done;
> -	tasklet_disable(&udc->rx_tasklet);
> +	tasklet_kill(&udc->rx_tasklet);
>  
>  	if (udc->nullmap) {
>  		dma_unmap_single(udc->gadget.dev.parent,
> @@ -2698,8 +2698,6 @@ static int __devexit qe_udc_remove(struct platform_device *ofdev)
>  	free_irq(udc->usb_irq, udc);
>  	irq_dispose_mapping(udc->usb_irq);
>  
> -	tasklet_kill(&udc->rx_tasklet);
> -
>  	iounmap(udc->usb_regs);
>  
>  	device_unregister(&udc->gadget.dev);
> -- 
> 1.7.9.5
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] usb: gadget: fsl_qe_udc: do not use tasklet_disable before tasklet_kill

[Li Yang]

On Wed, Oct 31, 2012 at 9:26 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Wed, Oct 31, 2012 at 04:06:00PM +0800, Xiaotian Feng wrote:
>> If tasklet_disable() is called before related tasklet handled,
>> tasklet_kill will never be finished. tasklet_kill is enough.
>
> how did you test this ? Why changing FSL driver instead of switching
> over to chipidea which is supposed to be shared by every licensee of the
> chipidea core ?

The QE UDC is an private controller that is not compatible with the
Chipidea core.

- Leo

Re: [PATCH 2/3] usb: gadget: fsl_qe_udc: do not use tasklet_disable before tasklet_kill

[Li Yang]

On Wed, Oct 31, 2012 at 9:26 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Wed, Oct 31, 2012 at 04:06:00PM +0800, Xiaotian Feng wrote:
>> If tasklet_disable() is called before related tasklet handled,
>> tasklet_kill will never be finished. tasklet_kill is enough.
>
> how did you test this ? Why changing FSL driver instead of switching
> over to chipidea which is supposed to be shared by every licensee of the
> chipidea core ?

The QE UDC is an private controller that is not compatible with the
Chipidea core.

- Leo

Re: [PATCH 2/3] usb: gadget: fsl_qe_udc: do not use tasklet_disable before tasklet_kill

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

Hi,

On Wed, Oct 31, 2012 at 10:35:37PM +0800, Li Yang wrote:
> On Wed, Oct 31, 2012 at 9:26 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Wed, Oct 31, 2012 at 04:06:00PM +0800, Xiaotian Feng wrote:
> >> If tasklet_disable() is called before related tasklet handled,
> >> tasklet_kill will never be finished. tasklet_kill is enough.
> >
> > how did you test this ? Why changing FSL driver instead of switching
> > over to chipidea which is supposed to be shared by every licensee of the
> > chipidea core ?
> 
> The QE UDC is an private controller that is not compatible with the
> Chipidea core.

thanks for the clarification, but you still haven't answered how you
tested this ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] usb: gadget: fsl_qe_udc: do not use tasklet_disable before tasklet_kill

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

Hi,

On Wed, Oct 31, 2012 at 10:35:37PM +0800, Li Yang wrote:
> On Wed, Oct 31, 2012 at 9:26 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Wed, Oct 31, 2012 at 04:06:00PM +0800, Xiaotian Feng wrote:
> >> If tasklet_disable() is called before related tasklet handled,
> >> tasklet_kill will never be finished. tasklet_kill is enough.
> >
> > how did you test this ? Why changing FSL driver instead of switching
> > over to chipidea which is supposed to be shared by every licensee of the
> > chipidea core ?
> 
> The QE UDC is an private controller that is not compatible with the
> Chipidea core.

thanks for the clarification, but you still haven't answered how you
tested this ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/3] tipc: do not use tasklet_disable before tasklet_kill

[Xiaotian Feng]

If tasklet_disable() is called before related tasklet handled,
tasklet_kill will never be finished. tasklet_kill is enough.

Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
Cc: Jon Maloy <jon.maloy@ericsson.com>
Cc: Allan Stephens <allan.stephens@windriver.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: tipc-discussion@lists.sourceforge.net
---
 net/tipc/handler.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/net/tipc/handler.c b/net/tipc/handler.c
index 111ff83..b36f0fc 100644
--- a/net/tipc/handler.c
+++ b/net/tipc/handler.c
@@ -116,7 +116,6 @@ void tipc_handler_stop(void)
 		return;
 
 	handler_enabled = 0;
-	tasklet_disable(&tipc_tasklet);
 	tasklet_kill(&tipc_tasklet);
 
 	spin_lock_bh(&qitem_lock);
-- 
1.7.9.5

Re: [PATCH 3/3] tipc: do not use tasklet_disable before tasklet_kill

[David Miller]

From: Xiaotian Feng <xtfeng@gmail.com>
Date: Wed, 31 Oct 2012 16:06:01 +0800

> If tasklet_disable() is called before related tasklet handled,
> tasklet_kill will never be finished. tasklet_kill is enough.
> 
> Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>

Applied.

Re: [PATCH 1/3] input: do not use tasklet_disable before tasklet_kill

[Dmitry Torokhov]

Hi Xiaotian,

On Wed, Oct 31, 2012 at 04:05:59PM +0800, Xiaotian Feng wrote:
> If tasklet_disable() is called before related tasklet handled,
> tasklet_kill will never be finished. tasklet_kill is enough.
> 

Could you please elaborate on this? Needing to disable tasket before
killing it is quite often needed when dealing with self-rescheduling
tasklets and so tasklet_disable() followed by tasklet_kill() must work.
If it does not we need to take care of it in softirq code instead of
individual drivers.

> Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> 
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Sourav Poddar <sourav.poddar@ti.com>
> Cc: Josh <joshua.taylor0@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-input@vger.kernel.org
> ---
>  drivers/input/keyboard/omap-keypad.c |    3 +--
>  drivers/input/serio/hil_mlc.c        |    1 -
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/input/keyboard/omap-keypad.c b/drivers/input/keyboard/omap-keypad.c
> index 4a5fcc8..6c52447 100644
> --- a/drivers/input/keyboard/omap-keypad.c
> +++ b/drivers/input/keyboard/omap-keypad.c
> @@ -362,12 +362,11 @@ static int __devexit omap_kp_remove(struct platform_device *pdev)
>  	struct omap_kp *omap_kp = platform_get_drvdata(pdev);
>  
>  	/* disable keypad interrupt handling */
> -	tasklet_disable(&kp_tasklet);
> +	tasklet_kill(&kp_tasklet);
>  	omap_writew(1, OMAP1_MPUIO_BASE + OMAP_MPUIO_KBD_MASKIT);
>  	free_irq(omap_kp->irq, omap_kp);
>  
>  	del_timer_sync(&omap_kp->timer);
> -	tasklet_kill(&kp_tasklet);

Exactly like here. If we do not disable tasklet before disabling IRQ and
freeing timer we may get into scenario when timer schedules tasket and
tasklet schedules timer again after we canceled it.

>  
>  	/* unregister everything */
>  	input_unregister_device(omap_kp->input);
> diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
> index bfd3865..7fc1700 100644
> --- a/drivers/input/serio/hil_mlc.c
> +++ b/drivers/input/serio/hil_mlc.c
> @@ -1011,7 +1011,6 @@ static void __exit hil_mlc_exit(void)
>  {
>  	del_timer_sync(&hil_mlcs_kicker);
>  
> -	tasklet_disable(&hil_mlcs_tasklet);
>  	tasklet_kill(&hil_mlcs_tasklet);

This seems like safe change.

Thanks.

-- 
Dmitry