Re: [patch 10/15] timers: Silently ignore timers with a NULL function

[Steven Rostedt]

On Tue, 15 Nov 2022 21:28:49 +0100 (CET)
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> @@ -1128,6 +1144,9 @@ 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
>   * * %1 - The timer was active and requeued to expire at @expires
> @@ -1154,6 +1173,9 @@ 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.
> + *
>   * Return:
>   * * %0 - The timer was inactive and started

For those that only read the "Return" portion of kernel-doc, perhaps add
here:
             "or the timer is in the shutdown state and was not started".


>   * * %1 - The timer was active and requeued to expire at @expires or
> @@ -1175,6 +1197,9 @@ EXPORT_SYMBOL(mod_timer);
>   * modify an enqueued timer if that would reduce the expiration time. If
>   * @timer is not enqueued it starts the timer.
>   *
> + * If @timer->function == NULL then the start operation is silently
> + * discarded.
> + *
>   * Return:
>   * * %0 - The timer was inactive and started
>   * * %1 - The timer was active and requeued to expire at @expires or
> @@ -1201,6 +1226,9 @@ EXPORT_SYMBOL(timer_reduce);
>   *
>   * If @timer->expires is already in the past @timer will be queued to
>   * expire at the next timer tick.
> + *
> + * If @timer->function == NULL then the start operation is silently
> + * discarded.
>   */
>  void add_timer(struct timer_list *timer)
>  {
> @@ -1217,13 +1245,18 @@ EXPORT_SYMBOL(add_timer);
>   *
>   * This can only operate on an inactive timer. Attempts to invoke this on
>   * an active timer are rejected with a warning.
> + *
> + * If @timer->function == NULL then the start operation is silently
> + * discarded.
>   */
>  void add_timer_on(struct timer_list *timer, int cpu)
>  {
>  	struct timer_base *new_base, *base;
>  	unsigned long flags;
>  
> -	if (WARN_ON_ONCE(timer_pending(timer) || !timer->function))
> +	debug_assert_init(timer);
> +
> +	if (WARN_ON_ONCE(timer_pending(timer)))
>  		return;
>  
>  	new_base = get_timer_cpu_base(timer->flags, cpu);
> @@ -1234,6 +1267,13 @@ void add_timer_on(struct timer_list *tim
>  	 * wrong base locked.  See lock_timer_base().
>  	 */
>  	base = lock_timer_base(timer, &flags);
> +	/*
> +	 * Has @timer been shutdown? This needs to be evaluated while
> +	 * holding base lock to prevent a race against the shutdown code.
> +	 */
> +	if (!timer->function)
> +		goto out_unlock;
> +
>  	if (base != new_base) {
>  		timer->flags |= TIMER_MIGRATING;
>  
> @@ -1247,6 +1287,7 @@ void add_timer_on(struct timer_list *tim
>  
>  	debug_timer_activate(timer);
>  	internal_add_timer(base, timer);
> +out_unlock:
>  	raw_spin_unlock_irqrestore(&base->lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(add_timer_on);
> @@ -1532,6 +1573,12 @@ static void expire_timers(struct timer_b
>  
>  		fn = timer->function;
>  
> +		if (WARN_ON_ONCE(!fn)) {
> +			/* Should never happen. Emphasis on should! */
> +			base->running_timer = NULL;
> +			return;

Why return and not continue?

Wont this drop the other timers in the queue?

-- Steve


> +		}
> +
>  		if (timer->flags & TIMER_IRQSAFE) {
>  			raw_spin_unlock(&base->lock);
>  			call_timer_fn(timer, fn, baseclk);