Re: [PATCH 1/6] Make linux drivers optional

[Samuel Thibault]

Damien Zammit, le dim. 28 mars 2021 17:03:15 +1100, a ecrit:
> @@ -26,6 +27,18 @@
>  queue_head_t main_intr_queue;
>  static boolean_t deliver_intr (int id, ipc_port_t dst_port);
>  
> +#ifndef LINUX_DEV
> +#define SA_SHIRQ 0x04000000
> +#define EBUSY 16
> +
> +struct intr_list {
> +  user_intr_t *user_intr;
> +  unsigned long flags;
> +  struct intr_list *next;
> +};
> +static struct intr_list *user_intr_handlers;

Can't this be an NINTR array? Otherwise it's painful to go through the
list to find the entry for the irq line. Or will we potentially have
sparse interrupt line ids? For now install_user_intr_handler starts with
an assert notably :)

> +user_irq_handler (int id)
> +{
> +  struct intr_list *handler_list = user_intr_handlers;
> +  user_intr_t *e;
> +  spl_t s;
> +
> +  s = splhigh();
> +
> +  while (handler_list)
> +    {
> +      e = handler_list->user_intr;
> +      if (e->id == id)
> +        deliver_user_intr(&irqtab, id, e);

As I mentioned, you have to also check the value returned by
deliver_user_intr, to remove the entry if it returns 0, see how
linux/dev/arch/i386/kernel/irq.c handles it.

> +install_user_intr_handler (struct irqdev *dev, int id, unsigned long flags,
> +                       user_intr_t *user_intr)
> +{
> +  unsigned int irq = dev->irq[id];
> +  struct intr_list *old = user_intr_handlers;
> +  struct intr_list *new;
> +  spl_t s;
> +
> +  assert (irq < NINTR);
> +
> +  while (old)

Rather use a for (old = user_intr_handlers; old; old = old->next);,
that's more canonical and thus more readable.


> +  if (old)
> +    {
> +      mach_print("Duplicate handler\n");
> +      new = (struct intr_list *)kalloc (sizeof (struct intr_list));
> +      new->user_intr = user_intr;
> +      new->flags = flags;
> +
> +      /* Duplicated, so test if sharing allowed */
> +      if (!(old->flags & new->flags & SA_SHIRQ))
> +        {

Do this check before allocating, otherwise you leak memory.

> +          mach_print ("EBUSY! (no sharing)\n");
> +          return EBUSY;

I'd say rather return D_ALREADY_OPEN

> +        }
> +      /* Fine, insert new handler */
> +      old->next = new;

? that's not enough, new->next needs to be set to old->next, otherwise
you lose the end of the list.

> +    }
> +  else
> +    {
> +      /* First entry */
> +      mach_print ("First handler\n");
> +      old = (struct intr_list *)kalloc (sizeof (struct intr_list));
> +      old->user_intr = user_intr;
> +      old->flags = flags;
> +      old->next = NULL;
> +      user_intr_handlers = old;

This doesn't need to be separate code. You can simply use a
struct intr_list **head; variable that you set either to &old->next or
to &user_intr_handlers. That'll be much simpler.

> +    }
> +
> +  s = splhigh();
> +
> +  ivect[irq] = user_irq_handler;

I believe we should also check that it was previously either already
user_irq_handler or intnull. Otherwise we might be letting userspace
replace hardclock/kdintr/fpintr, I don't think we want to blindly let it
do this (especially hardclock...). Ideally we'd be able to handle both,
but I don't think we need to implement that anyway, so simply check for
it at the beginning of the function and return D_ALREADY_OPEN otherwise.

Samuel