Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Fri, 20 Oct 2023 11:02:45 +0200
From:   Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     John Stultz <jstultz@google.com>,
        Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        Stephen Boyd <sboyd@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "Paul E. McKenney" <paulmck@kernel.org>, x86@kernel.org
Subject: Re: [PATCH] clocksource: disable irq when holding watchdog_lock.
Message-ID: <20231020090245.cPAViz09@linutronix.de>
References: <80ff5036-8449-44a6-ba2f-0130d3be6b57@I-love.SAKURA.ne.jp>
 <CANDhNCpw+hEHNbtdAZR01HsHW_L1C0BXjZq21eXouQGNnYuUNQ@mail.gmail.com>
 <878r826xys.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <878r826xys.ffs@tglx>
Precedence: bulk

On 2023-10-16 23:47:55 [+0200], Thomas Gleixner wrote:
>   But the explicit local_irq_disable()/enable() pair around the readout
>   means that the patch above is obviously broken:
> 
>     spin_lock_irqsave(&watchdog_lock, flags);
>     cs_watchdog_read()
>       local_irq_disable();
>       ...
>       local_irq_enable();  <--- FAIL
> 
>   Lockdep should have yelled at that too, but even if it failed to do
>   so then it's still very obviously wrong.

Lockdep will shout from local_irq_enable() if the lock is still held.
That is working and I just verified it. However I didn't get to that
part in cs_watchdog_read() in my testing so that might be why lockdep
didn't complain in Tetsuo's case. After adding a irq-enable before the
unlock for lockdep-testing I did see the splat.

> Something like the uncompiled/untested below should cure it for real. It
> really does not matter whether the TSC unstable event happens a bit
> later. The system is unhappy no matter what.
The snippet below is perfect :)

> That said, this whole clocksource watchdog mess wants a proper
> overhaul. It has become a pile of warts and duct tape by now and after
> staring at it long enough there is no real reason to run it in a timer
> callback anymore. It just can move into delayed work and the whole
> locking problem can be reduced to the clocksource_mutex and some well
> thought out atomic operations to handle the mark unstable case. But
> that's a different story and not relevant for curing the problem at
> hand.

A pile of warts. Okay. This explains why the lock is acquired everywhere
with irqsave while in-IRQ access is not desired.

> Thanks,
> 
>         tglx

Sebastian