[PATCH 0/7] replace del_timer by del_timer_sync

[PATCH 0/7] replace del_timer by del_timer_sync

[Julia Lawall]

These patches replace del_timer by del_timer_sync in module_exit functions.

[PATCH 1/7] isdn: replace del_timer by del_timer_sync

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use del_timer_sync to ensure that the timer is stopped on all CPUs before
the driver exists.

This change was suggested by Thomas Gleixner.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
declarer name module_exit;
identifier ex;
@@

module_exit(ex);

@@
identifier r.ex;
@@

ex(...) {
  <...
- del_timer
+ del_timer_sync
    (...)
  ...>
}
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Not tested.

 drivers/isdn/act2000/module.c  |    2 +-
 drivers/isdn/i4l/isdn_common.c |    2 +-
 drivers/isdn/sc/init.c         |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/isdn/sc/init.c b/drivers/isdn/sc/init.c
index 92acc81..d6f19b1 100644
--- a/drivers/isdn/sc/init.c
+++ b/drivers/isdn/sc/init.c
@@ -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));
 
 		/*
 		 * Tell I4L we're toast
diff --git a/drivers/isdn/i4l/isdn_common.c b/drivers/isdn/i4l/isdn_common.c
index 130f216..9b856e1 100644
--- a/drivers/isdn/i4l/isdn_common.c
+++ b/drivers/isdn/i4l/isdn_common.c
@@ -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);
 	/* call vfree with interrupts enabled, else it will hang */
 	vfree(dev);
 	printk(KERN_NOTICE "ISDN-subsystem unloaded\n");
diff --git a/drivers/isdn/act2000/module.c b/drivers/isdn/act2000/module.c
index b4147c0..c3a1b06 100644
--- a/drivers/isdn/act2000/module.c
+++ b/drivers/isdn/act2000/module.c
@@ -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);
 		card = card->next;
 	}
 	card = cards;

Re: [PATCH 1/7] isdn: replace del_timer by del_timer_sync

[David Miller]

From: Julia Lawall <Julia.Lawall@lip6.fr>
Date: Wed, 26 Mar 2014 22:33:39 +0100

> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Use del_timer_sync to ensure that the timer is stopped on all CPUs before
> the driver exists.
> 
> This change was suggested by Thomas Gleixner.
> 
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
 ...
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Applied.

Re: [PATCH 1/7] isdn: replace del_timer by del_timer_sync

[Tilman Schmidt]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 26.03.2014 22:33, schrieb Julia Lawall:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Use del_timer_sync to ensure that the timer is stopped on all CPUs
> before the driver exists.

Surely you meant "exits"?

Regards,
Tilman

- -- 
Tilman Schmidt                              E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlM2A5IACgkQQ3+did9BuFtuMACfarliJdOkqLmJJvRqbcM5eCFd
CWwAn3FuQ+juOeo2EgM8e1JBTjlHhG3o
=a90K
-----END PGP SIGNATURE-----

Re: [PATCH 1/7] isdn: replace del_timer by del_timer_sync

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 856 bytes --]



On Sat, 29 Mar 2014, Tilman Schmidt wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Am 26.03.2014 22:33, schrieb Julia Lawall:
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
> > 
> > Use del_timer_sync to ensure that the timer is stopped on all CPUs
> > before the driver exists.
> 
> Surely you meant "exits"?

Yes.

julia

> 
> Regards,
> Tilman
> 
> - -- 
> Tilman Schmidt                              E-Mail: tilman@imap.cc
> Bonn, Germany
> Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
> Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.22 (MingW32)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iEYEARECAAYFAlM2A5IACgkQQ3+did9BuFtuMACfarliJdOkqLmJJvRqbcM5eCFd
> CWwAn3FuQ+juOeo2EgM8e1JBTjlHhG3o
> =a90K
> -----END PGP SIGNATURE-----
> 

[PATCH 2/7] atm: replace del_timer by del_timer_sync

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use del_timer_sync to ensure that the timer is stopped on all CPUs before
the driver exists.

This change was suggested by Thomas Gleixner.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
declarer name module_exit;
identifier ex;
@@

module_exit(ex);

@@
identifier r.ex;
@@

ex(...) {
  <...
- del_timer
+ del_timer_sync
    (...)
  ...>
}
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Not tested.

 net/atm/mpc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/atm/mpc.c b/net/atm/mpc.c
index b71ff6b..91dc58f 100644
--- a/net/atm/mpc.c
+++ b/net/atm/mpc.c
@@ -1492,7 +1492,7 @@ static void __exit atm_mpoa_cleanup(void)
 
 	mpc_proc_clean();
 
-	del_timer(&mpc_timer);
+	del_timer_sync(&mpc_timer);
 	unregister_netdevice_notifier(&mpoa_notifier);
 	deregister_atm_ioctl(&atm_ioctl_ops);
 

Re: [PATCH 2/7] atm: replace del_timer by del_timer_sync

[Eric Dumazet]

On Wed, 2014-03-26 at 22:33 +0100, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>

> 
> diff --git a/net/atm/mpc.c b/net/atm/mpc.c
> index b71ff6b..91dc58f 100644
> --- a/net/atm/mpc.c
> +++ b/net/atm/mpc.c
> @@ -1492,7 +1492,7 @@ static void __exit atm_mpoa_cleanup(void)
>  
>  	mpc_proc_clean();
>  
> -	del_timer(&mpc_timer);
> +	del_timer_sync(&mpc_timer);
>  	unregister_netdevice_notifier(&mpoa_notifier);
>  	deregister_atm_ioctl(&atm_ioctl_ops);
>  

It seems this timer rearms itself, so this patch wont be enough I fear.

Re: [PATCH 2/7] atm: replace del_timer by del_timer_sync

[Thomas Gleixner]

On Wed, 26 Mar 2014, Eric Dumazet wrote:
> On Wed, 2014-03-26 at 22:33 +0100, Julia Lawall wrote:
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> > 
> > diff --git a/net/atm/mpc.c b/net/atm/mpc.c
> > index b71ff6b..91dc58f 100644
> > --- a/net/atm/mpc.c
> > +++ b/net/atm/mpc.c
> > @@ -1492,7 +1492,7 @@ static void __exit atm_mpoa_cleanup(void)
> >  
> >  	mpc_proc_clean();
> >  
> > -	del_timer(&mpc_timer);
> > +	del_timer_sync(&mpc_timer);
> >  	unregister_netdevice_notifier(&mpoa_notifier);
> >  	deregister_atm_ioctl(&atm_ioctl_ops);
> >  
> 
> It seems this timer rearms itself, so this patch wont be enough I fear.

That's why you need del_timer_sync() it waits for the timer handler to
complete and then removes it.

Thanks,

	tglx

Re: [PATCH 2/7] atm: replace del_timer by del_timer_sync

[Eric Dumazet]

On Wed, 2014-03-26 at 23:02 +0100, Thomas Gleixner wrote:

> That's why you need del_timer_sync() it waits for the timer handler to
> complete and then removes it.

Interesting... So maybe some comment is stale ?


diff --git a/kernel/timer.c b/kernel/timer.c
index accfd241b9e5..c948fa2fc065 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1011,10 +1011,10 @@ EXPORT_SYMBOL(try_to_del_timer_sync);
  * the timer it also makes sure the handler has finished executing on other
  * CPUs.
  *
- * Synchronization rules: Callers must prevent restarting of the timer,
- * otherwise this function is meaningless. It must not be called from
- * interrupt contexts unless the timer is an irqsafe one. The caller must
- * not hold locks which would prevent completion of the timer's
+ * Synchronization rules:
+ * It must not be called from interrupt contexts unless the timer is an
+ * irqsafe one.
+ * The caller must not hold locks which would prevent completion of the timer's
  * handler. The timer's handler must not call add_timer_on(). Upon exit the
  * timer is not queued and the handler is not running on any CPU.
  *

Re: [PATCH 2/7] atm: replace del_timer by del_timer_sync

[Thomas Gleixner]

On Wed, 26 Mar 2014, Eric Dumazet wrote:

> On Wed, 2014-03-26 at 23:02 +0100, Thomas Gleixner wrote:
> 
> > That's why you need del_timer_sync() it waits for the timer handler to
> > complete and then removes it.
> 
> Interesting... So maybe some comment is stale ?
> 
> 
> diff --git a/kernel/timer.c b/kernel/timer.c
> index accfd241b9e5..c948fa2fc065 100644
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -1011,10 +1011,10 @@ EXPORT_SYMBOL(try_to_del_timer_sync);
>   * the timer it also makes sure the handler has finished executing on other
>   * CPUs.
>   *
> - * Synchronization rules: Callers must prevent restarting of the timer,
> - * otherwise this function is meaningless. It must not be called from

We still need to prevent restarting from some other site,
i.e. interrupt. It's fine if the timer handler itself restarts the
timer, as del_timer_sync() wait for the handler to finish and then
delete the timer.

I can't find the reason why it can't call add_timer_on(). We
lock/unlock the timer base in every try_to_del_timer_sync() loop so we
detect the base change.

Thanks,

	tglx

Re: [PATCH 2/7] atm: replace del_timer by del_timer_sync

[Vijay Subramanian]

>
> It seems this timer rearms itself, so this patch wont be enough I fear.
>
>
>
Does that matter? It seems del_timer_sync() may be intended to work
despite this (i.e. timer will not fire after call to this function).
My earlier attempt to fix a similar issue in sch_red.c resulted in
this explanation.
http://www.spinics.net/lists/netdev/msg256410.html

Vijay

Re: [PATCH 2/7] atm: replace del_timer by del_timer_sync

[Eric Dumazet]

On Wed, 2014-03-26 at 15:19 -0700, Vijay Subramanian wrote:
> >
> > It seems this timer rearms itself, so this patch wont be enough I fear.
> >
> >
> >
> Does that matter? It seems del_timer_sync() may be intended to work
> despite this (i.e. timer will not fire after call to this function).
> My earlier attempt to fix a similar issue in sch_red.c resulted in
> this explanation.
> http://www.spinics.net/lists/netdev/msg256410.html

I know this very well, thank you.

If documentation is stale, it needs clarification.

If not, we need to make sure timer wont be restarted.

Re: [PATCH 2/7] atm: replace del_timer by del_timer_sync

[David Miller]

From: Julia Lawall <Julia.Lawall@lip6.fr>
Date: Wed, 26 Mar 2014 22:33:40 +0100

> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Use del_timer_sync to ensure that the timer is stopped on all CPUs before
> the driver exists.
> 
> This change was suggested by Thomas Gleixner.
> 
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
 ...
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Applied.

[PATCH 6/7] hsr: replace del_timer by del_timer_sync

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use del_timer_sync to ensure that the timer is stopped on all CPUs before
the driver exists.

This change was suggested by Thomas Gleixner.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
declarer name module_exit;
identifier ex;
@@

module_exit(ex);

@@
identifier r.ex;
@@

ex(...) {
  <...
- del_timer
+ del_timer_sync
    (...)
  ...>
}
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Not tested.

 net/hsr/hsr_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
index 10010c5..3fee521 100644
--- a/net/hsr/hsr_main.c
+++ b/net/hsr/hsr_main.c
@@ -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);
 	hsr_netlink_exit();
 	dev_remove_pack(&hsr_pt);
 }

Re: [PATCH 6/7] hsr: replace del_timer by del_timer_sync

[David Miller]

From: Julia Lawall <Julia.Lawall@lip6.fr>
Date: Wed, 26 Mar 2014 22:33:44 +0100

> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Use del_timer_sync to ensure that the timer is stopped on all CPUs before
> the driver exists.
> 
> This change was suggested by Thomas Gleixner.
> 
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
 ...
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Applied.

[PATCH 7/7] yam: replace del_timer by del_timer_sync

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Use del_timer_sync to ensure that the timer is stopped on all CPUs before
the driver exists.

This change was suggested by Thomas Gleixner

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
declarer name module_exit;
identifier ex;
@@

module_exit(ex);

@@
identifier r.ex;
@@

ex(...) {
  <...
- del_timer
+ del_timer_sync
    (...)
  ...>
}
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Not tested.

 drivers/net/hamradio/yam.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/hamradio/yam.c b/drivers/net/hamradio/yam.c
index 61dd244..8190165 100644
--- a/drivers/net/hamradio/yam.c
+++ b/drivers/net/hamradio/yam.c
@@ -1184,7 +1184,7 @@ static void __exit yam_cleanup_driver(void)
 	struct yam_mcs *p;
 	int i;
 
-	del_timer(&yam_timer);
+	del_timer_sync(&yam_timer);
 	for (i = 0; i < NR_PORTS; i++) {
 		struct net_device *dev = yam_devs[i];
 		if (dev) {

Re: [PATCH 7/7] yam: replace del_timer by del_timer_sync

[David Miller]

From: Julia Lawall <Julia.Lawall@lip6.fr>
Date: Wed, 26 Mar 2014 22:33:45 +0100

> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Use del_timer_sync to ensure that the timer is stopped on all CPUs before
> the driver exists.
> 
> This change was suggested by Thomas Gleixner
> 
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
 ...
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Applied.