[PATCH v6 0/3] pps: improve PREEMPT_RT performance

[PATCH v6 0/3] pps: improve PREEMPT_RT performance

[Calvin Owens]

Hello all,

Michael is busy, and I have some extra bandwidth, so I've volunteered to
help out with the patch schlepping here. These v6 patches are based on
7.1-rc5.

This is all Michael's work, and Michael's cover letter follows. I added
lore links to the older patch versions next to his change descriptions.

Note that I replaced two emdashes in comment blocks with regular hypens
in patch 1/3, to avoid 8bit encoding.

Thanks,
Calvin

---
From: Michael Byczkowski <by@by-online.de>

This is v6 of the PPS PREEMPT_RT patchset, addressing the review
feedback from Sebastian Andrzej Siewior on v5.

Changes since v5: https://lore.kernel.org/lkml/719A31CE-CA58-45C3-A013-1BFE81F724C5@by-online.de/
 - Reordered: the pps_kc_hardpps_lock conversion now precedes the
   pps_device.lock conversion. The previous order would have briefly
   produced a raw_spinlock holding a sleeping spinlock on PREEMPT_RT
   (Sebastian).
 - Patch 1/3: commit message reworded to describe the handler split
   structurally first, then its PREEMPT_RT benefit (Sebastian).
 - Patch 2/3: refactored pps_kc_bind() and pps_kc_remove() to use
   guard(raw_spinlock_irq) for scope-based lock release. Eliminates
   four duplicated unlock call sites in pps_kc_bind() and the
   ambiguous bracket structure that resulted from them (Sebastian).
 - Rodolfo's Acked-by on patch 2/3 is preserved from v5; the guard()
   refactor is purely stylistic and was suggested by Sebastian, but
   please re-ack or NAK if disagreement.

Changes since v4: https://lore.kernel.org/lkml/B24484C5-3117-4C56-9522-1EE9876E64BA@by-online.de/
 - Patch 2/3: added Acked-by: Rodolfo Giometti <giometti@enneenne.com>

Changes since v3: https://lore.kernel.org/lkml/83318241-44C3-48BE-829D-5C5F82A78A74@by-online.de/
 - Patch 2/3: fixed lost indentation on pps_kc_event() call
   (reported by Rodolfo Giometti <giometti@enneenne.com>)

Changes since v2: https://lore.kernel.org/lkml/1BB87C0C-33C1-45C3-B50E-C5F349DA3FDC@by-online.de/
 - Patch 2/3: moved wake_up_interruptible_all() and kill_fasync() out
   of raw_spinlock section to avoid sleeping-in-atomic on PREEMPT_RT
   (reported by Nikolaus Buchwitz <nb@buchwitz.com>)

This patchset addresses three sources of PPS jitter under PREEMPT_RT,
while being fully backward-compatible with non-RT kernels:

1. pps-gpio: The IRQ handler is force-threaded on PREEMPT_RT, so the
   PPS timestamp is captured after scheduling delay rather than at
   interrupt entry. Fix: split into a hardirq primary handler
   (captures timestamp only) and a threaded handler (processes the
   event).

2. pps_kc_hardpps_lock: spinlock_t becomes a sleeping mutex on
   PREEMPT_RT. Since pps_kc_event() calls hardpps() under this lock
   and hardpps() takes the raw_spinlock_t tk_core.lock, the nesting
   is invalid. Fix: convert to DEFINE_RAW_SPINLOCK.

3. pps_device.lock: same issue as (2), in the PPS event delivery
   path. Fix: convert to raw_spinlock_t and move sleeping calls out
   of the critical section.

All three patches are tested on a Raspberry Pi 5 running 7.0.1 and
7.1-rc PREEMPT_RT kernels. On non-RT kernels there is zero behavioral
change.

Michael Byczkowski (3):
  pps: pps-gpio: split IRQ handler into hardirq timestamper + threaded
    handler
  pps: kc: convert pps_kc_hardpps_lock to raw_spinlock_t
  pps: convert pps_device.lock to raw_spinlock_t

 drivers/pps/clients/pps-gpio.c | 37 +++++++++++++------
 drivers/pps/kapi.c             |  6 +--
 drivers/pps/kc.c               | 67 ++++++++++++++++------------------
 drivers/pps/pps.c              | 16 ++++----
 include/linux/pps_kernel.h     |  2 +-
 5 files changed, 68 insertions(+), 60 deletions(-)

-- 
2.47.3

[PATCH v6 1/3] pps: pps-gpio: split IRQ handler into hardirq timestamper + threaded handler

[Calvin Owens]

From: Michael Byczkowski <by@by-online.de>

Split the pps-gpio interrupt handler into a primary (hardirq) handler
that captures the PPS timestamp at interrupt entry, and a threaded
handler that processes the event. This produces the same two-part
handler structure on both PREEMPT_RT and non-RT kernels.

On non-RT kernels the threaded portion runs immediately after the
primary, with no behavioral change compared to the previous
single-handler implementation.

On PREEMPT_RT, where interrupt handlers are force-threaded by default,
the previous single-handler implementation captured the timestamp
inside the threaded portion, after IRQ-thread scheduling delay. With
the split, the timestamp is captured in true hardirq context as it is
on non-RT kernels, eliminating a significant source of PPS jitter on
RT systems.

Tested-by: Michael Byczkowski <by@by-online.de>
Tested-by: Calvin Owens <calvin@wbinvd.org>
Acked-by: Rodolfo Giometti <giometti@enneenne.com>
Signed-off-by: Michael Byczkowski <by@by-online.de>
Signed-off-by: Calvin Owens <calvin@wbinvd.org>
---
 drivers/pps/clients/pps-gpio.c | 37 +++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 935da68610c7..f37398fd6b10 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -35,33 +35,44 @@ struct pps_gpio_device_data {
 	bool capture_clear;
 	unsigned int echo_active_ms;	/* PPS echo active duration */
 	unsigned long echo_timeout;	/* timer timeout value in jiffies */
+	struct pps_event_time ts;	/* timestamp captured in hardirq */
 };
 
 /*
  * Report the PPS event
  */
 
-static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
+/*
+ * Primary hardirq handler - runs in hardirq context even on PREEMPT_RT.
+ * Only captures the timestamp; all other work is deferred to the thread.
+ */
+static irqreturn_t pps_gpio_irq_hardirq(int irq, void *data)
 {
-	const struct pps_gpio_device_data *info;
-	struct pps_event_time ts;
-	int rising_edge;
+	struct pps_gpio_device_data *info = data;
+
+	pps_get_ts(&info->ts);
 
-	/* Get the time stamp first */
-	pps_get_ts(&ts);
+	return IRQ_WAKE_THREAD;
+}
 
-	info = data;
+/*
+ * Threaded handler - processes the PPS event using the timestamp
+ * captured in hardirq context above.
+ */
+static irqreturn_t pps_gpio_irq_thread(int irq, void *data)
+{
+	struct pps_gpio_device_data *info = data;
+	int rising_edge;
 
-	/* Small trick to bypass the check on edge's direction when capture_clear is unset */
 	rising_edge = info->capture_clear ?
 		      gpiod_get_value(info->gpio_pin) : !info->assert_falling_edge;
 	if ((rising_edge && !info->assert_falling_edge) ||
 			(!rising_edge && info->assert_falling_edge))
-		pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
+		pps_event(info->pps, &info->ts, PPS_CAPTUREASSERT, data);
 	else if (info->capture_clear &&
 			((rising_edge && info->assert_falling_edge) ||
 			(!rising_edge && !info->assert_falling_edge)))
-		pps_event(info->pps, &ts, PPS_CAPTURECLEAR, data);
+		pps_event(info->pps, &info->ts, PPS_CAPTURECLEAR, data);
 	else
 		dev_warn_ratelimited(&info->pps->dev, "IRQ did not trigger any PPS event\n");
 
@@ -210,8 +221,10 @@ static int pps_gpio_probe(struct platform_device *pdev)
 	}
 
 	/* register IRQ interrupt handler */
-	ret = request_irq(data->irq, pps_gpio_irq_handler,
-			  get_irqf_trigger_flags(data), data->info.name, data);
+	ret = request_threaded_irq(data->irq,
+			  pps_gpio_irq_hardirq, pps_gpio_irq_thread,
+			  get_irqf_trigger_flags(data) | IRQF_ONESHOT,
+			  data->info.name, data);
 	if (ret) {
 		pps_unregister_source(data->pps);
 		dev_err(dev, "failed to acquire IRQ %d\n", data->irq);
-- 
2.47.3

[PATCH v6 2/3] pps: kc: convert pps_kc_hardpps_lock to raw_spinlock_t

[Calvin Owens]

From: Michael Byczkowski <by@by-online.de>

pps_kc_hardpps_lock is held in pps_kc_event(), which calls
hardpps(). hardpps() takes tk_core.lock, which is already a
raw_spinlock_t -- and nesting a sleeping lock (PREEMPT_RT's
spinlock_t) over a raw_spinlock_t is invalid.

Convert pps_kc_hardpps_lock and its call sites in kc.c to
raw_spinlock_t to make the nesting valid on PREEMPT_RT.

While here, refactor pps_kc_bind() and pps_kc_remove() to use
guard(raw_spinlock_irq) for scope-based lock release. This
eliminates four duplicated unlock call sites in pps_kc_bind()
and the ambiguous bracket structure that resulted from them.
Behavior is unchanged.

No functional change on non-RT kernels.

Tested-by: Michael Byczkowski <by@by-online.de>
Tested-by: Calvin Owens <calvin@wbinvd.org>
Acked-by: Rodolfo Giometti <giometti@enneenne.com>
Signed-off-by: Michael Byczkowski <by@by-online.de>
Signed-off-by: Calvin Owens <calvin@wbinvd.org>
---
 drivers/pps/kc.c | 67 ++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 36 deletions(-)

diff --git a/drivers/pps/kc.c b/drivers/pps/kc.c
index fbd23295afd7..715b775e9679 100644
--- a/drivers/pps/kc.c
+++ b/drivers/pps/kc.c
@@ -21,7 +21,7 @@
  */
 
 /* state variables to bind kernel consumer */
-static DEFINE_SPINLOCK(pps_kc_hardpps_lock);
+static DEFINE_RAW_SPINLOCK(pps_kc_hardpps_lock);
 /* PPS API (RFC 2783): current source and mode for kernel consumer */
 static struct pps_device *pps_kc_hardpps_dev;	/* unique pointer to device */
 static int pps_kc_hardpps_mode;		/* mode bits for kernel consumer */
@@ -36,35 +36,31 @@ static int pps_kc_hardpps_mode;		/* mode bits for kernel consumer */
 int pps_kc_bind(struct pps_device *pps, struct pps_bind_args *bind_args)
 {
 	/* Check if another consumer is already bound */
-	spin_lock_irq(&pps_kc_hardpps_lock);
+	guard(raw_spinlock_irq)(&pps_kc_hardpps_lock);
 
-	if (bind_args->edge == 0)
-		if (pps_kc_hardpps_dev == pps) {
-			pps_kc_hardpps_mode = 0;
-			pps_kc_hardpps_dev = NULL;
-			spin_unlock_irq(&pps_kc_hardpps_lock);
-			dev_info(&pps->dev, "unbound kernel"
-					" consumer\n");
-		} else {
-			spin_unlock_irq(&pps_kc_hardpps_lock);
-			dev_err(&pps->dev, "selected kernel consumer"
-					" is not bound\n");
+	if (bind_args->edge == 0) {
+		/* Unbind request */
+		if (pps_kc_hardpps_dev != pps) {
+			dev_err(&pps->dev,
+				"selected kernel consumer is not bound\n");
 			return -EINVAL;
 		}
-	else
-		if (pps_kc_hardpps_dev == NULL ||
-				pps_kc_hardpps_dev == pps) {
-			pps_kc_hardpps_mode = bind_args->edge;
-			pps_kc_hardpps_dev = pps;
-			spin_unlock_irq(&pps_kc_hardpps_lock);
-			dev_info(&pps->dev, "bound kernel consumer: "
-				"edge=0x%x\n", bind_args->edge);
-		} else {
-			spin_unlock_irq(&pps_kc_hardpps_lock);
-			dev_err(&pps->dev, "another kernel consumer"
-					" is already bound\n");
+		pps_kc_hardpps_mode = 0;
+		pps_kc_hardpps_dev = NULL;
+		dev_info(&pps->dev, "unbound kernel consumer\n");
+	} else {
+		/* Bind request */
+		if (pps_kc_hardpps_dev && pps_kc_hardpps_dev != pps) {
+			dev_err(&pps->dev,
+				"another kernel consumer is already bound\n");
 			return -EINVAL;
 		}
+		pps_kc_hardpps_mode = bind_args->edge;
+		pps_kc_hardpps_dev = pps;
+		dev_info(&pps->dev,
+			 "bound kernel consumer: edge=0x%x\n",
+			 bind_args->edge);
+	}
 
 	return 0;
 }
@@ -78,15 +74,14 @@ int pps_kc_bind(struct pps_device *pps, struct pps_bind_args *bind_args)
  */
 void pps_kc_remove(struct pps_device *pps)
 {
-	spin_lock_irq(&pps_kc_hardpps_lock);
-	if (pps == pps_kc_hardpps_dev) {
-		pps_kc_hardpps_mode = 0;
-		pps_kc_hardpps_dev = NULL;
-		spin_unlock_irq(&pps_kc_hardpps_lock);
-		dev_info(&pps->dev, "unbound kernel consumer"
-				" on device removal\n");
-	} else
-		spin_unlock_irq(&pps_kc_hardpps_lock);
+	guard(raw_spinlock_irq)(&pps_kc_hardpps_lock);
+
+	if (pps != pps_kc_hardpps_dev)
+		return;
+
+	pps_kc_hardpps_mode = 0;
+	pps_kc_hardpps_dev = NULL;
+	dev_info(&pps->dev, "unbound kernel consumer on device removal\n");
 }
 
 /* pps_kc_event - call hardpps() on PPS event
@@ -102,8 +97,8 @@ void pps_kc_event(struct pps_device *pps, struct pps_event_time *ts,
 	unsigned long flags;
 
 	/* Pass some events to kernel consumer if activated */
-	spin_lock_irqsave(&pps_kc_hardpps_lock, flags);
+	raw_spin_lock_irqsave(&pps_kc_hardpps_lock, flags);
 	if (pps == pps_kc_hardpps_dev && event & pps_kc_hardpps_mode)
 		hardpps(&ts->ts_real, &ts->ts_raw);
-	spin_unlock_irqrestore(&pps_kc_hardpps_lock, flags);
+	raw_spin_unlock_irqrestore(&pps_kc_hardpps_lock, flags);
 }
-- 
2.47.3

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

[Calvin Owens]

From: Michael Byczkowski <by@by-online.de>

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@by-online.de>
Acked-by: Rodolfo Giometti <giometti@enneenne.com>
Signed-off-by: Michael Byczkowski <by@by-online.de>
Tested-by: Calvin Owens <calvin@wbinvd.org>
Signed-off-by: Calvin Owens <calvin@wbinvd.org>
---
 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 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, &params, 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

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

[Calvin Owens]

On Monday 05/25 at 12:49 -0700, Calvin Owens wrote:
> From: Michael Byczkowski <by@by-online.de>
> 
> 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@by-online.de>
> Acked-by: Rodolfo Giometti <giometti@enneenne.com>
> Signed-off-by: Michael Byczkowski <by@by-online.de>
> Tested-by: Calvin Owens <calvin@wbinvd.org>
> Signed-off-by: Calvin Owens <calvin@wbinvd.org>
> ---
>  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, &params, 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
> 

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

[Michael Byczkowski]

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@enneenne.com>
       Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  Cc:  Sebastian Andrzej Siewior <bigeasy@linutronix.de>
       Clark Williams <clrkwllms@kernel.org>
       Steven Rostedt <rostedt@goodmis.org>
       Thomas Gleixner <tglx@linutronix.de>
       linux-kernel@vger.kernel.org
       linux-rt-devel@lists.linux.dev

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@wbinvd.org> wrote:
> 
> On Monday 05/25 at 12:49 -0700, Calvin Owens wrote:
>> From: Michael Byczkowski <by@by-online.de>
>> 
>> 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@by-online.de>
>> Acked-by: Rodolfo Giometti <giometti@enneenne.com>
>> Signed-off-by: Michael Byczkowski <by@by-online.de>
>> Tested-by: Calvin Owens <calvin@wbinvd.org>
>> Signed-off-by: Calvin Owens <calvin@wbinvd.org>
>> ---
>> 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, &params, 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

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

[Michael Byczkowski]

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@enneenne.com>
       Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  Cc:  Sebastian Andrzej Siewior <bigeasy@linutronix.de>
       Clark Williams <clrkwllms@kernel.org>
       Steven Rostedt <rostedt@goodmis.org>
       Thomas Gleixner <tglx@kernel.org>
       linux-kernel@vger.kernel.org
       linux-rt-devel@lists.linux.dev

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@by-online.de> 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@enneenne.com>
>       Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>  Cc:  Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>       Clark Williams <clrkwllms@kernel.org>
>       Steven Rostedt <rostedt@goodmis.org>
>       Thomas Gleixner <tglx@linutronix.de>
>       linux-kernel@vger.kernel.org
>       linux-rt-devel@lists.linux.dev
> 
> 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@wbinvd.org> wrote:
>> 
>> On Monday 05/25 at 12:49 -0700, Calvin Owens wrote:
>>> From: Michael Byczkowski <by@by-online.de>
>>> 
>>> 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@by-online.de>
>>> Acked-by: Rodolfo Giometti <giometti@enneenne.com>
>>> Signed-off-by: Michael Byczkowski <by@by-online.de>
>>> Tested-by: Calvin Owens <calvin@wbinvd.org>
>>> Signed-off-by: Calvin Owens <calvin@wbinvd.org>
>>> ---
>>> 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, &params, 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
> 
> 

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

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

[Calvin Owens]

On Thursday 05/28 at 09:49 +0200, Sebastian Andrzej Siewior wrote:
> 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.

Yes, I thought this was from a threaded context.

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

It isn't called from hardirq AFAICS.

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

This is what lockdep says on v6:

    =============================
    [ BUG: Invalid wait context ]
    7.1.0-rc5-x86-kvm-00005-ge4dc519be2c8 #7 Not tainted
    -----------------------------
    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:
    context-{5:5}
    6 locks held by ktimers/0/15:
     #0: ffffffff82c3f540 (local_bh){.+.+}-{1:3}, at: __local_bh_disable_ip+0x38/0x230
     #1: ffffffff82fd7340 (rcu_read_lock){....}-{1:3}, at: __local_bh_disable_ip+0x126/0x230
     #2: ffff88856bbd93e0 (&base->expiry_lock){+...}-{3:3}, at: timer_expire_remote+0x42/0x70
     #3: ffffffff82fd7340 (rcu_read_lock){....}-{1:3}, at: rt_spin_lock+0xd9/0x1a0
     #4: ffffc90000083c60 ((&ktimer)){+...}-{0:0}, at: call_timer_fn+0x68/0x250
     #5: ffff888107303540 (&pps->lock){....}-{2:2}, at: pps_event+0x3d/0x1e0
    stack backtrace:
    CPU: 0 UID: 0 PID: 15 Comm: ktimers/0 Not tainted 7.1.0-rc5-x86-kvm-00005-ge4dc519be2c8 #7 PREEMPT_RT 
    Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-20240910_120124-localhost 04/01/2014
    Call Trace:
     <TASK>
     dump_stack_lvl+0x5b/0x80
     __lock_acquire+0x896/0xbc0
     lock_acquire.part.0+0x58/0x180
     ? __wake_up+0x1f/0x50
     rt_spin_lock+0x32/0x1a0
     ? __wake_up+0x1f/0x50
     __wake_up+0x1f/0x50
     ? assert_show+0x40/0x40
     pps_event+0xaf/0x1e0
     ? assert_show+0x40/0x40
     ? assert_show+0x40/0x40
     pps_ktimer_event+0x48/0x70
     ? assert_show+0x40/0x40
     call_timer_fn+0x91/0x250

If I drop the lock early, lockdep and atomic_sleep are both slient.

Thanks,
Calvin

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

[Sebastian Andrzej Siewior]

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

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.

> 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

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

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

[Sebastian Andrzej Siewior]

On 2026-05-29 05:37:59 [-0700], Calvin Owens wrote:
> > 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".

That is correct. But wake_up_interruptible_all() and kill_fasync() must
not be used in hardirq 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.

Okay then.

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

pps_event() does invoke pps_kc_event(). So you need to swap this lock
(pps_kc_hardpps_lock) before swapping pps->lock. And hardpps() itself
already has a raw_spinlock_t so it is fine.

That is my interpretation.

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

Okay. In that case #1 should be sufficient to not delay the taking the
timestamp if the context switch to the threaded interrupt is indeed such
a problem.

Sebastian

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

[Michael Byczkowski]

Hi Sebastian, Calvin,

Thank you both, and you're right on all three points, I should
have caught this before sending v6.

Confirming the analysis:

- pps_event() is never called in hardirq context. After patch 1/3
  the pps-gpio path runs it from the threaded handler; pps-ldisc
  (via uart_handle_dcd_change, under the uart_port spinlock), PTP
  (via ptp_clock_event, under a normal spinlock), pps-parport and
  the pps-ktimer testcase all reach it from threaded or normal-
  spinlock context that would already splat if unsafe. So pps->lock
  never needs to be raw -- patch 3/3 is dropped.

- pps_kc_hardpps_lock is the outer lock; hardpps()'s tk_core.lock
  is the inner one. Taking a raw lock under a sleeping lock is the
  legal direction, and the lock isn't held across anything that
  sleeps, so it needn't be raw either -- patch 2/3 is dropped.

- Patch 1/3 stands alone: it removes the timestamp-capture delay
  that IRQ force-threading introduces on PREEMPT_RT, which was the
  real motivation. Thanks for testing it in isolation.

I'll resend patch 1/3 as a standalone patch (v7).

Best regards,
	Michael



> On 29. May 2026, at 14:57, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> On 2026-05-29 05:37:59 [-0700], Calvin Owens wrote:
>>> 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".
> 
> That is correct. But wake_up_interruptible_all() and kill_fasync() must
> not be used in hardirq 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.
> 
> Okay then.
> 
>> 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.
> 
> pps_event() does invoke pps_kc_event(). So you need to swap this lock
> (pps_kc_hardpps_lock) before swapping pps->lock. And hardpps() itself
> already has a raw_spinlock_t so it is fine.
> 
> That is my interpretation.
> 
>>> 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.
> 
> Okay. In that case #1 should be sufficient to not delay the taking the
> timestamp if the context switch to the threaded interrupt is indeed such
> a problem.
> 
> Sebastian