On Sun, Nov 16, 2025 at 11:28:37PM +0100, Jeremie Courreges-Anglas wrote:
> On Sun, Nov 16, 2025 at 02:21:51PM +0100, Claudio Jeker wrote:
> > On Sat, Nov 15, 2025 at 06:35:40PM +0100, Mark Kettenis wrote:
> [...]
> > > Clearing P_SINTR in __set_current_state() may very well be the right
> > > solution. The Linux code that uses __set_current_state() already
> > > checks for pending signals so we don't need to do it again.
> > >
> > > Curious what claudio@ has to say...
> >
> > I agree.
>
> Cool, so I'm now looking for oks for that fix. I think it is a
> serious candidate for a backport to 7.8.
>
> > > > Index: dev/pci/drm/drm_linux.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.c,v
> > > > diff -u -p -r1.126 drm_linux.c
> > > > --- dev/pci/drm/drm_linux.c 13 Jun 2025 07:01:37 -0000 1.126
> > > > +++ dev/pci/drm/drm_linux.c 15 Nov 2025 13:27:22 -0000
> > > > @@ -126,7 +126,7 @@ __set_current_state(int state)
> > > > SCHED_LOCK();
> > > > unsleep(p);
> > > > p->p_stat = SONPROC;
> > > > - atomic_clearbits_int(&p->p_flag, P_INSCHED);
> > > > + atomic_clearbits_int(&p->p_flag, P_INSCHED|P_SINTR);
> > > > SCHED_UNLOCK();
> > > > }
> > > >
> >
> > This bit is obviously right. This clears the sleep and needs to clear
> > P_SINTR as well. I once tried to use sleep_finish(INFSLP, 0); here instead
> > of hand rolling the same but it is not quite the same behaviour (since
> > __set_current_state code skips any signal checks).
> >
> > For the debug diffs below I think it is better to instead just KASSERT at
> > the start of sleep_setup that P_SINTR is not set.
> > This would detect such errors but not point to where the error was
> > caused, but if hit adding a hint to find the culprit should be easy
> > doable.
>
> Fine with me. For consistency with the surrounding code I didn't use
> KASSERT(). Without the drm_linux.c fix it triggers as early as
> mountroot. I do not plan to commit the check right now, but rather
> after the dust from h2k25 settles.
>
> Also ok?
OK claudio@ (on both the diff and the plan)
> Index: dev/pci/drm/drm_linux.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.c,v
> diff -u -p -r1.126 drm_linux.c
> --- dev/pci/drm/drm_linux.c 13 Jun 2025 07:01:37 -0000 1.126
> +++ dev/pci/drm/drm_linux.c 16 Nov 2025 21:43:24 -0000
> @@ -126,7 +126,7 @@ __set_current_state(int state)
> SCHED_LOCK();
> unsleep(p);
> p->p_stat = SONPROC;
> - atomic_clearbits_int(&p->p_flag, P_INSCHED);
> + atomic_clearbits_int(&p->p_flag, P_INSCHED|P_SINTR);
> SCHED_UNLOCK();
> }
>
> Index: kern/kern_synch.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_synch.c,v
> diff -u -p -r1.232 kern_synch.c
> --- kern/kern_synch.c 18 Aug 2025 04:15:35 -0000 1.232
> +++ kern/kern_synch.c 16 Nov 2025 21:25:23 -0000
> @@ -288,6 +288,8 @@ sleep_setup(const volatile void *ident,
> #ifdef DIAGNOSTIC
> if (p->p_flag & P_CANTSLEEP)
> panic("sleep: %s failed insomnia", p->p_p->ps_comm);
> + if (p->p_flag & P_SINTR)
> + panic("sleep: stale P_SINTR");
> if (ident == NULL)
> panic("sleep: no ident");
> if (p->p_stat != SONPROC)
>
>
> --
> jca
--
:wq Claudio
