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

[Calvin Owens]

On Friday 05/29 at 09:19 +0200, Sebastian Andrzej Siewior wrote:
> On 2026-05-28 08:57:37 [-0700], Calvin Owens wrote:
> …
> > > This does not work because the commit message claims to allow
> > > pps_event() to be called from hardirq context. So if this is called
> > > indeed from hardirq, context as claimed in the commit message, then
> > > dropping the lock early does not help here.
> > 
> > It isn't called from hardirq AFAICS.

Sorry, maybe I buried this short reply.

> > > Does it need to call pps_event() in hardirq? Patch #1 takes the
> > > timestamp in hardirq deferring the remaining part to the thread. What is
> > > the crucial part here that needs to happen in hardirq context?
> > 
> > This is the artificial testcase which just uses a ktimer, it both
> > "samples" the time and calls pps_event() from the same timer callback.
> > 
> > But I understood you to be saying it's threaded above, which mirrors my
> > own understanding. If that's true, I don't see why dropping the lock
> > early wouldn't be sufficient here?
> 
> 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".

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.

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.

> 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.

> > This is what lockdep says on v6:
> > 
> >     =============================
> >     [ BUG: Invalid wait context ]
> …
> >     -----------------------------
> >     ktimers/0/15 is trying to lock:
> >     ffff888107303118 (&pps->queue){....}-{3:3}, at: __wake_up+0x1f/0x50
> >     other info that might help us debug this:
> …
> > If I drop the lock early, lockdep and atomic_sleep are both slient.
> 
> Yes and might_sleep() would yell, too.
> 
> > Thanks,
> > Calvin
> 
> Sebastian