Re: [PATCH 4/3] pps: move wake_up and kill_fasync out of raw_spinlock section
From: Rodolfo Giometti
Date: Tue Apr 07 2026 - 10:51:54 EST
On 4/7/26 16:24, Michael Byczkowski wrote:
The raw_spinlock conversion in the previous patch introduced a latent
bug on PREEMPT_RT: wake_up_interruptible_all() and kill_fasync()
acquire sleeping locks internally (the waitqueue's spinlock_t and
fasync_lock respectively become rt_mutexes on RT). Calling them with
preemption disabled by raw_spin_lock is invalid and will trigger
"sleeping function called from invalid context" under
CONFIG_DEBUG_ATOMIC_SLEEP or hard-hang under contention.
Fix this by moving pps->last_ev++ (the event counter that waiters
check) inside the raw_spinlock critical section, then releasing the
lock before calling wake_up_interruptible_all() and kill_fasync().
This is safe because:
- The PPS_FETCH waiters use wait_event_interruptible_timeout() with a
condition on pps->last_ev, so they will re-check after waking.
- kill_fasync() does not require the PPS data to be locked.
- The timestamp, sequence number, and last_ev updates remain fully
serialised under the raw_spinlock.
On non-RT kernels there is no behavioral change, as raw_spinlock_t
compiles to the same code as spinlock_t.
Mmm.. first of all, you should resubmit the patchset and not a single "update" (maybe using the subject "[PATCH v2] pps: Condensed patch summary" - see as instance http://www.linuxpps.org/pipermail/discussions/2010-November/004142.html).
Then you should provide a single patch where you rework the pps_event() and __then__ do a simple replace of the spin_unlock_irqrestore() with its raw version.
Ciao,
Rodolfo
Suggested-by: Nikolaus Buchwitz <nb@xxxxxxxxxxxx>
Signed-off-by: Michael Byczkowski <by@xxxxxxxxxxxx>
---
drivers/pps/kapi.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index dc7fac75ec2738..XXXXXXXXXXXXXXXX 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -207,13 +207,17 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
pps_kc_event(pps, ts, event);
- /* Wake up if captured something */
- if (captured) {
+ if (captured)
pps->last_ev++;
+
+ raw_spin_unlock_irqrestore(&pps->lock, flags);
+
+ /*
+ * Wake up after releasing the lock: wake_up_interruptible_all()
+ * and kill_fasync() acquire sleeping locks on PREEMPT_RT.
+ */
+ if (captured) {
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);
On 6. Apr 2026, at 22:52, Michael Byczkowski <by@xxxxxxxxxxxx> wrote:
On PREEMPT_RT, spinlock_t becomes a sleeping mutex, which allows
pps_event() to be preempted mid-update by other RT threads. This
introduces jitter in the PPS event recording path.
Convert pps_device.lock to raw_spinlock_t so the timestamp and
sequence number updates remain non-preemptible on RT.
On non-RT kernels, raw_spinlock_t compiles to identical code as
spinlock_t — no behavioral change.
Signed-off-by: Michael Byczkowski <by@xxxxxxxxxxxx>
Acked-by: Rodolfo Giometti <giometti@xxxxxxxxxxxx>
Tested-by: Michael Byczkowski <by@xxxxxxxxxxxx>
Tested-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);
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index c6b8b6478276..ad96425208a1 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -103,12 +103,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)
@@ -139,7 +139,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;
@@ -163,7 +163,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;
@@ -190,7 +190,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;
@@ -200,7 +200,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)
@@ -278,7 +278,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;
@@ -291,7 +291,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
--
GNU/Linux Solutions e-mail: giometti@xxxxxxxxxxxx
Linux Device Driver giometti@xxxxxxxx
Embedded Systems phone: +39 349 2432127
UNIX programming