Re: [PATCH v6 3/3] pps: convert pps_device.lock to raw_spinlock_t

[Sebastian Andrzej Siewior]

On 2026-05-29 05:37:59 [-0700], Calvin Owens wrote:
> > So lets look at the series:
> > - #1 splits the handler to take time stamp in hardirq and the remaining
> >   work in the thread. It seems to be crucial to do so because doing so
> >   in thread breaks something. Okay. Granted, makes a bit of sense.
> > 
> > - #2 ignore for argument's sake
> > 
> > - #3 says swap the lock so we can use pps_event() in hardirq context.
> 
> I interpreted that as saying "swap the lock so the lock can be safely
> acquired from code running in both hardirq and threaded context".

That is correct. But wake_up_interruptible_all() and kill_fasync() must
not be used in hardirq context.

> But looking more closely, it doesn't seem that either lock is ever
> acquired in hardirq context before or after this series.
> 
> So I think patches 2 and 3 are unnecessary.

Okay then.

> As a quick confirmation, I tested only patch 1 on a bona fide pps-gpio
> setup with lockdep and atomic_sleep and saw no splats.
> 
> Patch 2 suggests tk_core.lock being a raw lock was somehow a problem,
> but it's never held across anything, it seems fine.

pps_event() does invoke pps_kc_event(). So you need to swap this lock
(pps_kc_hardpps_lock) before swapping pps->lock. And hardpps() itself
already has a raw_spinlock_t so it is fine.

That is my interpretation.

> > Now. Why or where do we need to use pps_event() in hardirq context?
> > If we use pps_event() is used in hardirq context, as claimed in the
> > commit message, then dropping the lock early here does not help and it
> > will lead to the same splat in wake_up_interruptible_all() because
> > interrupts are still disabled independent of the lock here. It is
> > hardirq-context after all.
> > 
> > You don't see this warning in your testcase because it gets here from a
> > timer, yes. But. pps_event() can't be used as-is from hardirq context
> > either.
> 
> In pps-gpio it's called from pps_gpio_irq_handler() which is threaded.
> 
> In pps-ldisc it's called from pps_tty_dcd_change(). That is called from
> uart_handle_dcd_change(), the uart_port lock is a normal spin lock and
> is held over the call, and it calls wake_up_interruptible(), so I think
> that one must be fine (I only looked closely at 8250).
> 
> PTP calls it from ptp_clock_event() which itself takes a normal spin
> lock, so if that one wasn't safe people would get a splat already.
> 
> In pps-parport it's called from parport_irq(). That is called from
> parport_irq_handler() and mfc3_interrupt() which both look threaded to
> me.
> 
> In the testcase the earlier trace was from it's called from
> pps_ktimer_event() which is threaded.
> 
> I don't see anywhere pps_event() is called in hardirq context.

Okay. In that case #1 should be sufficient to not delay the taking the
timestamp if the context switch to the threaded interrupt is indeed such
a problem.

Sebastian