[prev in list] [next in list] [prev in thread] [next in thread] 

List:       hurd-bug
Subject:    Re: [PATCH] Move user_intr handling to mach side
From:       Samuel Thibault <samuel.thibault () gnu ! org>
Date:       2020-08-05 13:43:41
Message-ID: 20200805134341.wqjuwxmadn7x3wbz () function
[Download RAW message or body]

Junling Ma, le mar. 04 août 2020 18:21:21 -0700, a ecrit:
> >> +    __disable_irq (irqtab.irq[id]);
> > 
> > I'd say add a struct irqdev * in the user_intr_t, so you don't have to
> > hardcode irqtab. Also, no need to keep it inside splhigh.
> 
> Sure we can keep a pointer inside user_intr_t.

> But IO don't think we need to protect __disable_irq, as __disable_irq itself does a splhigh/splx.

That's what I wrote above, yes.

> >> +  PROTECT(dev->lock, queue_enter (&dev->intr_queue, e, user_intr_t *, chain));
> >> +  printf("irq handler [%d]: new delivery port %p entry %p\n", id, dst_port, e);
> >> +  return e;
> > 
> > 
> >> @@ -178,8 +156,11 @@ intr_thread (void)
> >>       simple_lock(&irqtab.lock);
> >>       queue_iterate (&main_intr_queue, e, user_intr_t *, chain)
> >> 	{
> >> -	  if ((!e->dst_port || e->dst_port->ip_references == 1) && e->n_unacked)
> >> +	  /* e cannot be NULL, as we check against it when installing the handler */
> >> +	  assert(e != NULL);
> > 
> > Errr, it can't be null because it's a member of the queue.
> 
> It is gone in the separated patch. In the original code there was a test against NULL,

? No there wasn't.

> >> +	  while (e->dst_port->ip_references == 1)
> > 
> > while?? belowe you ipc_port_release(), so the reference will necesarily
> > fall down to 0 anyway.
> 
> The pointer e moves to the next item in the queue, see e = next, below.

Ah ok. That would definitely deserves a comment.

> >> diff --git a/linux/dev/arch/i386/kernel/irq.c b/linux/dev/arch/i386/kernel/irq.c
> >> index 1e911f33..5879165f 100644
> >> --- a/linux/dev/arch/i386/kernel/irq.c
> >> +++ b/linux/dev/arch/i386/kernel/irq.c
> >> 
> >> -  if (!irq_action[irq])
> >> -    {
> >> -      /* No handler any more, disable interrupt */
> >> -      mask_irq (irq);
> >> -      ivect[irq] = intnull;
> >> -      iunit[irq] = irq;
> >> -    }
> >> -
> > 
> > I'd say we want to keep it?
> 
> This is already done in free_irq.

Ah, right, ok.

> If we do it in two places, we might have a race condition?

? No, since the free_irq is under cli.

Samuel

[prev in list] [next in list] [prev in thread] [next in thread]