Re: [PATCH v6 3/3] pps: convert pps_device.lock to raw_spinlock_t
From: Michael Byczkowski
Date: Sat May 30 2026 - 07:14:40 EST
Dear Calvin,
Thanks again for the testing and the analysis. As discussed, v7 is
just the pps-gpio handler split (old patch 1); patches 2 and 3 are
dropped since neither lock is ever taken in hardirq context.
Branch:
https://github.com/by/linux-PPS/tree/pps-rt-v7-clean
Tip SHA: b1a8501137e93bb56dd47878571dca151723bdc3
Base: 0d9363a764d9d601a05591f9695cea8b429e9be3
Single patch, so no cover letter -- the version history is below the
--- line. Would you be willing to relay it once more?
Recipients (per get_maintainer.pl):
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@xxxxxxxxxx>
linux-kernel@xxxxxxxxxxxxxxx
linux-rt-devel@xxxxxxxxxxxxxxx
To regenerate from the branch:
git fetch https://github.com/by/linux-PPS.git pps-rt-v7-clean
git format-patch -1 \
--subject-prefix="PATCH v7" \
--base=0d9363a764d9d601a05591f9695cea8b429e9be3 \
FETCH_HEAD
Thanks and best regards,
Michael
> On 26. May 2026, at 20:31, Michael Byczkowski <by@xxxxxxxxxxxx> wrote:
>
> 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
>
>