Messages in this thread | | | Date | Mon, 7 Nov 2022 07:08:27 +0800 (+08) | From | Julia Lawall <> | Subject | Re: [PATCH v5a 5/5] treewide: Convert del_timer*() to timer_shutdown*() |
| |
On Sun, 6 Nov 2022, Steven Rostedt wrote:
> On Sun, 6 Nov 2022 12:51:46 -0800 > Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > Thanks, this looks reasonable. > > > > On Sat, Nov 5, 2022 at 10:46 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > The coccinelle script: > > > > > > @@ > > > expression E; > > > identifier ptr, timer, rfield, slab; > > > > I think Julia suggested making 'ptr' be an expression too, and I think > > And I forgot to add Julia to the Cc :-/ > > > she's right. Probably 'slab' should be too - there's no reason to > > limit it to just one identifier. > > > > > ... when strict > > > when != ptr->timer.function = E; > > > > I suspect any "ptr->timer" access anywhere between the > > del_timer_sync() and the freeing should disable things. > > > > Although hopefully there aren't any other odd cases than that one > > "clear timer function by hand" one. > > OK, I did the following with this new script: > > $ cat timer.cocci > @@ > expression E,ptr,slab; > identifier timer, rfield; > @@ > ( > - del_timer(&ptr->timer); > + timer_shutdown(&ptr->timer); > | > - del_timer_sync(&ptr->timer); > + timer_shutdown_sync(&ptr->timer); > ) > ... when strict > when != ptr->timer.function = E;
Maybe change the second when line to when != ptr->timer to see what are the differences in the results? That could show if there is anything important that has not been taken into account.
julia
> ( > kfree_rcu(ptr, rfield); > | > kmem_cache_free(slab, ptr); > | > kfree(ptr); > ) > > > $ git show | patch -p1 -R > $ spatch --dir timer.cocci . > /tmp/t.patch > $ patch -p1 < /tmp/t.patch > > And it caught some more: > > $ git diff > diff --git a/drivers/atm/idt77252.c b/drivers/atm/idt77252.c > index 99cae174d558..eec0cc2144e0 100644 > --- a/drivers/atm/idt77252.c > +++ b/drivers/atm/idt77252.c > @@ -2530,7 +2530,7 @@ idt77252_close(struct atm_vcc *vcc) > vc->tx_vcc = NULL; > > if (vc->estimator) { > - del_timer(&vc->estimator->timer); > + timer_shutdown(&vc->estimator->timer); > kfree(vc->estimator); > vc->estimator = NULL; > } > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c > index f9f18ff451ea..7ea2631b8069 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c > @@ -394,7 +394,7 @@ void brcmf_btcoex_detach(struct brcmf_cfg80211_info *cfg) > > if (cfg->btcoex->timer_on) { > cfg->btcoex->timer_on = false; > - del_timer_sync(&cfg->btcoex->timer); > + timer_shutdown_sync(&cfg->btcoex->timer); > } > > cancel_work_sync(&cfg->btcoex->work); > diff --git a/net/netfilter/xt_IDLETIMER.c b/net/netfilter/xt_IDLETIMER.c > index 0f8bb0bf558f..8d36303f3935 100644 > --- a/net/netfilter/xt_IDLETIMER.c > +++ b/net/netfilter/xt_IDLETIMER.c > @@ -413,7 +413,7 @@ static void idletimer_tg_destroy(const struct xt_tgdtor_param *par) > pr_debug("deleting timer %s\n", info->label); > > list_del(&info->timer->entry); > - del_timer_sync(&info->timer->timer); > + timer_shutdown_sync(&info->timer->timer); > cancel_work_sync(&info->timer->work); > sysfs_remove_file(idletimer_tg_kobj, &info->timer->attr.attr); > kfree(info->timer->attr.attr.name); > @@ -441,7 +441,7 @@ static void idletimer_tg_destroy_v1(const struct xt_tgdtor_param *par) > if (info->timer->timer_type & XT_IDLETIMER_ALARM) { > alarm_cancel(&info->timer->alarm); > } else { > - del_timer_sync(&info->timer->timer); > + timer_shutdown_sync(&info->timer->timer); > } > cancel_work_sync(&info->timer->work); > sysfs_remove_file(idletimer_tg_kobj, &info->timer->attr.attr); > > > Which all look legit. > > I'll post a v6a using the new script. > > -- Steve > >
| |