bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Move user_intr handling to mach side


From: Samuel Thibault
Subject: Re: [PATCH] Move user_intr handling to mach side
Date: Wed, 5 Aug 2020 02:24:20 +0200
User-agent: NeoMutt/20170609 (1.8.3)

Re,

Junling Ma, le dim. 02 août 2020 21:07:00 -0700, a ecrit:
> +struct pt_regs;
> +extern int request_irq (unsigned int irq, void (*handler) (int, void *, 
> struct pt_regs *),
> +          unsigned long flags, const char *device, void *dev_id);
> +extern void free_irq (unsigned int irq, void *dev_id);

Please put them in intr.h, with a comment saying that this for now plugs
into the Linux code, but will have to plug into the hardware-specific
code when we get rid of the Linux code.

> +#define SPLHIGH(critical_section) \
> +{\
> +  spl_t s = splhigh();\
> +  critical_section;\
> +  splx(s);\
> +}

Mmm.

> -  spl_t s = splhigh ();
> -  e->n_unacked++;
> -  e->interrupts++;
> -  dev->tot_num_intr++;
> -  splx (s);
> -
> +  SPLHIGH({
> +    user_intr_t *e = dev_id;
> +    /* Until userland has handled the IRQ in the driver, we have to keep it
> +     * disabled. Level-triggered interrupts would keep raising otherwise. */
> +    __disable_irq (irqtab.irq[id]);
> +    e->n_unacked++;
> +    e->interrupts++;
> +    irqtab.tot_num_intr++;
> +  });

I don't think this makes it really more readable, actually. And it won't
protect from e.g. goto, so it'd bring a fake sense of security.

> +    __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.

> -int
> -deliver_user_intr (struct irqdev *dev, int id, user_intr_t *e)
> -{
> -  /* The reference of the port was increased
> -   * when the port was installed.
> -   * If the reference is 1, it means the port should
> -   * have been destroyed and I destroy it now. */
> -  if (e->dst_port
> -      && e->dst_port->ip_references == 1)
> -    {
> -      printf ("irq handler [%d]: release a dead delivery port %p entry 
> %p\n", id, e->dst_port, e);
> -      ipc_port_release (e->dst_port);
> -      e->dst_port = MACH_PORT_NULL;
> -      thread_wakeup ((event_t) &intr_thread);
> -      return 0;

Did you actually test it? I don't remember exactly why we wanted to
release the port at this point. One of the reasons why we prefer patches
to be split is to be able to test each change separately, to be able to
determine which one breaks something.

>  user_intr_t *
>  insert_intr_entry (struct irqdev *dev, int id, ipc_port_t dst_port)
>  {
[...]
> +  /* we do not need to disable irq here, because the irq handler does not 
> touch the queue now. */
??? How so?

> +  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.

> +       while (e->dst_port->ip_references == 1)

while?? belowe you ipc_port_release(), so the reference will necesarily
fall down to 0 anyway.

>           {
> +           next = (user_intr_t *)queue_next(&e->chain);

See my comment in my other mail. While doing free_irq() and kfree() you
have no idea what could happen. Don't bother to try to optimize, it's a
rare case, just restart iterating from the beginning of the queue.

>             printf ("irq handler [%d]: release dead delivery %d unacked irqs 
> port %p entry %p\n", e->id, e->n_unacked, e->dst_port, e);
>             /* The reference of the port was increased
>              * when the port was installed.
> @@ -193,25 +174,22 @@ intr_thread (void)
>               __enable_irq (irqtab.irq[e->id]);
>               e->n_unacked--;
>             }
> +           ipc_port_release(e->dst_port);
> +           assert (!queue_empty (&main_intr_queue));
> +           queue_remove (&main_intr_queue, e, user_intr_t *, chain);
> +           printf("irq handler [%d]: removed entry %p\n", e->id, e);
> +           /* if e is the last handler registered for irq ID, then remove 
> the linux irq handler */

? We register a handler for each user of an irq, so we just always free
the irq, we don't need to care being "last handler".

> 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?

Samuel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]