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
