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

[Sebastian Andrzej Siewior]

On 2026-05-26 10:50:28 [-0700], Calvin Owens wrote:
> It does seem to be correct, with CONFIG_DEBUG_ATOMIC_SLEEP I get:
> 
>     BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
>     in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 31, name: ktimers/1
>     preempt_count: 1, expected: 0
>     RCU nest depth: 2, expected: 2
>     CPU: 1 UID: 0 PID: 31 Comm: ktimers/1 Not tainted 7.1.0-rc5-00004-g85f7c8ee247a-dirty #3 PREEMPT_{RT,LAZY}
>     Hardware name: Raspberry Pi 4 Model B Rev 1.5 (DT)
>     Call trace:
>      show_stack+0x1c/0x38 (C)
>      dump_stack_lvl+0x58/0x78
>      dump_stack+0x14/0x1c
>      __might_resched+0x128/0x168
>      rt_spin_lock+0x34/0x180
>      __wake_up+0x2c/0x70
>      pps_event+0xdc/0x2b0
>      pps_ktimer_event+0x44/0x80
>      call_timer_fn+0x34/0x2d0

This is all threaded and the atomic contex gets here due to the lock
swap.

> The lock isn't part of the lifetime of the pps_device object, so I think
> fixing that really is as simple as not holding the raw lock across the
> wakeup, something like below?
> 
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index dc7fac75ec27..f85929d01e3d 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -203,17 +203,16 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
>  
>  		captured = ~0;
>  	}
>  
>  	pps_kc_event(pps, ts, event);
> +	if (captured)
> +		pps->last_ev++;
> +	raw_spin_unlock_irqrestore(&pps->lock, flags);
>  
>  	/* Wake up if captured something */
>  	if (captured) {
> -		pps->last_ev++;
>  		wake_up_interruptible_all(&pps->queue);
> -
>  		kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
>  	}
> -
> -	raw_spin_unlock_irqrestore(&pps->lock, flags);
>  }
>  EXPORT_SYMBOL(pps_event);

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.

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?

> Thanks,
> Calvin

Sebastian