Re: [PATCH 4/3] pps: move wake_up and kill_fasync out of raw_spinlock section

[Rodolfo Giometti <giometti@enneenne.com>]

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@buchwitz.com>
> Signed-off-by: Michael Byczkowski <by@by-online.de>
> ---
>   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@by-online.de> 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@by-online.de>
>> Acked-by: Rodolfo Giometti <giometti@enneenne.com>
>> Tested-by: Michael Byczkowski <by@by-online.de>
>> Tested-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 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, &params, 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@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming