Re: [PATCH v7 1/2 hurd] libirqhelp: Add library

[Samuel Thibault]

Damien Zammit, le sam. 02 mars 2024 10:31:50 +0000, a ecrit:
> +static error_t
> +get_acpi(void)
> +{
> +  error_t err = 0;
> +  mach_port_t tryacpi, device_master;
> +
> +  acpidev = MACH_PORT_NULL;

This looks odd. If we had a previous port in acpidev, we want to
deallocate it, not forget it.

> +  err = get_privileged_ports (0, &device_master);
> +  if (!err)
> +    {
> +      err = device_open (device_master, D_READ, "acpi", &tryacpi);
> +      mach_port_deallocate (mach_task_self (), device_master);
> +      if (!err)
> +        {
> +       acpidev = tryacpi;
> +          return 0;
> +        }
> +    }
> +
> +  tryacpi = file_name_lookup (_SERVERS_ACPI, O_RDONLY, 0);
> +  if (tryacpi == MACH_PORT_NULL)
> +    return ENODEV;
> +
> +  acpidev = tryacpi;
> +  return 0;
> +}
> +
> +static error_t
> +get_irq(void)
> +{
> +  error_t err = 0;
> +  mach_port_t tryirq, device_master;
> +
> +  irqdev = MACH_PORT_NULL;

Same here.

> +  err = get_privileged_ports (0, &device_master);
> +  if (err)
> +    return err;
> +
> +  err = device_open (device_master, D_READ|D_WRITE, "irq", &tryirq);
> +  mach_port_deallocate (mach_task_self (), device_master);
> +  if (err)
> +    return err;
> +
> +  irqdev = tryirq;
> +  return err;
> +}
> +
> +error_t
> +irqhelp_init(void)
> +{
> +  static bool inited = false;
> +  error_t err;
> +
> +  if (inited)
> +    {
> +      log_error("already inited\n");
> +      return 0;

But we already have a guard against several initializations. So I don't
see the point of setting the ports to MACH_PORT_NULL? Or else you meant
to statically initalize them to MACH_PORT_NULL? (which will be much more
efficient).

> +    }
> +
> +  err = get_irq();
> +  if (err)
> +    {
> +      log_error("cannot grab irq device\n");
> +      return err;
> +    }
> +
> +  err = get_acpi();
> +  if (err)
> +    {
> +      log_error("cannot grab acpi device\n");
> +      return err;
> +    }
> +
> +  inited = true;
> +  return 0;
> +}
> +
> +void *
> +irqhelp_server_loop(void *arg)
> +{
> +  struct irq *irq = (struct irq *)arg;
> +  mach_port_t master_host;
> +  mach_port_t pset, psetcntl;
> +  error_t err;
> +
> +  if (!irq)
> +    {
> +      log_error("cannot start this irq thread\n");
> +      return NULL;
> +    }
> +
> +  err = get_privileged_ports (&master_host, 0);
> +  if (err)
> +    {
> +      log_error("cannot get master_host port\n");
> +      return NULL;
> +    }
> +
> +  err = thread_get_assignment (mach_thread_self (), &pset);
> +  if (err)
> +    goto fail;
> +
> +  err = host_processor_set_priv (master_host, pset, &psetcntl);
> +  if (err)
> +    goto fail;
> +
> +  thread_max_priority (mach_thread_self (), psetcntl, 0);
> +  err = thread_priority (mach_thread_self (), IRQ_THREAD_PRIORITY, 0);
> +  if (err)
> +    goto fail;
> +
> +  mach_port_deallocate (mach_task_self (), master_host);
> +
> +  int interrupt_demuxer (mach_msg_header_t *inp,
> +                      mach_msg_header_t *outp)
> +  {
> +    static bool printed0 = false;
> +    static bool printed1 = false;

Better put them in their respective blocks.

> +    device_intr_notification_t *n = (device_intr_notification_t *) inp;
> +
> +    ((mig_reply_header_t *) outp)->RetCode = MIG_NO_REPLY;
> +    if (n->intr_header.msgh_id != DEVICE_INTR_NOTIFY)
> +      {
> +     if (!printed0)
> +       {
> +         log_error("msg received is not an interrupt\n");
> +            printed0 = true;
> +       }
> +     return 0;
> +      }
> +
> +    /* FIXME: id <-> gsi now has an indirection, assuming 1:1 */
> +    if (n->id != irq->gsi)
> +      {
> +     if (!printed1)
> +       {
> +         log_error("interrupt expected on irq %d arrived on irq %d\n", 
> irq->gsi, n->id);
> +         printed1 = true;
> +       }
> +     return 0;
> +      }
> +
> +    /* wait if irq disabled */
> +    pthread_mutex_lock (&irq->irqlock);
> +    while (!irq->enabled)
> +      pthread_cond_wait (&irq->irqcond, &irq->irqlock);
> +    pthread_mutex_unlock (&irq->irqlock);
> + 
> +    /* call handler */
> +    irq->handler(irq->context);

You probably want to add an if (!irq->shutdown) before this? Otherwise
after the application called irqhelp_remove_interrupt_handler, the
handler will still be called once.

> +    /* ACK interrupt */
> +    device_intr_ack (irqdev, irq->port, MACH_MSG_TYPE_MAKE_SEND);

This might be spurious as well in the irq->shutdown case?

> +    if (irq->shutdown)
> +      mach_port_deallocate (mach_task_self (), irq->port);
> +
> +    return 1;
> +  }
> +
> +  /* Server loop */
> +  mach_msg_server (interrupt_demuxer, 0, irq->port);
> +  goto done;
> +
> +fail:
> +  log_error("failed to register irq %d\n", irq->gsi);
> +
> +done:
> +  pthread_cond_destroy(&irq->irqcond);
> +  pthread_mutex_destroy(&irq->irqlock);
> +  free(irq);
> +  return NULL;
> +}