Re: [patch V2 12/17] timers: Silently ignore timers with a NULL function
From: Anna-Maria Behnsen <anna-maria@linutronix.de>
Date: 2022-11-23 09:23:32
Also in:
linux-bluetooth, lkml
On Tue, 22 Nov 2022, Thomas Gleixner wrote:
Tearing down timers which have circular dependencies to other functionality, e.g. workqueues, where the timer can schedule work and work can arm timers is not trivial.
NIT (comma is missing): can arm timer, is not trivial.
In those cases it is desired to shutdown the timer in a way which prevents rearming of the timer. The mechanism to do so it to set timer->function to
s/to do so it/to do so is/
NULL and use this as an indicator for the timer arming functions to ignore the (re)arm request. In preparation for that replace the warnings in the relevant code pathes with checks for timer->function == NULL and discard the rearm request silently.
Here is a verb missing that this is a grammatically correct (and understandable) sentence.
quoted hunk ↗ jump to hunk
Add debug_assert_init() instead of the WARN_ON_ONCE(!timer->function) checks so that debug objects can warn about non-initialized timers. If developers fail to enable debug objects and then waste lots of time to figure out why their non-initialized timer is not firing, they deserve it. Co-developed-by: Steven Rostedt <rostedt@goodmis.org> Signed-off-by: Steven Rostedt <rostedt@goodmis.org> Signed-off-by: Thomas Gleixner <redacted> Tested-by: Guenter Roeck <linux@roeck-us.net> Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home (local) Link: https://lore.kernel.org/all/20221110064101.429013735@goodmis.org (local) --- V2: Use continue instead of return and amend the return value docs (Steven) --- kernel/time/timer.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 5 deletions(-)--- a/kernel/time/timer.c +++ b/kernel/time/timer.c@@ -1128,8 +1144,12 @@ static inline int * mod_timer_pending() is the same for pending timers as mod_timer(), but * will not activate inactive timers. * + * If @timer->function == NULL then the start operation is silently + * discarded. + * * Return: - * * %0 - The timer was inactive and not modified + * * %0 - The timer was inactive and not modified or was is in + * shutdown state and the operation was discarded
Do you mean "was" or "is"? Please have also a look at the places where you use the same phrase.
quoted hunk ↗ jump to hunk
* * %1 - The timer was active and requeued to expire at @expires */ int mod_timer_pending(struct timer_list *timer, unsigned long expires)@@ -1155,8 +1175,12 @@ EXPORT_SYMBOL(mod_timer_pending); * same timer, then mod_timer() is the only safe way to modify the timeout, * since add_timer() cannot modify an already running timer. * + * If @timer->function == NULL then the start operation is silently + * discarded, the return value is 0 and meaningless.
It's easier to read, if you make a dot instead of comma. Thanks, Anna-Maria