[PATCH-1] A new irq device interface

[Junling Ma]

Hi all,

When I am playing with the current irq device and user interrupt handling, I see that each device must implement the device_intr_register and device_intr_ack calls. However, they are only meaningful for the irq device. So when writing device translators, this seems a little confusing to me. Starting from this email, I am sending 3 patches that propose a new interface for user space irq handling:
1. A program requests for irq by opening one of the irq devices irq0 - irq15.
2. The program performs a read (of any size), which blocks until an interrupt happens on the requested line. The read returns with 0-byte data.
3. The interrupt is acked by writing to the device (of any sized data).
4. Go to step 2 to wait for the next irq.

PATCH 1 is included in this email: it prepares the stage by
1. Change devicer_user_intr to be a linux interrupt handler.
2. Make user_intr_t represent a specific interrupt line, so that notifications queue on each line.
3. Make struct irqdev store a vector user_intr_t, indexed by interrupt line.
4. The n_unacked counter only increases when successfully delivered an interrupt.

Any comments?

Best,
Junling Ma

---
device/device_init.c | 1 +
device/ds_routines.c | 6 +-
device/intr.c | 375 +++++++++++++++----------------
device/intr.h | 28 ++-
i386/i386/irq.c | 15 +-
linux/dev/arch/i386/kernel/irq.c | 68 +-----
6 files changed, 203 insertions(+), 290 deletions(-)

diff --git a/device/device_init.c b/device/device_init.c
index 794186ee..ee3e6dab 100644
--- a/device/device_init.c
+++ b/device/device_init.c
@@ -60,6 +60,7 @@ device_service_create(void)
net_io_init();
device_pager_init();
chario_init();
+ irq_init();

(void) kernel_thread(kernel_task, io_done_thread, 0);
(void) kernel_thread(kernel_task, net_thread, 0);
diff --git a/device/ds_routines.c b/device/ds_routines.c
index e94e5ca8..91787950 100644
--- a/device/ds_routines.c
+++ b/device/ds_routines.c
@@ -343,13 +343,9 @@ ds_device_intr_register (device_t dev, int id,
if (! name_equal(mdev->dev_ops->d_name, 3, "irq"))
return D_INVALID_OPERATION;

- user_intr_t *e = insert_intr_entry (&irqtab, id, receive_port);
- if (!e)
- return D_NO_MEMORY;
-
// TODO detect when the port get destroyed because the driver crashes and
// restart, to replace it when the same device driver calls it again.
- err = install_user_intr_handler (&irqtab, id, flags, e);
+ err = install_user_intr_handler (id, receive_port);
if (err == D_SUCCESS)
{
/* If the port is installed successfully, increase its reference by 1.
diff --git a/device/intr.c b/device/intr.c
index fbb9f495..682b12d7 100644
--- a/device/intr.c
+++ b/device/intr.c
@@ -20,219 +20,164 @@
#include <machine/spl.h>
#include <machine/irq.h>
#include <ipc/ipc_space.h>
+#include "io_req.h"
+#include "ds_routines.h"

#ifndef MACH_XEN

-queue_head_t main_intr_queue;
-static boolean_t deliver_intr (int id, ipc_port_t dst_port);
+#define SA_SHIRQ 0x04000000
+#define SA_INTERRUPT 0x20000000

-static user_intr_t *
-search_intr (struct irqdev *dev, ipc_port_t dst_port)
-{
- user_intr_t *e;
- queue_iterate (dev->intr_queue, e, user_intr_t *, chain)
- {
- if (e->dst_port == dst_port)
- return e;
- }
- return NULL;
+struct irqdev irqtab;
+static boolean_t deliver_intr (int id, ipc_port_t dst_port);
+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);
+
+#define SPLHIGH(critical_section) \
+{ \
+ spl_t s = splhigh(); \
+ critical_section; \
+ splx(s); \
}

-kern_return_t
-irq_acknowledge (ipc_port_t receive_port)
-{
- user_intr_t *e;
- kern_return_t ret = 0;
-
- spl_t s = splhigh ();
- e = search_intr (&irqtab, receive_port);
-
- if (!e)
- printf("didn't find user intr for interrupt !?\n");
- else
- {
- if (!e->n_unacked)
- ret = D_INVALID_OPERATION;
- else
- e->n_unacked--;
- }
- splx (s);
-
- if (ret)
- return ret;
-
- if (irqtab.irqdev_ack)
- (*(irqtab.irqdev_ack)) (&irqtab, e->id);
-
- __enable_irq (irqtab.irq[e->id]);
-
- return D_SUCCESS;
+#define PROTECT(lock, critical_section) \
+{ \
+ simple_lock(&(lock)); \
+ critical_section; \
+ simple_unlock(&(lock)); \
}

-/* This function can only be used in the interrupt handler. */
-static void
-queue_intr (struct irqdev *dev, int id, user_intr_t *e)
+static user_intr_notification_t
+search_notification (user_intr_t *handler, ipc_port_t dst_port)
{
- /* Until userland has handled the IRQ in the driver, we have to keep it
- * disabled. Level-triggered interrupts would keep raising otherwise. */
- __disable_irq (dev->irq[id]);
-
- spl_t s = splhigh ();
- e->n_unacked++;
- e->interrupts++;
- dev->tot_num_intr++;
- splx (s);
-
- thread_wakeup ((event_t) &intr_thread);
+ user_intr_notification_t n = NULL, p;
+ PROTECT(handler->lock,
+ {
+ queue_iterate (&handler->notification_queue, p, user_intr_notification_t, chain)
+ {
+ if (p->dst_port == dst_port) {
+ n = p;
+ break;
+ }
+ }
+ });
+ return n;
}

-int
-deliver_user_intr (struct irqdev *dev, int id, user_intr_t *e)
+kern_return_t
+irq_acknowledge (ipc_port_t receive_port)
{
- /* 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;
- }
+ user_intr_t *e;
+ unsigned id;
+ for (id = 0; id < NINTR; ++id)
+ {
+ e = irqtab.handler[id];
+ if (e && search_notification (e, receive_port))
+ break;
+ }
+ if (id == NINTR)
+ return D_INVALID_OPERATION;
+
+ kern_return_t ret = D_SUCCESS;
+ PROTECT(e->lock,
+ {
+ if (!e->n_unacked)
+ ret = D_INVALID_OPERATION;
+ else {
+ e->n_unacked--;
+ if (e->n_unacked == 0)
+ __enable_irq(id);
+ }
+ });
+ return ret;
}

-/* insert an interrupt entry in the queue.
- * This entry exists in the queue until
- * the corresponding interrupt port is removed.*/
-user_intr_t *
-insert_intr_entry (struct irqdev *dev, int id, ipc_port_t dst_port)
+static void
+deliver_user_intr (int id, void *dev_id, struct pt_regs *regs)
{
- user_intr_t *e, *new, *ret;
- int free = 0;
-
- new = (user_intr_t *) kalloc (sizeof (*new));
- if (new == NULL)
- return NULL;
-
- /* check whether the intr entry has been in the queue. */
- spl_t s = splhigh ();
- e = search_intr (dev, dst_port);
- if (e)
- {
- printf ("the interrupt entry for irq[%d] and port %p has already been inserted\n", id, dst_port);
- free = 1;
- ret = NULL;
- goto out;
- }
- printf("irq handler [%d]: new delivery port %p entry %p\n", id, dst_port, new);
- ret = new;
- new->id = id;
- new->dst_port = dst_port;
- new->interrupts = 0;
- new->n_unacked = 0;
-
- queue_enter (dev->intr_queue, new, user_intr_t *, chain);
-out:
- splx (s);
- if (free)
- kfree ((vm_offset_t) new, sizeof (*new));
- return ret;
+ user_intr_t *e;
+ SPLHIGH({
+ e = irqtab.handler[id];
+ if (e && !queue_empty(&e->notification_queue)) {
+ ++e->interrupts;
+ __disable_irq(id);
+ }
+ });
+ if (!e) return;
+ if (queue_empty(&e->notification_queue))
+ {
+ irqtab.handler[id] = NULL;
+ if (e->n_unacked)
+ __enable_irq (id);
+ free_irq(id, &irqtab);
+ e = NULL;
+ }
+ else if (e->interrupts)
+ thread_wakeup ((event_t) &intr_thread);
}

+/* main service thread for firing irqs */
void
intr_thread (void)
{
- user_intr_t *e;
- int id;
- ipc_port_t dst_port;
- queue_init (&main_intr_queue);
-
+ unsigned total_interrupts;
for (;;)
{
- assert_wait ((event_t) &intr_thread, FALSE);
- /* Make sure we wake up from times to times to check for aborted processes */
+ assert_wait ((event_t) &intr_thread, FALSE);
+ /* Make sure we wake up from times to times to check for aborted processes */
thread_set_timeout (hz);
- spl_t s = splhigh ();
-
- /* Check for aborted processes */
- queue_iterate (&main_intr_queue, e, user_intr_t *, chain)
- {
- if ((!e->dst_port || e->dst_port->ip_references == 1) && e->n_unacked)
- {
- 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.
- * If the reference is 1, it means the port should
- * have been destroyed and I clear unacked irqs now, so the Linux
- * handling can trigger, and we will cleanup later after the Linux
- * handler is cleared. */
- /* TODO: rather immediately remove from Linux handler */
- while (e->n_unacked)
- {
- __enable_irq (irqtab.irq[e->id]);
- e->n_unacked--;
- }
- }
- }
-
- /* Now check for interrupts */
- while (irqtab.tot_num_intr)
- {
- int del = 0;
-
- queue_iterate (&main_intr_queue, e, user_intr_t *, chain)
- {
- /* if an entry doesn't have dest port,
- * we should remove it. */
- if (e->dst_port == MACH_PORT_NULL)
- {
- clear_wait (current_thread (), 0, 0);
- del = 1;
- break;
- }
-
- if (e->interrupts)
- {
- clear_wait (current_thread (), 0, 0);
- id = e->id;
- dst_port = e->dst_port;
- e->interrupts--;
- irqtab.tot_num_intr--;
-
- splx (s);
- deliver_intr (id, dst_port);
- s = splhigh ();
- }
- }
-
- /* remove the entry without dest port from the queue and free it. */
- if (del)
- {
- 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)
- {
- __enable_irq (irqtab.irq[e->id]);
- e->n_unacked--;
- }
- printf("irq handler [%d]: removed entry %p\n", e->id, e);
- splx (s);
- kfree ((vm_offset_t) e, sizeof (*e));
- s = splhigh ();
- }
- }
- splx (s);
- thread_block (NULL);
+ thread_block(NULL);
+ do
+ {
+ total_interrupts = 0;
+ for (unsigned id = 0; id < NINTR; ++id)
+ {
+ user_intr_t *e = irqtab.handler[id];
+ if (!e) continue;
+ clear_wait (current_thread (), 0, 0);
+ user_intr_notification_t next;
+ user_intr_notification_t n;
+ /* go through each notification to fire or remove dead ones */
+ PROTECT(e->lock,
+ {
+ queue_iterate (&e->notification_queue, n, user_intr_notification_t, chain)
+ {
+ /* is the notification port dead? */
+ if (n->dst_port->ip_references == 1)
+ {
+ /* dead, move it to the dead queue */
+ next = (user_intr_notification_t)queue_next(&n->chain);
+ queue_remove(&e->notification_queue, n, user_intr_notification_t, chain);
+ ipc_port_release (n->dst_port);
+ kfree((vm_offset_t)n, sizeof(*n));
+ --e->n_unacked;
+ if (e->n_unacked == 0)
+ __enable_irq(id);
+ printf("dead notification port %p released \n", n->dst_port);
+ if (queue_empty(&e->notification_queue))
+ {
+ printf("irq %d has no registered notifications. remove\n", id);
+ kfree((vm_offset_t)e, sizeof(*e));
+ irqtab.handler[id] = NULL;
+ break;
+ }
+ n = next;
+ }
+ else if (e->interrupts)
+ {
+ SPLHIGH(e->interrupts--);
+ total_interrupts += e->interrupts;
+ if (deliver_intr(id, n->dst_port))
+ /* n_unacked is increased when firing. Without firing, there is no ack */
+ e->n_unacked++;
+ }
+ } /* end of queue_iterate */
+ }); /* end of PROTECT */
+ }
+ }
+ while (total_interrupts);
}
}

@@ -280,4 +225,54 @@ deliver_intr (int id, ipc_port_t dst_port)
return TRUE;
}

+int
+install_user_intr_handler (int id, ipc_port_t dst_port)
+{
+ if (id > NINTR || dst_port == NULL)
+ return D_INVALID_OPERATION;
+ user_intr_t *e = irqtab.handler[id];
+ if (e == NULL) {
+ e = (user_intr_t *) kalloc (sizeof (*e));
+ if (e == NULL)
+ return D_NO_MEMORY;
+ queue_init(&e->notification_queue);
+ e->interrupts = 0;
+ e->n_unacked = 0;
+ simple_lock_init(&e->lock);
+ irqtab.handler[id] = e;
+ /* install the new handler */
+ kern_return_t r = request_irq (id, deliver_user_intr, SA_SHIRQ, NULL, &irqtab);
+ if (r) {
+ printf("could not register irq handler: %d(%08x)\n", r, r);
+ irqtab.handler[id] = NULL;
+ kfree((vm_size_t)e, sizeof(*e));
+ return r;
+ }
+ }
+
+ /* check whether the intr entry has been in the queue. */
+ user_intr_notification_t n = search_notification (e, dst_port);
+ if (n)
+ {
+ printf ("the interrupt entry for irq %d and port %p has already been inserted\n", id, dst_port);
+ return D_ALREADY_OPEN;
+ }
+
+ n = (user_intr_notification_t) kalloc (sizeof (*n));
+ if (n == NULL)
+ return D_NO_MEMORY;
+
+ n->dst_port = dst_port;
+ PROTECT(e->lock, queue_enter (&e->notification_queue, n, user_intr_notification_t, chain));
+ printf("irq handler [%d]: new delivery port %p\n", id, dst_port);
+ return D_SUCCESS;
+}
+
+void irq_init()
+{
+ irqtab.name = "irq";
+ for (int i = 0; i < NINTR; ++i)
+ irqtab.handler[i] = NULL;
+}
+
#endif /* MACH_XEN */
diff --git a/device/intr.h b/device/intr.h
index cd3e0bce..90bc6f4c 100644
--- a/device/intr.h
+++ b/device/intr.h
@@ -30,29 +30,27 @@
struct irqdev;
#include <machine/irq.h>

-typedef struct {
+/* a struct to hold notifications */
+struct user_intr_notification {
queue_chain_t chain;
- int interrupts; /* Number of interrupts occurred since last run of intr_thread */
- int n_unacked; /* Number of times irqs were disabled for this */
ipc_port_t dst_port; /* Notification port */
- int id; /* Mapping to machine dependent irq_t array elem */
+};
+typedef struct user_intr_notification * user_intr_notification_t;
+
+typedef struct {
+ queue_head_t notification_queue;
+ unsigned interrupts; /* Number of interrupts occurred since last run of intr_thread */
+ unsigned n_unacked; /* Number of times irqs were disabled for this */
+ decl_simple_lock_data(, lock); /* a lock to protect the queues */
} user_intr_t;

struct irqdev {
char *name;
- void (*irqdev_ack)(struct irqdev *dev, int id);
-
- queue_head_t *intr_queue;
- int tot_num_intr; /* Total number of unprocessed interrupts */
-
- /* Machine dependent */
- irq_t irq[NINTR];
+ user_intr_t *handler[NINTR]; /* irq handlers for device_open */
};

-extern queue_head_t main_intr_queue;
-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 user_intr_t *insert_intr_entry (struct irqdev *dev, int id, ipc_port_t receive_port);
+extern int install_user_intr_handler (int id, ipc_port_t dst_port);
+extern void irq_init();

void intr_thread (void);
kern_return_t irq_acknowledge (ipc_port_t receive_port);
diff --git a/i386/i386/irq.c b/i386/i386/irq.c
index 35681191..c13138a8 100644
--- a/i386/i386/irq.c
+++ b/i386/i386/irq.c
@@ -24,15 +24,7 @@
#include <kern/assert.h>
#include <machine/machspl.h>

-extern queue_head_t main_intr_queue;
-
-static void
-irq_eoi (struct irqdev *dev, int id)
-{
- /* TODO EOI(dev->irq[id]) */
-}
-
-static unsigned int ndisabled_irq[NINTR];
+static unsigned int ndisabled_irq[NINTR] = {0};

void
__disable_irq (irq_t irq_nr)
@@ -60,8 +52,3 @@ __enable_irq (irq_t irq_nr)
splx(s);
}

-struct irqdev irqtab = {
- "irq", irq_eoi, &main_intr_queue, 0,
- {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15},
-};
-
diff --git a/linux/dev/arch/i386/kernel/irq.c b/linux/dev/arch/i386/kernel/irq.c
index 1e911f33..6f433acb 100644
--- a/linux/dev/arch/i386/kernel/irq.c
+++ b/linux/dev/arch/i386/kernel/irq.c
@@ -78,7 +78,6 @@ struct linux_action
void *dev_id;
struct linux_action *next;
unsigned long flags;
- user_intr_t *user_intr;
};

static struct linux_action *irq_action[16] =
@@ -98,7 +97,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]++;
@@ -110,21 +108,8 @@ linux_intr (int irq)

while (action)
{
- // 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;
- }
- }
- else if (action->handler)
- action->handler (irq, action->dev_id, &regs);
- prev = &action->next;
+ if (action->handler)
+ action->handler (irq, action->dev_id, &regs);
action = action->next;
}

@@ -194,7 +179,6 @@ setup_x86_irq (int irq, struct linux_action *new)
/* Can't share interrupts unless both are same type */
if ((old->flags ^ new->flags) & SA_INTERRUPT)
return (-EBUSY);
-
/* add new interrupt at end of irq queue */
do
{
@@ -219,53 +203,6 @@ setup_x86_irq (int irq, struct linux_action *new)
return 0;
}

-int
-install_user_intr_handler (struct irqdev *dev, int id, unsigned long flags,
- user_intr_t *user_intr)
-{
- struct linux_action *action;
- struct linux_action *old;
- int retval;
-
- unsigned int irq = dev->irq[id];
-
- assert (irq < 16);
-
- /* Test whether the irq handler has been set */
- // TODO I need to protect the array when iterating it.
- old = irq_action[irq];
- while (old)
- {
- if (old->user_intr && old->user_intr->dst_port == user_intr->dst_port)
- {
- printk ("The interrupt handler has already been installed on line %d", irq);
- return linux_to_mach_error (-EAGAIN);
- }
- old = old->next;
- }
-
- /*
- * Hmm... Should I use `kalloc()' ?
- * By OKUJI Yoshinori.
- */
- action = (struct linux_action *)
- linux_kmalloc (sizeof (struct linux_action), GFP_KERNEL);
- if (action == NULL)
- return linux_to_mach_error (-ENOMEM);
-
- action->handler = NULL;
- action->next = NULL;
- action->dev_id = NULL;
- action->flags = SA_SHIRQ;
- action->user_intr = user_intr;
-
- retval = setup_x86_irq (irq, action);
- if (retval)
- linux_kfree (action);
-
- return linux_to_mach_error (retval);
-}
-
/*
* Attach a handler to an IRQ.
*/
@@ -294,7 +231,6 @@ request_irq (unsigned int irq, void (*handler) (int, void *, struct pt_regs *),
action->next = NULL;
action->dev_id = dev_id;
action->flags = flags;
- action->user_intr = NULL;

retval = setup_x86_irq (irq, action);
if (retval)
--
2.28.0.rc1

[Junling Ma]

Hi all,

In this patch, the new interface of irq device is implemented. Please see the first patch for a description.

Junling Ma

---
device/ds_routines.c | 2 +-
device/intr.c | 184 +++++++++++++++++++++++++++++++++----------
device/intr.h | 11 ++-
i386/i386at/conf.c | 4 +-
4 files changed, 154 insertions(+), 47 deletions(-)

diff --git a/device/ds_routines.c b/device/ds_routines.c
index 91787950..333b66c9 100644
--- a/device/ds_routines.c
+++ b/device/ds_routines.c
@@ -345,7 +345,7 @@ ds_device_intr_register (device_t dev, int id,

// TODO detect when the port get destroyed because the driver crashes and
// restart, to replace it when the same device driver calls it again.

- err = install_user_intr_handler (id, receive_port);
+ err = install_user_intr_handler (id, receive_port, mdev, FALSE);

if (err == D_SUCCESS)
{
/* If the port is installed successfully, increase its reference by 1.
diff --git a/device/intr.c b/device/intr.c

index 682b12d7..e78f2193 100644
--- a/device/intr.c
+++ b/device/intr.c
@@ -22,6 +22,7 @@
#include <ipc/ipc_space.h>
#include "io_req.h"
#include "ds_routines.h"
+#include "device.server.h"

#ifndef MACH_XEN

@@ -70,16 +71,20 @@ kern_return_t
irq_acknowledge (ipc_port_t receive_port)
{
user_intr_t *e;
+ user_intr_notification_t n = NULL;
unsigned id;

for (id = 0; id < NINTR; ++id)

{
e = irqtab.handler[id];
- if (e && search_notification (e, receive_port))
+ if (e && (n = search_notification (e, receive_port)) != NULL)
break;
}
- if (id == NINTR)
+ if (id == NINTR || n->device_port)
return D_INVALID_OPERATION;
-
+ /* cannot ack twice */
+ if (n->acked)
+ return D_INVALID_OPERATION;
+ n->acked = TRUE;
kern_return_t ret = D_SUCCESS;
PROTECT(e->lock,
{
@@ -105,16 +110,7 @@ deliver_user_intr (int id, void *dev_id, struct pt_regs *regs)
__disable_irq(id);
}
});
- if (!e) return;
- if (queue_empty(&e->notification_queue))
- {
- irqtab.handler[id] = NULL;
- if (e->n_unacked)
- __enable_irq (id);
- free_irq(id, &irqtab);
- e = NULL;
- }
- else if (e->interrupts)
+ if (e && !queue_empty(&e->notification_queue) && e->interrupts)
thread_wakeup ((event_t) &intr_thread);
}

@@ -137,43 +133,65 @@ intr_thread (void)

user_intr_t *e = irqtab.handler[id];

if (!e) continue;

clear_wait (current_thread (), 0, 0);

- user_intr_notification_t next;
- user_intr_notification_t n;
- /* go through each notification to fire or remove dead ones */
+ user_intr_notification_t next, n;
+ /* go through each notification to fire interrupts or remove dead ones */
PROTECT(e->lock,
{
+ boolean_t delivered = FALSE;

queue_iterate (&e->notification_queue, n, user_intr_notification_t, chain)

{
- /* is the notification port dead? */
- if (n->dst_port->ip_references == 1)
+ /* check for dead port */
+ while (n && n->dst_port->ip_references == 1)
{

/* dead, move it to the dead queue */

+ printf("dead port: %p\n", n->dst_port);
next = (user_intr_notification_t)queue_next(&n->chain);

queue_remove(&e->notification_queue, n, user_intr_notification_t, chain);

- ipc_port_release (n->dst_port);
- kfree((vm_offset_t)n, sizeof(*n));
- --e->n_unacked;
- if (e->n_unacked == 0)
- __enable_irq(id);
- printf("dead notification port %p released \n", n->dst_port);
- if (queue_empty(&e->notification_queue))
+ //ipc_port_release (n->dst_port);
+ if (n->request)
+ io_req_free(n->request);
+ /* if waiting for acking, ack it */
+ if (!n->acked && e->n_unacked)
{
- printf("irq %d has no registered notifications. remove\n", id);
- kfree((vm_offset_t)e, sizeof(*e));
- irqtab.handler[id] = NULL;
- break;

+ --e->n_unacked;
+ if (e->n_unacked == 0)
+ __enable_irq(id);
}

+ kfree((vm_offset_t)n, sizeof(*n));
+ ds_device_close(&n->device->dev);
n = next;
}
- else if (e->interrupts)
+ if (!n) break;
+ /* if there is a DEVICE, the notification uses the new interface */
+ /* the notification uses the old interface. is the notification port dead? */
+ if (!e->interrupts)
+ continue;
+ if (!n->device_port && !deliver_intr(id, n->dst_port))
+ continue; /* failed to delivery using the notification port */
+ if (n->device_port)
{
- SPLHIGH(e->interrupts--);
- total_interrupts += e->interrupts;
- if (deliver_intr(id, n->dst_port))
- /* n_unacked is increased when firing. Without firing, there is no ack */
- e->n_unacked++;
+ if (!n->request)
+ continue;
+ ds_read_done(n->request);
+ io_req_free(n->request);
}
+ n->request = NULL;
+ e->n_unacked++;
+ n->acked = FALSE;
+ delivered = TRUE;

} /* end of queue_iterate */

+ if (delivered) /* if successfully delivered */

+ {
+ SPLHIGH(e->interrupts--);
+ total_interrupts += e->interrupts;
+ }

+ if (queue_empty(&e->notification_queue))
+ {

+ printf("there is no registered handler for irq %d. close the device\n", id);
+ free_irq(id, &irqtab);
+ SPLHIGH(irqtab.handler[id] = NULL);
+ kfree((vm_offset_t)e, sizeof(*e));

+ }
}); /* end of PROTECT */
}
}

@@ -226,9 +244,9 @@ deliver_intr (int id, ipc_port_t dst_port)
}

int
-install_user_intr_handler (int id, ipc_port_t dst_port)
+install_user_intr_handler (int id, ipc_port_t dst_port, mach_device_t device, boolean_t device_port)
{
- if (id > NINTR || dst_port == NULL)
+ if (id > NINTR || device == NULL)
return D_INVALID_OPERATION;

user_intr_t *e = irqtab.handler[id];

if (e == NULL) {
@@ -243,7 +261,7 @@ install_user_intr_handler (int id, ipc_port_t dst_port)

/* install the new handler */

kern_return_t r = request_irq (id, deliver_user_intr, SA_SHIRQ, NULL, &irqtab);

if (r) {
- printf("could not register irq handler: %d(%08x)\n", r, r);
+ printf("could not register irq handler for irq %d: %d(%08x)\n", id, r, r);
irqtab.handler[id] = NULL;
kfree((vm_size_t)e, sizeof(*e));
return r;
@@ -253,18 +271,20 @@ install_user_intr_handler (int id, ipc_port_t dst_port)

/* check whether the intr entry has been in the queue. */

user_intr_notification_t n = search_notification (e, dst_port);

if (n)

- {
- printf ("the interrupt entry for irq %d and port %p has already been inserted\n", id, dst_port);

- return D_ALREADY_OPEN;
- }
+ return D_ALREADY_OPEN;

n = (user_intr_notification_t) kalloc (sizeof (*n));

if (n == NULL)
return D_NO_MEMORY;

n->dst_port = dst_port;
+ ipc_port_reference(dst_port);
+ n->device = device;
+ n->device_port = device_port;
+ n->request = NULL;
+ n->acked = TRUE;

PROTECT(e->lock, queue_enter (&e->notification_queue, n, user_intr_notification_t, chain));

- printf("irq handler [%d]: new delivery port %p\n", id, dst_port);
+ printf("notification port %p register for irq %d\n", dst_port, id);
return D_SUCCESS;
}

@@ -275,4 +295,82 @@ void irq_init()
irqtab.handler[i] = NULL;
}

+extern int irqopen(dev_t dev, int flag, io_req_t ior)
+{
+ int id = minor(dev);
+ /* for now, we need to handle opening "irq", which is the same as irq0
+ this is for backward compatibility. */
+ if (id == 0)
+ return D_SUCCESS;
+ if (id > NINTR)
+ return D_NO_SUCH_DEVICE;

+ user_intr_t *e = irqtab.handler[id];

+ if (e)
+ return D_SUCCESS;
+ ior->io_error = D_SUCCESS;
+ return install_user_intr_handler (id, ior->io_reply_port, ior->io_device, TRUE);
+}
+
+void irqclose(dev_t dev, int flags)
+{
+ /* device closes are handled by port death detection */
+}
+
+extern int irqread(dev_t dev, io_req_t ior)
+{
+ irq_t id = minor(dev);
+ if (id > NINTR)
+ return D_NO_SUCH_DEVICE;

+ user_intr_t *e = irqtab.handler[id];

+ user_intr_notification_t n = NULL;
+ if (e)
+ n = search_notification(e, ior->io_reply_port);
+ if (!n)
+ {
+ int err = install_user_intr_handler(id, ior->io_reply_port, ior->io_device, TRUE);
+ if (err)
+ return err;

+ e = irqtab.handler[id];

+ n = search_notification(e, ior->io_reply_port);
+ }
+ /* already requested */
+ if (n->request || !n->acked)
+ return D_INVALID_OPERATION;
+ n->request = ior;
+ ior->io_error = D_SUCCESS;
+ ior->io_residual = ior->io_count;
+ return D_IO_QUEUED;
+}
+
+extern int irqwrite(dev_t dev, io_req_t ior)
+{
+ irq_t id = minor(dev);
+ if (id > NINTR)
+ return D_NO_SUCH_DEVICE;

+ user_intr_t *e = irqtab.handler[id];
+ if (e == NULL)

+ return D_DEVICE_DOWN;

+ if (e->n_unacked == 0)

+ return D_INVALID_OPERATION;
+ user_intr_notification_t n = search_notification(e, ior->io_reply_port);
+ if (!n || n->request || n->acked)
+ return D_INVALID_OPERATION;
+ n->acked = TRUE;
+ PROTECT(e->lock,
+ {

+ e->n_unacked--;
+ if (e->n_unacked == 0)
+ __enable_irq(id);

+ });
+ boolean_t wait = FALSE;
+ int rc = device_write_get(ior, &wait);
+ if (rc != KERN_SUCCESS)
+ return rc;
+ if (wait)
+ return D_INVALID_OPERATION;
+ ior->io_residual = ior->io_count;
+ ior->io_error = D_SUCCESS;

+ return D_SUCCESS;
+}
+

#endif /* MACH_XEN */
diff --git a/device/intr.h b/device/intr.h

index 90bc6f4c..88b40516 100644
--- a/device/intr.h
+++ b/device/intr.h
@@ -33,7 +33,11 @@ struct irqdev;

/* a struct to hold notifications */

struct user_intr_notification {
queue_chain_t chain;
+ io_req_t request;
+ boolean_t acked;
+ mach_device_t device;

ipc_port_t dst_port; /* Notification port */

+ boolean_t device_port; /* whether dst_port is a device port, i.e, using the new interface */
};
typedef struct user_intr_notification * user_intr_notification_t;

@@ -49,12 +53,17 @@ struct irqdev {

user_intr_t *handler[NINTR]; /* irq handlers for device_open */
};

-extern int install_user_intr_handler (int id, ipc_port_t dst_port);
+extern int install_user_intr_handler (int id, ipc_port_t dst_port, mach_device_t device, boolean_t device_port);

extern void irq_init();

void intr_thread (void);
kern_return_t irq_acknowledge (ipc_port_t receive_port);

+extern int irqopen(dev_t dev, int flag, io_req_t ior);
+extern void irqclose(dev_t dev, int flags);
+extern int irqread(dev_t dev, io_req_t ior);
+extern int irqwrite(dev_t dev, io_req_t ior);

+
#endif /* MACH_XEN */

#endif
diff --git a/i386/i386at/conf.c b/i386/i386at/conf.c
index ca5d0dfb..69f12ae2 100644
--- a/i386/i386at/conf.c
+++ b/i386/i386at/conf.c
@@ -152,8 +152,8 @@ struct dev_ops dev_name_list[] =
nodev },
#endif /* MACH_HYP */

- { irqname, nulldev_open, nulldev_close, nulldev_read,
- nulldev_write,nulldev_getstat,nulldev_setstat, nomap,
+ { irqname, irqopen, irqclose, irqread,
+ irqwrite,nulldev_getstat,nulldev_setstat, nomap,
nodev, nulldev, nulldev_portdeath,0,
nodev },

--
2.28.0.rc1

[Junling Ma]

In this patch, the old interface of device_intr_register and device_intr_ack is removed.

Junling Ma

---
Makefrag.am | 2 -
device/ds_routines.c | 62 ----------------------
device/intr.c | 104 ++++---------------------------------
device/intr.h | 4 +-
include/device/device.defs | 19 -------
include/device/notify.defs | 36 -------------
include/device/notify.h | 34 ------------
7 files changed, 10 insertions(+), 251 deletions(-)
delete mode 100644 include/device/notify.defs
delete mode 100644 include/device/notify.h

diff --git a/Makefrag.am b/Makefrag.am
index ea612275..eb9eaa64 100644
--- a/Makefrag.am
+++ b/Makefrag.am
@@ -363,8 +363,6 @@ include_device_HEADERS = \
include/device/device_types.h \
include/device/disk_status.h \
include/device/net_status.h \
- include/device/notify.defs \
- include/device/notify.h \
include/device/tape_status.h \
include/device/tty_status.h

diff --git a/device/ds_routines.c b/device/ds_routines.c
index 333b66c9..3a6921ec 100644
--- a/device/ds_routines.c
+++ b/device/ds_routines.c
@@ -320,68 +320,6 @@ ds_device_map (device_t dev, vm_prot_t prot, vm_offset_t offset,
offset, size, pager, unmap);
}

-/* TODO: missing deregister support */
-io_return_t
-ds_device_intr_register (device_t dev, int id,
- int flags, ipc_port_t receive_port)
-{
-#if defined(MACH_XEN) || defined(__x86_64__)
- return D_INVALID_OPERATION;
-#else /* MACH_XEN || __x86_64__ */
- kern_return_t err;
- mach_device_t mdev = dev->emul_data;
-
- /* Refuse if device is dead or not completely open. */
- if (dev == DEVICE_NULL)
- return D_NO_SUCH_DEVICE;
-
- /* No flag is defined for now */
- if (flags != 0)
- return D_INVALID_OPERATION;
-
- /* Must be called on the irq device only */
- if (! name_equal(mdev->dev_ops->d_name, 3, "irq"))
- return D_INVALID_OPERATION;
-
- // TODO detect when the port get destroyed because the driver crashes and
- // restart, to replace it when the same device driver calls it again.
- err = install_user_intr_handler (id, receive_port, mdev, FALSE);
- if (err == D_SUCCESS)
- {
- /* If the port is installed successfully, increase its reference by 1.
- * Thus, the port won't be destroyed after its task is terminated. */
- ip_reference (receive_port);
- }
- return err;
-#endif /* MACH_XEN || __x86_64__ */
-}
-
-kern_return_t
-ds_device_intr_ack (device_t dev, ipc_port_t receive_port)
-{
-#if defined(MACH_XEN) || defined(__x86_64__)
- return D_INVALID_OPERATION;
-#else /* MACH_XEN || __x86_64__ */
- mach_device_t mdev = dev->emul_data;
- kern_return_t ret;
-
- /* Refuse if device is dead or not completely open. */
- if (dev == DEVICE_NULL)
- return D_NO_SUCH_DEVICE;
-
- /* Must be called on the irq device only */
- if (! name_equal(mdev->dev_ops->d_name, 3, "irq"))
- return D_INVALID_OPERATION;
-
- ret = irq_acknowledge(receive_port);
-
- if (ret == D_SUCCESS)
- ipc_port_release_send(receive_port);
-
- return ret;
-#endif /* MACH_XEN || __x86_64__ */
-}
-
boolean_t
ds_notify (mach_msg_header_t *msg)
{
diff --git a/device/intr.c b/device/intr.c
index e78f2193..509788e1 100644
--- a/device/intr.c
+++ b/device/intr.c
@@ -15,7 +15,6 @@
#include <device/intr.h>
#include <device/device_types.h>
#include <device/device_port.h>
-#include <device/notify.h>
#include <kern/printf.h>
#include <machine/spl.h>
#include <machine/irq.h>
@@ -30,7 +29,6 @@
#define SA_INTERRUPT 0x20000000

struct irqdev irqtab;
-static boolean_t deliver_intr (int id, ipc_port_t dst_port);
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);

@@ -67,38 +65,6 @@ search_notification (user_intr_t *handler, ipc_port_t dst_port)
return n;

}

-kern_return_t
-irq_acknowledge (ipc_port_t receive_port)
-{
- user_intr_t *e;

- user_intr_notification_t n = NULL;
- unsigned id;
- for (id = 0; id < NINTR; ++id)
- {
- e = irqtab.handler[id];
- if (e && (n = search_notification (e, receive_port)) != NULL)
- break;
- }
- if (id == NINTR || n->device_port)
- return D_INVALID_OPERATION;
- /* cannot ack twice */
- if (n->acked)
- return D_INVALID_OPERATION;
- n->acked = TRUE;
- kern_return_t ret = D_SUCCESS;
- PROTECT(e->lock,

- {
- if (!e->n_unacked)
- ret = D_INVALID_OPERATION;
- else {
- e->n_unacked--;

- if (e->n_unacked == 0)
- __enable_irq(id);
- }

- });

- return ret;
-}
-

static void

deliver_user_intr (int id, void *dev_id, struct pt_regs *regs)

{
@@ -141,13 +107,12 @@ intr_thread (void)

queue_iterate (&e->notification_queue, n, user_intr_notification_t, chain)
{

/* check for dead port */

- while (n && n->dst_port->ip_references == 1)
+ while (n && !n->dst_port->ip_references)

{
/* dead, move it to the dead queue */

printf("dead port: %p\n", n->dst_port);
next = (user_intr_notification_t)queue_next(&n->chain);
queue_remove(&e->notification_queue, n, user_intr_notification_t, chain);

- //ipc_port_release (n->dst_port);
if (n->request)
io_req_free(n->request);

/* if waiting for acking, ack it */

@@ -157,8 +122,8 @@ intr_thread (void)

if (e->n_unacked == 0)

__enable_irq(id);
}
- kfree((vm_offset_t)n, sizeof(*n));
ds_device_close(&n->device->dev);
+ kfree((vm_offset_t)n, sizeof(*n));
n = next;
}
if (!n) break;
@@ -166,15 +131,10 @@ intr_thread (void)

/* the notification uses the old interface. is the notification port dead? */

if (!e->interrupts)
continue;
- if (!n->device_port && !deliver_intr(id, n->dst_port))
- continue; /* failed to delivery using the notification port */
- if (n->device_port)
- {
- if (!n->request)
- continue;
- ds_read_done(n->request);
- io_req_free(n->request);
- }

+ if (!n->request)
+ continue;
+ ds_read_done(n->request);
+ io_req_free(n->request);

n->request = NULL;
e->n_unacked++;
n->acked = FALSE;
@@ -199,52 +159,8 @@ intr_thread (void)
}
}

-static boolean_t
-deliver_intr (int id, ipc_port_t dst_port)
-{
- ipc_kmsg_t kmsg;
- device_intr_notification_t *n;
- mach_port_t dest = (mach_port_t) dst_port;
-
- if (dest == MACH_PORT_NULL)
- return FALSE;
-
- kmsg = ikm_alloc(sizeof *n);
- if (kmsg == IKM_NULL)
- return FALSE;
-
- ikm_init(kmsg, sizeof *n);
- n = (device_intr_notification_t *) &kmsg->ikm_header;
-
- mach_msg_header_t *m = &n->intr_header;
- mach_msg_type_t *t = &n->intr_type;
-
- m->msgh_bits = MACH_MSGH_BITS(MACH_MSG_TYPE_PORT_SEND, 0);
- m->msgh_size = sizeof *n;
- m->msgh_seqno = DEVICE_NOTIFY_MSGH_SEQNO;
- m->msgh_local_port = MACH_PORT_NULL;
- m->msgh_remote_port = MACH_PORT_NULL;
- m->msgh_id = DEVICE_INTR_NOTIFY;
-
- t->msgt_name = MACH_MSG_TYPE_INTEGER_32;
- t->msgt_size = 32;
- t->msgt_number = 1;
- t->msgt_inline = TRUE;
- t->msgt_longform = FALSE;
- t->msgt_deallocate = FALSE;
- t->msgt_unused = 0;
-
- n->intr_header.msgh_remote_port = dest;
- n->id = id;
-
- ipc_port_copy_send (dst_port);
- ipc_mqueue_send_always(kmsg);
-
- return TRUE;
-}
-
int
-install_user_intr_handler (int id, ipc_port_t dst_port, mach_device_t device, boolean_t device_port)
+install_user_intr_handler (int id, ipc_port_t dst_port, mach_device_t device)
{

if (id > NINTR || device == NULL)
return D_INVALID_OPERATION;

@@ -278,9 +194,7 @@ install_user_intr_handler (int id, ipc_port_t dst_port, mach_device_t device, bo

return D_NO_MEMORY;

n->dst_port = dst_port;

- ipc_port_reference(dst_port);
n->device = device;
- n->device_port = device_port;
n->request = NULL;

n->acked = TRUE;
PROTECT(e->lock, queue_enter (&e->notification_queue, n, user_intr_notification_t, chain));

@@ -308,7 +222,7 @@ extern int irqopen(dev_t dev, int flag, io_req_t ior)
if (e)
return D_SUCCESS;
ior->io_error = D_SUCCESS;
- return install_user_intr_handler (id, ior->io_reply_port, ior->io_device, TRUE);
+ return install_user_intr_handler (id, ior->io_reply_port, ior->io_device);

}

void irqclose(dev_t dev, int flags)

@@ -327,7 +241,7 @@ extern int irqread(dev_t dev, io_req_t ior)

n = search_notification(e, ior->io_reply_port);

if (!n)
{
- int err = install_user_intr_handler(id, ior->io_reply_port, ior->io_device, TRUE);
+ int err = install_user_intr_handler(id, ior->io_reply_port, ior->io_device);
if (err)
return err;
e = irqtab.handler[id];
diff --git a/device/intr.h b/device/intr.h
index 88b40516..07863026 100644
--- a/device/intr.h
+++ b/device/intr.h
@@ -37,7 +37,6 @@ struct user_intr_notification {
boolean_t acked;

mach_device_t device;
ipc_port_t dst_port; /* Notification port */

- boolean_t device_port; /* whether dst_port is a device port, i.e, using the new interface */

};
typedef struct user_intr_notification * user_intr_notification_t;

@@ -53,11 +52,10 @@ struct irqdev {

user_intr_t *handler[NINTR]; /* irq handlers for device_open */
};

-extern int install_user_intr_handler (int id, ipc_port_t dst_port, mach_device_t device, boolean_t device_port);
+extern int install_user_intr_handler (int id, ipc_port_t dst_port, mach_device_t device);

extern void irq_init();

void intr_thread (void);

-kern_return_t irq_acknowledge (ipc_port_t receive_port);

extern int irqopen(dev_t dev, int flag, io_req_t ior);

extern void irqclose(dev_t dev, int flags);

diff --git a/include/device/device.defs b/include/device/device.defs
index ec4b5bf8..31ed42e2 100644
--- a/include/device/device.defs
+++ b/include/device/device.defs
@@ -141,22 +141,3 @@ routine device_set_filter(
in priority : int;
in filter : filter_array_t
);
-
-routine device_intr_register(
- device : device_t;
- in id : int;
- in flags : int;
- in receive_port : mach_port_send_t
- );
-
-/*
- * 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
- */
-routine device_intr_ack(
- device : device_t;
- in receive_port : mach_port_send_t);
-
diff --git a/include/device/notify.defs b/include/device/notify.defs
deleted file mode 100644
index 7919b339..00000000
--- a/include/device/notify.defs
+++ /dev/null
@@ -1,36 +0,0 @@
-/*
- * Mach Operating System
- * Copyright (c) 1991,1990,1989 Carnegie Mellon University
- * All Rights Reserved.
- *
- * Permission to use, copy, modify and distribute this software and its
- * documentation is hereby granted, provided that both the copyright
- * notice and this permission notice appear in all copies of the
- * software, derivative works or modified versions, and any portions
- * thereof, and that both notices appear in supporting documentation.
- *
- * CARNEGIE MELLON ALLOWS FREE USE OF THIS SOFTWARE IN ITS "AS IS"
- * CONDITION. CARNEGIE MELLON DISCLAIMS ANY LIABILITY OF ANY KIND FOR
- * ANY DAMAGES WHATSOEVER RESULTING FROM THE USE OF THIS SOFTWARE.
- *
- * Carnegie Mellon requests users of this software to return to
- *
- * Software Distribution Coordinator or Software.D...@CS.CMU.EDU
- * School of Computer Science
- * Carnegie Mellon University
- * Pittsburgh PA 15213-3890
- *
- * any improvements or extensions that they make and grant Carnegie Mellon
- * the rights to redistribute these changes.
- */
-
-subsystem notify 100;
-
-#include <mach/std_types.defs>
-
-serverprefix do_;
-serverdemux device_intr_notify_server;
-
-simpleroutine device_intr_notify(
- notify : notify_port_t;
- id : int);
diff --git a/include/device/notify.h b/include/device/notify.h
deleted file mode 100644
index addf9114..00000000
--- a/include/device/notify.h
+++ /dev/null
@@ -1,34 +0,0 @@
-/*
- * Copyright (c) 2010 Free Software Foundation, Inc.
- *
- * Permission to use, copy, modify and distribute this software and its
- * documentation is hereby granted, provided that both the copyright
- * notice and this permission notice appear in all copies of the
- * software, derivative works or modified versions, and any portions
- * thereof, and that both notices appear in supporting documentation.
- *
- * THE FREE SOFTWARE FOUNDATIONALLOWS FREE USE OF THIS SOFTWARE IN ITS "AS IS"
- * CONDITION. THE FREE SOFTWARE FOUNDATION DISCLAIMS ANY LIABILITY OF ANY KIND
- * FOR ANY DAMAGES WHATSOEVER RESULTING FROM THE USE OF THIS SOFTWARE.
- */
-
-/*
- * Device notification definitions.
- */
-
-#ifndef _MACH_DEVICE_NOTIFY_H_
-#define _MACH_DEVICE_NOTIFY_H_
-
-#include <mach/port.h>
-#include <mach/message.h>
-
-typedef struct
-{
- mach_msg_header_t intr_header;
- mach_msg_type_t intr_type;
- int id;
-} device_intr_notification_t;
-
-#define DEVICE_INTR_NOTIFY 100
-
-#endif /* _MACH_DEVICE_NOTIFY_H_ */
--
2.28.0.rc1

[Samuel Thibault]

Hello,

Junling Ma, le sam. 01 août 2020 18:59:58 -0700, a ecrit:

> PATCH 1 is included in this email: it prepares the stage by

Please provide separate patches for each over these. Otherwise this is
all conflated and way more difficult to review.

> 1. Change devicer_user_intr to be a linux interrupt handler.

Right, this makes sense, but did you actually try it? Check the existing
code twice to realize why it's done that way. Here linux_intr is looking
through the list of actions for an irq. You are making action->handler()
call free_irq itself when the handler should be removed. But then the
loop of linux_intr will have its action pointer undefined since freed by
free_irq.

I agree that we should probably remove some of the "user_intr" pieces of
linux_intr(). But we cannot afford removing the part that checks the
value returned by the handler, otherwise it'll break.

> 2. Make user_intr_t represent a specific interrupt line, so that notifications queue on each line.
> 3. Make struct irqdev store a vector user_intr_t, indexed by interrupt line.

? But then we can't have several userland translators using the same
irq? That'll break.

> 4. The n_unacked counter only increases when successfully delivered an interrupt.

? It is meant to know how many times __disable_irq was called, and thus
how many times we should call __enable_irq if the port dies.

Samuel

[Samuel Thibault]

Junling Ma, le sam. 01 août 2020 19:06:02 -0700, a ecrit:

> In this patch, the old interface of device_intr_register and device_intr_ack is removed.

We don't want to remove it yet, we need to keep it for smooth upgrade.

Please everybody always remember to test smooth upgrades. Otherwise I
end up having to do it when I merge the patches in and discover the
world breaks. And thus I have less time to spend on reviewing more
features, and thus you get your patches queued yet longer and get
frustrated.

Really, the more you work ahead of submission, the least I have to do,
the more I can merge in.

Samuel

[Junling Ma]

Hi Samual,

Thanks for reviewing.

> Junling Ma, le sam. 01 août 2020 18:59:58 -0700, a ecrit:
>> PATCH 1 is included in this email: it prepares the stage by
>
> Please provide separate patches for each over these. Otherwise this is
> all conflated and way more difficult to review.

Ok.

>> 1. Change devicer_user_intr to be a linux interrupt handler.
>
> Right, this makes sense, but did you actually try it? Check the existing
> code twice to realize why it's done that way. Here linux_intr is looking
> through the list of actions for an irq. You are making action->handler()
> call free_irq itself when the handler should be removed. But then the
> loop of linux_intr will have its action pointer undefined since freed by
> free_irq.

Right I realized that, and moved the free_irq to intr_thread. Will clean up the patch.

> I agree that we should probably remove some of the "user_intr" pieces of
> linux_intr(). But we cannot afford removing the part that checks the
> value returned by the handler, otherwise it'll break.

If we move the cleaning-up-dead-notification code to intr_thread, then the code should not break. (I did try with a set program). The handler does not care about notifications. It sees that irqtab.handler[id] is not null, then increase the interrupt counter, disable the irq, and wake up the intr_interupt thread. The thread, if finds an empty queue, then free the irq.

>> 2. Make user_intr_t represent a specific interrupt line, so that notifications queue on each line.
>> 3. Make struct irqdev store a vector user_intr_t, indexed by interrupt line.
>
> ? But then we can't have several userland translators using the same
> irq? That'll break.

Each user_intr_t has a queue of notifications. Originally, each notification was queued up on the linux side. In this patch we just move them back to the mach side and sort them by interrupt line (i.e., attach to user_intr_t). I am not sure why it would break.

IN fact, I was pondering on the idea that each irq* device should be opened exclusively, and user land device translator does the multiplexing, because in the future, when we have MSI or MSI-X, the interrupt will not be shared, and they should be opened exclusively.. However, I am afraid that an interrupt going through two indirect calls would be too time costly to hurt driver performance. So this patch still does multiplexing and queue up multiple notifications.

>> 4. The n_unacked counter only increases when successfully delivered an interrupt.
>
> ? It is meant to know how many times __disable_irq was called, and thus
> how many times we should call __enable_irq if the port dies.

The __disable_irq uses its own counter. I think, the n_unacked, as the name suggests, counts how many interrupts have not asked. The original way of counting means that the interrupt will be unfortunately blocked forever if a message somehow failed to deliver. This patch increases n_unacked only if a notification is delivered, and thus waiting for asking becomes sensible.

I am of course happy to produce a new version of the patches. But I also feel that whether we want to introduce this new scheme still needs more discussion.


Best,
Junling

[Samuel Thibault]

Junling Ma, le dim. 02 août 2020 15:32:01 -0700, a ecrit:

> >> 1. Change devicer_user_intr to be a linux interrupt handler.
> >
> > Right, this makes sense, but did you actually try it? Check the existing
> > code twice to realize why it's done that way. Here linux_intr is looking
> > through the list of actions for an irq. You are making action->handler()
> > call free_irq itself when the handler should be removed. But then the
> > loop of linux_intr will have its action pointer undefined since freed by
> > free_irq.
>
> Right I realized that, and moved the free_irq to intr_thread. Will clean up the patch.
>
> > I agree that we should probably remove some of the "user_intr" pieces of
> > linux_intr(). But we cannot afford removing the part that checks the
> > value returned by the handler, otherwise it'll break.
>
> If we move the cleaning-up-dead-notification code to intr_thread, then the code should not break.

Ok.

> >> 2. Make user_intr_t represent a specific interrupt line, so that notifications queue on each line.
> >> 3. Make struct irqdev store a vector user_intr_t, indexed by interrupt line.
> >
> > ? But then we can't have several userland translators using the same
> > irq? That'll break.
>
> Each user_intr_t has a queue of notifications. Originally, each notification was queued up on the linux side. In this patch we just move them back to the mach side and sort them by interrupt line (i.e., attach to user_intr_t). I am not sure why it would break.

To be honest, I don't know. Because your patch changes so many thing
that I can't even take the time to try to track what is being changed
how. What I can see easily, however, is:

+typedef struct {
+ queue_head_t notification_queue;
+ unsigned interrupts; /* Number of interrupts occurred since last run of intr_thread */
+ unsigned n_unacked; /* Number of times irqs were disabled for this */
+ decl_simple_lock_data(, lock); /* a lock to protect the queues */
} user_intr_t;

So the port is gone indeed, so it tends to say that now it's per intr
instead of per-user. But then why calling it user_intr_t? Changing the
role of an existing structure is really not the way for changes to be
reviewable.

Now, in that struct there is n_unacked. But how can this be per-irq? As
I mentioned elsewhere and it is kept here, it's the number of times irq
where disabled for this. But that's for a given *user*, not for a given
irq. If it's not per-user, when a user dies you don't know how many
times you need to call __enable_irq.

> IN fact, I was pondering on the idea that each irq* device should be opened exclusively,

Then things won't work. Shared IRQ is a thing you can't circumvent with
the hardware that people have atm.

> I am afraid that an interrupt going through two indirect calls would be too time costly to hurt driver performance.

Interrupts *are* inherently costly. A couple software layers are really
not much compared to that. Drivers are already used to have to gather
notifications so as to spare interrupts.

> >> 4. The n_unacked counter only increases when successfully delivered an interrupt.
> >
> > ? It is meant to know how many times __disable_irq was called, and thus
> > how many times we should call __enable_irq if the port dies.
>
> The __disable_irq uses its own counter.

Yes, and the way you do it just duplicates it, that's nonsense. Either
it's the same, or it's not. As mentioned above, it's not.µ

> The original way of counting means that the interrupt will be unfortunately blocked forever if a message somehow failed to deliver.

There is no such thing. Either the message gets delivered, or it does
not. It can take some time to get delivered. Then other translators
using the same IRQ will suffer from the situation. But that's what we
*have* to do. Until the userland translator either acknowledges the irq
or dies, we *have* to keep the irq disabled (and know that we have done
so for the death case). We can't go around it anyway, so there is no
point in trying to fix it.

> This patch increases n_unacked only if a notification is delivered,

That would mean that the responsibility of ack-ing interrupts belongs to
userland?

> I am of course happy to produce a new version of the patches. But I
> also feel that whether we want to introduce this new scheme still
> needs more discussion.

So far (except for the first part of your first patch) what you
have proposed really does not convince me at all as being an actual
improvement over what we currently have, on the contrary.

Samuel

[Junling Ma]

user_intr_t to me means a user interrupt. A notification port that is registered represents a notification. Multiple notifications may queue on a single irq.

Now, in that struct there is n_unacked. But how can this be per-irq? As
I mentioned elsewhere and it is kept here, it's the number of times irq
where disabled for this. But that's for a given *user*, not for a given
irq. If it's not per-user, when a user dies you don't know how many
times you need to call __enable_irq. 

Ok, I can see that in my patch n_unacked duplicates the counter used by __enable_irq. I was distracted by using a counter at all in the original code, because an irq cannot be fired more than once before it is acked. What we really need is a boolean_t type per notification, 

IN fact, I was pondering on the idea that each irq* device should be opened exclusively,

Then things won't work. Shared IRQ is a thing you can't circumvent with
the hardware that people have atm.

You may have missed the part where I say let user side [/dev/irq*] translator do the multiplexing. And when MSI/MSI-X is introduced, irq sharing will not be needed.

I am afraid that an interrupt going through two indirect calls would be too time costly to hurt driver performance.

Interrupts *are* inherently costly. A couple software layers are really
not much compared to that. Drivers are already used to have to gather
notifications so as to spare interrupts.

ok.

4. The n_unacked counter only increases when successfully delivered an interrupt.

? It is meant to know how many times __disable_irq was called, and thus
how many times we should call __enable_irq if the port dies.

The __disable_irq uses its own counter.

Yes, and the way you do it just duplicates it, that's nonsense. Either
it's the same, or it's not. As mentioned above, it's not.µ

Yes. See the above discussion.

The original way of counting means that the interrupt will be unfortunately blocked forever if a message somehow failed to deliver.

There is no such thing. Either the message gets delivered, or it does
not. It can take some time to get delivered. Then other translators
using the same IRQ will suffer from the situation. But that's what we
*have* to do. Until the userland translator either acknowledges the irq
or dies, we *have* to keep the irq disabled (and know that we have done
so for the death case).  We can't go around it anyway, so there is no
point in trying to fix it.

The delivery could have returned an error, for example, if ikm_alloc return NULL, as checked in deliver_intr.

This patch increases n_unacked only if a notification is delivered,

That would mean that the responsibility of ack-ing interrupts belongs to
userland?

We already require that user land ack the interrupt by calling device_intr_ack, right? The kernel side keeps track of who acked and who hasn’t. As I understand, if a handler has not acked, then the n_unacked remain positive, and also __enable_irq will not be called, and thus the irq remains disabled, for all other handlers attached to the same irq.

I am of course happy to produce a new version of the patches. But I
also feel that whether we want to introduce this new scheme still
needs more discussion.

So far (except for the first part of your first patch) what you
have proposed really does not convince me at all as being an actual
improvement over what we currently have, on the contrary.

That is what I felt from the IRC discussion yesterday, too. And that is fine if there is no interest in this patch. But still, new comes may ask the same question as I did, why would any device driver need to implement device_intr_register and device_intr_ack, when they are excluded from the scene at all (only irq device has a chance of receiving these calls). Maybe there are other, better, solutions than this patch. Maybe it is not a problem at all for others.

Junling

[Samuel Thibault]

Junling Ma, le dim. 02 août 2020 16:19:39 -0700, a ecrit:

> I was distracted by using a counter at all in the original code,
> because an irq cannot be fired more than once before it is acked.

Right, normally that doesn't happen.

> What we really need is a boolean_t type per notification,

And that could be an option.

> > > IN fact, I was pondering on the idea that each irq* device should be
> > > opened exclusively,
> >
> > Then things won't work. Shared IRQ is a thing you can't circumvent with
> > the hardware that people have atm.
>
> You may have missed the part where I say let user side [/dev/irq*] translator
> do the multiplexing.

I didn't but that looks like extra complexity, and makes the whole thing
less robust: if the multiplexer gets to die somehow, everything dies.

> And when MSI/MSI-X is introduced, irq sharing will not be needed.

That would still ignore all the hardware that people still have.

> > > The original way of counting means that the interrupt will be
> > > unfortunately blocked forever if a message somehow failed to deliver.
> >
> > There is no such thing. Either the message gets delivered, or it does
> > not. It can take some time to get delivered. Then other translators
> > using the same IRQ will suffer from the situation. But that's what we
> > *have* to do. Until the userland translator either acknowledges the irq
> > or dies, we *have* to keep the irq disabled (and know that we have done
> > so for the death case). We can't go around it anyway, so there is no
> > point in trying to fix it.
>
> The delivery could have returned an error, for example, if ikm_alloc return
> NULL, as checked in deliver_intr.

In such a case, I don't think we can do anything better than closing
the user port, so the user can get notified of the missed IRQ, and
possibly try to restart the driver (which is fine for network cards for
instance). So we're back to the "port death" case.

> > > This patch increases n_unacked only if a notification is delivered,
> >
> > That would mean that the responsibility of ack-ing interrupts belongs to
> > userland?
>
> We already require that user land ack the interrupt by calling device_intr_ack,
> right?

That it acks, yes. That it tracks how many times __enable_irq() should
be called on port death, no.

Again, just checking for port death is a simple way and gets everything
working as expected. What I can grasp from what you proposed, it is much
less clear.

> As I understand, if a handler has not acked, then the n_unacked remain
> positive, and also __enable_irq will not be called, and thus the irq
> remains disabled, for all other handlers attached to the same irq.

Yes. Until the user notices the translator is stuck, and thus kills it,
and thus the kernel notices the port death, and can unlock the IRQ.

> That is what I felt from the IRC discussion yesterday, too. And that is fine if
> there is no interest in this patch.

The thing is: I do not see what improvement there is here. It took us
some time to get to this working state of ack semantic. If you want to
improve it, then fine, but you have not explained where there is actual
improvement (and just rewriting the whole thing for the sake of it will
most probably only bring bugs).

> But still, new comes may ask the same question as I did, why would
> any device driver need to implement device_intr_register and
> device_intr_ack,

That question is complementely independent of everything we have been
discussing above. Again a reason why to separate changes into separate
patches, so we can discuss separately and avoid squashing together
things that don't actually need to be, thus blocking improvements on
some side just because some other side poses question.

My only comment on patch-2 was that it was introducing other unrelated
changes, making it unreviewable. As discussed on IRC the other day, your
device IPC change makes sense. But I don't see any relation with the IRQ
*management* that you have introduced. Make that patch independent from
the whole rest, and we'll be able to discuss it.


Really, I'm wondering how it cannot be obvious for newcomers that yes,
you need to separate out changes so we can discuss things separately,
otherwise it's just a big mess and we can't work things out.

Samuel

[Samuel Thibault]

Samuel Thibault, le lun. 03 août 2020 01:33:53 +0200, a ecrit:

> Junling Ma, le dim. 02 août 2020 16:19:39 -0700, a ecrit:

> > You may have missed the part where I say let user side [/dev/irq*] translator
> > do the multiplexing.
>
> I didn't but that looks like extra complexity, and makes the whole thing
> less robust: if the multiplexer gets to die somehow, everything dies.

Also, the kernel will anyway have to support shared irq, for its
own drivers. So let's just reuse this, I don't see why introducing
complexity in userland.

Samuel

[Junling Ma]

I guess you misunderstood what I mean. I mean that there seems to be no interest in the discussion on IRC yesterday that we introduce a new scheme. If so, I will not work on that part. For part I, ie., moving the irq management to mach side, it people think it is useful, I will prepare a series of revised patch for that.

Junling

> On Aug 2, 2020, at 4:33 PM, Samuel Thibault <samuel....@gnu.org> wrote:
>
> Junling Ma, le dim. 02 août 2020 16:19:39 -0700, a ecrit:

>> I was distracted by using a counter at all in the original code,
>> because an irq cannot be fired more than once before it is acked.
>
> Right, normally that doesn't happen.
>
>> What we really need is a boolean_t type per notification,
>
> And that could be an option.
>
>>>> IN fact, I was pondering on the idea that each irq* device should be
>>>> opened exclusively,
>>>
>>> Then things won't work. Shared IRQ is a thing you can't circumvent with
>>> the hardware that people have atm.
>>

>> You may have missed the part where I say let user side [/dev/irq*] translator
>> do the multiplexing.
>
> I didn't but that looks like extra complexity, and makes the whole thing
> less robust: if the multiplexer gets to die somehow, everything dies.
>

[Samuel Thibault]

Junling Ma, le dim. 02 août 2020 16:45:22 -0700, a ecrit:

> I guess you misunderstood what I mean. I mean that there seems to be no interest in the discussion on IRC yesterday that we introduce a new scheme.

That's because you asked on IRC at a random time.

That's what IRC is not good at: collecting the community opinion on
something. If you ask on IRC at a random time, you'll only get the opion
of the people who actually look at IRC at that random time.

I (and I'm maintainer here...) do believe that the way you propose makes
sense, for the reason you mentioned (not having to implement the methods
for all device interface implementations while it's only useful for one
of them). Introducing another interface (like the i386 perm) could make
sense, but since device_read/write can offer exactly the same semantic,
there is no reason to introduce one.

> If so, I will not work on that part.

Please do. That part does make sense, is an improvement, and does not
require to revamp the whole thing, just the interface. Please however
also fix libddekit so we don't completely break netdde along the way
(and so that we actually have a well-tested testcase for the interface).

> For part I, ie., moving the irq management to mach side, it people
> think it is useful, I will prepare a series of revised patch for that.

I'm not sure what feature exactly you mean, I'll just remind the
(current) requirements ahead of time:

- we still need the linux/ part to be working.
- we want to get rid of the whole linux/ directory at some point

so working on moving pieces of the IRQ handling to mach does make a lot
of sense since that's what we'll want in the end. Just make sure to keep
the linux/ part working for now.

Samuel

[Junling Ma]

Ok. Will do.

Junling