Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221110064101.429013735@goodmis.org> <20221110064147.343514404@goodmis.org>
 <875yfitpdu.ffs@tglx> <20221113191518.0a3b29b1@rorschach.local.home>
 <87sfims7ca.ffs@tglx> <87iljhsftt.ffs@tglx>
In-Reply-To: <87iljhsftt.ffs@tglx>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon, 14 Nov 2022 09:16:31 -0800
Message-ID: <CAHk-=wj7DtViDctAzV3PqdYBEh5vcQnRJPtFFB=uaAP=W-VG4A@mail.gmail.com>
Subject: Re: [PATCH v6 4/6] timers: Add timer_shutdown_sync() to be called
 before freeing timers
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Steven Rostedt <rostedt@goodmis.org>, linux-kernel@vger.kernel.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>,
        Eric Dumazet <edumazet@google.com>,
        Marcel Holtmann <marcel@holtmann.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Nov 14, 2022 at 7:42 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> So if we want to make this solid and make the life of driver writers
> easier, then we cannot issue a warning as I said in the original thread
> already.

So I think that there are two issues at play:

 (a) do we want to *find* problem places after the conversion

 (b) do we want to make driver writing easier

and (a) argues for warning on timer re-arming, but (b) just says
"don't warn, just ignore it, the driver is being shut down".

I'm personally ok with either of those approaches, and it's literally
just a question of mindset.

> The semantics of timer_shutdown_sync() have to be:
>
>    After return:
>      - the timer is not queued
>      - the timer callbacks is not running
>      - the timer cannot be enqueued again

Yes, but that last case is literally a "do we expect the *driver* to
not enqueue it and warn if it tries, or do we just silently enforce
it"?

I agree with all three points. I'm just not sure about who we expect
to do the "don't enqueue again".

There's a big argument for "make it easy for driver writers" in just
saying "make mod_timer() silently just ignore a re-arming". Making
things easier for driver writers is a good thing.

But maybe it's a "you shouldn't have done that in the first place"
thing, and merits a warning?

I have no strong opinions on that.

What I *do* still want to happen is for subsystems to be able to start
doing the conversion one by one. Which is why I'd still prefer to have
the new names available just so that we don't have to have one
50-patch series, but we can have subsystems apply the obvious cases.

And I'd still like the mindless "let's get the non-semantic changes
out of the way" as one single patch, to get rid of mindless noise.

And honestly, for that to happen I'd be perfectly happy with something like

  #define timer_shutdown(t) del_timer(t)
  #define timer_shutdown_sync(t) del_timer_sync(t)

(obviously with the patches that first remove the existing
'timer_shutdown()' uses first). That wouldn't introduce the *new*
semantics, but it would at least allow the different subsystems to do
the obvious cases, and let the networking people wonder about the much
less obvious ones.

              Linus