All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Steven Rostedt <rostedt@goodmis.org>, linux-kernel@vger.kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Anna-Maria Gleixner <anna-maria@linutronix.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Julia Lawall <Julia.Lawall@inria.fr>
Subject: Re: [PATCH v6 4/6] timers: Add timer_shutdown_sync() to be called before freeing timers
Date: Sun, 13 Nov 2022 22:52:16 +0100	[thread overview]
Message-ID: <87cz9qttdb.ffs@tglx> (raw)
In-Reply-To: <20221110064147.343514404@goodmis.org>

On Thu, Nov 10 2022 at 01:41, Steven Rostedt wrote:

$Subject: -ENOPARSE

 timers: Provide timer_shutdown_sync()

and then have some reasonable explanation in the change log?

> We are hitting a common bug were a timer is being triggered after it
> is

We are hitting? Talking in pluralis majestatis by now?

> freed. This causes a corruption in the timer link list and crashes the
> kernel. Unfortunately it is not easy to know what timer it was that was

Well, that's not entirely true. debugobjects can tell you exactly what
happens. 

> freed. Looking at the code, it appears that there are several cases that
> del_timer() is used when del_timer_sync() should have been.
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 717fcb9fb14a..111a3550b3f2 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1017,7 +1017,8 @@ __mod_timer(struct timer_list *timer, unsigned long expires, unsigned int option
>  	unsigned int idx = UINT_MAX;
>  	int ret = 0;
>  
> -	BUG_ON(!timer->function);
> +	if (WARN_ON_ONCE(!timer->function))
> +		return -EINVAL;

Can you please make these BUG -> WARN conversions a separate patch?

> +/**
> + * timer_shutdown_sync - called before freeing the timer

1) The sentence after the dash starts with an upper case letter as all
   sentences do.

2) "called before freeing the timer" tells us what?

   See below.

> + * @timer: The timer to be freed
> + *
> + * Shutdown the timer before freeing. This will return when all pending timers
> + * have finished and it is safe to free the timer.

   "_ALL_ pending timers have finished?"

This is about exactly _ONE_ timer, i.e. the one which is handed in via
the @timer argument.

You want to educate people to do the right thing and then you go and
provide them uncomprehensible documentation garbage. How is that
supposed to work?

Can you please stop this frenzy and get your act together?

> + *
> + * Note, after calling this, if the timer is added back to the queue
> + * it will fail to be added and a WARNING will be triggered.

There is surely a way to express this so that the average driver writer
who does not have the background of you working on this understands this
"note".

> + *
> + * Returns if it deactivated a pending timer or not.

Please look up the kernel-doc syntax for documenting return values.

Thanks,

        tglx

  reply	other threads:[~2022-11-13 21:52 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-10  6:41 [PATCH v6 0/6] timers: Use timer_shutdown*() before freeing timers Steven Rostedt
2022-11-10  6:41 ` [PATCH v6 1/6] ARM: spear: Do not use timer namespace for timer_shutdown() function Steven Rostedt
2022-11-10  6:41   ` Steven Rostedt
2022-11-10  6:41 ` [PATCH v6 2/6] clocksource/drivers/arm_arch_timer: " Steven Rostedt
2022-11-10  6:41   ` Steven Rostedt
2022-11-10  6:41 ` [PATCH v6 3/6] clocksource/drivers/sp804: " Steven Rostedt
2022-11-10  6:41 ` [PATCH v6 4/6] timers: Add timer_shutdown_sync() to be called before freeing timers Steven Rostedt
2022-11-13 21:52   ` Thomas Gleixner [this message]
2022-11-14  0:11     ` Steven Rostedt
2022-11-14  1:04       ` Thomas Gleixner
2022-11-14 14:08         ` Steven Rostedt
2022-11-14 18:53           ` Thomas Gleixner
2022-11-14 19:14             ` Steven Rostedt
2022-11-13 23:18   ` Thomas Gleixner
2022-11-14  0:15     ` Steven Rostedt
2022-11-14  0:33       ` Thomas Gleixner
2022-11-14 13:36         ` Steven Rostedt
2022-11-14 19:13           ` Thomas Gleixner
2022-11-14 19:28             ` Steven Rostedt
2022-11-14 19:54               ` Thomas Gleixner
2022-11-14 15:42         ` Thomas Gleixner
2022-11-14 16:04           ` Steven Rostedt
2022-11-14 17:16           ` Linus Torvalds
2022-11-14 17:50             ` Steven Rostedt
2022-11-14 17:54               ` Linus Torvalds
2022-11-14 19:45             ` Thomas Gleixner
2022-11-24 14:15           ` [tip: timers/core] Bluetooth: hci_qca: Fix the teardown problem for real tip-bot2 for Thomas Gleixner
2022-11-10  6:41 ` [PATCH v6 5/6] timers: Add timer_shutdown() to be called before freeing timers Steven Rostedt
2022-11-13 22:20   ` Thomas Gleixner
2022-11-10  6:41 ` [PATCH v6 6/6] timers: Update the documentation to reflect on the new timer_shutdown() API Steven Rostedt
2022-11-24 14:16   ` [tip: timers/core] " tip-bot2 for Steven Rostedt (Google)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87cz9qttdb.ffs@tglx \
    --to=tglx@linutronix.de \
    --cc=Julia.Lawall@inria.fr \
    --cc=akpm@linux-foundation.org \
    --cc=anna-maria@linutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=rostedt@goodmis.org \
    --cc=sboyd@kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.