> On Aug 4, 2020, at 5:24 PM, Samuel Thibault <samuel.thiba...@gnu.org> wrote:
>
> 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.
Yes the separated patch does not contain it.
>> + __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.
>> -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.
Yes I tested. S I figured out in the other email, the dsp_port may become dead
while we are in the delivery loop. So one way to solve it is to mark the
user_intr_t with a dead_port free-able, and free after the loop. Another is to
simply combine the delete loop and the delivery loop together: if the port is
dead free, otherwise deliver. Then there would be no hairy racing conditions.
>> 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?
You answered in your next email. Yes. The irq handler does not touch the queue.
>> + 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, which I believe wouldn’t happen, so I changed it to alert. Then I
removed it in the separated patch.
>> + 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. So we
remove all consecutive dead ports in the queue. Again, I propose to combine the
dead port loop with the delivery loop, it would be more readable.
>> {
>> + 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".
It is gone in the separated patch.
>> 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. If we do it in two places, we might have a
race condition?
Junling
