From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtpcmd13147.aruba.it (smtpcmd13147.aruba.it [62.149.156.147]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9F47530BB8C for ; Tue, 7 Apr 2026 14:47:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=62.149.156.147 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775573238; cv=none; b=ngdzm+8HDvY5FC4inx0iwDv+KYPve6i4R+RIeBclsk7yWMcglS/3anksYvSYKHLE6C5KJvEsX7FattYCYhCUVi3uyqSzbY9V2EptkZuI9YH7Y5RTGUymFV0rIwiuMX4sUljA+doilAwnmWrKsaQbQI4x6K7pZuls46EU5rpd0tY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775573238; c=relaxed/simple; bh=P8ll4qtrN+gPFzQEpPfGc1zP8uITo0fxU5yhAKpyS1s=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=I57R+EQErusXklNyE+hGGoYwHnuboejofSC7rA/xAJLlb8zr1qh8jHXKcO5pNO75NH0nWhiO3QkM6END6gspMbZIsEmjv5BhgqUyuXFGOOEW8KL5PEqDUKJm+m4PeGcb7kUV5/269lC/siZkA3Uy08JIcs2YV/vSoJbW53VSvtU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=enneenne.com; spf=pass smtp.mailfrom=enneenne.com; dkim=pass (2048-bit key) header.d=aruba.it header.i=@aruba.it header.b=XFzQUAYJ; arc=none smtp.client-ip=62.149.156.147 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=enneenne.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=enneenne.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=aruba.it header.i=@aruba.it header.b="XFzQUAYJ" Received: from [192.168.1.58] ([79.0.204.227]) by Aruba SMTP with ESMTPSA id A7erwTjw2RHshA7eswNxqz; Tue, 07 Apr 2026 16:44:03 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=aruba.it; s=a1; t=1775573043; bh=P8ll4qtrN+gPFzQEpPfGc1zP8uITo0fxU5yhAKpyS1s=; h=Date:MIME-Version:Subject:To:From:Content-Type; b=XFzQUAYJey63nwsjgFNylDYzE0kh1vaHxe7DPQ8Uv25GtfafmdE7v+ZYTOH0gpgtb ffXSPWuq73UsgmTtt2I1UVrajIabzCb3pNSEOduYKM3ReFdEFKukGW0niMciAsisQ6 0IwOKBxKu35dxzWVzbTJf3XrGe8vSWmI16WWhGu0REfPTkDSsWmPUbHqfcDrAYg75L mF2H7oLWiNEJXdKBsnFX5rAsAoWWRrPBL1On9LdTfQ/d6pSVcRjkBQkXzAOrpW8k5A sJn2HRGxt2DChpRuwQt+gpVsL7fyaopRu97hF/Ht29nhMjUkmuWSnlObwiwwNMSfMT 0BDN2Hn1m8Tbw== Message-ID: <2221764c-1146-4e02-b561-64c84dc8b4e1@enneenne.com> Date: Tue, 7 Apr 2026 16:44:01 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 4/3] pps: move wake_up and kill_fasync out of raw_spinlock section Content-Language: en-US To: Michael Byczkowski Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org References: <1BB87C0C-33C1-45C3-B50E-C5F349DA3FDC@by-online.de> <9F77A769-3F79-4C9E-B93B-B41990CF822F@by-online.de> <6C4789E8-7954-4056-8C2B-7EEA65118B94@by-online.de> From: Rodolfo Giometti In-Reply-To: <6C4789E8-7954-4056-8C2B-7EEA65118B94@by-online.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-CMAE-Envelope: MS4xfEorFhNmnRxhWaVl9KPH3ONRMOQZMnGB4Mo6b/n/HsnxeDQv7bKDjuY77U+lWdkarRaK4iLXlybrT4WlLCKiSAxsTjdgm+l82Wxfbip0Y3JMUWfqVb7v KBfLh7/uSmMurRuhJ5vUXKO1hSR5dnN1BgNcYb/pK1XYAr7p1xcs9aK87sX9maj/3CZhgILr+MqKNYZH2s935M1jbR9XDAKNuV0+p5r6SoKooH30XBdhygYP 6dLewYdI64JE9HTMVbZ7UXSEvEvF3eNgv5Zx9UEF0DY= 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 > Signed-off-by: Michael Byczkowski > --- > 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 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 >> Acked-by: Rodolfo Giometti >> Tested-by: Michael Byczkowski >> Tested-by: Calvin Owens >> --- >> 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@enneenne.com Linux Device Driver giometti@linux.it Embedded Systems phone: +39 349 2432127 UNIX programming