Re: [PATCH v6 3/3] pps: convert pps_device.lock to raw_spinlock_t
From: Michael Byczkowski
Date: Tue May 26 2026 - 14:32:27 EST
Dear Calvin,
Thank you very much for catching this and for going the extra mile
of actually running v6 with CONFIG_DEBUG_ATOMIC_SLEEP -- I hadn't,
and clearly I should have. You're right that this is the same
pattern as the v2->v3 fix Nikolaus Buchwitz reported for the
pps_kc path back then; I just failed to apply the same reasoning
to pps_event() in kapi.c.
v7 is on GitHub with your fix folded in:
https://github.com/by/linux-PPS/tree/pps-rt-v7
Tip SHA: 58043ec91e1720ca18bb0dd5d83d3f6cdcd3f3da
Base: 0d9363a764d9d601a05591f9695cea8b429e9be3
Patch 3/3's trailer block credits you with Reported-by, Closes
(pointing at your lore archive entry), Suggested-by, and Tested-by.
Rodolfo's Acked-by on 3/3 is dropped since the change is
substantive; the cover letter notes this and invites him to re-ack.
Would you be willing to relay v7 the same way? Recipient list per
scripts/get_maintainer.pl (same as v6):
To: Rodolfo Giometti <giometti@xxxxxxxxxxxx>
Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Cc: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
Clark Williams <clrkwllms@xxxxxxxxxx>
Steven Rostedt <rostedt@xxxxxxxxxxx>
Thomas Gleixner <tglx@xxxxxxxxxxxxx>
linux-kernel@xxxxxxxxxxxxxxx
linux-rt-devel@xxxxxxxxxxxxxxx
Cover letter blurb for the 0000 file:
----------------------------------------------------------------
[paste the blurb from your v7 cover letter -- the text starting
"This is v7 of the PPS PREEMPT_RT patchset..." through the end
of the "All three patches are tested..." paragraph]
----------------------------------------------------------------
Thank you very much again, this would have bitten people in the field
had v6 landed as-is.
Best regards,
Michael
> On 26. May 2026, at 19:50, Calvin Owens <calvin@xxxxxxxxxx> wrote:
>
> On Monday 05/25 at 12:49 -0700, Calvin Owens wrote:
>> From: Michael Byczkowski <by@xxxxxxxxxxxx>
>>
>> pps_device.lock is acquired from the PPS hardirq path
>> (pps_event() and friends). On PREEMPT_RT, spinlock_t becomes a
>> sleeping rt_mutex, so taking it in hardirq context produces
>> "BUG: scheduling while atomic" splats and breaks PPS event
>> delivery entirely.
>>
>> Convert pps_device.lock to raw_spinlock_t, which remains a true
>> spinning lock on RT, and update all call sites in kapi.c and
>> pps.c to use raw_spin_lock_irqsave() / raw_spin_unlock_irqrestore()
>> (and the _irq variants where appropriate). The critical sections
>> are short and deterministic, so raw_spinlock_t semantics are
>> appropriate.
>>
>> No functional change on non-RT kernels.
>>
>> Tested-by: Michael Byczkowski <by@xxxxxxxxxxxx>
>> Acked-by: Rodolfo Giometti <giometti@xxxxxxxxxxxx>
>> Signed-off-by: Michael Byczkowski <by@xxxxxxxxxxxx>
>> Tested-by: Calvin Owens <calvin@xxxxxxxxxx>
>> Signed-off-by: Calvin Owens <calvin@xxxxxxxxxx>
>> ---
>> drivers/pps/kapi.c | 6 +++---
>> drivers/pps/pps.c | 16 ++++++++--------
>> include/linux/pps_kernel.h | 2 +-
>> 3 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
>> index 1bf0335a1b41..dc7fac75ec27 100644
>> --- a/drivers/pps/kapi.c
>> +++ b/drivers/pps/kapi.c
>> @@ -102,7 +102,7 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
>> pps->info.echo = pps_echo_client_default;
>>
>> init_waitqueue_head(&pps->queue);
>> - spin_lock_init(&pps->lock);
>> + raw_spin_lock_init(&pps->lock);
>>
>> /* Create the char device */
>> err = pps_register_cdev(pps);
>> @@ -167,7 +167,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
>>
>> timespec_to_pps_ktime(&ts_real, ts->ts_real);
>>
>> - spin_lock_irqsave(&pps->lock, flags);
>> + raw_spin_lock_irqsave(&pps->lock, flags);
>>
>> /* Must call the echo function? */
>> if ((pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
>> @@ -214,6 +214,6 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
>> kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
>> }
>>
>> - spin_unlock_irqrestore(&pps->lock, flags);
>> + raw_spin_unlock_irqrestore(&pps->lock, flags);
>> }
>> EXPORT_SYMBOL(pps_event);
>
> The Sashiko LLM says:
>
> Both wake_up_interruptible_all() and kill_fasync() internally take
> standard spinlocks and rwlocks, which map to sleepable rt_mutexes on
> PREEMPT_RT.
>
> 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
> expire_timers+0xcc/0x228
> __run_timer_base.part.0+0x18c/0x1a0
> timer_expire_remote+0x40/0x58
> tmigr_handle_remote_cpu+0xb4/0x278
> tmigr_handle_remote_up+0xdc/0x270
> __walk_groups_from+0x3c/0x98
> tmigr_handle_remote+0x8c/0xc0
> run_timer_softirq+0xa4/0xb8
> handle_softirqs.isra.0+0x100/0x3f8
> run_ktimerd+0x58/0xa8
> smpboot_thread_fn+0x204/0x338
> kthread+0x128/0x150
> ret_from_fork+0x10/0x20
>
> 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);
>
> Thanks,
> Calvin
>
>> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
>> index de1122bb69ea..75eb7973e37c 100644
>> --- a/drivers/pps/pps.c
>> +++ b/drivers/pps/pps.c
>> @@ -106,12 +106,12 @@ static long pps_cdev_ioctl(struct file *file,
>> case PPS_GETPARAMS:
>> dev_dbg(&pps->dev, "PPS_GETPARAMS\n");
>>
>> - spin_lock_irq(&pps->lock);
>> + raw_spin_lock_irq(&pps->lock);
>>
>> /* Get the current parameters */
>> params = pps->params;
>>
>> - spin_unlock_irq(&pps->lock);
>> + raw_spin_unlock_irq(&pps->lock);
>>
>> err = copy_to_user(uarg, ¶ms, sizeof(struct pps_kparams));
>> if (err)
>> @@ -142,7 +142,7 @@ static long pps_cdev_ioctl(struct file *file,
>> return -EINVAL;
>> }
>>
>> - spin_lock_irq(&pps->lock);
>> + raw_spin_lock_irq(&pps->lock);
>>
>> /* Save the new parameters */
>> pps->params = params;
>> @@ -166,7 +166,7 @@ static long pps_cdev_ioctl(struct file *file,
>> pps->params.assert_off_tu.flags = 0;
>> pps->params.clear_off_tu.flags = 0;
>>
>> - spin_unlock_irq(&pps->lock);
>> + raw_spin_unlock_irq(&pps->lock);
>>
>> break;
>>
>> @@ -193,7 +193,7 @@ static long pps_cdev_ioctl(struct file *file,
>> return err;
>>
>> /* Return the fetched timestamp and save last fetched event */
>> - spin_lock_irq(&pps->lock);
>> + raw_spin_lock_irq(&pps->lock);
>>
>> pps->last_fetched_ev = pps->last_ev;
>>
>> @@ -203,7 +203,7 @@ static long pps_cdev_ioctl(struct file *file,
>> fdata.info.clear_tu = pps->clear_tu;
>> fdata.info.current_mode = pps->current_mode;
>>
>> - spin_unlock_irq(&pps->lock);
>> + raw_spin_unlock_irq(&pps->lock);
>>
>> err = copy_to_user(uarg, &fdata, sizeof(struct pps_fdata));
>> if (err)
>> @@ -281,7 +281,7 @@ static long pps_cdev_compat_ioctl(struct file *file,
>> return err;
>>
>> /* Return the fetched timestamp and save last fetched event */
>> - spin_lock_irq(&pps->lock);
>> + raw_spin_lock_irq(&pps->lock);
>>
>> pps->last_fetched_ev = pps->last_ev;
>>
>> @@ -294,7 +294,7 @@ static long pps_cdev_compat_ioctl(struct file *file,
>> memcpy(&compat.info.clear_tu, &pps->clear_tu,
>> sizeof(struct pps_ktime_compat));
>>
>> - spin_unlock_irq(&pps->lock);
>> + raw_spin_unlock_irq(&pps->lock);
>>
>> return copy_to_user(uarg, &compat,
>> sizeof(struct pps_fdata_compat)) ? -EFAULT : 0;
>> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
>> index aab0aebb529e..f2fe504071ed 100644
>> --- a/include/linux/pps_kernel.h
>> +++ b/include/linux/pps_kernel.h
>> @@ -59,7 +59,7 @@ struct pps_device {
>> void const *lookup_cookie; /* For pps_lookup_dev() only */
>> struct device dev;
>> struct fasync_struct *async_queue; /* fasync method */
>> - spinlock_t lock;
>> + raw_spinlock_t lock;
>> };
>>
>> /*
>> --
>> 2.47.3