[PATCH v2] irqdev: make deliver_user_intr return void

[Junling Ma]

This sets up the stage for making deliver_user_intr a linux irq handler, which does not have a
return value. So we make deliver_user_intr return void, and move the check for dead notification
port into intr_thread. Now deliver_user_intr does not touch main_intr_queue, and only the server
thread and intr_thread manupulate the queue, adn the manipulations have been protected by a lock.

---
device/intr.c | 47 ++++++++++++--------------------
device/intr.h | 2 +-
linux/dev/arch/i386/kernel/irq.c | 14 ++--------
3 files changed, 21 insertions(+), 42 deletions(-)

diff --git a/device/intr.c b/device/intr.c
index 705dc1c6..c98e5c39 100644
--- a/device/intr.c
+++ b/device/intr.c
@@ -27,6 +27,8 @@ extern struct irqdev irqtab;
#define main_intr_queue irqtab.intr_queue
static boolean_t deliver_intr (int id, ipc_port_t dst_port);

+extern void free_irq (unsigned int irq, void *dev_id);
+
#define PROTECT(lock, critical_section) \
{\
simple_lock(&lock);\
@@ -82,9 +84,8 @@ irq_acknowledge (ipc_port_t receive_port)
return D_SUCCESS;
}

-/* This function can only be used in the interrupt handler. */
-static void
-queue_intr (struct irqdev *dev, int id, user_intr_t *e)
+void
+deliver_user_intr (struct irqdev *dev, int id, user_intr_t *e)
{
/* Until userland has handled the IRQ in the driver, we have to keep it
* disabled. Level-triggered interrupts would keep raising otherwise. */
@@ -99,29 +100,6 @@ queue_intr (struct irqdev *dev, int id, user_intr_t *e)
thread_wakeup ((event_t) &intr_thread);
}

-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;
- }
- else
- {
- queue_intr (dev, id, e);
- return 1;
- }
-}
-
/* insert an interrupt entry in the queue.
* This entry exists in the queue until
* the corresponding interrupt port is removed.*/
@@ -163,7 +141,7 @@ out:
void
intr_thread (void)
{
- user_intr_t *e;
+ user_intr_t *e, *next;
int id;
ipc_port_t dst_port;

@@ -178,7 +156,7 @@ 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)
+ while (e->dst_port->ip_references == 1)
{
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
@@ -188,11 +166,22 @@ intr_thread (void)
* handling can trigger, and we will cleanup later after the Linux
* handler is cleared. */
/* TODO: rather immediately remove from Linux handler */
+ next = (user_intr_t *)queue_next(&e->chain);
+ id = irqtab.irq[e->id];
while (e->n_unacked)
{
- __enable_irq (irqtab.irq[e->id]);
+ __enable_irq (id);
e->n_unacked--;
}
+ ipc_port_release(e->dst_port);
+ queue_remove (&main_intr_queue, e, user_intr_t *, chain);
+ printf("irq handler [%d]: removed entry %p\n", id, e);
+ /* if e is the last handler registered for irq ID, then remove the linux irq handler */
+ free_irq(id, e);
+ if (e->interrupts != 0)
+ irqtab.tot_num_intr -= e->interrupts;
+ kfree ((vm_offset_t) e, sizeof (*e));
+ e = next;
}
}

diff --git a/device/intr.h b/device/intr.h
index 54ddb331..55fc60dc 100644
--- a/device/intr.h
+++ b/device/intr.h
@@ -51,7 +51,7 @@ struct irqdev {
};

extern int install_user_intr_handler (struct irqdev *dev, int id, unsigned long flags, user_intr_t *e);
-extern int deliver_user_intr (struct irqdev *dev, int id, user_intr_t *e);
+extern void deliver_user_intr (struct irqdev *dev, int id, user_intr_t *e);
extern user_intr_t *insert_intr_entry (struct irqdev *dev, int id, ipc_port_t receive_port);

void intr_thread (void);
diff --git a/linux/dev/arch/i386/kernel/irq.c b/linux/dev/arch/i386/kernel/irq.c
index 1e911f33..a01f7ab6 100644
--- a/linux/dev/arch/i386/kernel/irq.c
+++ b/linux/dev/arch/i386/kernel/irq.c
@@ -98,7 +98,6 @@ linux_intr (int irq)
{
struct pt_regs regs;
struct linux_action *action = *(irq_action + irq);
- struct linux_action **prev = &irq_action[irq];
unsigned long flags;

kstat.interrupts[irq]++;
@@ -113,18 +112,9 @@ linux_intr (int irq)
// TODO I might need to check whether the interrupt belongs to
// the current device. But I don't do it for now.
if (action->user_intr)
- {
- if (!deliver_user_intr(&irqtab, irq, action->user_intr))
- {
- *prev = action->next;
- linux_kfree(action);
- action = *prev;
- continue;
- }
- }
+ deliver_user_intr(&irqtab, irq, action->user_intr);
else if (action->handler)
action->handler (irq, action->dev_id, &regs);
- prev = &action->next;
action = action->next;
}

@@ -255,7 +245,7 @@ install_user_intr_handler (struct irqdev *dev, int id, unsigned long flags,

action->handler = NULL;
action->next = NULL;
- action->dev_id = NULL;
+ action->dev_id = user_intr;
action->flags = SA_SHIRQ;
action->user_intr = user_intr;

--
2.28.0.rc1

[Samuel Thibault]

Ah, I actually reviewed the all-in-one patch. Put please proceed with
the separate patches, that'll be much more testable, and all my comments
probably apply just the same.

Samuel