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@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, &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
>