Re: [PATCH 3/3] intr: Protect internals with a lock

[Sergey Bugaev]

On Tue, Dec 10, 2024 at 3:19 PM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Sergey Bugaev, le mar. 10 déc. 2024 14:57:05 +0300, a ecrit:
> > @@ -103,11 +105,15 @@ queue_intr (struct irqdev *dev, int id, user_intr_t 
> > *e)
> >     * disabled. Level-triggered interrupts would keep raising otherwise. */
> >    __disable_irq (dev->irq[id]);
> >
> > -  spl_t s = splhigh ();
> > +#ifdef LINUX_DEV
> > +  spl_t s = simple_lock_irq(&intr_lock);
> > +#endif
>
> Why only ifdef LINUX_DEV? AIUI We do want to always lock.

As said, I'm not sure if that's the correct thing. But the reasoning
here has been that deliver_user_intr (which is what calls queue_intr)
is called from two different contexts: without LINUX_DEV, from
user_irq_handler in this same file, which does its own locking (and so
would likely deadlock if we were to take the lock in queue_intr), and
from linux_intr with LINUX_DEV, which doesn't seem to do its own
locking (and so is probably quite broken on SMP, if we care about
that).

Sergey