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

[Michael Byczkowski]

Hi Sebastian, Calvin,

Thank you both, and you're right on all three points, I should
have caught this before sending v6.

Confirming the analysis:

- pps_event() is never called in hardirq context. After patch 1/3
  the pps-gpio path runs it from the threaded handler; pps-ldisc
  (via uart_handle_dcd_change, under the uart_port spinlock), PTP
  (via ptp_clock_event, under a normal spinlock), pps-parport and
  the pps-ktimer testcase all reach it from threaded or normal-
  spinlock context that would already splat if unsafe. So pps->lock
  never needs to be raw -- patch 3/3 is dropped.

- pps_kc_hardpps_lock is the outer lock; hardpps()'s tk_core.lock
  is the inner one. Taking a raw lock under a sleeping lock is the
  legal direction, and the lock isn't held across anything that
  sleeps, so it needn't be raw either -- patch 2/3 is dropped.

- Patch 1/3 stands alone: it removes the timestamp-capture delay
  that IRQ force-threading introduces on PREEMPT_RT, which was the
  real motivation. Thanks for testing it in isolation.

I'll resend patch 1/3 as a standalone patch (v7).

Best regards,
	Michael



> On 29. May 2026, at 14:57, Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> wrote:
> 
> 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