2012-10-31 08:05:34

by Xiaotian Feng

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

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

Signed-off-by: Xiaotian Feng <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
Cc: Tony Lindgren <[email protected]>
Cc: Sourav Poddar <[email protected]>
Cc: Josh <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
---
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


2012-10-31 08:05:48

by Xiaotian Feng

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

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

Signed-off-by: Xiaotian Feng <[email protected]>
Cc: Li Yang <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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

2012-10-31 08:06:06

by Xiaotian Feng

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

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

Signed-off-by: Xiaotian Feng <[email protected]>
Cc: Jon Maloy <[email protected]>
Cc: Allan Stephens <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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

2012-10-31 13:32:57

by Felipe Balbi

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

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 <[email protected]>
> Cc: Li Yang <[email protected]>
> Cc: Felipe Balbi <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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


Attachments:
(No filename) (1.47 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2012-10-31 14:35:40

by Li Yang

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

On Wed, Oct 31, 2012 at 9:26 PM, Felipe Balbi <[email protected]> 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

2012-10-31 14:41:06

by Felipe Balbi

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

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 <[email protected]> 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


Attachments:
(No filename) (694.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2012-11-03 19:16:00

by David Miller

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

From: Xiaotian Feng <[email protected]>
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 <[email protected]>

Applied.

2012-11-25 07:58:52

by Dmitry Torokhov

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

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 <[email protected]>
> Cc: Dmitry Torokhov <[email protected]>
> Cc: Tony Lindgren <[email protected]>
> Cc: Sourav Poddar <[email protected]>
> Cc: Josh <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> ---
> 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