Re: [PATCH] clocksource: disable irq when holding watchdog_lock.

[Sebastian Andrzej Siewior]

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