Re: gnumach: [PATCH] - irq as a mach device

[Samuel Thibault]

Hello,

Thanks for reworking this!  With a couple RPC interface changes we can
probably commit it.

Damien Zammit, le sam. 04 juil. 2020 15:31:20 +1000, a ecrit:
> +      /* For now netdde calls device_intr_enable once after registration. 
> Assume
> +       * it does so for now. When we move to IRQ acknowledgment convention 
> we will
> +       * change this. */
> +      irq_disable (irqtab.irq[e->id]);
[...]
> -  /* For now netdde calls device_intr_enable once after registration. Assume
> +  /* XXX For now netdde calls device_intr_enable once after registration. 
> Assume
>     * it does so for now. When we move to IRQ acknowledgment convention we 
> will
>     * change this. */
> -  new->unacked_interrupts = 1;
> +  new->n_unacked = 1;
[...]
>  /*
> - *   enable/disable the specified line.
> + *     enable the specified irq id
> -/* XXX: Naming a function taht can disable something "xxx_enable" is 
> confusing. */
> -/* Is the disable part actually used at all? AIUI, the kernel IRQ handler
> -should always disable the line; and the userspace driver only has to
> -reenable it, after acknowledging and handling the interrupt...
> +/* AIUI, the kernel IRQ handler should always disable the line
> + * and the userspace driver only has to reenable it, 
> + * after acknowledging and handling the interrupt...
>   */
> -*/

Please take the RPC interface change opportunity to remove that
part: I don't see a reason for doing this. It's cleaner to really turn
device_intr_enable into an interrupt acknowledgment rather than keeping
a notion of "intr enabled/disabled" which is meaningless for shared
IRQs.

I.e. remove this disable and initial unacked interrupt, and remove the
initial intr_enable call from netdde / rump.

Then you can completely replace the documentation above with
something like:

“
Acknowledge the specified interrupt notification.

When an IRQ happens and an intr notification is thus sent, the IRQ line
is kept disabled until the notification is acknowledged with this RPC
”

> -ds_device_intr_enable(ipc_port_t master_port, int line, char status)
> +ds_device_intr_enable (device_t dev, int id)

Please rename to ds_device_intr_ack, we don't want to keep an underlying
intr disable/enable semantic (notably since you are dropping the status
parameter).

Also, as mentioned in the previous user_intr_enable() prototype, this
RPC should be taking the delivery port rather than the irq id, so we use
search_intr instead of search_intr_line.

> +/* This function can only be used in the interrupt handler. */
> +static void
> +queue_intr (struct irqdev *dev, int id, user_intr_t *e)

> +static boolean_t
> +deliver_intr (int id, ipc_port_t dst_port)
> +{

Please avoid moving functions around in the file, that makes reviewing
way more difficult, I won't diff the pieces by hand to check what might
have changed while moving the function, so I can't review it. If needed,
add a function declaration.

We can separately commit changes that only moves code around, but not
mix moving code and changing it.

> -           assert (!queue_empty (&intr_queue));
> -           queue_remove (&intr_queue, e, struct intr_entry *, chain);
> -           if (e->unacked_interrupts)
> -             printf("irq handler %d: still %d unacked irqs in entry %p\n", 
> e->line, e->unacked_interrupts, e);
> -           while (e->unacked_interrupts)
> +           assert (!queue_empty (&main_intr_queue));
> +           queue_remove (&main_intr_queue, e, user_intr_t *, chain);
> +           if (e->n_unacked)
> +             printf("irq handler [%d]: still %d unacked irqs in entry %p\n", 
> e->id, e->n_unacked, e);
> +           while (e->n_unacked)

See how renaming stuff brought such lengthy pieces which I have to
review. Better also put them in a separate patch which only performs the
renaming, so I don't have to skim over all of this.

The smaller the patch is, the less time it takes to review it, and thus
the easier it is for me to find time to review it.

> +void
> +irq_enable (irq_t irq)
> +{
> +  unmask_irq (irq);
> +}

This isn't enough: we need to disable irqs while doing this! Just like
__dis/enable_irq does in the Linux source.

Also, I don't think this will play nice with Linux drivers' own
disabling counting management to properly share IRQs between
Linux drivers and userland drivers. Why not just moving the
__dis/enable_irq() functions from the Linux parT?

> +  {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15},

Yes, an indirection will probably be welcome.

Samuel