timers: Plug debugobject leaks and use del_timer_sync() in exit/teardown

del_timer() does not wait for a possible running callback to complete. So the call side might free request and the associated objects while on another cpu the timer handler runs.

Otherwise we leak a tracking object when DEBUG_OBJECTS is enabled.

Otherwise we leak a tracking object when DEBUG_OBJECTS is enabled.

 drivers/s390/net/lcs.c |    1 +
 1 file changed, 1 insertion(+)

Index: tip/drivers/s390/net/lcs.c
===================================================================

Otherwise we leak a tracking object if DEBUG_OBJECTS is enabled.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Josh Triplett <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

---
 kernel/rcu/torture.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: tip/kernel/rcu/torture.c
===================================================================

kernel/rcu/torture.c +3 -1

@@ -1038,8 +1038,10 @@ rcu_torture_reader(void *arg)
 	} while (!kthread_should_stop() && fullstop == FULLSTOP_DONTSTOP);
 	VERBOSE_PRINTK_STRING("rcu_torture_reader task stopping");
 	rcutorture_shutdown_absorb("rcu_torture_reader");
-	if (irqreader && cur_ops->irq_capable)
+	if (irqreader && cur_ops->irq_capable) {
 		del_timer_sync(&t);
+		destroy_timer_on_stack(&t);
+	}
 	while (!kthread_should_stop())
 		schedule_timeout_uninterruptible(1);
 	return 0;

Otherwise we leak a tracking object if DEBUG_OBJECTS is enabled.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Josh Triplett <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

 kernel/rcu/torture.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: tip/kernel/rcu/torture.c
===================================================================

kernel/rcu/torture.c +3 -1

@@ -1038,8 +1038,10 @@ rcu_torture_reader(void *arg)
 	} while (!kthread_should_stop() && fullstop == FULLSTOP_DONTSTOP);
 	VERBOSE_PRINTK_STRING("rcu_torture_reader task stopping");
 	rcutorture_shutdown_absorb("rcu_torture_reader");
-	if (irqreader && cur_ops->irq_capable)
+	if (irqreader && cur_ops->irq_capable) {
 		del_timer_sync(&t);
+		destroy_timer_on_stack(&t);
+	}
 	while (!kthread_should_stop())
 		schedule_timeout_uninterruptible(1);
 	return 0;

The device is about to vanish. So we need to make sure that the timer is completely stopped and the callback is not running on another CPU.

Signed-off-by Thomas Gleixner <[email protected]>

The module is about to go away. Make sure everything is stopped safely before we pull the plug.

Signed-off-by Thomas Gleixner <[email protected]>

Make sure no timer callback is running before releasing the datastructure which contains it.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Marcel Holtmann <[email protected]>

Cc bt <[email protected]>

---
drivers/bluetooth/bluecard_cs.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

Make sure no timer callback is running before freeing the datastructure which contains it.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Marcel Holtmann <[email protected]>

Cc bt <[email protected]>

---
drivers/bluetooth/hci_bcsp.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

Make sure no timer callback is running before releasing the datastructure which contains it.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Marcel Holtmann <[email protected]>

Cc bt <[email protected]>

---
drivers/bluetooth/hci_h5.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

We are about to free the data structure. Make sure no timer callback is running. I might be paranoid, but the ->exit callback can be invoked from so many places, that it is not entirely clear whether del_timer is always called on the cpu on which it is enqueued.

While looking through the call sites I noticed, that cpufreq_init_policy() can fail and invoke cpufreq_driver->exit() but it does not return the failure and the callsite happily proceeds.

Signed-off-by Thomas Gleixner <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc cpufreq <[email protected]>

Cc pm <[email protected]>

---

 drivers/cpufreq/intel_pstate.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/cpufreq/intel_pstate.c
===================================================================

drivers/cpufreq/intel_pstate.c +1 -1

@@ -777,7 +777,7 @@ static int intel_pstate_cpu_exit(struct
 {
 	int cpu = policy->cpu;
 
-	del_timer(&all_cpu_data[cpu]->timer);
+	del_timer_sync(&all_cpu_data[cpu]->timer);
 	kfree(all_cpu_data[cpu]);
 	all_cpu_data[cpu] = NULL;
 	return 0;

Quoted 9 lines · Thomas Gleixner

We are about to free the data structure. Make sure no timer callback is running. I might be paranoid, but the ->exit callback can be invoked from so many places, that it is not entirely clear whether del_timer is always called on the cpu on which it is enqueued.

While looking through the call sites I noticed, that cpufreq_init_policy() can fail and invoke cpufreq_driver->exit() but it does not return the failure and the callsite happily proceeds.

Quoted 4 lines

Signed-off-by Thomas Gleixner <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc cpufreq <[email protected]>

Cc pm <[email protected]>

Dirk?

Quoted 20 lines

---

  drivers/cpufreq/intel_pstate.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/cpufreq/intel_pstate.c
===================================================================

drivers/cpufreq/intel_pstate.c +1 -1

@@ -777,7 +777,7 @@ static int intel_pstate_cpu_exit(struct
  {
  	int cpu = policy->cpu;
 
-	del_timer(&all_cpu_data[cpu]->timer);
+	del_timer_sync(&all_cpu_data[cpu]->timer);
  	kfree(all_cpu_data[cpu]);
  	all_cpu_data[cpu] = NULL;
  	return 0;

We are about to free the data structure. Make sure no timer callback is running. I might be paranoid, but the ->exit callback can be invoked from so many places, that it is not entirely clear whether del_timer is always called on the cpu on which it is enqueued.

While looking through the call sites I noticed, that cpufreq_init_policy() can fail and invoke cpufreq_driver->exit() but it does not return the failure and the callsite happily proceeds.

Signed-off-by Thomas Gleixner <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc cpufreq <[email protected]>

Cc pm <[email protected]>

---

 drivers/cpufreq/intel_pstate.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/cpufreq/intel_pstate.c
===================================================================

Make sure that no callback is running before we teardown the module.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Cc input <[email protected]>

---
 drivers/input/serio/hp_sdc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/input/serio/hp_sdc.c
===================================================================

drivers/input/serio/hp_sdc.c +1 -1

@@ -984,7 +984,7 @@ static void hp_sdc_exit(void)
 	free_irq(hp_sdc.irq, &hp_sdc);
 	write_unlock_irq(&hp_sdc.lock);
 
-	del_timer(&hp_sdc.kicker);
+	del_timer_sync(&hp_sdc.kicker);
 
 	tasklet_kill(&hp_sdc.task);
 

As far as I could tell, (part of) the issue is that any kind of exit or close function should use del_timer_sync, because they could be called from a different CPU than the one that started up the timer?

Here is a semantic patch that takes care of the case of simple module_exit functions:

@r@
declarer name module_exit;
identifier ex;
@@

module_exit(ex);

@@
identifier r.ex;
@@

ex(...) {
<...
- del_timer
+ del_timer_sync

    (...)

...>
}

The transformations it makes are below. I haven't had a chance to check which results overlap with what Thomas has already sent, but I could look

into it if this is the right idea. I guess other kinds of close/exit functions would have to be identified manually, to make more rules.

In what circumstance can one be sure that two instructions are executed on the same CPU?

Quoted 22 lines · Julia Lawall

As far as I could tell, (part of) the issue is that any kind of exit or close function should use del_timer_sync, because they could be called from a different CPU than the one that started up the timer?

Here is a semantic patch that takes care of the case of simple module_exit functions:

@r@
declarer name module_exit;
identifier ex;
@@

module_exit(ex);

@@
identifier r.ex;
@@

ex(...) {
<...
- del_timer
+ del_timer_sync

Quoted 6 lines

    (...)

...>
}

The transformations it makes are below. I haven't had a chance to check which results overlap with what Thomas has already sent, but I could look

Minimal overlap, but as I said I did just a few conversions.

Quoted 2 lines

into it if this is the right idea. I guess other kinds of close/exit functions would have to be identified manually, to make more rules.

If you look through the examples I sent, you'll find the close() callbacks of various devices. So everything which is a function pointer to a ops->close(), ops->remove(), ops_>teardown() is dangerous if using del_timer(). There are a few exceptions, but....

Another thing I saw is

	del_timer(&bla->timer);
	....
	kfree(&bla);

That's also a potential source of trouble. You can't tell without analyzing the code, whether it's a problem or not. But making the responsible people to look at it is definitely a good thing.

Quoted 2 lines

In what circumstance can one be sure that two instructions are executed on the same CPU?

If interrupts or preemption are disabled. But that's not the issue at hand.

The del_timer vs. del_timer_sync problem is:

CPU0 CPU1

add_timer(&bla->timer);

close(bla)
timer expires del_timer(&bla->timer);
callback is invoked

				   kfree(bla);
    derefernces bla

I'll look at your findings on Tuesday, but feel free to send them to the relevant maintainers for review.

Another thing I saw is

	del_timer(&bla->timer);
	....
	kfree(&bla);

Quoted 5 lines

Another thing I saw is

	del_timer(&bla->timer);
	....
	kfree(&bla);

In one case I saw the following:

        if (isac->dch.timer.function != NULL) {
                del_timer(&isac->dch.timer);
                isac->dch.timer.function = NULL;
        }
        kfree(isac->mon_rx);
        isac->mon_rx = NULL;

Is the assignment isac->dch.timer.function = NULL good enough to solve the problem?

drivers/staging/lustre/lustre/llite/super25.c +0 -0

 	ll_capa_thread_stop();
 	LASSERTF(capa_count[CAPA_SITE_CLIENT] == 0,
 		 "client remaining capa count %d\n",

diff -u -p a/drivers/atm/idt77105.c b/drivers/atm/idt77105.c

arch/mips/lasat/picvue_proc.c static void pvc_proc_cleanup(void) · line 178

Quoted 9 lines · Julia Lawall

diff -u -p a/arch/mips/lasat/picvue_proc.c b/arch/mips/lasat/picvue_proc.c

arch/mips/lasat/picvue_proc.c +1 -1

@@ -175,7 +175,7 @@ static void pvc_proc_cleanup(void)
 	remove_proc_entry("scroll", pvc_display_dir);
 	remove_proc_entry(DISPLAY_DIR_NAME, NULL);
 
-	del_timer(&timer);
+	del_timer_sync(&timer);

Correct.

drivers/tty/serial/mux.c static void __exit mux_exit(void) · line 616

Quoted 12 lines

arch/mips/lasat/picvue_proc.c +0 -0

 }
 
 static int __init pvc_proc_init(void)

diff -u -p a/drivers/tty/serial/mux.c b/drivers/tty/serial/mux.c

drivers/tty/serial/mux.c +1 -1

@@ -613,7 +613,7 @@ static void __exit mux_exit(void)
 {
 	/* Delete the Mux timer. */
 	if(port_cnt > 0) {
-		del_timer(&mux_timer);
+		del_timer_sync(&mux_timer);

Correct.

drivers/input/serio/hp_sdc.c static void hp_sdc_exit(void) · line 987

Quoted 12 lines

drivers/tty/serial/mux.c +0 -0

 #ifdef CONFIG_SERIAL_MUX_CONSOLE
 		unregister_console(&mux_console);
 #endif

diff -u -p a/drivers/input/serio/hp_sdc.c b/drivers/input/serio/hp_sdc.c

drivers/input/serio/hp_sdc.c +1 -1

@@ -984,7 +984,7 @@ static void hp_sdc_exit(void)
 	free_irq(hp_sdc.irq, &hp_sdc);
 	write_unlock_irq(&hp_sdc.lock);
 
-	del_timer(&hp_sdc.kicker);
+	del_timer_sync(&hp_sdc.kicker);

Correct.

drivers/isdn/sc/init.c static void __exit sc_exit(void) · line 394

Quoted 13 lines

drivers/input/serio/hp_sdc.c +0 -0

 	tasklet_kill(&hp_sdc.task);
 

diff -u -p a/drivers/isdn/sc/init.c b/drivers/isdn/sc/init.c

drivers/isdn/sc/init.c +2 -2

@@ -390,8 +390,8 @@ static void __exit sc_exit(void)
 		/*
 		 * kill the timers
 		 */
-		del_timer(&(sc_adapter[i]->reset_timer));
-		del_timer(&(sc_adapter[i]->stat_timer));
+		del_timer_sync(&(sc_adapter[i]->reset_timer));
+		del_timer_sync(&(sc_adapter[i]->stat_timer));

Correct.

drivers/isdn/i4l/isdn_common.c static void __exit isdn_exit(void) · line 2384

Quoted 11 lines

drivers/isdn/sc/init.c +0 -0

 		/*
 		 * Tell I4L we're toast

diff -u -p a/drivers/isdn/i4l/isdn_common.c b/drivers/isdn/i4l/isdn_common.c

drivers/isdn/i4l/isdn_common.c +1 -1

@@ -2381,7 +2381,7 @@ static void __exit isdn_exit(void)
 	}
 	isdn_tty_exit();
 	unregister_chrdev(ISDN_MAJOR, "isdn");
-	del_timer(&dev->timer);
+	del_timer_sync(&dev->timer);

Correct.

drivers/isdn/hardware/mISDN/hfcpci.c static void __exit · line 2355

Quoted 12 lines

drivers/isdn/i4l/isdn_common.c +0 -0

 	/* call vfree with interrupts enabled, else it will hang */
 	vfree(dev);
 	printk(KERN_NOTICE "ISDN-subsystem unloaded\n");

diff -u -p a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c

drivers/isdn/hardware/mISDN/hfcpci.c +1 -1

@@ -2352,7 +2352,7 @@ static void __exit
 HFC_cleanup(void)
 {
 	if (timer_pending(&hfc_tl))
-		del_timer(&hfc_tl);
+		del_timer_sync(&hfc_tl);

That's incomplete the timer_pending() check is racy as well and the timer is rearming itself from the callback.

drivers/isdn/act2000/module.c static void __exit act2000_exit(void) · line 799

Quoted 11 lines

drivers/isdn/hardware/mISDN/hfcpci.c +0 -0

 	pci_unregister_driver(&hfc_driver);
 }

diff -u -p a/drivers/isdn/act2000/module.c b/drivers/isdn/act2000/module.c

drivers/isdn/act2000/module.c +1 -1

@@ -796,7 +796,7 @@ static void __exit act2000_exit(void)
 	act2000_card *last;
 	while (card) {
 		unregister_card(card);
-		del_timer(&card->ptimer);
+		del_timer_sync(&card->ptimer);

Correct.

drivers/net/hamradio/yam.c static void __exit yam_cleanup_driver(vo · line 1187

Quoted 12 lines

drivers/isdn/act2000/module.c +0 -0

 		card = card->next;
 	}
 	card = cards;

diff -u -p a/drivers/net/hamradio/yam.c b/drivers/net/hamradio/yam.c

drivers/net/hamradio/yam.c +1 -1

@@ -1184,7 +1184,7 @@ static void __exit yam_cleanup_driver(vo
 	struct yam_mcs *p;
 	int i;
 
-	del_timer(&yam_timer);
+	del_timer_sync(&yam_timer);

Correct.

drivers/staging/speakup/main.c static void __exit speakup_exit(void) · line 2221

Quoted 12 lines

drivers/net/hamradio/yam.c +0 -0

 	for (i = 0; i < NR_PORTS; i++) {
 		struct net_device *dev = yam_devs[i];
 		if (dev) {

diff -u -p a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c

drivers/staging/speakup/main.c +1 -1

@@ -2218,7 +2218,7 @@ static void __exit speakup_exit(void)
 	unregister_keyboard_notifier(&keyboard_notifier_block);
 	unregister_vt_notifier(&vt_notifier_block);
 	speakup_unregister_devsynth();
-	del_timer(&cursor_timer);
+	del_timer_sync(&cursor_timer);

Not sure about that one as the timer might be rearmed, so the invocation might need to be further down the cleanup path.

drivers/staging/panel/panel.c static void __exit panel_cleanup_module( · line 2301

Quoted 12 lines

drivers/staging/speakup/main.c +0 -0

 	kthread_stop(speakup_task);
 	speakup_task = NULL;
 	mutex_lock(&spk_mutex);

diff -u -p a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c

drivers/staging/panel/panel.c +1 -1

@@ -2298,7 +2298,7 @@ static void __exit panel_cleanup_module(
 	unregister_reboot_notifier(&panel_notifier);
 
 	if (scan_timer.function != NULL)
-		del_timer(&scan_timer);
+		del_timer_sync(&scan_timer);

Correct.

drivers/staging/lustre/lustre/llite/super25.c static void __exit exit_lustre_lite(void · line 200

Quoted 11 lines

drivers/staging/panel/panel.c +0 -0

 	if (pprt != NULL) {
 		if (keypad_enabled) {

diff -u -p a/drivers/staging/lustre/lustre/llite/super25.c b/drivers/staging/lustre/lustre/llite/super25.c

drivers/staging/lustre/lustre/llite/super25.c +1 -1

@@ -197,7 +197,7 @@ static void __exit exit_lustre_lite(void
 {
 	ll_xattr_fini();
 	vvp_global_fini();
-	del_timer(&ll_capa_timer);
+	del_timer_sync(&ll_capa_timer);

Looks wrong, as the timer might be rearmed by the thread which is not yet stopped.

Quoted 7 lines

drivers/staging/lustre/lustre/llite/super25.c +0 -0

 	ll_capa_thread_stop();
 	LASSERTF(capa_count[CAPA_SITE_CLIENT] == 0,
 		 "client remaining capa count %d\n",

diff -u -p a/drivers/atm/idt77105.c b/drivers/atm/idt77105.c

drivers/atm/idt77105.c EXPORT_SYMBOL(idt77105_init); · line 373

Quoted 7 lines

drivers/atm/idt77105.c +2 -2

@@ -369,8 +369,8 @@ EXPORT_SYMBOL(idt77105_init);
 static void __exit idt77105_exit(void)
 {
         /* turn off timers */
-        del_timer(&stats_timer);
-        del_timer(&restart_timer);
+        del_timer_sync(&stats_timer);
+        del_timer_sync(&restart_timer);

Yep.

drivers/atm/nicstar.c static void __exit nicstar_cleanup(void) · line 310

Quoted 13 lines

drivers/atm/idt77105.c +0 -0

 }
 
 module_exit(idt77105_exit);

diff -u -p a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c

drivers/atm/nicstar.c +1 -1

@@ -306,7 +306,7 @@ static void __exit nicstar_cleanup(void)
 {
 	XPRINTK("nicstar: nicstar_cleanup() called.\n");
 
-	del_timer(&ns_timer);
+	del_timer_sync(&ns_timer);
 

The driver might still be in use, as the devices are shut down via the pci_unregister_driver() call below. So while it's correct to use del_timer_sync() it's not clear w/o digging deeper into the code whether it's actually sufficient.

drivers/atm/iphase.c static void __exit ia_module_exit(void) · line 3292

Quoted 11 lines

drivers/atm/nicstar.c +0 -0

 	pci_unregister_driver(&nicstar_driver);
 

diff -u -p a/drivers/atm/iphase.c b/drivers/atm/iphase.c

drivers/atm/iphase.c +1 -1

@@ -3289,7 +3289,7 @@ static void __exit ia_module_exit(void)
 {
 	pci_unregister_driver(&ia_driver);
 
-        del_timer(&ia_timer);
+        del_timer_sync(&ia_timer);

The timer is self rearming and has protection against the removal of the devices. So it should be fine.

net/hsr/hsr_main.c static int __init hsr_init(void) · line 462

Quoted 12 lines

drivers/atm/iphase.c +0 -0

 }
 
 module_init(ia_module_init);

diff -u -p a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c

net/hsr/hsr_main.c +1 -1

@@ -459,7 +459,7 @@ static int __init hsr_init(void)
 static void __exit hsr_exit(void)
 {
 	unregister_netdevice_notifier(&hsr_nb);
-	del_timer(&prune_timer);
+	del_timer_sync(&prune_timer);

Correct as the only rearm is from the timer callback itself

net/atm/mpc.c static void __exit atm_mpoa_cleanup(void · line 1495

Quoted 12 lines

net/hsr/hsr_main.c +0 -0

 	hsr_netlink_exit();
 	dev_remove_pack(&hsr_pt);
 }

diff -u -p a/net/atm/mpc.c b/net/atm/mpc.c

net/atm/mpc.c +1 -1

@@ -1492,7 +1492,7 @@ static void __exit atm_mpoa_cleanup(void
 
 	mpc_proc_clean();
 
-	del_timer(&mpc_timer);
+	del_timer_sync(&mpc_timer);

Looks correct as well. Timer is rearming no other dependencies.

net/atm/mpc.c static void __exit atm_mpoa_cleanup(void · line 1497

Quoted 2 lines

net/atm/mpc.c +0 -0

 	unregister_netdevice_notifier(&mpoa_notifier);
 	deregister_atm_ioctl(&atm_ioctl_ops);

Thanks,

tglx

arch/mips/lasat/picvue_proc.c static void pvc_proc_cleanup(void) · line 178

Quoted 9 lines · Julia Lawall

diff -u -p a/arch/mips/lasat/picvue_proc.c b/arch/mips/lasat/picvue_proc.c

arch/mips/lasat/picvue_proc.c +1 -1

@@ -175,7 +175,7 @@ static void pvc_proc_cleanup(void)
 	remove_proc_entry("scroll", pvc_display_dir);
 	remove_proc_entry(DISPLAY_DIR_NAME, NULL);
 
-	del_timer(&timer);
+	del_timer_sync(&timer);

Correct.

drivers/tty/serial/mux.c static void __exit mux_exit(void) · line 616

Quoted 12 lines

arch/mips/lasat/picvue_proc.c +0 -0

 }
 
 static int __init pvc_proc_init(void)

diff -u -p a/drivers/tty/serial/mux.c b/drivers/tty/serial/mux.c

drivers/tty/serial/mux.c +1 -1

@@ -613,7 +613,7 @@ static void __exit mux_exit(void)
 {
 	/* Delete the Mux timer. */
 	if(port_cnt > 0) {
-		del_timer(&mux_timer);
+		del_timer_sync(&mux_timer);

Correct.

drivers/input/serio/hp_sdc.c static void hp_sdc_exit(void) · line 987

Quoted 12 lines

drivers/tty/serial/mux.c +0 -0

 #ifdef CONFIG_SERIAL_MUX_CONSOLE
 		unregister_console(&mux_console);
 #endif

diff -u -p a/drivers/input/serio/hp_sdc.c b/drivers/input/serio/hp_sdc.c

drivers/input/serio/hp_sdc.c +1 -1

@@ -984,7 +984,7 @@ static void hp_sdc_exit(void)
 	free_irq(hp_sdc.irq, &hp_sdc);
 	write_unlock_irq(&hp_sdc.lock);
 
-	del_timer(&hp_sdc.kicker);
+	del_timer_sync(&hp_sdc.kicker);

Correct.

drivers/isdn/sc/init.c static void __exit sc_exit(void) · line 394

Quoted 13 lines

drivers/input/serio/hp_sdc.c +0 -0

 	tasklet_kill(&hp_sdc.task);
 

diff -u -p a/drivers/isdn/sc/init.c b/drivers/isdn/sc/init.c

drivers/isdn/sc/init.c +2 -2

@@ -390,8 +390,8 @@ static void __exit sc_exit(void)
 		/*
 		 * kill the timers
 		 */
-		del_timer(&(sc_adapter[i]->reset_timer));
-		del_timer(&(sc_adapter[i]->stat_timer));
+		del_timer_sync(&(sc_adapter[i]->reset_timer));
+		del_timer_sync(&(sc_adapter[i]->stat_timer));

Correct.

drivers/isdn/i4l/isdn_common.c static void __exit isdn_exit(void) · line 2384

Quoted 11 lines

drivers/isdn/sc/init.c +0 -0

 		/*
 		 * Tell I4L we're toast

diff -u -p a/drivers/isdn/i4l/isdn_common.c b/drivers/isdn/i4l/isdn_common.c

drivers/isdn/i4l/isdn_common.c +1 -1

@@ -2381,7 +2381,7 @@ static void __exit isdn_exit(void)
 	}
 	isdn_tty_exit();
 	unregister_chrdev(ISDN_MAJOR, "isdn");
-	del_timer(&dev->timer);
+	del_timer_sync(&dev->timer);

Correct.

drivers/isdn/hardware/mISDN/hfcpci.c static void __exit · line 2355

Quoted 12 lines

drivers/isdn/i4l/isdn_common.c +0 -0

 	/* call vfree with interrupts enabled, else it will hang */
 	vfree(dev);
 	printk(KERN_NOTICE "ISDN-subsystem unloaded\n");

diff -u -p a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c

drivers/isdn/hardware/mISDN/hfcpci.c +1 -1

@@ -2352,7 +2352,7 @@ static void __exit
 HFC_cleanup(void)
 {
 	if (timer_pending(&hfc_tl))
-		del_timer(&hfc_tl);
+		del_timer_sync(&hfc_tl);

That's incomplete the timer_pending() check is racy as well and the timer is rearming itself from the callback.

drivers/isdn/act2000/module.c static void __exit act2000_exit(void) · line 799

Quoted 11 lines

drivers/isdn/hardware/mISDN/hfcpci.c +0 -0

 	pci_unregister_driver(&hfc_driver);
 }

diff -u -p a/drivers/isdn/act2000/module.c b/drivers/isdn/act2000/module.c

drivers/isdn/act2000/module.c +1 -1

@@ -796,7 +796,7 @@ static void __exit act2000_exit(void)
 	act2000_card *last;
 	while (card) {
 		unregister_card(card);
-		del_timer(&card->ptimer);
+		del_timer_sync(&card->ptimer);

Correct.

drivers/net/hamradio/yam.c static void __exit yam_cleanup_driver(vo · line 1187

Quoted 12 lines

drivers/isdn/act2000/module.c +0 -0

 		card = card->next;
 	}
 	card = cards;

diff -u -p a/drivers/net/hamradio/yam.c b/drivers/net/hamradio/yam.c

drivers/net/hamradio/yam.c +1 -1

@@ -1184,7 +1184,7 @@ static void __exit yam_cleanup_driver(vo
 	struct yam_mcs *p;
 	int i;
 
-	del_timer(&yam_timer);
+	del_timer_sync(&yam_timer);

Correct.

drivers/staging/speakup/main.c static void __exit speakup_exit(void) · line 2221

Quoted 12 lines

drivers/net/hamradio/yam.c +0 -0

 	for (i = 0; i < NR_PORTS; i++) {
 		struct net_device *dev = yam_devs[i];
 		if (dev) {

diff -u -p a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c

drivers/staging/speakup/main.c +1 -1

@@ -2218,7 +2218,7 @@ static void __exit speakup_exit(void)
 	unregister_keyboard_notifier(&keyboard_notifier_block);
 	unregister_vt_notifier(&vt_notifier_block);
 	speakup_unregister_devsynth();
-	del_timer(&cursor_timer);
+	del_timer_sync(&cursor_timer);

Not sure about that one as the timer might be rearmed, so the invocation might need to be further down the cleanup path.

drivers/staging/panel/panel.c static void __exit panel_cleanup_module( · line 2301

Quoted 12 lines

drivers/staging/speakup/main.c +0 -0

 	kthread_stop(speakup_task);
 	speakup_task = NULL;
 	mutex_lock(&spk_mutex);

diff -u -p a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c

drivers/staging/panel/panel.c +1 -1

@@ -2298,7 +2298,7 @@ static void __exit panel_cleanup_module(
 	unregister_reboot_notifier(&panel_notifier);
 
 	if (scan_timer.function != NULL)
-		del_timer(&scan_timer);
+		del_timer_sync(&scan_timer);

Correct.

drivers/staging/lustre/lustre/llite/super25.c static void __exit exit_lustre_lite(void · line 200

Quoted 11 lines

drivers/staging/panel/panel.c +0 -0

 	if (pprt != NULL) {
 		if (keypad_enabled) {

diff -u -p a/drivers/staging/lustre/lustre/llite/super25.c b/drivers/staging/lustre/lustre/llite/super25.c

drivers/staging/lustre/lustre/llite/super25.c +1 -1

@@ -197,7 +197,7 @@ static void __exit exit_lustre_lite(void
 {
 	ll_xattr_fini();
 	vvp_global_fini();
-	del_timer(&ll_capa_timer);
+	del_timer_sync(&ll_capa_timer);

Looks wrong, as the timer might be rearmed by the thread which is not yet stopped.

Quoted 7 lines

drivers/staging/lustre/lustre/llite/super25.c +0 -0

 	ll_capa_thread_stop();
 	LASSERTF(capa_count[CAPA_SITE_CLIENT] == 0,
 		 "client remaining capa count %d\n",

diff -u -p a/drivers/atm/idt77105.c b/drivers/atm/idt77105.c

drivers/atm/idt77105.c EXPORT_SYMBOL(idt77105_init); · line 373

Quoted 7 lines

drivers/atm/idt77105.c +2 -2

@@ -369,8 +369,8 @@ EXPORT_SYMBOL(idt77105_init);
 static void __exit idt77105_exit(void)
 {
         /* turn off timers */
-        del_timer(&stats_timer);
-        del_timer(&restart_timer);
+        del_timer_sync(&stats_timer);
+        del_timer_sync(&restart_timer);

Yep.

drivers/atm/nicstar.c static void __exit nicstar_cleanup(void) · line 310

Quoted 13 lines

drivers/atm/idt77105.c +0 -0

 }
 
 module_exit(idt77105_exit);

diff -u -p a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c

drivers/atm/nicstar.c +1 -1

@@ -306,7 +306,7 @@ static void __exit nicstar_cleanup(void)
 {
 	XPRINTK("nicstar: nicstar_cleanup() called.\n");
 
-	del_timer(&ns_timer);
+	del_timer_sync(&ns_timer);
 

The driver might still be in use, as the devices are shut down via the pci_unregister_driver() call below. So while it's correct to use del_timer_sync() it's not clear w/o digging deeper into the code whether it's actually sufficient.

drivers/atm/iphase.c static void __exit ia_module_exit(void) · line 3292

Quoted 11 lines

drivers/atm/nicstar.c +0 -0

 	pci_unregister_driver(&nicstar_driver);
 

diff -u -p a/drivers/atm/iphase.c b/drivers/atm/iphase.c

drivers/atm/iphase.c +1 -1

@@ -3289,7 +3289,7 @@ static void __exit ia_module_exit(void)
 {
 	pci_unregister_driver(&ia_driver);
 
-        del_timer(&ia_timer);
+        del_timer_sync(&ia_timer);

The timer is self rearming and has protection against the removal of the devices. So it should be fine.

net/hsr/hsr_main.c static int __init hsr_init(void) · line 462

Quoted 12 lines

drivers/atm/iphase.c +0 -0

 }
 
 module_init(ia_module_init);

diff -u -p a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c

net/hsr/hsr_main.c +1 -1

@@ -459,7 +459,7 @@ static int __init hsr_init(void)
 static void __exit hsr_exit(void)
 {
 	unregister_netdevice_notifier(&hsr_nb);
-	del_timer(&prune_timer);
+	del_timer_sync(&prune_timer);

Correct as the only rearm is from the timer callback itself

net/atm/mpc.c static void __exit atm_mpoa_cleanup(void · line 1495

Quoted 12 lines

net/hsr/hsr_main.c +0 -0

 	hsr_netlink_exit();
 	dev_remove_pack(&hsr_pt);
 }

diff -u -p a/net/atm/mpc.c b/net/atm/mpc.c

net/atm/mpc.c +1 -1

@@ -1492,7 +1492,7 @@ static void __exit atm_mpoa_cleanup(void
 
 	mpc_proc_clean();
 
-	del_timer(&mpc_timer);
+	del_timer_sync(&mpc_timer);

Looks correct as well. Timer is rearming no other dependencies.

net/atm/mpc.c static void __exit atm_mpoa_cleanup(void · line 1497

Quoted 2 lines

net/atm/mpc.c +0 -0

 	unregister_netdevice_notifier(&mpoa_notifier);
 	deregister_atm_ioctl(&atm_ioctl_ops);

Thanks,

tglx