[Xenomai-core] [PATCH] Shared interrupts (ready to merge)

[Xenomai-core] [PATCH] Shared interrupts (ready to merge)

[Dmitry Adamushko]

[-- Attachment #1.1: Type: text/plain, Size: 484 bytes --]

Hello everybody,

being inspired by successful results of tests conducted recently by Jan &
team,
I'm presenting the final (yep, yet another final :) combo-patch.

The shirq support now is optional, so that

CONFIG_XENO_OPT_SHIRQ_LEVEL -    enables shirq for level-triggered
interrupts;

CONFIG_XENO_OPT_SHIRQ_EDGE  -    -//- for edge-triggered ones.

I addressed all the remarks and now, IMHO, it's (hopefully) ready for merge.


--
Best regards,
Dmitry Adamushko

[-- Attachment #1.2: Type: text/html, Size: 597 bytes --]

[-- Attachment #2: shirq-combo.patch --]
[-- Type: application/octet-stream, Size: 33713 bytes --]

diff -urp xenomai-CVS/include/asm-generic/hal.h xenomai/include/asm-generic/hal.h
--- xenomai-CVS/include/asm-generic/hal.h	2006-02-02 12:17:23.000000000 +0100
+++ xenomai/include/asm-generic/hal.h	2006-02-15 18:16:22.000000000 +0100
@@ -64,6 +64,11 @@
 #define RTHAL_NR_CPUS		IPIPE_NR_CPUS
 #define RTHAL_ROOT_PRIO		IPIPE_ROOT_PRIO
 #define RTHAL_NR_FAULTS		IPIPE_NR_FAULTS
+#define RTHAL_NR_IRQS		IPIPE_NR_IRQS
+#define RTHAL_VIRQ_BASE		IPIPE_VIRQ_BASE
+
+#define rthal_virtual_irq_p(irq)	((irq) >= RTHAL_VIRQ_BASE && \
+					(irq) < RTHAL_NR_IRQS)
 
 #define RTHAL_SERVICE_IPI0	IPIPE_SERVICE_IPI0
 #define RTHAL_SERVICE_VECTOR0	IPIPE_SERVICE_VECTOR0
diff -urp xenomai-CVS/include/asm-generic/system.h xenomai/include/asm-generic/system.h
--- xenomai-CVS/include/asm-generic/system.h	2006-02-02 12:17:23.000000000 +0100
+++ xenomai/include/asm-generic/system.h	2006-02-15 18:15:16.000000000 +0100
@@ -101,6 +101,8 @@ typedef struct { volatile unsigned long 
 
 #define XNARCH_NR_CPUS               RTHAL_NR_CPUS
 
+#define XNARCH_TIMER_IRQ	     RTHAL_TIMER_IRQ
+
 #define XNARCH_ROOT_STACKSZ   0	/* Only a placeholder -- no stack */
 
 #define XNARCH_PROMPT "Xenomai: "
diff -urp xenomai-CVS/include/asm-i386/hal.h xenomai/include/asm-i386/hal.h
--- xenomai-CVS/include/asm-i386/hal.h	2006-01-31 14:09:47.000000000 +0100
+++ xenomai/include/asm-i386/hal.h	2006-02-15 16:44:52.000000000 +0100
@@ -139,6 +139,9 @@ static inline __attribute_const__ unsign
 #define RTHAL_APIC_TIMER_VECTOR    RTHAL_SERVICE_VECTOR3
 #define RTHAL_APIC_TIMER_IPI       RTHAL_SERVICE_IPI3
 #define RTHAL_APIC_ICOUNT          ((RTHAL_TIMER_FREQ + HZ/2)/HZ)
+#define RTHAL_TIMER_IRQ 	   RTHAL_APIC_TIMER_IPI
+#else  /* !CONFIG_X86_LOCAL_APIC */
+#define RTHAL_TIMER_IRQ		   RTHAL_8254_IRQ
 #endif /* CONFIG_X86_LOCAL_APIC */
 
 #define RTHAL_NMICLK_FREQ	RTHAL_CPU_FREQ
diff -urp xenomai-CVS/include/asm-sim/system.h xenomai/include/asm-sim/system.h
--- xenomai-CVS/include/asm-sim/system.h	2006-02-02 12:17:23.000000000 +0100
+++ xenomai/include/asm-sim/system.h	2006-02-15 18:15:36.000000000 +0100
@@ -75,6 +75,9 @@ typedef unsigned long xnlock_t;
 
 #define XNARCH_NR_CPUS              1
 
+/* Should be equal to the value used for creating the mvmtimer object (mvm_start_timer). */
+#define XNARCH_TIMER_IRQ	    1
+
 #define XNARCH_DEFAULT_TICK         10000000 /* ns, i.e. 10ms */
 #define XNARCH_HOST_TICK            0 /* No host ticking service. */
 
diff -urp xenomai-CVS/include/asm-uvm/system.h xenomai/include/asm-uvm/system.h
--- xenomai-CVS/include/asm-uvm/system.h	2006-02-02 12:17:23.000000000 +0100
+++ xenomai/include/asm-uvm/system.h	2006-02-15 18:15:28.000000000 +0100
@@ -74,6 +74,8 @@ typedef unsigned long xnlock_t;
 
 #define XNARCH_NR_CPUS             1
 
+#define XNARCH_TIMER_IRQ	   0
+
 #define XNARCH_DEFAULT_TICK        1000000 /* ns, i.e. 1ms */
 #define XNARCH_SIG_RESTART         SIGUSR1
 #define XNARCH_HOST_TICK           0	/* No host ticking service */
diff -urp xenomai-CVS/include/native/intr.h xenomai/include/native/intr.h
--- xenomai-CVS/include/native/intr.h	2005-11-01 23:37:45.000000000 +0100
+++ xenomai/include/native/intr.h	2006-02-15 16:44:52.000000000 +0100
@@ -50,9 +50,14 @@ typedef struct rt_intr_placeholder {
 
 #define XENO_INTR_MAGIC 0x55550a0a
 
+/* Creation flags. */
+#define I_SHARED	XN_ISR_SHARED
+#define I_EDGE		XN_ISR_EDGE
+
 #define RT_INTR_HANDLED XN_ISR_HANDLED
 #define RT_INTR_CHAINED XN_ISR_CHAINED
 #define RT_INTR_ENABLE  XN_ISR_ENABLE
+#define RT_INTR_NOINT	XN_ISR_NOINT
 
 #define I_DESC(xintr)  ((RT_INTR *)(xintr)->cookie)
 
@@ -71,8 +76,6 @@ typedef struct rt_intr {
 
     rt_handle_t handle;	/* !< Handle in registry -- zero if unregistered. */
 
-    char name[XNOBJECT_NAME_LEN]; /* !< Symbolic name. */
-
 #if defined(__KERNEL__) && defined(CONFIG_XENO_OPT_PERVASIVE)
 
     int mode;		/* !< Interrupt control mode. */
@@ -101,9 +104,11 @@ int __native_intr_pkg_init(void);
 void __native_intr_pkg_cleanup(void);
 
 int rt_intr_create(RT_INTR *intr,
+		   const char *name,
 		   unsigned irq,
 		   rt_isr_t isr,
-		   rt_iack_t iack);
+		   rt_iack_t iack,
+		   int mode);
 
 #ifdef CONFIG_XENO_OPT_PERVASIVE
 int rt_intr_handler(xnintr_t *cookie);
@@ -122,7 +127,7 @@ extern "C" {
 #endif
 
 int rt_intr_bind(RT_INTR *intr,
-		 unsigned irq,
+		 const char *name,
 		 RTIME timeout);
 
 static inline int rt_intr_unbind (RT_INTR *intr)
@@ -133,6 +138,7 @@ static inline int rt_intr_unbind (RT_INT
 }
 
 int rt_intr_create(RT_INTR *intr,
+		   const char *name,
 		   unsigned irq,
 		   int mode);
 
diff -urp xenomai-CVS/include/nucleus/intr.h xenomai/include/nucleus/intr.h
--- xenomai-CVS/include/nucleus/intr.h	2005-11-01 23:37:45.000000000 +0100
+++ xenomai/include/nucleus/intr.h	2006-02-15 16:44:52.000000000 +0100
@@ -25,22 +25,36 @@
 #define XN_ISR_HANDLED   0x0
 #define XN_ISR_CHAINED   0x1
 #define XN_ISR_ENABLE    0x2
+#define XN_ISR_NOINT	 0x4
 
-#if defined(__KERNEL__) || defined(__XENO_UVM__) || defined(__XENO_SIM__)
+/* Creation flags. */
+#define XN_ISR_SHARED	 0x1
+#define XN_ISR_EDGE	 0x2
+
+/* Operational flags. */
+#define XN_ISR_ATTACHED	 0x100
 
-struct xnintr;
+#if defined(__KERNEL__) || defined(__XENO_UVM__) || defined(__XENO_SIM__)
 
 typedef struct xnintr {
 
-    unsigned irq;	/* !< IRQ number. */
+#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
+    struct xnintr *next; /* !< Next object in the IRQ-sharing chain. */
+#endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 
     xnisr_t isr;	/* !< Interrupt service routine. */
 
-    xniack_t iack;	/* !< Interrupt acknowledge routine. */
+    void *cookie;	/* !< User-defined cookie value. */
 
     unsigned long hits;	/* !< Number of receipts (since attachment). */
 
-    void *cookie;	/* !< User-defined cookie value. */
+    xnflags_t flags; 	/* !< Creation flags. */
+
+    unsigned irq;	/* !< IRQ number. */
+
+    xniack_t iack;	/* !< Interrupt acknowledge routine. */
+
+    char name[XNOBJECT_NAME_LEN]; /* !< Symbolic name. */
 
 } xnintr_t;
 
@@ -50,11 +64,16 @@ extern xnintr_t nkclock;
 extern "C" {
 #endif
 
+int xnintr_mount(void);
+
 void xnintr_clock_handler(void);
 
+int xnintr_irq_proc(unsigned int irq, char *str);
+
     /* Public interface. */
 
 int xnintr_init(xnintr_t *intr,
+		const char *name,
 		unsigned irq,
 		xnisr_t isr,
 		xniack_t iack,
diff -urp xenomai-CVS/include/rtdm/rtdm_driver.h xenomai/include/rtdm/rtdm_driver.h
--- xenomai-CVS/include/rtdm/rtdm_driver.h	2006-01-25 02:40:14.000000000 +0100
+++ xenomai/include/rtdm/rtdm_driver.h	2006-02-15 16:44:52.000000000 +0100
@@ -693,6 +693,17 @@ typedef unsigned long               rtdm
 
 typedef xnintr_t                    rtdm_irq_t;
 
+/*!
+ * @anchor RTDM_IRQTYPE_xxx   @name RTDM_IRQTYPE_xxx
+ * Interrupt registrations flags
+ * @{
+ */
+/** Enable IRQ-sharing with other real-time drivers */
+#define RTDM_IRQTYPE_SHARED         XN_ISR_SHARED
+/** Additional flag to enable IRQ-sharing of edge-triggered interrupts */
+#define RTDM_IRQTYPE_EDGE	    XN_ISR_EDGE
+/** @} */
+
 /**
  * Interrupt handler
  *
@@ -702,7 +713,6 @@ typedef xnintr_t                    rtdm
  */
 typedef int (*rtdm_irq_handler_t)(rtdm_irq_t *irq_handle);
 
-
 /*!
  * @anchor RTDM_IRQ_xxx   @name RTDM_IRQ_xxx
  * Return flags of interrupt handlers
@@ -712,6 +722,8 @@ typedef int (*rtdm_irq_handler_t)(rtdm_i
 #define RTDM_IRQ_PROPAGATE          XN_ISR_CHAINED
 /** Re-enable interrupt line on return */
 #define RTDM_IRQ_ENABLE             XN_ISR_ENABLE
+/** Denote unhandled interrupt */
+#define RTDM_IRQ_NOINT		    XN_ISR_NOINT
 /** @} */
 
 /**
@@ -741,7 +753,7 @@ static inline int rtdm_irq_request(rtdm_
                                    const char *device_name,
                                    void *arg)
 {
-    xnintr_init(irq_handle, irq_no, handler, NULL, flags);
+    xnintr_init(irq_handle, device_name, irq_no, handler, NULL, flags);
     return xnintr_attach(irq_handle, arg);
 }
 
diff -urp xenomai-CVS/ksrc/arch/generic/hal.c xenomai/ksrc/arch/generic/hal.c
--- xenomai-CVS/ksrc/arch/generic/hal.c	2006-02-02 12:17:23.000000000 +0100
+++ xenomai/ksrc/arch/generic/hal.c	2006-02-15 16:44:52.000000000 +0100
@@ -697,45 +697,6 @@ static int hal_read_proc (char *page,
     return len;
 }
 
-static int irq_read_proc (char *page,
-			  char **start,
-			  off_t off,
-			  int count,
-			  int *eof,
-			  void *data)
-{
-    int len = 0, cpu, irq;
-    char *p = page;
-
-    p += sprintf(p,"IRQ ");
-
-    for_each_online_cpu(cpu) {
-	p += sprintf(p,"        CPU%d",cpu);
-    }
-
-    for (irq = 0; irq < IPIPE_NR_IRQS; irq++) {
-
-	if (rthal_irq_handler(&rthal_domain, irq) == NULL)
-	    continue;
-
-	p += sprintf(p,"\n%3d:",irq);
-
-	for_each_online_cpu(cpu) {
-	    p += sprintf(p,"%12lu",rthal_cpudata_irq_hits(&rthal_domain,cpu,irq));
-	}
-    }
-
-    p += sprintf(p,"\n");
-
-    len = p - page - off;
-    if (len <= off + count) *eof = 1;
-    *start = page + off;
-    if (len > count) len = count;
-    if (len < 0) len = 0;
-
-    return len;
-}
-
 static int faults_read_proc (char *page,
 			     char **start,
 			     off_t off,
@@ -862,12 +823,6 @@ static int rthal_proc_register (void)
 		  NULL,
 		  rthal_proc_root);
 
-    __rthal_add_proc_leaf("irq",
-		  &irq_read_proc,
-		  NULL,
-		  NULL,
-		  rthal_proc_root);
-
     __rthal_add_proc_leaf("faults",
 		  &faults_read_proc,
 		  NULL,
@@ -890,7 +845,6 @@ static void rthal_proc_unregister (void)
 {
     rthal_nmi_proc_unregister();
     remove_proc_entry("hal",rthal_proc_root);
-    remove_proc_entry("irq",rthal_proc_root);
     remove_proc_entry("faults",rthal_proc_root);
     remove_proc_entry("apc",rthal_proc_root);
     remove_proc_entry("xenomai",NULL);
diff -urp xenomai-CVS/ksrc/drivers/16550A/16550A.c xenomai/ksrc/drivers/16550A/16550A.c
--- xenomai-CVS/ksrc/drivers/16550A/16550A.c	2006-01-07 23:32:10.000000000 +0100
+++ xenomai/ksrc/drivers/16550A/16550A.c	2006-02-15 16:44:52.000000000 +0100
@@ -238,7 +238,7 @@ static int rt_16550_interrupt(rtdm_irq_t
     int                     rbytes = 0;
     int                     events = 0;
     int                     modem;
-    int                     ret = RTDM_IRQ_PROPAGATE;
+    int                     ret = RTDM_IRQ_PROPAGATE | RTDM_IRQ_NOINT;
 
 
     ctx = rtdm_irq_get_arg(irq_context, struct rt_16550_context);
@@ -446,7 +446,8 @@ int rt_16550_open(struct rtdm_dev_contex
     ctx = (struct rt_16550_context *)context->dev_private;
 
     ret = rtdm_irq_request(&ctx->irq_handle, irq[dev_id], rt_16550_interrupt,
-                           0, context->device->proc_name, ctx);
+			    RTDM_IRQTYPE_SHARED|RTDM_IRQTYPE_EDGE,
+			    context->device->proc_name, ctx);
     if (ret < 0)
         return ret;
 
diff -urp xenomai-CVS/ksrc/nucleus/intr.c xenomai/ksrc/nucleus/intr.c
--- xenomai-CVS/ksrc/nucleus/intr.c	2006-02-02 12:17:23.000000000 +0100
+++ xenomai/ksrc/nucleus/intr.c	2006-02-15 18:43:42.000000000 +0100
@@ -41,8 +41,16 @@ xnintr_t nkclock;
 static void xnintr_irq_handler(unsigned irq,
 			       void *cookie);
 
+#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
+
+/* Helper functions. */
+static int xnintr_shirq_attach(xnintr_t *intr, void *cookie);
+static int xnintr_shirq_detach(xnintr_t *intr);
+
+#endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
+
 /*! 
- * \fn int xnintr_init (xnintr_t *intr,unsigned irq,xnisr_t isr,xniack_t iack,xnflags_t flags)
+ * \fn int xnintr_init (xnintr_t *intr,const char *name,unsigned irq,xnisr_t isr,xniack_t iack,xnflags_t flags)
  * \brief Initialize an interrupt object.
  *
  * Associates an interrupt object with an IRQ line.
@@ -68,9 +76,10 @@ static void xnintr_irq_handler(unsigned 
  * layer to forward the IRQ. For instance, this would cause the Adeos
  * control layer to propagate the interrupt down the interrupt
  * pipeline to other Adeos domains, such as Linux. This is the regular
- * way to share interrupts between the nucleus and the host system. At
- * the opposite, RT_INTR_HANDLED can be used instead to indicate that
- * the interrupt request has been fulfilled.
+ * way to share interrupts between the nucleus and the host system.
+ * 
+ * - XN_ISR_NOINT indicates that the interrupt request has not been fulfilled
+ * by the ISR.
  *
  * A count of interrupt receipts is tracked into the interrupt
  * descriptor, and reset to zero each time the interrupt object is
@@ -82,6 +91,9 @@ static void xnintr_irq_handler(unsigned 
  * descriptor must always be valid while the object is active
  * therefore it must be allocated in permanent memory.
  *
+ * @param name An ASCII string standing for the symbolic name of the
+ * interrupt object.
+ *
  * @param irq The hardware interrupt channel associated with the
  * interrupt object. This value is architecture-dependent. An
  * interrupt object must then be attached to the hardware interrupt
@@ -102,9 +114,13 @@ static void xnintr_irq_handler(unsigned 
  * the interrupt has been properly acknowledged. If @a iack is NULL,
  * the default routine will be used instead.
  *
- * @param flags A set of creation flags affecting the operation. Since
- * no flags are currently defined, zero should be passed for this
- * parameter.
+ * @param flags A set of creation flags affecting the operation. The
+ * valid flags are:
+ *
+ * - XN_ISR_SHARED enables IRQ-sharing with other interrupt objects.
+ *
+ * - XN_ISR_EDGE is an additional flag need to be set together with XN_ISR_SHARED
+ * to enable IRQ-sharing of edge-triggered interrupts.
  *
  * @return No error condition being defined, 0 is always returned.
  *
@@ -120,6 +136,7 @@ static void xnintr_irq_handler(unsigned 
  */
 
 int xnintr_init (xnintr_t *intr,
+		 const char *name,
 		 unsigned irq,
 		 xnisr_t isr,
 		 xniack_t iack,
@@ -130,6 +147,11 @@ int xnintr_init (xnintr_t *intr,
     intr->iack = iack;
     intr->cookie = NULL;
     intr->hits = 0;
+    intr->flags = flags;
+    xnobject_copy_name(intr->name,name);
+#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
+    intr->next = NULL;
+#endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 
     return 0;
 }
@@ -164,7 +186,8 @@ int xnintr_init (xnintr_t *intr,
 int xnintr_destroy (xnintr_t *intr)
 
 {
-    return xnintr_detach(intr);
+    xnintr_detach(intr);
+    return 0;
 }
 
 /*! 
@@ -206,7 +229,11 @@ int xnintr_attach (xnintr_t *intr,
 {
     intr->hits = 0;
     intr->cookie = cookie;
+#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
+    return xnintr_shirq_attach(intr,cookie);
+#else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
     return xnarch_hook_irq(intr->irq,&xnintr_irq_handler,intr->iack,intr);
+#endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 }
 
 /*! 
@@ -241,7 +268,11 @@ int xnintr_attach (xnintr_t *intr,
 int xnintr_detach (xnintr_t *intr)
 
 {
+#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
+    return xnintr_shirq_detach(intr);
+#else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
     return xnarch_release_irq(intr->irq);
+#endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 }
 
 /*! 
@@ -364,8 +395,7 @@ static void xnintr_irq_handler (unsigned
 
     if (s & XN_ISR_ENABLE)
 	xnarch_end_irq(irq);
-
-    if (s & XN_ISR_CHAINED)
+    else if (s & XN_ISR_CHAINED)
 	xnarch_chain_irq(irq);
 
     if (sched->inesting == 0 && xnsched_resched_p())
@@ -386,6 +416,343 @@ static void xnintr_irq_handler (unsigned
 
 /*@}*/
 
+/* Optional support for shared interrupts. */
+
+#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
+
+typedef struct xnintr_shirq {
+
+    xnintr_t *handlers;
+#ifdef CONFIG_SMP
+    atomic_counter_t active;
+#endif /* CONFIG_SMP */
+
+} xnintr_shirq_t;
+
+static xnintr_shirq_t xnshirqs[RTHAL_NR_IRQS];
+
+#ifdef CONFIG_SMP
+static inline void xnintr_shirq_lock(xnintr_shirq_t *shirq) {
+    xnarch_atomic_inc(&shirq->active);
+}
+
+static inline void xnintr_shirq_unlock(xnintr_shirq_t *shirq) {
+    xnarch_atomic_dec(&shirq->active);
+}
+
+static inline void xnintr_shirq_spin(xnintr_shirq_t *shirq) {
+    while (xnarch_atomic_get(&shirq->active))
+	cpu_relax();
+}
+#else /* !CONFIG_SMP */
+static inline void xnintr_shirq_lock(xnintr_shirq_t *shirq) {}
+static inline void xnintr_shirq_unlock(xnintr_shirq_t *shirq) {}
+static inline void xnintr_shirq_spin(xnintr_shirq_t *shirq) {}
+#endif /* CONFIG_SMP */
+
+#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL)
+
+/*
+ * Low-level interrupt handler dispatching the user-defined ISRs for
+ * shared interrupts -- Called with interrupts off.
+ */
+
+static void xnintr_shirq_handler (unsigned irq, void *cookie)
+{
+    xnsched_t *sched = xnpod_current_sched();
+    xnintr_shirq_t *shirq = &xnshirqs[irq];
+    xnintr_t *intr;
+    int s = 0;
+
+    xnarch_memory_barrier();
+
+    xnltt_log_event(xeno_ev_ienter,irq);
+
+    ++sched->inesting;
+
+    xnintr_shirq_lock(shirq);
+    intr = shirq->handlers;
+
+    while (intr)
+        {
+        s |= intr->isr(intr);
+        ++intr->hits;
+        intr = intr->next;
+        }
+    xnintr_shirq_unlock(shirq);
+
+    --sched->inesting;
+
+    if (s & XN_ISR_ENABLE)
+	xnarch_end_irq(irq);
+    else if (s & XN_ISR_CHAINED)
+	xnarch_chain_irq(irq);
+
+    if (sched->inesting == 0 && xnsched_resched_p())
+	xnpod_schedule();
+
+    xnltt_log_event(xeno_ev_iexit,irq);
+}
+
+#endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL */
+
+#if defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
+
+/*
+ * Low-level interrupt handler dispatching the user-defined ISRs for
+ * shared edge-triggered interrupts -- Called with interrupts off.
+ */
+
+static void xnintr_edge_shirq_handler (unsigned irq, void *cookie)
+{
+    const int MAX_EDGEIRQ_COUNTER = 128;
+
+    xnsched_t *sched = xnpod_current_sched();
+    xnintr_shirq_t *shirq = &xnshirqs[irq];
+    xnintr_t *intr, *end = NULL;
+    int s = 0, counter = 0;
+
+    xnarch_memory_barrier();
+
+    xnltt_log_event(xeno_ev_ienter,irq);
+
+    ++sched->inesting;
+
+    xnintr_shirq_lock(shirq);
+    intr = shirq->handlers;
+
+    while (intr != end)
+	{
+	int ret = intr->isr(intr);
+
+	if (!(ret & XN_ISR_NOINT))
+	    {
+	    ++intr->hits;
+	    end = NULL;
+	    s |= ret;
+	    }
+	else if (end == NULL)
+	    end = intr;
+
+	if (counter++ > MAX_EDGEIRQ_COUNTER)
+	    break;
+
+	if (!(intr = intr->next))
+	    intr = shirq->handlers;
+	}
+
+    xnintr_shirq_unlock(shirq);
+
+    --sched->inesting;
+
+    if (counter > MAX_EDGEIRQ_COUNTER)
+	xnlogerr("xnintr_edge_shirq_handler() : failed to get the IRQ%d line free.\n", irq);
+
+    if (s & XN_ISR_ENABLE)
+	xnarch_end_irq(irq);
+    else if (s & XN_ISR_CHAINED)
+	xnarch_chain_irq(irq);
+
+    if (sched->inesting == 0 && xnsched_resched_p())
+	xnpod_schedule();
+
+    xnltt_log_event(xeno_ev_iexit,irq);
+}
+
+#endif /* CONFIG_XENO_OPT_SHIRQ_EDGE */
+
+static int xnintr_shirq_attach (xnintr_t *intr,
+			        void *cookie)
+{
+    xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
+    xnintr_t *prev, **p;
+    unsigned long flags;
+    int err = 0;
+
+    if (intr->irq >= RTHAL_NR_IRQS)
+	return -EINVAL;
+
+    flags = rthal_critical_enter(NULL);
+
+    if (__testbits(intr->flags,XN_ISR_ATTACHED))
+	{
+	err = -EPERM;
+	goto unlock_and_exit;
+	}
+
+    p = &shirq->handlers;
+
+    if ((prev = *p) != NULL)
+	{
+	/* Check on whether the shared mode is allowed. */
+	if (!(prev->flags & intr->flags & XN_ISR_SHARED) || (prev->iack != intr->iack)
+	    || ((prev->flags & XN_ISR_EDGE) != (intr->flags & XN_ISR_EDGE)))
+	    {
+	    err = -EBUSY;
+	    goto unlock_and_exit;
+	    }
+
+	/* Get a position at the end of the list to insert the new element. */
+	while (prev) 
+	    {
+	    p = &prev->next;
+	    prev = *p;
+	    }
+	}
+    else
+	{
+	/* Initialize the corresponding interrupt channel */
+	void (*handler)(unsigned, void *) = &xnintr_irq_handler;
+
+	if (intr->flags & XN_ISR_SHARED)
+	    {
+#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL)
+	    handler = &xnintr_shirq_handler;
+#endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL */
+
+#if defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
+	    if (intr->flags & XN_ISR_EDGE)
+		handler = &xnintr_edge_shirq_handler;
+#endif /* CONFIG_XENO_OPT_SHIRQ_EDGE */
+	    }
+
+	err = xnarch_hook_irq(intr->irq,handler,intr->iack,intr);
+	if (err)
+	    goto unlock_and_exit;
+	}
+
+    __setbits(intr->flags,XN_ISR_ATTACHED);
+
+    /* Add a given interrupt object. */
+    intr->next = NULL;
+    *p = intr;
+
+unlock_and_exit:
+
+    rthal_critical_exit(flags);
+    return err;
+}
+
+int xnintr_shirq_detach (xnintr_t *intr)
+{
+    xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
+    unsigned long flags;
+    xnintr_t *e, **p;
+    int err = 0;
+
+    if (intr->irq >= RTHAL_NR_IRQS)
+	return -EINVAL;
+
+    flags = rthal_critical_enter(NULL);
+
+    if (!__testbits(intr->flags,XN_ISR_ATTACHED))
+	{
+	rthal_critical_exit(flags);
+	return -EPERM;
+	}
+
+    __clrbits(intr->flags,XN_ISR_ATTACHED);
+
+    p = &shirq->handlers;
+
+    while ((e = *p) != NULL)
+	{
+	if (e == intr)
+	    {
+	    /* Remove a given interrupt object from the list. */
+	    *p = e->next;
+
+	    /* Release the IRQ line if this was the last user */
+	    if (shirq->handlers == NULL)
+		err = xnarch_release_irq(intr->irq);
+
+	    rthal_critical_exit(flags);
+
+	    /* The idea here is to keep a detached interrupt object valid as long
+	       as the corresponding irq handler is running. This is one of the requirements
+	       to iterate over the xnintr_shirq_t::handlers list in xnintr_irq_handler()
+	       in a lockless way. */
+
+	    xnintr_shirq_spin(shirq);
+	    return err;
+	    }
+	p = &e->next;
+	}
+
+    rthal_critical_exit(flags);
+
+    xnlogerr("Attempted to detach a non previously attached interrupt object");
+    return err;
+}
+
+int xnintr_mount(void)
+{
+    int i;
+    for (i = 0; i < RTHAL_NR_IRQS; ++i)
+	{
+	xnshirqs[i].handlers = NULL;
+#ifdef CONFIG_SMP
+	atomic_set(&xnshirqs[i].active,0);
+#endif /* CONFIG_SMP */
+	}
+    return 0;
+}
+
+int xnintr_irq_proc(unsigned int irq, char *str)
+{
+    xnintr_shirq_t *shirq;
+    xnintr_t *intr;
+    char *p = str;
+
+    if (rthal_virtual_irq_p(irq))
+	{
+	p += sprintf(p, "         [virtual]");
+	return p - str;
+	}
+    else if (irq == XNARCH_TIMER_IRQ)
+	{
+	p += sprintf(p, "         %s", nkclock.name);
+	return p - str;
+	}
+
+    shirq = &xnshirqs[irq];
+
+    xnintr_shirq_lock(shirq);
+    intr = shirq->handlers;
+
+    if (intr)
+	p += sprintf(p, "        ");
+
+    while (intr)
+	{
+	if (*(intr->name))
+	    p += sprintf(p, " %s,", intr->name);
+
+	intr = intr->next;
+	}
+
+    xnintr_shirq_unlock(shirq);
+
+    if (p != str)
+	--p;
+
+    return p - str;
+}
+
+#else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
+
+int xnintr_mount(void)
+{
+    return 0;
+}
+
+int xnintr_irq_proc(unsigned int irq, char *str)
+{
+    return 0;
+}
+
+#endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
+
 EXPORT_SYMBOL(xnintr_attach);
 EXPORT_SYMBOL(xnintr_destroy);
 EXPORT_SYMBOL(xnintr_detach);
diff -urp xenomai-CVS/ksrc/nucleus/module.c xenomai/ksrc/nucleus/module.c
--- xenomai-CVS/ksrc/nucleus/module.c	2005-11-30 10:36:33.000000000 +0100
+++ xenomai/ksrc/nucleus/module.c	2006-02-15 16:44:52.000000000 +0100
@@ -526,6 +526,48 @@ static int timer_read_proc (char *page,
     return len;
 }
 
+static int irq_read_proc (char *page,
+			  char **start,
+			  off_t off,
+			  int count,
+			  int *eof,
+			  void *data)
+{
+    int len = 0, cpu, irq;
+    char *p = page;
+
+    p += sprintf(p,"IRQ ");
+
+    for_each_online_cpu(cpu) {
+	p += sprintf(p,"        CPU%d",cpu);
+    }
+
+    for (irq = 0; irq < RTHAL_NR_IRQS; irq++) {
+
+	if (rthal_irq_handler(&rthal_domain, irq) == NULL)
+	    continue;
+
+	p += sprintf(p,"\n%3d:",irq);
+
+	for_each_online_cpu(cpu) {
+	    p += sprintf(p,"%12lu",rthal_cpudata_irq_hits(&rthal_domain,cpu,irq));
+	}
+
+	p += xnintr_irq_proc(irq, p); 
+    }
+
+    p += sprintf(p,"\n");
+
+    len = p - page - off;
+    if (len <= off + count) *eof = 1;
+    *start = page + off;
+    if (len > count) len = count;
+    if (len < 0) len = 0;
+
+    return len;
+}
+
+
 static struct proc_dir_entry *add_proc_leaf (const char *name,
 					     read_proc_t rdproc,
 					     write_proc_t wrproc,
@@ -614,6 +656,12 @@ void xnpod_init_proc (void)
 		  NULL,
 		  rthal_proc_root);
 
+    add_proc_leaf("irq",
+		  &irq_read_proc,
+		  NULL,
+		  NULL,
+		  rthal_proc_root);
+
 #ifdef CONFIG_XENO_OPT_PERVASIVE
     iface_proc_root = create_proc_entry("interfaces",
 					S_IFDIR,
@@ -633,6 +681,7 @@ void xnpod_delete_proc (void)
 
     remove_proc_entry("interfaces",rthal_proc_root);
 #endif /* CONFIG_XENO_OPT_PERVASIVE */
+    remove_proc_entry("irq",rthal_proc_root);
     remove_proc_entry("timer",rthal_proc_root);
     remove_proc_entry("version",rthal_proc_root);
     remove_proc_entry("latency",rthal_proc_root);
@@ -713,6 +762,8 @@ int __init __xeno_sys_init (void)
     xnpod_init_proc();
 #endif /* CONFIG_PROC_FS */
 
+    xnintr_mount();
+
 #ifdef CONFIG_LTT
     xnltt_mount();
 #endif /* CONFIG_LTT */
diff -urp xenomai-CVS/ksrc/nucleus/pod.c xenomai/ksrc/nucleus/pod.c
--- xenomai-CVS/ksrc/nucleus/pod.c	2006-01-29 12:34:29.000000000 +0100
+++ xenomai/ksrc/nucleus/pod.c	2006-02-15 16:44:52.000000000 +0100
@@ -3040,11 +3040,10 @@ unlock_and_exit:
 
     /* The clock interrupt does not need to be attached since the
        timer service will handle the arch-dependent setup. The IRQ
-       number (arg #2) is not used since the IRQ source will be
-       attached directly by the arch-dependent layer
+       source will be attached directly by the arch-dependent layer
        (xnarch_start_timer). */
 
-    xnintr_init(&nkclock,0,tickhandler,NULL,0);
+    xnintr_init(&nkclock,"[timer]",XNARCH_TIMER_IRQ,tickhandler,NULL,0);
 
     __setbits(nkpod->status,XNTIMED);
 
diff -urp xenomai-CVS/ksrc/skins/native/intr.c xenomai/ksrc/skins/native/intr.c
--- xenomai-CVS/ksrc/skins/native/intr.c	2005-11-29 15:15:54.000000000 +0100
+++ xenomai/ksrc/skins/native/intr.c	2006-02-15 16:44:52.000000000 +0100
@@ -119,7 +119,7 @@ static RT_OBJECT_PROCNODE __intr_pnode =
 #endif /* CONFIG_XENO_NATIVE_EXPORT_REGISTRY */
 
 /*! 
- * \fn int rt_intr_create (RT_INTR *intr,unsigned irq,rt_isr_t isr,rt_iack_t iack)
+ * \fn int rt_intr_create (RT_INTR *intr,const char *name,unsigned irq,rt_isr_t isr,rt_iack_t iack,int mode)
  * \brief Create an interrupt object from kernel space.
  *
  * Initializes and associates an interrupt object with an IRQ line. In
@@ -155,6 +155,11 @@ static RT_OBJECT_PROCNODE __intr_pnode =
  * be valid while the object is active therefore it must be allocated
  * in permanent memory.
  *
+ * @param name An ASCII string standing for the symbolic name of the
+ * interrupt object. When non-NULL and non-empty, this string is copied
+ * to a safe place into the descriptor, and passed to the registry package
+ * if enabled for indexing the created interrupt objects.
+ *
  * @param irq The hardware interrupt channel associated with the
  * interrupt object. This value is architecture-dependent.
  *
@@ -173,6 +178,14 @@ static RT_OBJECT_PROCNODE __intr_pnode =
  * the interrupt has been properly acknowledged. If @a iack is NULL,
  * the default routine will be used instead.
  *
+ * @param mode The interrupt object creation mode. The following flags can be
+ * OR'ed into this bitmask, each of them affecting the new interrupt object:
+ *
+ * - I_SHARED enables IRQ-sharing with other interrupt objects.
+ *
+ * - I_EDGE is an additional flag need to be set together with I_SHARED
+ * to enable IRQ-sharing of edge-triggered interrupts.
+ *
  * @return 0 is returned upon success. Otherwise:
  *
  * - -ENOMEM is returned if the system fails to get enough dynamic
@@ -205,9 +218,11 @@ static RT_OBJECT_PROCNODE __intr_pnode =
  */
 
 int rt_intr_create (RT_INTR *intr,
+		    const char *name,
 		    unsigned irq,
 		    rt_isr_t isr,
-		    rt_iack_t iack)
+		    rt_iack_t iack,
+		    int mode)
 {
     int err;
     spl_t s;
@@ -215,7 +230,7 @@ int rt_intr_create (RT_INTR *intr,
     if (xnpod_asynch_p())
 	return -EPERM;
 
-    xnintr_init(&intr->intr_base,irq,isr,iack,0);
+    xnintr_init(&intr->intr_base,name,irq,isr,iack,mode);
 #if defined(__KERNEL__) && defined(CONFIG_XENO_OPT_PERVASIVE)
     xnsynch_init(&intr->synch_base,XNSYNCH_PRIO);
     intr->pending = 0;
@@ -228,7 +243,6 @@ int rt_intr_create (RT_INTR *intr,
     xnlock_get_irqsave(&nklock,s);
     appendq(&__xeno_intr_q,&intr->link);
     xnlock_put_irqrestore(&nklock,s);
-    snprintf(intr->name,sizeof(intr->name),"irq%u",irq);
 
     err = xnintr_attach(&intr->intr_base,intr);
 
@@ -238,7 +252,7 @@ int rt_intr_create (RT_INTR *intr,
        half-baked objects... */
 
     if (!err)
-	err = rt_registry_enter(intr->name,intr,&intr->handle,&__intr_pnode);
+	err = rt_registry_enter(intr->intr_base.name,intr,&intr->handle,&__intr_pnode);
 #endif /* CONFIG_XENO_OPT_NATIVE_REGISTRY */
 
     if (err)
@@ -492,7 +506,7 @@ int rt_intr_inquire (RT_INTR *intr,
         goto unlock_and_exit;
         }
     
-    strcpy(info->name,intr->name);
+    strcpy(info->name,intr->intr_base.name);
     info->hits = intr->intr_base.hits;
     info->irq = intr->intr_base.irq;
 
diff -urp xenomai-CVS/ksrc/skins/native/snippets/user_irq.c xenomai/ksrc/skins/native/snippets/user_irq.c
--- xenomai-CVS/ksrc/skins/native/snippets/user_irq.c	2005-11-01 23:37:45.000000000 +0100
+++ xenomai/ksrc/skins/native/snippets/user_irq.c	2006-02-15 16:44:52.000000000 +0100
@@ -33,7 +33,7 @@ int main (int argc, char *argv[])
 
     /* ... */
 
-    err = rt_intr_create(&intr_desc,IRQ_NUMBER,I_AUTOENA);
+    err = rt_intr_create(&intr_desc,"MyIrq",IRQ_NUMBER,I_AUTOENA);
 
     /* ... */
 
diff -urp xenomai-CVS/ksrc/skins/native/syscall.c xenomai/ksrc/skins/native/syscall.c
--- xenomai-CVS/ksrc/skins/native/syscall.c	2006-01-31 19:35:45.000000000 +0100
+++ xenomai/ksrc/skins/native/syscall.c	2006-02-15 16:44:52.000000000 +0100
@@ -2894,6 +2894,7 @@ EXPORT_SYMBOL(rt_intr_handler);
 
 /*
  * int __rt_intr_create(RT_INTR_PLACEHOLDER *ph,
+ *			const char *name,
  *                      unsigned irq,
  *                      int mode)
  */
@@ -2901,6 +2902,7 @@ EXPORT_SYMBOL(rt_intr_handler);
 static int __rt_intr_create (struct task_struct *curr, struct pt_regs *regs)
 
 {
+    char name[XNOBJECT_NAME_LEN];
     RT_INTR_PLACEHOLDER ph;
     int err, mode;
     RT_INTR *intr;
@@ -2909,11 +2911,22 @@ static int __rt_intr_create (struct task
     if (!__xn_access_ok(curr,VERIFY_WRITE,__xn_reg_arg1(regs),sizeof(ph)))
 	return -EFAULT;
 
+    if (__xn_reg_arg2(regs))
+	{
+	if (!__xn_access_ok(curr,VERIFY_READ,__xn_reg_arg2(regs),sizeof(name)))
+	    return -EFAULT;
+
+	__xn_strncpy_from_user(curr,name,(const char __user *)__xn_reg_arg2(regs),sizeof(name) - 1);
+	name[sizeof(name) - 1] = '\0';
+	}
+    else
+	*name = '\0';
+
     /* Interrupt line number. */
-    irq = (unsigned)__xn_reg_arg2(regs);
+    irq = (unsigned)__xn_reg_arg3(regs);
 
     /* Interrupt control mode. */
-    mode = (int)__xn_reg_arg3(regs);
+    mode = (int)__xn_reg_arg4(regs);
 
     if (mode & ~(I_AUTOENA|I_PROPAGATE))
 	return -EINVAL;
@@ -2923,7 +2936,7 @@ static int __rt_intr_create (struct task
     if (!intr)
 	return -ENOMEM;
 
-    err = rt_intr_create(intr,irq,&rt_intr_handler,NULL);
+    err = rt_intr_create(intr,name,irq,&rt_intr_handler,NULL,0);
 
     if (err == 0)
 	{
diff -urp xenomai-CVS/ksrc/skins/posix/intr.c xenomai/ksrc/skins/posix/intr.c
--- xenomai-CVS/ksrc/skins/posix/intr.c	2006-01-03 21:41:36.000000000 +0100
+++ xenomai/ksrc/skins/posix/intr.c	2006-02-15 16:44:52.000000000 +0100
@@ -27,7 +27,7 @@ int pse51_intr_attach (struct pse51_inte
     int err;
     spl_t s;
 
-    xnintr_init(&intr->intr_base,irq,isr,iack,0);
+    xnintr_init(&intr->intr_base,NULL,irq,isr,iack,0);
 
 #if defined(__KERNEL__) && defined(CONFIG_XENO_OPT_PERVASIVE)
     xnsynch_init(&intr->synch_base,XNSYNCH_PRIO);
diff -urp xenomai-CVS/ksrc/skins/rtdm/drvlib.c xenomai/ksrc/skins/rtdm/drvlib.c
--- xenomai-CVS/ksrc/skins/rtdm/drvlib.c	2006-01-30 19:47:13.000000000 +0100
+++ xenomai/ksrc/skins/rtdm/drvlib.c	2006-02-15 16:44:52.000000000 +0100
@@ -1200,7 +1200,7 @@ EXPORT_SYMBOL(rtdm_mutex_unlock);
  * @param[in,out] irq_handle IRQ handle
  * @param[in] irq_no Line number of the addressed IRQ
  * @param[in] handler Interrupt handler
- * @param[in] flags Currently unused, pass 0
+ * @param[in] flags Registration flags, see @ref RTDM_IRQTYPE_xxx for details
  * @param[in] device_name Optional device name to show up in real-time IRQ
  * lists (not yet implemented)
  * @param[in] arg Pointer to be passed to the interrupt handler on invocation
diff -urp xenomai-CVS/src/skins/native/intr.c xenomai/src/skins/native/intr.c
--- xenomai-CVS/src/skins/native/intr.c	2005-11-01 23:37:45.000000000 +0100
+++ xenomai/src/skins/native/intr.c	2006-02-15 16:44:52.000000000 +0100
@@ -24,24 +24,22 @@
 extern int __native_muxid;
 
 int rt_intr_create (RT_INTR *intr,
+		    const char *name,
 		    unsigned irq,
 		    int mode)
 {
-    return XENOMAI_SKINCALL3(__native_muxid,
+    return XENOMAI_SKINCALL4(__native_muxid,
 			     __native_intr_create,
 			     intr,
+			     name,
 			     irq,
 			     mode);
 }
 
 int rt_intr_bind (RT_INTR *intr,
-		  unsigned irq,
+		  const char *name,
 		  RTIME timeout)
 {
-    char name[XNOBJECT_NAME_LEN];
-
-    snprintf(name,sizeof(name),"irq%u",irq);
-
     return XENOMAI_SKINCALL3(__native_muxid,
 			     __native_intr_bind,
 			     intr,

[-- Attachment #3: shirq-KConfig.patch --]
[-- Type: application/octet-stream, Size: 1556 bytes --]

diff -urp nucleus-CVS/Config.in nucleus/Config.in
--- nucleus-CVS/Config.in	2005-11-26 17:08:47.000000000 +0100
+++ nucleus/Config.in	2006-02-15 18:53:08.000000000 +0100
@@ -27,6 +27,13 @@ if [ "$CONFIG_XENO_OPT_NUCLEUS" != "n" ]
 		int 'Number of priority levels' CONFIG_XENO_OPT_SCALABLE_PRIOS 256
 	fi
 	endmenu
+
+	mainmenu_option next_comment
+	comment 'Shared interrupts'
+		bool 'Level-triggered interrupts' CONFIG_XENO_OPT_SHIRQ_LEVEL
+		bool 'Edge-triggered interrupts' CONFIG_XENO_OPT_SHIRQ_EDGE
+	endmenu
+
 	if [ "$CONFIG_LTT" != "n" ]; then
 		mainmenu_option next_comment
 		comment 'LTT tracepoints filtering'
diff -urp nucleus-CVS/Kconfig nucleus/Kconfig
--- nucleus-CVS/Kconfig	2005-11-01 23:37:45.000000000 +0100
+++ nucleus/Kconfig	2006-02-15 19:00:02.000000000 +0100
@@ -131,6 +131,30 @@ config XENO_OPT_SCALABLE_PRIOS
 
 endmenu
 
+menu "Shared interrupts"
+
+config XENO_OPT_SHIRQ_LEVEL
+	bool "Level-triggered interrupts"
+	default n
+	help
+	
+	Enables support for shared _level-triggered_ interrupts, so that
+	multiple real-time interrupt handlers are allowed to control
+	dedicated hardware devices which are configured to share
+	the same interrupt channel.
+
+config XENO_OPT_SHIRQ_EDGE
+	bool "Edge-triggered interrupts"
+	default n
+	help
+
+	Enables support for shared _edge-triggered_ interrupts, so that
+	multiple real-time interrupt handlers are allowed to control
+	dedicated hardware devices which are configured to share
+	the same interrupt channel.
+
+endmenu
+
 menu "LTT tracepoints filtering"
 
 	depends on LTT

[-- Attachment #4: ChangeLog.patch --]
[-- Type: application/octet-stream, Size: 593 bytes --]

--- ChangeLog-old	2006-02-15 19:20:57.000000000 +0100
+++ ChangeLog	2006-02-15 19:21:20.000000000 +0100
@@ -1,3 +1,11 @@
+2006-02-15  Dmitry Adamushko  <dmitry.adamushko@domain.hid>
+
+	* include/nucleus/intr.h, nucleus/intr.c: Add optional support
+	for shared interrupts (level- and edge-triggered), so that
+	multiple real-time interrupt handlers are allowed to control
+	dedicated hardware devices which are configured to share
+	the same interrupt channel.
+
 2006-02-02  Wolfgang Grandegger  <wolfgang.grandegger@domain.hid>
 
 	* include/asm-generic/hal.h, include/asm-generic/system.h

[Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 806 bytes --]

Dmitry Adamushko wrote:
> Hello everybody,
> 
> being inspired by successful results of tests conducted recently by Jan &
> team,
> I'm presenting the final (yep, yet another final :) combo-patch.
> 
> The shirq support now is optional, so that
> 
> CONFIG_XENO_OPT_SHIRQ_LEVEL -    enables shirq for level-triggered
> interrupts;
> 
> CONFIG_XENO_OPT_SHIRQ_EDGE  -    -//- for edge-triggered ones.
> 
> I addressed all the remarks and now, IMHO, it's (hopefully) ready for merge.
> 

Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this
before merging (i.e. make XN_ISR_HANDLED non-zero)?

Moreover, I attached an add-on patch to overcome the name buffer in
xnintr_t. Note that this patch is only compile-tested, I have no native
interrupt test-case at hand.

Jan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: shirq-name.patch --]
[-- Type: text/x-patch; name="shirq-name.patch", Size: 3041 bytes --]

diff -u include/nucleus/intr.h include/nucleus/intr.h
--- include/nucleus/intr.h	(Arbeitskopie)
+++ include/nucleus/intr.h	(Arbeitskopie)
@@ -54,7 +54,7 @@
 
     xniack_t iack;	/* !< Interrupt acknowledge routine. */
 
-    char name[XNOBJECT_NAME_LEN]; /* !< Symbolic name. */
+    const char *name; /* !< Symbolic name. */
 
 } xnintr_t;
 
diff -u ksrc/skins/native/syscall.c ksrc/skins/native/syscall.c
--- ksrc/skins/native/syscall.c	(Arbeitskopie)
+++ ksrc/skins/native/syscall.c	(Arbeitskopie)
@@ -2883,23 +2883,15 @@
     char name[XNOBJECT_NAME_LEN];
     RT_INTR_PLACEHOLDER ph;
     int err, mode;
-    RT_INTR *intr;
+    struct {
+	RT_INTR intr;
+	char name[XNOBJECT_NAME_LEN];
+    } *intr_buf;
     unsigned irq;
 
     if (!__xn_access_ok(curr,VERIFY_WRITE,__xn_reg_arg1(regs),sizeof(ph)))
 	return -EFAULT;
 
-    if (__xn_reg_arg2(regs))
-	{
-	if (!__xn_access_ok(curr,VERIFY_READ,__xn_reg_arg2(regs),sizeof(name)))
-	    return -EFAULT;
-
-	__xn_strncpy_from_user(curr,name,(const char __user *)__xn_reg_arg2(regs),sizeof(name) - 1);
-	name[sizeof(name) - 1] = '\0';
-	}
-    else
-	*name = '\0';
-
     /* Interrupt line number. */
     irq = (unsigned)__xn_reg_arg3(regs);
 
@@ -2909,23 +2901,41 @@
     if (mode & ~(I_AUTOENA|I_PROPAGATE))
 	return -EINVAL;
 
-    intr = (RT_INTR *)xnmalloc(sizeof(*intr));
+    intr_buf = (typeof(intr_buf))xnmalloc(sizeof(*intr_buf));
 
-    if (!intr)
+    if (!intr_buf)
 	return -ENOMEM;
 
-    err = rt_intr_create(intr,name,irq,&rt_intr_handler,NULL,0);
+    if (__xn_reg_arg2(regs))
+	{
+	if (!__xn_access_ok(curr,VERIFY_READ,__xn_reg_arg2(regs),sizeof(name)))
+	    {
+	    xnfree(intr_buf);
+	    return -EFAULT;
+	    }
+
+	__xn_strncpy_from_user(curr,
+	                       intr_buf->name,
+	                       (const char __user *)__xn_reg_arg2(regs),
+	                       sizeof(intr_buf->name) - 1);
+	intr_buf->name[sizeof(intr_buf->name) - 1] = '\0';
+	}
+    else
+	intr_buf->name[0] = '\0';
+
+
+    err = rt_intr_create(&intr_buf->intr,intr_buf->name,irq,&rt_intr_handler,NULL,0);
 
     if (err == 0)
 	{
-	intr->mode = mode;
-	intr->cpid = curr->pid;
+	intr_buf->intr.mode = mode;
+	intr_buf->intr.cpid = curr->pid;
 	/* Copy back the registry handle to the ph struct. */
-	ph.opaque = intr->handle;
+	ph.opaque = intr_buf->intr.handle;
 	__xn_copy_to_user(curr,(void __user *)__xn_reg_arg1(regs),&ph,sizeof(ph));
 	}
     else
-	xnfree(intr);
+	xnfree(intr_buf);
 
     return err;
 }
diff -u ksrc/nucleus/intr.c ksrc/nucleus/intr.c
--- ksrc/nucleus/intr.c	(Arbeitskopie)
+++ ksrc/nucleus/intr.c	(Arbeitskopie)
@@ -148,7 +148,7 @@
     intr->cookie = NULL;
     intr->hits = 0;
     intr->flags = flags;
-    xnobject_copy_name(intr->name,name);
+    intr->name = name;
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
     intr->next = NULL;
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

[Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Dmitry Adamushko]

[-- Attachment #1: Type: text/plain, Size: 2542 bytes --]

On 16/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:

Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this
> before merging (i.e. make XN_ISR_HANDLED non-zero)?


Ok, XN_ISR_NOINT is removed in favor of XN_ISR_HANDLED which is now
non-zero.
Actually, at first I wanted to make it just the other way about.


Moreover, I attached an add-on patch to overcome the name buffer in
> xnintr_t. Note that this patch is only compile-tested, I have no native
> interrupt test-case at hand.



Heh.. someone once suggested the -p key with "diff" to me? :o)

I bet your patch would even work but it's, well, slightly broken indeed ;-)

Look at the native/syscall.c : __rt_intr_delete() :

RT_INTR *intr = (RT_INTR *)rt_registry_fetch(ph.opaque);
...
xnfree(intr);

"intr" actually points to intr_buf->intr and it works as expected only
because "RT_INTR intr" is the first member of the struct, so that it's
address == the address of the whole object of this struct.

To do it properly you would need to make your "unnamed" struct visible
for both __rt_intr_create() and __rt_intr_delete(). Something like :

struct RT_INTR_ENV {

#define link2intr_env(laddr) \
((RT_INTR_ENV *)(((char* )laddr) - (int)(&((RT_INTR_ENV *)0)->intr)))

    RT_INTR intr;
    char name[XNOBJECT_NAME_LEN];
};

and then make use of link2intr_env() in __rt_intr_delet().


Ok, with you approach we would avoid allocating memory
for name[XNOBJECT_NAME_LEN] when an object is created in kernel-mode
and do it via RT_INTR_ENV only when it's created in user-mode.

We could even use that approach for all objects in native skin, but we can't
because of "anonymous" objects supported in kernel-mode too and, well, I
personally would not like those RT_OBJECT_ENV structures all over the map in
syscall.c :)

So I don't want to break the whole picture only for the RT_INTR object (it
doesn't support "anonymous" objects now).

Nevertheless, if you still want to save a hundred bytes of data or so (how
many interrupt objects would a normal system have? and I guess the lenght of
name is about 10 symbols on average :)

for the users that don't use native skin, then I would suggest the following
way :

xnintr_t contains just the link "char *name" as you suggested but RT_INTR
contains the name[XNOBJECT_NAME_LEN] member (as it was before actually).

If it's ok,  then I'll send um... yet another final patch that addresses
both issues :)


Jan
>
>
>

--
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 3540 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4808 bytes --]

Dmitry Adamushko wrote:
> On 16/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> 
> Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this
>> before merging (i.e. make XN_ISR_HANDLED non-zero)?
> 
> 
> Ok, XN_ISR_NOINT is removed in favor of XN_ISR_HANDLED which is now
> non-zero.
> Actually, at first I wanted to make it just the other way about.
> 

Hmm, thinking about this again, I would like to revert my suggestion in
favour of a third approach (trying to keep you busy ;) ).

Recommended default values:
<new>XN_ISR_HANDLED => <old non-zero>XN_ISR_HANDLED | <old>XN_ISR_ENABLE

<new>XN_ISR_NOINT => <new>XN_ISR_NO_ENABLE


Additional bits:
<new>XN_ISR_NO_ENABLE
 => indicate that <old>XN_ISR_ENABLE is not set
    (use case: handle but delay re-enable)

<new>XN_ISR_PROPAGATE
 => <old>XN_ISR_CHAINED (the new name is closer to the related function
                         calls - at least for me).


Not expressible combination:
<old>XN_ISR_ENABLE | <new>XN_ISR_NOINT
 => Doesn't make sense, does it?


Still expressible invalid combination:
<new>XN_ISR_HANDLED | <new>XN_ISR_PROPAGATE
 => The implicit enable request should be easy to ignore at nucleus
    level.


Rational:

Most users will either indicate that an IRQ was handled and should be
re-enabled as soon as possible or that it was unhandled and should
better remain masked if no one else cares.

XN_ISR_PROPAGATE (or related skin defines) are only useful for very very
special corner-case drivers and applications. Normal generic drivers
should not touch this (including my own xeno_16550A... bah!). BTW, if
there is some unintentional IRQ sharing so that spurious RT interrupts
occur, I think we should detect this at nucleus level and bail out.
Otherwise, systems quickly lock up (I just faced this once again two
days ago on an old RTAI box).


This will break some stuff (RTDM e.g., but we have some version flags
for such cases), but it takes us closer to the standard Linux API
without loosing control over the details when required.

Comments?

> 
> Moreover, I attached an add-on patch to overcome the name buffer in
>> xnintr_t. Note that this patch is only compile-tested, I have no native
>> interrupt test-case at hand.
> 
> 
> 
> Heh.. someone once suggested the -p key with "diff" to me? :o)

Mmpf. Must have been too late, and I was too impressed how interdiff
worked out this patch. :)

> 
> I bet your patch would even work but it's, well, slightly broken indeed ;-)
> 
> Look at the native/syscall.c : __rt_intr_delete() :
> 
> RT_INTR *intr = (RT_INTR *)rt_registry_fetch(ph.opaque);
> ...
> xnfree(intr);
> 
> "intr" actually points to intr_buf->intr and it works as expected only
> because "RT_INTR intr" is the first member of the struct, so that it's
> address == the address of the whole object of this struct.

But this is not an unusual trick, and I see no practical problems.

> 
> To do it properly you would need to make your "unnamed" struct visible
> for both __rt_intr_create() and __rt_intr_delete(). Something like :
> 
> struct RT_INTR_ENV {
> 
> #define link2intr_env(laddr) \
> ((RT_INTR_ENV *)(((char* )laddr) - (int)(&((RT_INTR_ENV *)0)->intr)))
> 
>     RT_INTR intr;
>     char name[XNOBJECT_NAME_LEN];
> };
> 
> and then make use of link2intr_env() in __rt_intr_delet().
> 
> 
> Ok, with you approach we would avoid allocating memory
> for name[XNOBJECT_NAME_LEN] when an object is created in kernel-mode
> and do it via RT_INTR_ENV only when it's created in user-mode.
> 
> We could even use that approach for all objects in native skin, but we can't
> because of "anonymous" objects supported in kernel-mode too and, well, I
> personally would not like those RT_OBJECT_ENV structures all over the map in
> syscall.c :)

Me too.

> 
> So I don't want to break the whole picture only for the RT_INTR object (it
> doesn't support "anonymous" objects now).
> 
> Nevertheless, if you still want to save a hundred bytes of data or so (how
> many interrupt objects would a normal system have? and I guess the lenght of
> name is about 10 symbols on average :)

RT_INTR is now disabled by default in the kernel config. Thus most users
should not see its code in their kernels, only the additional data and
code related to xnintr_t.name.

> 
> for the users that don't use native skin, then I would suggest the following
> way :
> 
> xnintr_t contains just the link "char *name" as you suggested but RT_INTR
> contains the name[XNOBJECT_NAME_LEN] member (as it was before actually).
> 
> If it's ok,  then I'll send um... yet another final patch that addresses
> both issues :)

I'm fine with this as well if you prefer it, no problem.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Dmitry Adamushko]

[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

On 16/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
> Dmitry Adamushko wrote:
> > On 16/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> >
> > Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this
> >> before merging (i.e. make XN_ISR_HANDLED non-zero)?
> >
> >
> > Ok, XN_ISR_NOINT is removed in favor of XN_ISR_HANDLED which is now
> > non-zero.
> > Actually, at first I wanted to make it just the other way about.
> >
>
> Hmm, thinking about this again, I would like to revert my suggestion in
> favour of a third approach (trying to keep you busy ;) ).


Ok, you are wellcome :)

I didn't get it, at least while looking briefly. To make it a bit easier, at
least for me, let's go another way.

At the left is the list of allowed values as Philippe pointed out.
At the right is another list which corresponds to the left one but when
NOINT is used instead of HANDLED. Moreover, I have added another case - pure
NOINT. The ISR says it's not mine and, well, it doesn't know whether it
should be propagated or no
(ok, so far CHAINED standed for NOINT).

HANDLED                     ->  0
HANDLED | ENABLE     ->  ENABLE
HANDLED | CHAINED    ->  CHAINED
CHAINED                      ->  CHAINED | NOINT
                                    ->   NOINT

Could you provide the 3-d corresponding table using your new flags?


> xnintr_t contains just the link "char *name" as you suggested but RT_INTR
> > contains the name[XNOBJECT_NAME_LEN] member (as it was before actually).
> >
> > If it's ok,  then I'll send um... yet another final patch that addresses
> > both issues :)
>
> I'm fine with this as well if you prefer it, no problem.


Yep, let's go this way.


Jan
>
>
>

--
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 3073 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

Dmitry Adamushko wrote:
> On 16/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>> Dmitry Adamushko wrote:
>>> On 16/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>>
>>> Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this
>>>> before merging (i.e. make XN_ISR_HANDLED non-zero)?
>>>
>>> Ok, XN_ISR_NOINT is removed in favor of XN_ISR_HANDLED which is now
>>> non-zero.
>>> Actually, at first I wanted to make it just the other way about.
>>>
>> Hmm, thinking about this again, I would like to revert my suggestion in
>> favour of a third approach (trying to keep you busy ;) ).
> 
> 
> Ok, you are wellcome :)
> 
> I didn't get it, at least while looking briefly. To make it a bit easier, at
> least for me, let's go another way.
> 
> At the left is the list of allowed values as Philippe pointed out.
> At the right is another list which corresponds to the left one but when
> NOINT is used instead of HANDLED. Moreover, I have added another case - pure
> NOINT. The ISR says it's not mine and, well, it doesn't know whether it
> should be propagated or no
> (ok, so far CHAINED standed for NOINT).
> 
> 1.) HANDLED                     ->  0
> 2.) HANDLED | ENABLE     ->  ENABLE
> 3.) HANDLED | CHAINED    ->  CHAINED
> 4.) CHAINED                      ->  CHAINED | NOINT
> 5.)                                    ->   NOINT
> 
> Could you provide the 3-d corresponding table using your new flags?
> 

Still not 3D, but hopefully clarifying: :)

1.) XN_ISR_HANDLED |XN_ISR_NO_ENABLE
2.) XN_ISR_HANDLED
3.) XN_ISR_HANDLED | XN_ISR_PROPAGATE
    (nucleus ignores implicit IRQ enable)
4.) XN_ISR_NOINT | XN_ISR_PROPAGATE
5.) XN_ISR_NOINT

2.) and 5.) are most common, the others for special scenarios.
Especially, as long as we have no API for ending IRQs outside the
handler, 1.) is of limited usage I think.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Dmitry Adamushko]

[-- Attachment #1: Type: text/plain, Size: 4287 bytes --]

On 16/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
> Dmitry Adamushko wrote:
> > On 16/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> >> Dmitry Adamushko wrote:
> >>> On 16/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> >>>
> >>> Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this
> >>>> before merging (i.e. make XN_ISR_HANDLED non-zero)?
> >>>
> >>> Ok, XN_ISR_NOINT is removed in favor of XN_ISR_HANDLED which is now
> >>> non-zero.
> >>> Actually, at first I wanted to make it just the other way about.
> >>>
> >> Hmm, thinking about this again, I would like to revert my suggestion in
> >> favour of a third approach (trying to keep you busy ;) ).
> >
> >
> > Ok, you are wellcome :)
> >
> > I didn't get it, at least while looking briefly. To make it a bit
> easier, at
> > least for me, let's go another way.
> >
> > At the left is the list of allowed values as Philippe pointed out.
> > At the right is another list which corresponds to the left one but when
> > NOINT is used instead of HANDLED. Moreover, I have added another case -
> pure
> > NOINT. The ISR says it's not mine and, well, it doesn't know whether it
> > should be propagated or no
> > (ok, so far CHAINED standed for NOINT).
> >
> > 1.) HANDLED                     ->  0
> > 2.) HANDLED | ENABLE     ->  ENABLE
> > 3.) HANDLED | CHAINED    ->  CHAINED
> > 4.) CHAINED                      ->  CHAINED | NOINT
> > 5.)                                    ->   NOINT
> >
> > Could you provide the 3-d corresponding table using your new flags?
> >
>
> Still not 3D, but hopefully clarifying: :)



1.) XN_ISR_HANDLED |XN_ISR_NO_ENABLE
> 2.) XN_ISR_HANDLED
> 3.) XN_ISR_HANDLED | XN_ISR_PROPAGATE
>     (nucleus ignores implicit IRQ enable)
> 4.) XN_ISR_NOINT | XN_ISR_PROPAGATE
> 5.) XN_ISR_NOINT


Then why do we need XN_ISR_NOINT? is it not just the same as ~HANDLED?

Moreover, I tend to think now that PROPAGATE (CHAINED) should not be
normally used by ISRs at all. The only exception would be irq sharing
between RT and non-RT domains, but that's of no concern for most part of rt
drivers and, as last events revealed, it's very partially supported at the
moment and possible implementations are quite arguable.

So HANDLED means the irq line is .end-ed by xnintr_irq_handler()
(xnarch_end_irq(irq)  is called).

If one wants to do it on his own later, NOENABLE can be returned
additionally.
But then, one must use xnintr_end_irq() and not just xnintr_irq_enable().

If xnintr_end_irq() would be always the same as xnintr_irq_enable() or we
would decouple ending and enabling operations on the line (don't have
sources at hand); we could go Linux way and make use of a "nested" counter
in xnintr_t, so that xnintr_irq_disable() could be called in a nested way.
This said, calling xnintr_irq_disable() in ISR leads to the interrupt line
being still disabled upon return from xnintr_irq_handler() and re-enabled
again by the user later.

~HANDLED means the ISR was not concerned about this interrupt. i.e. :

- there is ISR in another domain (down the pipeline) which is interested in
this irq ;

     well, the designer of the system should be aware of such cases and
probably avoid that on hw layer or, at least, to know what she is doing.

- this is a spurious irq.

So upon getting ~HANDLED, xnintr_irq_handler() does :

success = xnintr_irq_chained(irq);

if (success)
{
/* i.e. another domain is really interested and got this irq as pending*/

return without .end-ing
}
else
{
/* this is a spurious irq */

make use of some accounting of spurious interrupts (if we need it)), i.e.
keep the irq line disabled if the % of unhandled requests > some limit.

otherwise

xnintr_end_irq() and return
}

too long :) ok, it's quite similar indeed to what you have suggested,
with the exception that

- no need for NOINT (if it's really == ~HANDLED);
- nucleus provides some additional logic while handling ~HANDLED case.
- user don't need to return ENABLE explicitly - in most cases, that's what
he actually needs but sometimes just forgets (I recall a few questions on
the mailing list about "only one interrupt".


Jan
>
>
--
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 5917 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5251 bytes --]

Dmitry Adamushko wrote:
> On 16/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>> Dmitry Adamushko wrote:
>>> On 16/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>>> Dmitry Adamushko wrote:
>>>>> On 16/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>>>>
>>>>> Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this
>>>>>> before merging (i.e. make XN_ISR_HANDLED non-zero)?
>>>>> Ok, XN_ISR_NOINT is removed in favor of XN_ISR_HANDLED which is now
>>>>> non-zero.
>>>>> Actually, at first I wanted to make it just the other way about.
>>>>>
>>>> Hmm, thinking about this again, I would like to revert my suggestion in
>>>> favour of a third approach (trying to keep you busy ;) ).
>>>
>>> Ok, you are wellcome :)
>>>
>>> I didn't get it, at least while looking briefly. To make it a bit
>> easier, at
>>> least for me, let's go another way.
>>>
>>> At the left is the list of allowed values as Philippe pointed out.
>>> At the right is another list which corresponds to the left one but when
>>> NOINT is used instead of HANDLED. Moreover, I have added another case -
>> pure
>>> NOINT. The ISR says it's not mine and, well, it doesn't know whether it
>>> should be propagated or no
>>> (ok, so far CHAINED standed for NOINT).
>>>
>>> 1.) HANDLED                     ->  0
>>> 2.) HANDLED | ENABLE     ->  ENABLE
>>> 3.) HANDLED | CHAINED    ->  CHAINED
>>> 4.) CHAINED                      ->  CHAINED | NOINT
>>> 5.)                                    ->   NOINT
>>>
>>> Could you provide the 3-d corresponding table using your new flags?
>>>
>> Still not 3D, but hopefully clarifying: :)
> 
> 
> 
> 1.) XN_ISR_HANDLED |XN_ISR_NO_ENABLE
>> 2.) XN_ISR_HANDLED
>> 3.) XN_ISR_HANDLED | XN_ISR_PROPAGATE
>>     (nucleus ignores implicit IRQ enable)
>> 4.) XN_ISR_NOINT | XN_ISR_PROPAGATE
>> 5.) XN_ISR_NOINT
> 
> 
> Then why do we need XN_ISR_NOINT? is it not just the same as ~HANDLED?

The idea is to have at least a define so that

a) users explicitly have to state what they mean
b) we can map the define to some other value whenever we may need this
in the future

The first property could even be used to apply some debug checks in the
user's return value.

> 
> Moreover, I tend to think now that PROPAGATE (CHAINED) should not be
> normally used by ISRs at all. The only exception would be irq sharing
> between RT and non-RT domains, but that's of no concern for most part of rt
> drivers and, as last events revealed, it's very partially supported at the
> moment and possible implementations are quite arguable.

Ack. But you should still export this feature for those who think they
know what they are doing. ;)

> 
> So HANDLED means the irq line is .end-ed by xnintr_irq_handler()
> (xnarch_end_irq(irq)  is called).
> 
> If one wants to do it on his own later, NOENABLE can be returned
> additionally.

Yep. So let's call it XN_ISR_NOENABLE.

> But then, one must use xnintr_end_irq() and not just xnintr_irq_enable().
> 

Yep, but a different (add-on) topic.

> If xnintr_end_irq() would be always the same as xnintr_irq_enable() or we
> would decouple ending and enabling operations on the line (don't have
> sources at hand); we could go Linux way and make use of a "nested" counter
> in xnintr_t, so that xnintr_irq_disable() could be called in a nested way.
> This said, calling xnintr_irq_disable() in ISR leads to the interrupt line
> being still disabled upon return from xnintr_irq_handler() and re-enabled
> again by the user later.
> 
> ~HANDLED means the ISR was not concerned about this interrupt. i.e. :
> 
> - there is ISR in another domain (down the pipeline) which is interested in
> this irq ;
> 
>      well, the designer of the system should be aware of such cases and
> probably avoid that on hw layer or, at least, to know what she is doing.
> 
> - this is a spurious irq.
> 
> So upon getting ~HANDLED, xnintr_irq_handler() does :
> 
> success = xnintr_irq_chained(irq);

Only if requested via XN_ISR_PROPAGATE (or _CHAINED)! There is still a
real-time ISR registered on that IRQ you are about to forward now, and
you don't know if someone is handling it down the pipeline. Actually,
this is a fatal situation. The system is broken, bail out, and switch
this line off.

> 
> if (success)
> {
> /* i.e. another domain is really interested and got this irq as pending*/
> 
> return without .end-ing
> }
> else
> {
> /* this is a spurious irq */
> 
> make use of some accounting of spurious interrupts (if we need it)), i.e.
> keep the irq line disabled if the % of unhandled requests > some limit.
> 
> otherwise
> 
> xnintr_end_irq() and return
> }
> 
> too long :) ok, it's quite similar indeed to what you have suggested,
> with the exception that
> 
> - no need for NOINT (if it's really == ~HANDLED);
> - nucleus provides some additional logic while handling ~HANDLED case.
> - user don't need to return ENABLE explicitly - in most cases, that's what
> he actually needs but sometimes just forgets (I recall a few questions on
> the mailing list about "only one interrupt".
> 
> 
> Jan
>>
> --
> Best regards,
> Dmitry Adamushko
> 

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Dmitry Adamushko]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

Hi Jan,

let's make yet another revision of the bits :

new XN_ISR_HANDLED  == old XN_ISR_HANDLED + old XN_ISR_NO_ENABLE

ok.

new XN_ISR_NOENABLE == ~ old XN_ISR_ENABLE

ok.

new XN_ISR_PROPAGATE == XN_ISR_CHAINED

ok.

new XN_ISR_NOINT == ?

does it suppose the interrupt line to be .end-ed (enabled) and irq not to be
propagated? Should be so, I guess, if it's different from 5). Then nucleus
ignores implicit IRQ enable for 5) as well as for 3).

Do we really need that NOINT then, as it seems to be the same as ~HANDLED?

or NOINT == 0 and then it's a scalar value, not a bit.

So one may consider HANDLED == 1 and NOINT == 0 as really scalar values

and

NOENABLE and PROPAGATE as additional bits (used only if needed).


--
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 952 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]

Hi Dmitry,

Dmitry Adamushko wrote:
> Hi Jan,
> 
> let's make yet another revision of the bits :
> 
> new XN_ISR_HANDLED  == old XN_ISR_HANDLED + old XN_ISR_NO_ENABLE
> 
> ok.
> 
> new XN_ISR_NOENABLE == ~ old XN_ISR_ENABLE
> 
> ok.
> 
> new XN_ISR_PROPAGATE == XN_ISR_CHAINED
> 
> ok.
> 

Just to make sure that you understand my weird ideas: each of the three
new XN_ISR_xxx above should be encoded with an individual bit

> new XN_ISR_NOINT == ?
> 
> does it suppose the interrupt line to be .end-ed (enabled) and irq not to be
> propagated? Should be so, I guess, if it's different from 5). Then nucleus
> ignores implicit IRQ enable for 5) as well as for 3).
> 
> Do we really need that NOINT then, as it seems to be the same as ~HANDLED?
> 
> or NOINT == 0 and then it's a scalar value, not a bit.
> 
> So one may consider HANDLED == 1 and NOINT == 0 as really scalar values
> 
> and
> 
> NOENABLE and PROPAGATE as additional bits (used only if needed).
> 

My idea is to urge the user specifying one of the base return types
(HANDLED or NOINT) + any of the two additional bits (NOENABLE and
PROPAGATE).

For correct drivers NOINT could be 0 indeed, but to check that the user
picked a new constant we may want to set NOINT != 0. With the old API
"return 0" expressed HANDLED + ~ENABLE for the old API. With the new one
the user signals no interest and the nucleus may raise a warning that a
spurious IRQ occurred. So I would add a debug bit for NOINT here to
optionally (on OPT_XENO_DEBUG) detect old-style usage (return 0).
Moreover, we gain freedom to move bits in the future when every state is
encoded via constants. Or am I too paranoid here?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Anders Blomdell]

Jan Kiszka wrote:
> Hi Dmitry,
> 
> Dmitry Adamushko wrote:
> 
>>Hi Jan,
>>
>>let's make yet another revision of the bits :
>>
>>new XN_ISR_HANDLED  == old XN_ISR_HANDLED + old XN_ISR_NO_ENABLE
>>
>>ok.
>>
>>new XN_ISR_NOENABLE == ~ old XN_ISR_ENABLE
>>
>>ok.
>>
>>new XN_ISR_PROPAGATE == XN_ISR_CHAINED
>>
>>ok.
>>
> 
> 
> Just to make sure that you understand my weird ideas: each of the three
> new XN_ISR_xxx above should be encoded with an individual bit
> 
> 
>>new XN_ISR_NOINT == ?
>>
>>does it suppose the interrupt line to be .end-ed (enabled) and irq not to be
>>propagated? Should be so, I guess, if it's different from 5). Then nucleus
>>ignores implicit IRQ enable for 5) as well as for 3).
>>
>>Do we really need that NOINT then, as it seems to be the same as ~HANDLED?
>>
>>or NOINT == 0 and then it's a scalar value, not a bit.
>>
>>So one may consider HANDLED == 1 and NOINT == 0 as really scalar values
>>
>>and
>>
>>NOENABLE and PROPAGATE as additional bits (used only if needed).
>>
> 
> 
> My idea is to urge the user specifying one of the base return types
> (HANDLED or NOINT) + any of the two additional bits (NOENABLE and
> PROPAGATE).
> 
> For correct drivers NOINT could be 0 indeed, but to check that the user
> picked a new constant we may want to set NOINT != 0. With the old API
> "return 0" expressed HANDLED + ~ENABLE for the old API. With the new one
> the user signals no interest and the nucleus may raise a warning that a
> spurious IRQ occurred. So I would add a debug bit for NOINT here to
> optionally (on OPT_XENO_DEBUG) detect old-style usage (return 0).
> Moreover, we gain freedom to move bits in the future when every state is
> encoded via constants. Or am I too paranoid here?
After reading the above discussion (of which I understand very little), and 
looking at (what I believe to be) the relevant code:

+    intr = shirq->handlers;
+
+    while (intr)
+        {
+        s |= intr->isr(intr);
+        ++intr->hits;
+        intr = intr->next;
+        }
+    xnintr_shirq_unlock(shirq);
+
+    --sched->inesting;
+
+    if (s & XN_ISR_ENABLE)
+       xnarch_end_irq(irq);
+    else if (s & XN_ISR_CHAINED)
+       xnarch_chain_irq(irq);

A number of questions arise:

1. What happens if one of the shared handlers leaves the interrupt asserted, 
returns NOENABLE|HANDLED and another return only HANDLED?

2. What happens if one returns PROPAGATE and another returns HANDLED?

As far as I can tell, after all RT handlers havve run, the following scenarios 
are possible:

1. The interrupt is deasserted (i.e. it was a RT interrupt)
2. The interrupt is still asserted, it will be deasserted later
    by some RT task (i.e. it was a RT interrupt)
3. The interrupt is still asserted and will be deasserted
    by the Linux IRQ handler.

IMHO that leads to the conclusion that the IRQ handlers should return a scalar

#define UNHANDLED 0
#define HANDLED_ENABLE 1
#define HANDLED_NOENABLE 2
#define PROPAGATE 3

and the loop should be

     s = UNHANDLED
     while (intr) {
       tmp = intr->isr(intr);
       if (tmp > s) { s = tmp; }
       intr = intr->next;
     }
     if (s == PROPAGATE) {
       xnarch_chain_irq(irq);
     } else if (s == HANDLED_ENABLE) {
       xnarch_end_irq(irq);
     }

To be really honest, I think that PROPAGATE should be excluded from the RT 
IRQ-handlers, since with the current scheme all RT-handlers has to test if the 
IRQ was a Linux interrupt (otherwise the system will only work when the handler 
that returns PROPAGATE is installed)

-- 

Anders

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Dmitry Adamushko]

[-- Attachment #1: Type: text/plain, Size: 3186 bytes --]

N.B. Amongst other things, some thoughts about CHAINED with shared
interrupts.


On 20/02/06, Anders Blomdell <anders.blomdell@domain.hid> wrote:



> A number of questions arise:
>
> 1. What happens if one of the shared handlers leaves the interrupt
> asserted,
> returns NOENABLE|HANDLED and another return only HANDLED?
>
> 2. What happens if one returns PROPAGATE and another returns HANDLED?


Yep, each ISR may return a different value and all of them are
accumulated in the "s" variable ( s |= intr->isr(intr); ).

So the loop may end up with "s" which contains all of the possible bits:

(e.g.

isr1 - HANDLED | ENABLE
isr2 - HANDLED (don't want the irq to be enabled)
isr3 - CHAINED

)

s = HANDLED | ENABLE | CHAINED;

Then CHAINED will be ignored because of the following code :

+    if (s & XN_ISR_ENABLE)
+       xnarch_end_irq(irq);
+    else if (s & XN_ISR_CHAINED)    (*)
+       xnarch_chain_irq(irq);

the current code in the CVS doen not contain "else" in (*), so that ENABLE |
CHAINED is possible, though it's a wrong combination.

This said, we suppose that one knows what he is doing.

In the case of a single ISR per line, it's not that difficult to achieve.
But if there are a few ISRs, then one should analize and take into account
all possible return values of all the ISRs, as each of them may affect
others (e.g. if one returns CHAINED when another - HANDLED | ENABLE).

So my feeling is that CHAINED should not be used by drivers which registered
their ISRs as SHARED.

Moreover, I actually see the only scenario of CHAINED (I provided it before)
:

all ISRs in the primary domain have reported UNHANDLED  =>  nucleus
propagates the interrupt down the pipeline with xnacrh_chain_irq().
This call actually returns 1 upon successful propagation (some domain down
the pipeline was interested in this irq) and 0 otherwise.

Upon 0, this is a spurious irq (none of domains was interested in its
handling).

ok, let's suppose now :

we have 2 ISRs on the same shared line :

isr1 : HANDLED (will be enabled by rt task. Note, rt task must call
xnarch_end_irq() and not just xnarch_enable_irq()! )

isr2 : CHAINED

So HANDLED | CHAINED is ok for the single ISR on the line, but it may lead
to HANDLED | CHAINED | ENABLE in a case of the shared line.

rt task that works jointly with isr1 just calls xnarch_end_irq() at some
moment of time and some ISR in the linux domain does the same later  =>  the
line is .end-ed 2 times.

ISR should never return CHAINED as to indicate _only_ that it is not
interested in this irq, but ~HANDLED or NOINT (if we'll support it) instead.

If the ISR nevertheless wants to propagate the IRQ to the Linux domain
_explicitly_, it _must not_ register itself as SHARED, i.e. it _must_ be the
only ISR on this line, otherwise that may lead to the IRQ line being .end-ed
twice (lost interrupts in some cases).


#define UNHANDLED 0
> #define HANDLED_ENABLE 1
> #define HANDLED_NOENABLE 2
> #define PROPAGATE 3


Yep, I'd agree with you. Moreover, PROPAGATE should not be used for shared
interrupts.


--
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 4072 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4044 bytes --]

Dmitry Adamushko wrote:
> N.B. Amongst other things, some thoughts about CHAINED with shared
> interrupts.
> 
> 
> On 20/02/06, Anders Blomdell <anders.blomdell@domain.hid> wrote:
> 
> 
> 
>> A number of questions arise:
>>
>> 1. What happens if one of the shared handlers leaves the interrupt
>> asserted,
>> returns NOENABLE|HANDLED and another return only HANDLED?
>>
>> 2. What happens if one returns PROPAGATE and another returns HANDLED?
> 
> 
> Yep, each ISR may return a different value and all of them are
> accumulated in the "s" variable ( s |= intr->isr(intr); ).
> 
> So the loop may end up with "s" which contains all of the possible bits:
> 
> (e.g.
> 
> isr1 - HANDLED | ENABLE
> isr2 - HANDLED (don't want the irq to be enabled)
> isr3 - CHAINED
> 
> )
> 
> s = HANDLED | ENABLE | CHAINED;
> 
> Then CHAINED will be ignored because of the following code :
> 
> +    if (s & XN_ISR_ENABLE)
> +       xnarch_end_irq(irq);
> +    else if (s & XN_ISR_CHAINED)    (*)
> +       xnarch_chain_irq(irq);

Which may actually be fatal: consider one ISR intentionally not handling
an IRQ but propagating it while another ISR thinks it has to re-enable
it - bang! Anders' as well as my original suggestion were to ignore the
ENABLE in this case.

> 
> the current code in the CVS doen not contain "else" in (*), so that ENABLE |
> CHAINED is possible, though it's a wrong combination.
> 
> This said, we suppose that one knows what he is doing.
> 
> In the case of a single ISR per line, it's not that difficult to achieve.
> But if there are a few ISRs, then one should analize and take into account
> all possible return values of all the ISRs, as each of them may affect
> others (e.g. if one returns CHAINED when another - HANDLED | ENABLE).
> 
> So my feeling is that CHAINED should not be used by drivers which registered
> their ISRs as SHARED.

That's true for standard drivers, but we should not prevent special
corner-case designs which can be tuned to make even shared RT-IRQs +
propagation possible. Sharing does not necessarily mean that different
RT drivers are involved...

> 
> Moreover, I actually see the only scenario of CHAINED (I provided it before)
> :
> 
> all ISRs in the primary domain have reported UNHANDLED  =>  nucleus
> propagates the interrupt down the pipeline with xnacrh_chain_irq().
> This call actually returns 1 upon successful propagation (some domain down
> the pipeline was interested in this irq) and 0 otherwise.

But this only means that some handler is registered down there, it
cannot predict if that handler will actually care about the event.

> 
> Upon 0, this is a spurious irq (none of domains was interested in its
> handling).
> 
> ok, let's suppose now :
> 
> we have 2 ISRs on the same shared line :
> 
> isr1 : HANDLED (will be enabled by rt task. Note, rt task must call
> xnarch_end_irq() and not just xnarch_enable_irq()! )
> 
> isr2 : CHAINED
> 
> So HANDLED | CHAINED is ok for the single ISR on the line, but it may lead
> to HANDLED | CHAINED | ENABLE in a case of the shared line.
> 
> rt task that works jointly with isr1 just calls xnarch_end_irq() at some
> moment of time and some ISR in the linux domain does the same later  =>  the
> line is .end-ed 2 times.
> 
> ISR should never return CHAINED as to indicate _only_ that it is not
> interested in this irq, but ~HANDLED or NOINT (if we'll support it) instead.
> 
> If the ISR nevertheless wants to propagate the IRQ to the Linux domain
> _explicitly_, it _must not_ register itself as SHARED, i.e. it _must_ be the
> only ISR on this line, otherwise that may lead to the IRQ line being .end-ed
> twice (lost interrupts in some cases).
> 
> 
> #define UNHANDLED 0
>> #define HANDLED_ENABLE 1
>> #define HANDLED_NOENABLE 2
>> #define PROPAGATE 3
> 
> 
> Yep, I'd agree with you. Moreover, PROPAGATE should not be used for shared
> interrupts.
> 
> 
> --
> Best regards,
> Dmitry Adamushko
> 

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Dmitry Adamushko]

[-- Attachment #1: Type: text/plain, Size: 4604 bytes --]

On 21/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
> Dmitry Adamushko wrote:
> > N.B. Amongst other things, some thoughts about CHAINED with shared
> > interrupts.
> >
> >
> > On 20/02/06, Anders Blomdell <anders.blomdell@domain.hid> wrote:
> >
> >
> >
> >> A number of questions arise:
> >>
> >> 1. What happens if one of the shared handlers leaves the interrupt
> >> asserted,
> >> returns NOENABLE|HANDLED and another return only HANDLED?
> >>
> >> 2. What happens if one returns PROPAGATE and another returns HANDLED?
> >
> >
> > Yep, each ISR may return a different value and all of them are
> > accumulated in the "s" variable ( s |= intr->isr(intr); ).
> >
> > So the loop may end up with "s" which contains all of the possible bits:
> >
> > (e.g.
> >
> > isr1 - HANDLED | ENABLE
> > isr2 - HANDLED (don't want the irq to be enabled)
> > isr3 - CHAINED
> >
> > )
> >
> > s = HANDLED | ENABLE | CHAINED;
> >
> > Then CHAINED will be ignored because of the following code :
> >
> > +    if (s & XN_ISR_ENABLE)
> > +       xnarch_end_irq(irq);
> > +    else if (s & XN_ISR_CHAINED)    (*)
> > +       xnarch_chain_irq(irq);
>
> Which may actually be fatal: consider one ISR intentionally not handling
> an IRQ but propagating it while another ISR thinks it has to re-enable
> it - bang! Anders' as well as my original suggestion were to ignore the
> ENABLE in this case.
>
> >
> > the current code in the CVS doen not contain "else" in (*), so that
> ENABLE |
> > CHAINED is possible, though it's a wrong combination.
> >
> > This said, we suppose that one knows what he is doing.
> >
> > In the case of a single ISR per line, it's not that difficult to
> achieve.
> > But if there are a few ISRs, then one should analize and take into
> account
> > all possible return values of all the ISRs, as each of them may affect
> > others (e.g. if one returns CHAINED when another - HANDLED | ENABLE).
> >
> > So my feeling is that CHAINED should not be used by drivers which
> registered
> > their ISRs as SHARED.
>
> That's true for standard drivers, but we should not prevent special
> corner-case designs which can be tuned to make even shared RT-IRQs +
> propagation possible. Sharing does not necessarily mean that different
> RT drivers are involved...


Yep.  But in this case the designer of the system should _care_ about all
corners of the system, i.e. to make sure that all ISRs return values which
are not contradictory to each other.

e.g.

1)
isr1 : HANDLED (keep disabled)
isr2 : CHAINED

or

2)
isr1 : HANDLED | NOENABLE (will be enabled by rt_task1)
isr2 : HANDLED | NOENABLE (-//- by rt_task2)

1) and 2) are wrong! Both lead to the IRQ line being .end-ed
(xnarch_end_irq()) twice! And on some archs, as we have seen before, that
may lead to lost interrupts.

Of course, when designing the real-time system, one should take charge of
all corners of the system, so that analyzing the possible return values of
all other ISRs that may share the same IRQ line is not anything extravagant,
but a requirement.

This said, I see no reason then why we should prevent users
from some cases (like CHAINED | ENABLE) and let him do the ones like 2).
2) is much more common scenario for irq sharing and looks sane  from the
point of view of a separate ISR, when the global picture (and nucleus
innards) is not taken into account.

p.s.
my 2) is equal to 2 ISRs returning HANDLED_NOENABLE in terms of Anders. This
case is one of the most common with shared interrupts which I would imagine.
And it nevertheless leads to problems when the whole picture is not taken
into account by the designer of a given ISR.


>
> Moreover, I actually see the only scenario of CHAINED (I provided it
before)
> :
>
> all ISRs in the primary domain have reported UNHANDLED  =>  nucleus
> propagates the interrupt down the pipeline with xnacrh_chain_irq().
> This call actually returns 1 upon successful propagation (some domain down
> the pipeline was interested in this irq) and 0 otherwise.

>But this only means that some handler is registered down there,
> it
>cannot predict if that handler will actually care about the event.

Linux (not only vanilla, but ipipe-aware too) always .end-s (calls
desc->handler->end(irq)) on return from __do_IRQ(), no matter this interrupt
was handled by some ISR or not. It also re-enables the IRQ line if it was
not disabled by some ISR explicitly.
We don't care about anything else.



> Jan
>
>
>

--
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 5764 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Dmitry Adamushko]

[-- Attachment #1: Type: text/plain, Size: 2811 bytes --]

On 21/02/06, Anders Blomdell <anders.blomdell@domain.hid> wrote:
>
> Dmitry Adamushko wrote:
> >
> > N.B. Amongst other things, some thoughts about CHAINED with shared
> > interrupts.
> >
> >
> > On 20/02/06, *Anders Blomdell* <anders.blomdell@domain.hid
> > <mailto:anders.blomdell@domain.hid>> wrote:
> >
> >
> >
> >     A number of questions arise:
> >
> >     1. What happens if one of the shared handlers leaves the interrupt
> >     asserted,
> >     returns NOENABLE|HANDLED and another return only HANDLED?
> >
> >     2. What happens if one returns PROPAGATE and another returns
> HANDLED?
> >
> >
> > Yep, each ISR may return a different value and all of them are
> > accumulated in the "s" variable ( s |= intr->isr(intr); ).
> >
> > So the loop may end up with "s" which contains all of the possible bits:
> >
> > (e.g.
> >
> > isr1 - HANDLED | ENABLE
> > isr2 - HANDLED (don't want the irq to be enabled)
> > isr3 - CHAINED
> >
> > )
> >
> > s = HANDLED | ENABLE | CHAINED;
> >
> > Then CHAINED will be ignored because of the following code :
> >
> > +    if (s & XN_ISR_ENABLE)
> > +       xnarch_end_irq(irq);
> > +    else if (s & XN_ISR_CHAINED)    (*)
> > +       xnarch_chain_irq(irq);
> Which is the worst way possible of prioritizing them, if a Linux interrupt
> is
> active when we get there with ENABLE|CHAINED, interrupts will be enabled
> with
> the Linux interrupt still asserted -> the IRQ-handlers will be called once
> more,
> probably returning ENABLE|CHAINED again -> infinite loop...
>
> >
> > the current code in the CVS doen not contain "else" in (*), so that
> > ENABLE | CHAINED is possible, though it's a wrong combination.
> >
> > This said, we suppose that one knows what he is doing.
> >
> > In the case of a single ISR per line, it's not that difficult to
> > achieve. But if there are a few ISRs, then one should analize and take
> > into account all possible return values of all the ISRs, as each of them
> > may affect others (e.g. if one returns CHAINED when another - HANDLED |
> > ENABLE).
> Which is somewhat contrary to the concept of shared interrupts, if we have
> to
> take care of the global picture, why make them shared in the first place?
> (I like the concept of shared interrupts, but it is important that the
> framework
> gives a separation of concerns)


Unfortunately, it looks to me that the current picture (even with your
scalar values) requires from the user who develops a given IRQ to take into
account the possible return values of other ISRs.

As I pointed out, the situation when 2 ISRs return HANDLED_NOENABLE may lead
to problems on some archs.

---
> ...

I'll take a look at the rest of the message a bit later.


--
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 3687 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Anders Blomdell]

Dmitry Adamushko wrote:
> 
> On 21/02/06, *Anders Blomdell* <anders.blomdell@domain.hid 
> <mailto:anders.blomdell@domain.hid>> wrote:
> 
>     Dmitry Adamushko wrote:
>      >
>      > N.B. Amongst other things, some thoughts about CHAINED with shared
>      > interrupts.
>      >
>      >
>      > On 20/02/06, *Anders Blomdell* < anders.blomdell@domain.hid
>     <mailto:anders.blomdell@domain.hid>
>      > <mailto:anders.blomdell@domain.hid
>     <mailto:anders.blomdell@domain.hid>>> wrote:
>      >
>      >
>      >
>      >     A number of questions arise:
>      >
>      >     1. What happens if one of the shared handlers leaves the
>     interrupt
>      >     asserted,
>      >     returns NOENABLE|HANDLED and another return only HANDLED?
>      >
>      >     2. What happens if one returns PROPAGATE and another returns
>     HANDLED?
>      >
>      >
>      > Yep, each ISR may return a different value and all of them are
>      > accumulated in the "s" variable ( s |= intr->isr(intr); ).
>      >
>      > So the loop may end up with "s" which contains all of the
>     possible bits:
>      >
>      > (e.g.
>      >
>      > isr1 - HANDLED | ENABLE
>      > isr2 - HANDLED (don't want the irq to be enabled)
>      > isr3 - CHAINED
>      >
>      > )
>      >
>      > s = HANDLED | ENABLE | CHAINED;
>      >
>      > Then CHAINED will be ignored because of the following code :
>      >
>      > +    if (s & XN_ISR_ENABLE)
>      > +       xnarch_end_irq(irq);
>      > +    else if (s & XN_ISR_CHAINED)    (*)
>      > +       xnarch_chain_irq(irq);
>     Which is the worst way possible of prioritizing them, if a Linux
>     interrupt is
>     active when we get there with ENABLE|CHAINED, interrupts will be
>     enabled with
>     the Linux interrupt still asserted -> the IRQ-handlers will be
>     called once more,
>     probably returning ENABLE|CHAINED again -> infinite loop...
> 
>      >
>      > the current code in the CVS doen not contain "else" in (*), so that
>      > ENABLE | CHAINED is possible, though it's a wrong combination.
>      >
>      > This said, we suppose that one knows what he is doing.
>      >
>      > In the case of a single ISR per line, it's not that difficult to
>      > achieve. But if there are a few ISRs, then one should analize and
>     take
>      > into account all possible return values of all the ISRs, as each
>     of them
>      > may affect others (e.g. if one returns CHAINED when another -
>     HANDLED |
>      > ENABLE).
>     Which is somewhat contrary to the concept of shared interrupts, if
>     we have to
>     take care of the global picture, why make them shared in the first
>     place?
>     (I like the concept of shared interrupts, but it is important that
>     the framework
>     gives a separation of concerns)
> 
> 
> Unfortunately, it looks to me that the current picture (even with your 
> scalar values) requires from the user who develops a given IRQ to take 
> into account the possible return values of other ISRs.
> 
> As I pointed out, the situation when 2 ISRs return HANDLED_NOENABLE may 
> lead to problems on some archs.
Good point, leaves us with 2 possible return values for shared handlers:

   HANDLED
   NOT_HANDLED

i.e. shared handlers should never defer the end'ing of the interrupt (which 
makes sense, since this would affect the other [shared] handlers).

-- 
Anders

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4536 bytes --]

Anders Blomdell wrote:
> Dmitry Adamushko wrote:
>>
>> On 21/02/06, *Anders Blomdell* <anders.blomdell@domain.hid
>> <mailto:anders.blomdell@domain.hid>> wrote:
>>
>>     Dmitry Adamushko wrote:
>>      >
>>      > N.B. Amongst other things, some thoughts about CHAINED with shared
>>      > interrupts.
>>      >
>>      >
>>      > On 20/02/06, *Anders Blomdell* < anders.blomdell@domain.hid
>>     <mailto:anders.blomdell@domain.hid>
>>      > <mailto:anders.blomdell@domain.hid
>>     <mailto:anders.blomdell@domain.hid>>> wrote:
>>      >
>>      >
>>      >
>>      >     A number of questions arise:
>>      >
>>      >     1. What happens if one of the shared handlers leaves the
>>     interrupt
>>      >     asserted,
>>      >     returns NOENABLE|HANDLED and another return only HANDLED?
>>      >
>>      >     2. What happens if one returns PROPAGATE and another returns
>>     HANDLED?
>>      >
>>      >
>>      > Yep, each ISR may return a different value and all of them are
>>      > accumulated in the "s" variable ( s |= intr->isr(intr); ).
>>      >
>>      > So the loop may end up with "s" which contains all of the
>>     possible bits:
>>      >
>>      > (e.g.
>>      >
>>      > isr1 - HANDLED | ENABLE
>>      > isr2 - HANDLED (don't want the irq to be enabled)
>>      > isr3 - CHAINED
>>      >
>>      > )
>>      >
>>      > s = HANDLED | ENABLE | CHAINED;
>>      >
>>      > Then CHAINED will be ignored because of the following code :
>>      >
>>      > +    if (s & XN_ISR_ENABLE)
>>      > +       xnarch_end_irq(irq);
>>      > +    else if (s & XN_ISR_CHAINED)    (*)
>>      > +       xnarch_chain_irq(irq);
>>     Which is the worst way possible of prioritizing them, if a Linux
>>     interrupt is
>>     active when we get there with ENABLE|CHAINED, interrupts will be
>>     enabled with
>>     the Linux interrupt still asserted -> the IRQ-handlers will be
>>     called once more,
>>     probably returning ENABLE|CHAINED again -> infinite loop...
>>
>>      >
>>      > the current code in the CVS doen not contain "else" in (*), so
>> that
>>      > ENABLE | CHAINED is possible, though it's a wrong combination.
>>      >
>>      > This said, we suppose that one knows what he is doing.
>>      >
>>      > In the case of a single ISR per line, it's not that difficult to
>>      > achieve. But if there are a few ISRs, then one should analize and
>>     take
>>      > into account all possible return values of all the ISRs, as each
>>     of them
>>      > may affect others (e.g. if one returns CHAINED when another -
>>     HANDLED |
>>      > ENABLE).
>>     Which is somewhat contrary to the concept of shared interrupts, if
>>     we have to
>>     take care of the global picture, why make them shared in the first
>>     place?
>>     (I like the concept of shared interrupts, but it is important that
>>     the framework
>>     gives a separation of concerns)
>>
>>
>> Unfortunately, it looks to me that the current picture (even with your
>> scalar values) requires from the user who develops a given IRQ to take
>> into account the possible return values of other ISRs.
>>
>> As I pointed out, the situation when 2 ISRs return HANDLED_NOENABLE
>> may lead to problems on some archs.
> Good point, leaves us with 2 possible return values for shared handlers:
> 
>   HANDLED
>   NOT_HANDLED
> 
> i.e. shared handlers should never defer the end'ing of the interrupt
> (which makes sense, since this would affect the other [shared] handlers).
> 

Yes, "should". And this "should" is best be handled by

a) Documenting the potential conflict in the same place when describing
the return values

b) Placing some debug warning in the nucleus' IRQ trampoline function to
bail out (once per line) when running into such situation

But I'm against any further runtime restrictions, especially as most
drivers will never return anything else than NOT_HANDLED or HANDLED.
Actually, this was the reason why I tried to separate the NO_ENABLE and
PROPAGATE features as *additional* bits from HANDLED and
NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all valid bit
combination present as constants can be more helpful for the user. We
just have to draw some line between the standard values and the
additional gurus return codes (documentation: "don't use NO_ENABLE or
PROPAGATE unless you understand their side-effects and pitfalls precisely").

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Dmitry Adamushko]

[-- Attachment #1: Type: text/plain, Size: 2827 bytes --]

> Good point, leaves us with 2 possible return values for shared handlers:
>
>   HANDLED
>   NOT_HANDLED
>
> i.e. shared handlers should never defer the end'ing of the interrupt
(which
> makes sense, since this would affect the other [shared] handlers).

HANDLED_NOEBNABLE could be supported too. There would be no need in
reenventing
a wheel, just do it the way Linux does it.
But it's about some additional re-designing of the current codebase
(e.g. nested calling for irq_enable/disable())
I'm not sure we do need it for something else rather than irq sharing code
but it affects the rest of the code.

And we have a kind of wrong concept :

XN_ISR_ENABLE (or NOENABLE) corresponds to xnarch_end_irq().

But the later one is not only about enabling the line, but
on some archs - about .end-ing it too (sending EOI).

And to support HANDLED_NOENABLE properly, those 2 have to be decoupled, i.e.
EOI should always be sent from xnintr_shirq_handler().

> Yes, "should". And this "should" is best be handled by
>
> a) Documenting the potential conflict in the same place when describing
> the return values
>
> b) Placing some debug warning in the nucleus' IRQ trampoline function to
> bail out (once per line) when running into such situation
>
> But I'm against any further runtime restrictions, especially as most
> drivers will never return anything else than NOT_HANDLED or HANDLED.
> Actually, this was the reason why I tried to separate the NO_ENABLE and
> PROPAGATE features as *additional* bits from HANDLED and
> NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all valid bit
> combination present as constants can be more helpful for the user. We
> just have to draw some line between the standard values and the
> additional gurus return codes
>
(documentation: "don't use NO_ENABLE or
> PROPAGATE unless you understand their side-effects and pitfalls
precisely").

I agree with you on PROPAGATE case, but NO_ENABLE that, as pointed out
above,
should (IMHO and at least, in theory) only mean "keep the IRQ line disabled"
(and have nothing to do with .end-ing the IRQ line) would be better
supported.
But this is, again as was pointed out above, about some redesigning of the
current code => some overhead that likely affects non-shared aware code too.


So on one hand,

I'm ready to re-work code with :

HANDLED and UNHANDLED (or NOINT)

+ 2 additional bits : NOENABLE and PROPAGATE.

and document it like you suggested "don't use NO_ENABLE or
PROPAGATE with shared interrupts
unless you understand their side-effects and pitfalls precisely";

on the other hand,

I'd say that I'm almost ready to vote against merging the irq sharing code
at all as it looks to be a rather partial solution.


--
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 3266 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Anders Blomdell]

Dmitry Adamushko wrote:
> 
>  > Good point, leaves us with 2 possible return values for shared handlers:
>  >
>  >   HANDLED
>  >   NOT_HANDLED
>  >
>  > i.e. shared handlers should never defer the end'ing of the interrupt 
> (which
>  > makes sense, since this would affect the other [shared] handlers).
> 
> HANDLED_NOEBNABLE could be supported too. 
Yes, but it breaks decoupling between shared handlers; interrupts will be 
deferred for all [shared] handlers until it is properly ended.

> There would be no need in 
> reenventing
> a wheel, just do it the way Linux does it.
> But it's about some additional re-designing of the current codebase
> (e.g. nested calling for irq_enable/disable())
> I'm not sure we do need it for something else rather than irq sharing 
> code but it affects the rest of the code.
> 
> And we have a kind of wrong concept :
> 
> XN_ISR_ENABLE (or NOENABLE) corresponds to xnarch_end_irq().
Agree

> 
> But the later one is not only about enabling the line, but
> on some archs - about .end-ing it too (sending EOI).
> 
> And to support HANDLED_NOENABLE properly, those 2 have to be decoupled, i.e.
> EOI should always be sent from xnintr_shirq_handler().
But the one returning HANDLED_NOENABLE is likely to leave the interrupt 
asserted, hence we can't EOI at this point (unless NO_ENABLE means DISABLE).

> 
>  > Yes, "should". And this "should" is best be handled by
>  >
>  > a) Documenting the potential conflict in the same place when describing
>  > the return values
>  >
>  > b) Placing some debug warning in the nucleus' IRQ trampoline function to
>  > bail out (once per line) when running into such situation
>  >
>  > But I'm against any further runtime restrictions, especially as most
>  > drivers will never return anything else than NOT_HANDLED or HANDLED.
>  > Actually, this was the reason why I tried to separate the NO_ENABLE and
>  > PROPAGATE features as *additional* bits from HANDLED and
>  > NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all valid bit
>  > combination present as constants can be more helpful for the user. We
>  > just have to draw some line between the standard values and the
>  > additional gurus return codes
>  >
> (documentation: "don't use NO_ENABLE or
>  > PROPAGATE unless you understand their side-effects and pitfalls 
> precisely").
> 
> I agree with you on PROPAGATE case, but NO_ENABLE that, as pointed out 
> above,
> should (IMHO and at least, in theory) only mean "keep the IRQ line disabled"
> (and have nothing to do with .end-ing the IRQ line) would be better 
> supported.
> But this is, again as was pointed out above, about some redesigning of the
> current code => some overhead that likely affects non-shared aware code too.
> 
> 
> So on one hand,
> 
> I'm ready to re-work code with :
> 
> HANDLED and UNHANDLED (or NOINT)
> 
> + 2 additional bits : NOENABLE and PROPAGATE.
> 
> and document it like you suggested "don't use NO_ENABLE or
> PROPAGATE with shared interrupts
> unless you understand their side-effects and pitfalls precisely";
> 
> on the other hand,
> 
> I'd say that I'm almost ready to vote against merging the irq sharing 
> code at all as it looks to be a rather partial solution.
I vote for (even though I'm the one who complains the most), BUT I think it is 
important to keep the rules for using it simple (that's why I worry about the 
plethora of return-flags).


-- 

Regards

Anders

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4915 bytes --]

Anders Blomdell wrote:
> Dmitry Adamushko wrote:
>>
>>  > Good point, leaves us with 2 possible return values for shared
>> handlers:
>>  >
>>  >   HANDLED
>>  >   NOT_HANDLED
>>  >
>>  > i.e. shared handlers should never defer the end'ing of the
>> interrupt (which
>>  > makes sense, since this would affect the other [shared] handlers).
>>
>> HANDLED_NOEBNABLE could be supported too. 
> Yes, but it breaks decoupling between shared handlers; interrupts will
> be deferred for all [shared] handlers until it is properly ended.
> 
>> There would be no need in reenventing
>> a wheel, just do it the way Linux does it.
>> But it's about some additional re-designing of the current codebase
>> (e.g. nested calling for irq_enable/disable())
>> I'm not sure we do need it for something else rather than irq sharing
>> code but it affects the rest of the code.
>>
>> And we have a kind of wrong concept :
>>
>> XN_ISR_ENABLE (or NOENABLE) corresponds to xnarch_end_irq().
> Agree
> 
>>
>> But the later one is not only about enabling the line, but
>> on some archs - about .end-ing it too (sending EOI).
>>
>> And to support HANDLED_NOENABLE properly, those 2 have to be
>> decoupled, i.e.
>> EOI should always be sent from xnintr_shirq_handler().
> But the one returning HANDLED_NOENABLE is likely to leave the interrupt
> asserted, hence we can't EOI at this point (unless NO_ENABLE means
> DISABLE).

I guess this is what Dmitry meant: explicitly call disable() if one or
more ISRs returned NOENABLE - at least on archs where end != enable.
Will this work? We could then keep on using the existing IRQ-enable API
from bottom-half IRQ tasks. But what about NOENABLE+PROPAGATE? Will this
special case still mean NOT to end the ISR (as Linux will do)?

Bah, we are running in circles, I'm afraid. I think it's better to call
NOENABLE NOEOI, which will indeed mean to not end this line (as it is
the current situation anyway, isn't it?), and leave the user with what
(s)he can do with such a feature. We found out that there are trillions
of ways to shoot oneself into the foot with NOENABLE and PROPAGATE, and
we cannot prevent most of them. So let's stop trying, at least for this
patch!

> 
>>
>>  > Yes, "should". And this "should" is best be handled by
>>  >
>>  > a) Documenting the potential conflict in the same place when
>> describing
>>  > the return values
>>  >
>>  > b) Placing some debug warning in the nucleus' IRQ trampoline
>> function to
>>  > bail out (once per line) when running into such situation
>>  >
>>  > But I'm against any further runtime restrictions, especially as most
>>  > drivers will never return anything else than NOT_HANDLED or HANDLED.
>>  > Actually, this was the reason why I tried to separate the NO_ENABLE
>> and
>>  > PROPAGATE features as *additional* bits from HANDLED and
>>  > NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all
>> valid bit
>>  > combination present as constants can be more helpful for the user. We
>>  > just have to draw some line between the standard values and the
>>  > additional gurus return codes
>>  >
>> (documentation: "don't use NO_ENABLE or
>>  > PROPAGATE unless you understand their side-effects and pitfalls
>> precisely").
>>
>> I agree with you on PROPAGATE case, but NO_ENABLE that, as pointed out
>> above,
>> should (IMHO and at least, in theory) only mean "keep the IRQ line
>> disabled"
>> (and have nothing to do with .end-ing the IRQ line) would be better
>> supported.
>> But this is, again as was pointed out above, about some redesigning of
>> the
>> current code => some overhead that likely affects non-shared aware
>> code too.
>>
>>
>> So on one hand,
>>
>> I'm ready to re-work code with :
>>
>> HANDLED and UNHANDLED (or NOINT)
>>
>> + 2 additional bits : NOENABLE and PROPAGATE.
>>
>> and document it like you suggested "don't use NO_ENABLE or
>> PROPAGATE with shared interrupts
>> unless you understand their side-effects and pitfalls precisely";
>>
>> on the other hand,
>>
>> I'd say that I'm almost ready to vote against merging the irq sharing
>> code at all as it looks to be a rather partial solution.
> I vote for (even though I'm the one who complains the most), BUT I think
> it is important to keep the rules for using it simple (that's why I
> worry about the plethora of return-flags).
> 

And I'm with you here: My original proposal (2 base-states + 2 bits)
created 8 expressible states while your version only knows 4 states -
those which make sense most (and 2 of them are still the ones recommand
for the masses).

For RTDM I'm now almost determined to rework the API in way that only
HANDLED/UNHANDLED (or what ever their names will be) get exported, any
additional guru features will remain excluded as long as we have no
clean usage policy for them.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Anders Blomdell]

Jan Kiszka wrote:
> Anders Blomdell wrote:
> 
>>Dmitry Adamushko wrote:
>>
>>> > Good point, leaves us with 2 possible return values for shared
>>>handlers:
>>> >
>>> >   HANDLED
>>> >   NOT_HANDLED
>>> >
>>> > i.e. shared handlers should never defer the end'ing of the
>>>interrupt (which
>>> > makes sense, since this would affect the other [shared] handlers).
>>>
>>>HANDLED_NOEBNABLE could be supported too. 
>>
>>Yes, but it breaks decoupling between shared handlers; interrupts will
>>be deferred for all [shared] handlers until it is properly ended.
>>
>>
>>>There would be no need in reenventing
>>>a wheel, just do it the way Linux does it.
>>>But it's about some additional re-designing of the current codebase
>>>(e.g. nested calling for irq_enable/disable())
>>>I'm not sure we do need it for something else rather than irq sharing
>>>code but it affects the rest of the code.
>>>
>>>And we have a kind of wrong concept :
>>>
>>>XN_ISR_ENABLE (or NOENABLE) corresponds to xnarch_end_irq().
>>
>>Agree
>>
>>
>>>But the later one is not only about enabling the line, but
>>>on some archs - about .end-ing it too (sending EOI).
>>>
>>>And to support HANDLED_NOENABLE properly, those 2 have to be
>>>decoupled, i.e.
>>>EOI should always be sent from xnintr_shirq_handler().
>>
>>But the one returning HANDLED_NOENABLE is likely to leave the interrupt
>>asserted, hence we can't EOI at this point (unless NO_ENABLE means
>>DISABLE).
> 
> 
> I guess this is what Dmitry meant: explicitly call disable() if one or
> more ISRs returned NOENABLE - at least on archs where end != enable.
> Will this work? We could then keep on using the existing IRQ-enable API
> from bottom-half IRQ tasks. But what about NOENABLE+PROPAGATE? Will this
> special case still mean NOT to end the ISR (as Linux will do)?
> 
> Bah, we are running in circles, I'm afraid. I think it's better to call
> NOENABLE NOEOI, which will indeed mean to not end this line (as it is
> the current situation anyway, isn't it?), and leave the user with what
> (s)he can do with such a feature. We found out that there are trillions
> of ways to shoot oneself into the foot with NOENABLE and PROPAGATE, and
> we cannot prevent most of them. So let's stop trying, at least for this
> patch!
> 
> 
>>> > Yes, "should". And this "should" is best be handled by
>>> >
>>> > a) Documenting the potential conflict in the same place when
>>>describing
>>> > the return values
>>> >
>>> > b) Placing some debug warning in the nucleus' IRQ trampoline
>>>function to
>>> > bail out (once per line) when running into such situation
>>> >
>>> > But I'm against any further runtime restrictions, especially as most
>>> > drivers will never return anything else than NOT_HANDLED or HANDLED.
>>> > Actually, this was the reason why I tried to separate the NO_ENABLE
>>>and
>>> > PROPAGATE features as *additional* bits from HANDLED and
>>> > NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all
>>>valid bit
>>> > combination present as constants can be more helpful for the user. We
>>> > just have to draw some line between the standard values and the
>>> > additional gurus return codes
>>> >
>>>(documentation: "don't use NO_ENABLE or
>>> > PROPAGATE unless you understand their side-effects and pitfalls
>>>precisely").
>>>
>>>I agree with you on PROPAGATE case, but NO_ENABLE that, as pointed out
>>>above,
>>>should (IMHO and at least, in theory) only mean "keep the IRQ line
>>>disabled"
>>>(and have nothing to do with .end-ing the IRQ line) would be better
>>>supported.
>>>But this is, again as was pointed out above, about some redesigning of
>>>the
>>>current code => some overhead that likely affects non-shared aware
>>>code too.
>>>
>>>
>>>So on one hand,
>>>
>>>I'm ready to re-work code with :
>>>
>>>HANDLED and UNHANDLED (or NOINT)
>>>
>>>+ 2 additional bits : NOENABLE and PROPAGATE.
>>>
>>>and document it like you suggested "don't use NO_ENABLE or
>>>PROPAGATE with shared interrupts
>>>unless you understand their side-effects and pitfalls precisely";
>>>
>>>on the other hand,
>>>
>>>I'd say that I'm almost ready to vote against merging the irq sharing
>>>code at all as it looks to be a rather partial solution.
>>
>>I vote for (even though I'm the one who complains the most), BUT I think
>>it is important to keep the rules for using it simple (that's why I
>>worry about the plethora of return-flags).
>>
> 
> 
> And I'm with you here: My original proposal (2 base-states + 2 bits)
> created 8 expressible states while your version only knows 4 states -
> those which make sense most (and 2 of them are still the ones recommand
> for the masses).
> 
> For RTDM I'm now almost determined to rework the API in way that only
> HANDLED/UNHANDLED (or what ever their names will be) get exported, any
> additional guru features will remain excluded as long as we have no
> clean usage policy for them.
You have my vote for this.

-- 
Anders

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Dmitry Adamushko]

[-- Attachment #1: Type: text/plain, Size: 3988 bytes --]

> For RTDM I'm now almost determined to rework the API in way that only
> HANDLED/UNHANDLED (or what ever their names will be) get exported, any
> additional guru features will remain excluded as long as we have no
> clean usage policy for them.

Good. Then let's go for

HANDLED, UNHANDLED - we may consider them even as 2 scalar values

+

NOENABLE, CHAINED  - additional bits.

They are not encouraged to be used with shared interrupts
(explained in docs + debug messages when XENO_OPT_DEBUG is on).

Any ISR on the shared irq line should "understand" that it's
just one among the equals. That said, it should not do anything
that may affect other ISRs and not require any special treatment
(like CHAINED or NOENABLE).
If it wants it indeed, then don't declare itself as SHARED.

We may come back to the topic about possible return values of ISRs
a bit later maybe having got more feedback (hm.. hopefully)
on shared irq support.


>>>
>>> But the later one is not only about enabling the line, but
>>> on some archs - about .end-ing it too (sending EOI).
>>>
>>> And to support HANDLED_NOENABLE properly, those 2 have to be
>>> decoupled, i.e.
>>> EOI should always be sent from xnintr_shirq_handler().
>> But the one returning HANDLED_NOENABLE is likely to leave the interrupt
>> asserted, hence we can't EOI at this point (unless NO_ENABLE means
>> DISABLE).
>
>I guess this is what Dmitry meant: explicitly call disable() if one or
>more ISRs returned NOENABLE - at least on archs where end != enable.
>Will this work? We could then keep on using the existing IRQ-enable API
>from bottom-half IRQ tasks.

Almost.

Let's consider the following only as anorther way of doing some things;
I don't propose to implement it, it's just to illustrate my thoughts.
So one may simply ski-skip-skip it :o)

Let's keep in mind that what is behind .end-ing the IRQ line depends on
archs.
Sometimes end == enable (EOI was sent on .ack step), while in other cases
end == send_EOI [+ re-enabling].

But anyway, all ISRs are running with a given IRQ line disabled.

Supported values : HANDLED, UNHANDLED, PROPAGATE.

nucleus :: xnintr_irq_handler()
{
    ret = 0;

    ...

    for each isr in isr_list[ IRQ ]
    {
    temp = isr->handler();

    if (temp > ret)
        ret = temp;
    }

    if (ret == PROPAGATE)
    {
    // quite dengerous with shared interrupts, be sure you understand
    // what you are doing!

    xnarch_chain_irq(irq); // will be .end-ed in Linux domain
    }
    else
    {
    // HANDLED or UNHANDLED

    xnarch_end_irq();
    }

    ...

}

ENABLE or NOENABLE is missing? Nop.

let's say we have 2 rt-ISRs :

isr1 : HANDLED
isr2 : HANDLED + WISH

WISH == I want the IRQ line remain disabled until later
        (e.g. bottom half in rt_task will enable it)

How may isr2 express this WISH? Simply, xnarch_irq_disable/enable() should
support an internal counter that allows them to be called in a nested way.

So e.g. if there are 2 consecutive calls to disable_irq(), then
2 calls to enable_irq() are needed to really enable the IRQ line.

This way, the WISH is only about directly calling xnarch_irq_disable() in
isr2
and there is no need in ENABLE or NOENABLE flags.

This way, PROPAGATE really means NOEND - the IRQ will be .end-ed in Linux
domain;
while WISH==NOENABLE - has nothing to do with sending EOI, but only with
enabling/disabling the given IRQ line.

Yes, if one isr (or a few) defers the IRQ line enabling until later, it will
affect
all others ISR => all interrupts are temporary not accepted on this line.
This scenario is possible under Linux, but should be used with even more
care in real-time system. At least, this way HANDLED_NOENABLE case works
and doesn't lead to lost interrupts on some archs.

Moreover, it avoids the need for ENABLE flag even in non-shared interrupt
case.


--
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 4954 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Anders Blomdell]

Dmitry Adamushko wrote:
> 
>  > For RTDM I'm now almost determined to rework the API in way that only
>  > HANDLED/UNHANDLED (or what ever their names will be) get exported, any
>  > additional guru features will remain excluded as long as we have no
>  > clean usage policy for them.
> 
> Good. Then let's go for
> 
> HANDLED, UNHANDLED - we may consider them even as 2 scalar values
> 
> +
> 
> NOENABLE, CHAINED  - additional bits.
> 
> They are not encouraged to be used with shared interrupts
> (explained in docs + debug messages when XENO_OPT_DEBUG is on).
> 
> Any ISR on the shared irq line should "understand" that it's
> just one among the equals. That said, it should not do anything
> that may affect other ISRs and not require any special treatment
> (like CHAINED or NOENABLE).
> If it wants it indeed, then don't declare itself as SHARED.
> 
> We may come back to the topic about possible return values of ISRs
> a bit later maybe having got more feedback (hm.. hopefully)
> on shared irq support.
> 
> 
>  >>>
>  >>> But the later one is not only about enabling the line, but
>  >>> on some archs - about .end-ing it too (sending EOI).
>  >>>
>  >>> And to support HANDLED_NOENABLE properly, those 2 have to be
>  >>> decoupled, i.e.
>  >>> EOI should always be sent from xnintr_shirq_handler().
>  >> But the one returning HANDLED_NOENABLE is likely to leave the interrupt
>  >> asserted, hence we can't EOI at this point (unless NO_ENABLE means
>  >> DISABLE).
>  >
>  >I guess this is what Dmitry meant: explicitly call disable() if one or
>  >more ISRs returned NOENABLE - at least on archs where end != enable.
>  >Will this work? We could then keep on using the existing IRQ-enable API
>  >from bottom-half IRQ tasks.
> 
> Almost.
> 
> Let's consider the following only as anorther way of doing some things;
> I don't propose to implement it, it's just to illustrate my thoughts.
> So one may simply ski-skip-skip it :o)
> 
> Let's keep in mind that what is behind .end-ing the IRQ line depends on 
> archs.
> Sometimes end == enable (EOI was sent on .ack step), while in other cases
> end == send_EOI [+ re-enabling].
> 
> But anyway, all ISRs are running with a given IRQ line disabled.
> 
> Supported values : HANDLED, UNHANDLED, PROPAGATE.
> 
> nucleus :: xnintr_irq_handler()
> {
>     ret = 0;
> 
>     ...
> 
>     for each isr in isr_list[ IRQ ]
>     {
>     temp = isr->handler();
> 
>     if (temp > ret)
>         ret = temp;
>     }
> 
>     if (ret == PROPAGATE)
>     {
>     // quite dengerous with shared interrupts, be sure you understand
>     // what you are doing!
> 
>     xnarch_chain_irq(irq); // will be .end-ed in Linux domain
>     }
>     else
>     {
>     // HANDLED or UNHANDLED
> 
>     xnarch_end_irq();
>     }
> 
>     ...
> 
> }
> 
> ENABLE or NOENABLE is missing? Nop.
> 
> let's say we have 2 rt-ISRs :
> 
> isr1 : HANDLED
> isr2 : HANDLED + WISH
> 
> WISH == I want the IRQ line remain disabled until later
>         (e.g. bottom half in rt_task will enable it)
> 
> How may isr2 express this WISH? Simply, xnarch_irq_disable/enable() should
> support an internal counter that allows them to be called in a nested way.
> 
> So e.g. if there are 2 consecutive calls to disable_irq(), then
> 2 calls to enable_irq() are needed to really enable the IRQ line.
> 
> This way, the WISH is only about directly calling xnarch_irq_disable() 
> in isr2
> and there is no need in ENABLE or NOENABLE flags.
> 
> This way, PROPAGATE really means NOEND - the IRQ will be .end-ed in 
> Linux domain;
> while WISH==NOENABLE - has nothing to do with sending EOI, but only with
> enabling/disabling the given IRQ line.
> 
> Yes, if one isr (or a few) defers the IRQ line enabling until later, it 
> will affect
> all others ISR => all interrupts are temporary not accepted on this line.
> This scenario is possible under Linux, but should be used with even more
> care in real-time system. At least, this way HANDLED_NOENABLE case works
> and doesn't lead to lost interrupts on some archs.
> 
> Moreover, it avoids the need for ENABLE flag even in non-shared 
> interrupt case.
Lokks clean enough to me, i.e. no objections...

-- 
Anders

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4288 bytes --]

Dmitry Adamushko wrote:
>> For RTDM I'm now almost determined to rework the API in way that only
>> HANDLED/UNHANDLED (or what ever their names will be) get exported, any
>> additional guru features will remain excluded as long as we have no
>> clean usage policy for them.
> 
> Good. Then let's go for
> 
> HANDLED, UNHANDLED - we may consider them even as 2 scalar values

Ok, just avoid the 0.

> 
> +
> 
> NOENABLE, CHAINED  - additional bits.
> 
> They are not encouraged to be used with shared interrupts
> (explained in docs + debug messages when XENO_OPT_DEBUG is on).
> 
> Any ISR on the shared irq line should "understand" that it's
> just one among the equals. That said, it should not do anything
> that may affect other ISRs and not require any special treatment
> (like CHAINED or NOENABLE).
> If it wants it indeed, then don't declare itself as SHARED.
> 
> We may come back to the topic about possible return values of ISRs
> a bit later maybe having got more feedback (hm.. hopefully)
> on shared irq support.
> 
> 
>>>> But the later one is not only about enabling the line, but
>>>> on some archs - about .end-ing it too (sending EOI).
>>>>
>>>> And to support HANDLED_NOENABLE properly, those 2 have to be
>>>> decoupled, i.e.
>>>> EOI should always be sent from xnintr_shirq_handler().
>>> But the one returning HANDLED_NOENABLE is likely to leave the interrupt
>>> asserted, hence we can't EOI at this point (unless NO_ENABLE means
>>> DISABLE).
>> I guess this is what Dmitry meant: explicitly call disable() if one or
>> more ISRs returned NOENABLE - at least on archs where end != enable.
>> Will this work? We could then keep on using the existing IRQ-enable API
>>from bottom-half IRQ tasks.
> 
> Almost.
> 
> Let's consider the following only as anorther way of doing some things;
> I don't propose to implement it, it's just to illustrate my thoughts.
> So one may simply ski-skip-skip it :o)
> 
> Let's keep in mind that what is behind .end-ing the IRQ line depends on
> archs.
> Sometimes end == enable (EOI was sent on .ack step), while in other cases
> end == send_EOI [+ re-enabling].
> 
> But anyway, all ISRs are running with a given IRQ line disabled.
> 
> Supported values : HANDLED, UNHANDLED, PROPAGATE.
> 
> nucleus :: xnintr_irq_handler()
> {
>     ret = 0;
> 
>     ...
> 
>     for each isr in isr_list[ IRQ ]
>     {
>     temp = isr->handler();
> 
>     if (temp > ret)
>         ret = temp;
>     }
> 
>     if (ret == PROPAGATE)
>     {
>     // quite dengerous with shared interrupts, be sure you understand
>     // what you are doing!
> 
>     xnarch_chain_irq(irq); // will be .end-ed in Linux domain
>     }
>     else
>     {
>     // HANDLED or UNHANDLED
> 
>     xnarch_end_irq();
>     }
> 
>     ...
> 
> }
> 
> ENABLE or NOENABLE is missing? Nop.
> 
> let's say we have 2 rt-ISRs :
> 
> isr1 : HANDLED
> isr2 : HANDLED + WISH
> 
> WISH == I want the IRQ line remain disabled until later
>         (e.g. bottom half in rt_task will enable it)
> 
> How may isr2 express this WISH? Simply, xnarch_irq_disable/enable() should
> support an internal counter that allows them to be called in a nested way.
> 
> So e.g. if there are 2 consecutive calls to disable_irq(), then
> 2 calls to enable_irq() are needed to really enable the IRQ line.
> 
> This way, the WISH is only about directly calling xnarch_irq_disable() in
> isr2
> and there is no need in ENABLE or NOENABLE flags.
> 
> This way, PROPAGATE really means NOEND - the IRQ will be .end-ed in Linux
> domain;
> while WISH==NOENABLE - has nothing to do with sending EOI, but only with
> enabling/disabling the given IRQ line.
> 
> Yes, if one isr (or a few) defers the IRQ line enabling until later, it will
> affect
> all others ISR => all interrupts are temporary not accepted on this line.
> This scenario is possible under Linux, but should be used with even more
> care in real-time system. At least, this way HANDLED_NOENABLE case works
> and doesn't lead to lost interrupts on some archs.
> 
> Moreover, it avoids the need for ENABLE flag even in non-shared interrupt
> case.
> 

Yep, really looks like this is the best way to go.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Philippe Gerum]

Dmitry Adamushko wrote:
> 
>  > For RTDM I'm now almost determined to rework the API in way that only
>  > HANDLED/UNHANDLED (or what ever their names will be) get exported, any
>  > additional guru features will remain excluded as long as we have no
>  > clean usage policy for them.
> 
> Good. Then let's go for
> 
> HANDLED, UNHANDLED - we may consider them even as 2 scalar values
> 
> +
> 
> NOENABLE, CHAINED  - additional bits.
> 
> They are not encouraged to be used with shared interrupts
> (explained in docs + debug messages when XENO_OPT_DEBUG is on).
> 
> Any ISR on the shared irq line should "understand" that it's
> just one among the equals. That said, it should not do anything
> that may affect other ISRs and not require any special treatment
> (like CHAINED or NOENABLE).
> If it wants it indeed, then don't declare itself as SHARED.
> 
> We may come back to the topic about possible return values of ISRs
> a bit later maybe having got more feedback (hm.. hopefully)
> on shared irq support.
> 
> 
>  >>>
>  >>> But the later one is not only about enabling the line, but
>  >>> on some archs - about .end-ing it too (sending EOI).
>  >>>
>  >>> And to support HANDLED_NOENABLE properly, those 2 have to be
>  >>> decoupled, i.e.
>  >>> EOI should always be sent from xnintr_shirq_handler().
>  >> But the one returning HANDLED_NOENABLE is likely to leave the interrupt
>  >> asserted, hence we can't EOI at this point (unless NO_ENABLE means
>  >> DISABLE).
>  >
>  >I guess this is what Dmitry meant: explicitly call disable() if one or
>  >more ISRs returned NOENABLE - at least on archs where end != enable.
>  >Will this work? We could then keep on using the existing IRQ-enable API
>  >from bottom-half IRQ tasks.
> 
> Almost.
> 
> Let's consider the following only as anorther way of doing some things;
> I don't propose to implement it, it's just to illustrate my thoughts.
> So one may simply ski-skip-skip it :o)
> 
> Let's keep in mind that what is behind .end-ing the IRQ line depends on 
> archs.
> Sometimes end == enable (EOI was sent on .ack step), while in other cases
> end == send_EOI [+ re-enabling].
> 
> But anyway, all ISRs are running with a given IRQ line disabled.
> 
> Supported values : HANDLED, UNHANDLED, PROPAGATE.
> 
> nucleus :: xnintr_irq_handler()
> {
>     ret = 0;
> 
>     ...
> 
>     for each isr in isr_list[ IRQ ]
>     {
>     temp = isr->handler();
> 
>     if (temp > ret)
>         ret = temp;
>     }
> 
>     if (ret == PROPAGATE)
>     {
>     // quite dengerous with shared interrupts, be sure you understand
>     // what you are doing!
> 
>     xnarch_chain_irq(irq); // will be .end-ed in Linux domain
>     }
>     else
>     {
>     // HANDLED or UNHANDLED
> 
>     xnarch_end_irq();
>     }
> 
>     ...
> 
> }
> 
> ENABLE or NOENABLE is missing? Nop.
> 
> let's say we have 2 rt-ISRs :
> 
> isr1 : HANDLED
> isr2 : HANDLED + WISH
> 
> WISH == I want the IRQ line remain disabled until later
>         (e.g. bottom half in rt_task will enable it)
> 
> How may isr2 express this WISH? Simply, xnarch_irq_disable/enable() should
> support an internal counter that allows them to be called in a nested way.
> 
> So e.g. if there are 2 consecutive calls to disable_irq(), then
> 2 calls to enable_irq() are needed to really enable the IRQ line.
> 
> This way, the WISH is only about directly calling xnarch_irq_disable() 
> in isr2
> and there is no need in ENABLE or NOENABLE flags.
> 
> This way, PROPAGATE really means NOEND - the IRQ will be .end-ed in 
> Linux domain;
> while WISH==NOENABLE - has nothing to do with sending EOI, but only with
> enabling/disabling the given IRQ line.
> 
> Yes, if one isr (or a few) defers the IRQ line enabling until later, it 
> will affect
> all others ISR => all interrupts are temporary not accepted on this line.
> This scenario is possible under Linux, but should be used with even more
> care in real-time system. At least, this way HANDLED_NOENABLE case works
> and doesn't lead to lost interrupts on some archs.
> 
> Moreover, it avoids the need for ENABLE flag even in non-shared 
> interrupt case.
> 
> 

Ok, general bottom line regarding IRQ support (and the rest):

1- we want the rules applicable to the common case to be simple, well-defined and 
straightforward. This applies to the ISR return value as exposed.
2- some requirements might fall outside of the common case; to support them, we 
need to refrain from imposing a strong policy, but only provide sound mechanisms 
to implement that. Specifically, let's keep the basic ability to control the Adeos 
pipelining from Xenomai intact.
3- however, let's be true to the good old UNIX way-of-life: there is no point in 
penalizing 99% of the common user base to please only 1% of them who happen to 
have very specific needs. E.g., don't slow down the fast path everyone takes, 
don't impose hard rules most of the users won't benefit from. [I'm not referring 
to the enable/disable nesting count here: this _is_ correct, and is currently 
missing -- this should be done at Xeno's arch-level, not from the HAL which should 
be kept the way it is to have a direct, unmoderated access to the PIC handling].

IOW, the patch, the way you discussed it recently, is ok for me too.

-- 

Philippe.

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Dmitry Adamushko]

[-- Attachment #1: Type: text/plain, Size: 3068 bytes --]

> Ok, general bottom line regarding IRQ support (and the rest):
>
> 1- we want the rules applicable to the common case to be simple,
well-defined and
> straightforward. This applies to the ISR return value as exposed.
> 2- some requirements might fall outside of the common case; to support
them, we
> need to refrain from imposing a strong policy, but only provide sound
mechanisms
> to implement that. Specifically, let's keep the basic ability to control
the Adeos
> pipelining from Xenomai intact.
> 3- however, let's be true to the good old UNIX way-of-life: there is no
point in
> penalizing 99% of the common user base to please only 1% of them who
happen to
> have very specific needs. E.g., don't slow down the fast path everyone
takes,
> don't impose hard rules most of the users won't benefit from.

Ack.

> [I'm not referring
> to the enable/disable nesting count here: this _is_ correct, and is
currently
> missing -- this should be done at Xeno's arch-level, not from the HAL
which should
> be kept the way it is to have a direct, unmoderated access to the PIC
handling].

Let's consider it a bit. The counter as well as XENO_IRQ_DISABLED bit (or
something similar)
should be kept somewhere. So it's either xnintr_t or irq description in
low-level
ipipe code; we just don't want to introduce new structures and probably
it's better to implement this feature on Xeno layer, rather than on ipipe
one.

Actually, xnintr_t is not visible from arch-level code.

In order to support nested calls of irq_enable/disable, we should take
additional control
in all places where irq_desc[irq]->handler->enable/disable() are used (ok,
hal interface
is out of the scope, but e.g. xnintr_enable() and xnintr_disable() must be
modified
appropriately not to use this hal interface).

Another thing is xnarch_irq_end() which re-enables the IRQ line upon
exitting from
interrupt handling code.

It must make check for DISABLED bit (ISR might have called xnintr_disable()
to defer
the IRQ line enabling).

For most part of the archs supported by Xeno, .ending (xnarch_irq_end()) is
only about
calling irq_desc[irq]->handler->enable()
(handler->end() is never used) so it's not a problem to make a small wrapper
that re-enables the IRQ line only when XENO_IRQ_DISABLED bit is not set.

On ppc, irq_desc[irq]->handler->end() can be called indeed and we don't know
what's all about - at least, on the Xeno layer.

So either

handler->end() must check for XENO_IRQ_DISABLED - but this is an arch-level
code
in Linux (i.e. not the Xeno layer);

or Xeno/arch/ppc must represent an .end substitution for each sub-arch
supported.


> IOW, the patch, the way you discussed it recently, is ok for me too.

This said, I'm going to publish the shirq patch (after finalizing ISR return
bits,
where I still have some doubts) without enable/disable nesting support.
It can be supported at some point of time later, if it's really needed.


--
>
> Philippe.
>



--
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 3565 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]

Dmitry Adamushko wrote:
> ...
> This said, I'm going to publish the shirq patch (after finalizing ISR return
> bits,
> where I still have some doubts) without enable/disable nesting support.
> It can be supported at some point of time later, if it's really needed.
> 

Regarding enable/disable nesting and existing driver patterns: I
currently do the following on devices init via RTDM (and users may have
copied this):

rtdm_irq_request(...);
<init_hardware, also clear pending IRQs of the device>
rtdm_irq_enable(...);

But I do not disable the IRQ before rtdm_irq_free() again. Is this
unbalanced enabling still needed today? Is it even wrong these days? Is
it arch-dependent? I think the pattern dates back in RTAI times and was
needed for so far unused IRQs. Disabling them on device closure blocked
the line for later use under Linux.

I'm asking now in case we have to change the usage: we may better do it
early (e.g. with the introduction of Xenomai 2.1), so that the number of
surprises can be kept low when the underlying mechanisms get reworked later.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Philippe Gerum]

Jan Kiszka wrote:
> Dmitry Adamushko wrote:
> 
>>...
>>This said, I'm going to publish the shirq patch (after finalizing ISR return
>>bits,
>>where I still have some doubts) without enable/disable nesting support.
>>It can be supported at some point of time later, if it's really needed.
>>
> 
> 
> Regarding enable/disable nesting and existing driver patterns: I
> currently do the following on devices init via RTDM (and users may have
> copied this):
> 
> rtdm_irq_request(...);
> <init_hardware, also clear pending IRQs of the device>
> rtdm_irq_enable(...);
> 
> But I do not disable the IRQ before rtdm_irq_free() again. Is this
> unbalanced enabling still needed today? Is it even wrong these days?

Looks unsafe, since nothing says that freeing the descriptor associated with some 
IRQ should disable this IRQ line at hw level. However, we would be correct to 
assume that no IRQ could happen after we have been asked to free its associated 
descriptor.

  Is
> it arch-dependent?

Nope. Both APIs are arch-agnostic anyway.

  I think the pattern dates back in RTAI times and was
> needed for so far unused IRQs. Disabling them on device closure blocked
> the line for later use under Linux.
> 

We never had this problem with Xeno, since we always relied on the standard IRQ 
controllers defined by Linux for managing interrupt lines. IOW, Linux can undo 
what Xenomai did wrt IRQ line enabling/disabling.

> I'm asking now in case we have to change the usage: we may better do it
> early (e.g. with the introduction of Xenomai 2.1), so that the number of
> surprises can be kept low when the underlying mechanisms get reworked later.
> 
> Jan
> 


-- 

Philippe.

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Dmitry Adamushko wrote:
>>
>>> ...
>>> This said, I'm going to publish the shirq patch (after finalizing ISR
>>> return
>>> bits,
>>> where I still have some doubts) without enable/disable nesting support.
>>> It can be supported at some point of time later, if it's really needed.
>>>
>>
>>
>> Regarding enable/disable nesting and existing driver patterns: I
>> currently do the following on devices init via RTDM (and users may have
>> copied this):
>>
>> rtdm_irq_request(...);
>> <init_hardware, also clear pending IRQs of the device>
>> rtdm_irq_enable(...);
>>
>> But I do not disable the IRQ before rtdm_irq_free() again. Is this
>> unbalanced enabling still needed today? Is it even wrong these days?
> 
> Looks unsafe, since nothing says that freeing the descriptor associated
> with some IRQ should disable this IRQ line at hw level. However, we
> would be correct to assume that no IRQ could happen after we have been
> asked to free its associated descriptor.
> 
>  Is
>> it arch-dependent?
> 
> Nope. Both APIs are arch-agnostic anyway.
> 
>  I think the pattern dates back in RTAI times and was
>> needed for so far unused IRQs. Disabling them on device closure blocked
>> the line for later use under Linux.
>>
> 
> We never had this problem with Xeno, since we always relied on the
> standard IRQ controllers defined by Linux for managing interrupt lines.
> IOW, Linux can undo what Xenomai did wrt IRQ line enabling/disabling.
> 

So the enable is definitely needed and a disable on release should not
cause harm anymore? If that's the case, we could start re-introducing
rtdm_irq_disable before rtdm_irq_free again.

>> I'm asking now in case we have to change the usage: we may better do it
>> early (e.g. with the introduction of Xenomai 2.1), so that the number of
>> surprises can be kept low when the underlying mechanisms get reworked
>> later.
>>
>> Jan
>>
> 
> 

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
> 
>>Jan Kiszka wrote:
>>
>>>Dmitry Adamushko wrote:
>>>
>>>
>>>>...
>>>>This said, I'm going to publish the shirq patch (after finalizing ISR
>>>>return
>>>>bits,
>>>>where I still have some doubts) without enable/disable nesting support.
>>>>It can be supported at some point of time later, if it's really needed.
>>>>
>>>
>>>
>>>Regarding enable/disable nesting and existing driver patterns: I
>>>currently do the following on devices init via RTDM (and users may have
>>>copied this):
>>>
>>>rtdm_irq_request(...);
>>><init_hardware, also clear pending IRQs of the device>
>>>rtdm_irq_enable(...);
>>>
>>>But I do not disable the IRQ before rtdm_irq_free() again. Is this
>>>unbalanced enabling still needed today? Is it even wrong these days?
>>
>>Looks unsafe, since nothing says that freeing the descriptor associated
>>with some IRQ should disable this IRQ line at hw level. However, we
>>would be correct to assume that no IRQ could happen after we have been
>>asked to free its associated descriptor.
>>
>> Is
>>
>>>it arch-dependent?
>>
>>Nope. Both APIs are arch-agnostic anyway.
>>
>> I think the pattern dates back in RTAI times and was
>>
>>>needed for so far unused IRQs. Disabling them on device closure blocked
>>>the line for later use under Linux.
>>>
>>
>>We never had this problem with Xeno, since we always relied on the
>>standard IRQ controllers defined by Linux for managing interrupt lines.
>>IOW, Linux can undo what Xenomai did wrt IRQ line enabling/disabling.
>>
> 
> 
> So the enable is definitely needed and a disable on release should not
> cause harm anymore? If that's the case, we could start re-introducing
> rtdm_irq_disable before rtdm_irq_free again.
> 

It should work that way.

> 
>>>I'm asking now in case we have to change the usage: we may better do it
>>>early (e.g. with the introduction of Xenomai 2.1), so that the number of
>>>surprises can be kept low when the underlying mechanisms get reworked
>>>later.
>>>
>>>Jan
>>>
>>
>>
> 
> Jan
> 


-- 

Philippe.

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Anders Blomdell]

Jan Kiszka wrote:
> Philippe Gerum wrote:
> 
>>Jan Kiszka wrote:
>>
>>>Dmitry Adamushko wrote:
>>>
>>>
>>>>...
>>>>This said, I'm going to publish the shirq patch (after finalizing ISR
>>>>return
>>>>bits,
>>>>where I still have some doubts) without enable/disable nesting support.
>>>>It can be supported at some point of time later, if it's really needed.
>>>>
>>>
>>>
>>>Regarding enable/disable nesting and existing driver patterns: I
>>>currently do the following on devices init via RTDM (and users may have
>>>copied this):
>>>
>>>rtdm_irq_request(...);
>>><init_hardware, also clear pending IRQs of the device>
>>>rtdm_irq_enable(...);
>>>
>>>But I do not disable the IRQ before rtdm_irq_free() again. Is this
>>>unbalanced enabling still needed today? Is it even wrong these days?
>>
>>Looks unsafe, since nothing says that freeing the descriptor associated
>>with some IRQ should disable this IRQ line at hw level. However, we
>>would be correct to assume that no IRQ could happen after we have been
>>asked to free its associated descriptor.
>>
>> Is
>>
>>>it arch-dependent?
>>
>>Nope. Both APIs are arch-agnostic anyway.
>>
>> I think the pattern dates back in RTAI times and was
>>
>>>needed for so far unused IRQs. Disabling them on device closure blocked
>>>the line for later use under Linux.
>>>
>>
>>We never had this problem with Xeno, since we always relied on the
>>standard IRQ controllers defined by Linux for managing interrupt lines.
>>IOW, Linux can undo what Xenomai did wrt IRQ line enabling/disabling.
>>
> 
> 
> So the enable is definitely needed and a disable on release should not
> cause harm anymore? If that's the case, we could start re-introducing
> rtdm_irq_disable before rtdm_irq_free again.
Except for interrupts shared between RT/non-RT, the don't need enable (since 
they are enabled by Linux already), and probably doesn't fare well with a final 
disable.

> 
> 
>>>I'm asking now in case we have to change the usage: we may better do it
>>>early (e.g. with the introduction of Xenomai 2.1), so that the number of
>>>surprises can be kept low when the underlying mechanisms get reworked
>>>later.
>>>
-- 

Anders

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2567 bytes --]

Anders Blomdell wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>
>>> Jan Kiszka wrote:
>>>
>>>> Dmitry Adamushko wrote:
>>>>
>>>>
>>>>> ...
>>>>> This said, I'm going to publish the shirq patch (after finalizing ISR
>>>>> return
>>>>> bits,
>>>>> where I still have some doubts) without enable/disable nesting
>>>>> support.
>>>>> It can be supported at some point of time later, if it's really
>>>>> needed.
>>>>>
>>>>
>>>>
>>>> Regarding enable/disable nesting and existing driver patterns: I
>>>> currently do the following on devices init via RTDM (and users may have
>>>> copied this):
>>>>
>>>> rtdm_irq_request(...);
>>>> <init_hardware, also clear pending IRQs of the device>
>>>> rtdm_irq_enable(...);
>>>>
>>>> But I do not disable the IRQ before rtdm_irq_free() again. Is this
>>>> unbalanced enabling still needed today? Is it even wrong these days?
>>>
>>> Looks unsafe, since nothing says that freeing the descriptor associated
>>> with some IRQ should disable this IRQ line at hw level. However, we
>>> would be correct to assume that no IRQ could happen after we have been
>>> asked to free its associated descriptor.
>>>
>>> Is
>>>
>>>> it arch-dependent?
>>>
>>> Nope. Both APIs are arch-agnostic anyway.
>>>
>>> I think the pattern dates back in RTAI times and was
>>>
>>>> needed for so far unused IRQs. Disabling them on device closure blocked
>>>> the line for later use under Linux.
>>>>
>>>
>>> We never had this problem with Xeno, since we always relied on the
>>> standard IRQ controllers defined by Linux for managing interrupt lines.
>>> IOW, Linux can undo what Xenomai did wrt IRQ line enabling/disabling.
>>>
>>
>>
>> So the enable is definitely needed and a disable on release should not
>> cause harm anymore? If that's the case, we could start re-introducing
>> rtdm_irq_disable before rtdm_irq_free again.
> Except for interrupts shared between RT/non-RT, the don't need enable
> (since they are enabled by Linux already), and probably doesn't fare
> well with a final disable.
> 

This does not apply to the drivers I have in mind (e.g. RTnet NIC
drivers). None of them is prepared to share the IRQ line with Linux.
There is only the scenario that a Linux driver for the same hardware
gets loaded later after removing the RT driver (e.g. switching from
RTnet to standard Linux networking).

Anyway, before changing anything here we need some tests - and counting
enable/disable. Otherwise, we will already run into troubles with shared
RT-IRQs.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Philippe Gerum]

Anders Blomdell wrote:
> Jan Kiszka wrote:
> 
>> Philippe Gerum wrote:
>>
>>> Jan Kiszka wrote:
>>>
>>>> Dmitry Adamushko wrote:
>>>>
>>>>
>>>>> ...
>>>>> This said, I'm going to publish the shirq patch (after finalizing ISR
>>>>> return
>>>>> bits,
>>>>> where I still have some doubts) without enable/disable nesting 
>>>>> support.
>>>>> It can be supported at some point of time later, if it's really 
>>>>> needed.
>>>>>
>>>>
>>>>
>>>> Regarding enable/disable nesting and existing driver patterns: I
>>>> currently do the following on devices init via RTDM (and users may have
>>>> copied this):
>>>>
>>>> rtdm_irq_request(...);
>>>> <init_hardware, also clear pending IRQs of the device>
>>>> rtdm_irq_enable(...);
>>>>
>>>> But I do not disable the IRQ before rtdm_irq_free() again. Is this
>>>> unbalanced enabling still needed today? Is it even wrong these days?
>>>
>>>
>>> Looks unsafe, since nothing says that freeing the descriptor associated
>>> with some IRQ should disable this IRQ line at hw level. However, we
>>> would be correct to assume that no IRQ could happen after we have been
>>> asked to free its associated descriptor.
>>>
>>> Is
>>>
>>>> it arch-dependent?
>>>
>>>
>>> Nope. Both APIs are arch-agnostic anyway.
>>>
>>> I think the pattern dates back in RTAI times and was
>>>
>>>> needed for so far unused IRQs. Disabling them on device closure blocked
>>>> the line for later use under Linux.
>>>>
>>>
>>> We never had this problem with Xeno, since we always relied on the
>>> standard IRQ controllers defined by Linux for managing interrupt lines.
>>> IOW, Linux can undo what Xenomai did wrt IRQ line enabling/disabling.
>>>
>>
>>
>> So the enable is definitely needed and a disable on release should not
>> cause harm anymore? If that's the case, we could start re-introducing
>> rtdm_irq_disable before rtdm_irq_free again.
> 
> Except for interrupts shared between RT/non-RT, the don't need enable 
> (since they are enabled by Linux already), and probably doesn't fare 
> well with a final disable.
> 

That's the uncommon case by essence, for which a different set of rules already 
applies anyway.

>>
>>
>>>> I'm asking now in case we have to change the usage: we may better do it
>>>> early (e.g. with the introduction of Xenomai 2.1), so that the 
>>>> number of
>>>> surprises can be kept low when the underlying mechanisms get reworked
>>>> later.
>>>>


-- 

Philippe.

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Anders Blomdell]

Dmitry Adamushko wrote:
> 
> N.B. Amongst other things, some thoughts about CHAINED with shared 
> interrupts.
> 
> 
> On 20/02/06, *Anders Blomdell* <anders.blomdell@domain.hid 
> <mailto:anders.blomdell@domain.hid>> wrote:
> 
> 
> 
>     A number of questions arise:
> 
>     1. What happens if one of the shared handlers leaves the interrupt
>     asserted,
>     returns NOENABLE|HANDLED and another return only HANDLED?
> 
>     2. What happens if one returns PROPAGATE and another returns HANDLED?
> 
> 
> Yep, each ISR may return a different value and all of them are
> accumulated in the "s" variable ( s |= intr->isr(intr); ).
> 
> So the loop may end up with "s" which contains all of the possible bits:
> 
> (e.g.
> 
> isr1 - HANDLED | ENABLE
> isr2 - HANDLED (don't want the irq to be enabled)
> isr3 - CHAINED
> 
> )
> 
> s = HANDLED | ENABLE | CHAINED;
> 
> Then CHAINED will be ignored because of the following code :
>  
> +    if (s & XN_ISR_ENABLE)
> +       xnarch_end_irq(irq);
> +    else if (s & XN_ISR_CHAINED)    (*)
> +       xnarch_chain_irq(irq);
Which is the worst way possible of prioritizing them, if a Linux interrupt is
active when we get there with ENABLE|CHAINED, interrupts will be enabled with
the Linux interrupt still asserted -> the IRQ-handlers will be called once more,
probably returning ENABLE|CHAINED again -> infinite loop...

> 
> the current code in the CVS doen not contain "else" in (*), so that 
> ENABLE | CHAINED is possible, though it's a wrong combination.
> 
> This said, we suppose that one knows what he is doing.
> 
> In the case of a single ISR per line, it's not that difficult to 
> achieve. But if there are a few ISRs, then one should analize and take 
> into account all possible return values of all the ISRs, as each of them 
> may affect others (e.g. if one returns CHAINED when another - HANDLED | 
> ENABLE).
Which is somewhat contrary to the concept of shared interrupts, if we have to
take care of the global picture, why make them shared in the first place?
(I like the concept of shared interrupts, but it is important that the framework
gives a separation of concerns)

> So my feeling is that CHAINED should not be used by drivers which 
> registered their ISRs as SHARED.
Well, CHAINED should not be used by drivers which return ENABLE (and are of
course hence incompatible with true realtime IRQ's)
> 
> Moreover, I actually see the only scenario of CHAINED (I provided it 
> before) :
> 
> all ISRs in the primary domain have reported UNHANDLED  =>  nucleus 
> propagates the interrupt down the pipeline with xnacrh_chain_irq().
> This call actually returns 1 upon successful propagation (some domain 
> down the pipeline was interested in this irq) and 0 otherwise.
> 
> Upon 0, this is a spurious irq (none of domains was interested in its 
> handling).
> 
> ok, let's suppose now :
> 
> we have 2 ISRs on the same shared line :
> 
> isr1 : HANDLED (will be enabled by rt task. Note, rt task must call 
> xnarch_end_irq() and not just xnarch_enable_irq()! )
> 
> isr2 : CHAINED
> 
> So HANDLED | CHAINED is ok for the single ISR on the line, but it may 
> lead to HANDLED | CHAINED | ENABLE in a case of the shared line.
> 
> rt task that works jointly with isr1 just calls xnarch_end_irq() at some 
> moment of time and some ISR in the linux domain does the same later  =>  
> the line is .end-ed 2 times.
> 
> ISR should never return CHAINED as to indicate _only_ that it is not 
> interested in this irq, but ~HANDLED or NOINT (if we'll support it) instead.
> 
> If the ISR nevertheless wants to propagate the IRQ to the Linux domain 
> _explicitly_, it _must not_ register itself as SHARED, i.e. it _must_ be 
> the only ISR on this line, otherwise that may lead to the IRQ line being 
> .end-ed twice (lost interrupts in some cases).
> 
> 
>     #define UNHANDLED 0
>     #define HANDLED_ENABLE 1
>     #define HANDLED_NOENABLE 2
>     #define PROPAGATE 3 
> 
> 
> Yep, I'd agree with you. Moreover, PROPAGATE should not be used for 
> shared interrupts.
My feeling is that it should be considered an error to attach a RT IRQ handler
to a line that has a Linux IRQ handler (this should be possible to check, since
/proc/interrupts contains the relevant information), unless a "Linux IRQ-mask"
function is installed. This IRQ-mask function should the be called:

   1. each time domains are switched
   2. each time an interrupt is generated

The IRQ-mask function should look something like:

unsigned int rt_irq_mask(struct ipipe_domain *ipd, unsigned int irq)
{
   int result = 0;
   static int enabled = true;
   int enable = enabled;

   if (irq >= 0) {
     // Interrupt has occured, we are about to run IRQ handlers
     if (disable_early) {
       enable = false;
     }
     if (for_linux(irq)) {
       result = XN_ISR_CHAINED;
     }
   } else if (ipd == ipipe_root_domain) {
     // Entering Linux
     enable = true;
   } else {
     // Other doamin, block linux interrupts
     enable = false;
   }
   if (enable != enabled) {
     enabled = enable
     if (enable) {
       // Enable Linux interrupts by unmasking appropriate
       // device registers (and possibly entire IRQ's)
     } else {
       // Disable Linux interrupts by masking appropriate
       // device registers (and possibly entire IRQ's)
     }
   }
   return result;
}

The advantages with this scheme is:

1. Linux interrupts are handled in one (platform specific) routine, and does not
clobber the RT IRQ-handlers, thus making it simpler to port a RT application to
a new platform: only the irq_mask funtion needs to be changed, i.e. separation
of concerns.

2. By setting disable_early and masking out all Linux IRQ's, the RT part only
needs to take the hit from one Linux interrupt even if they all occur.

-- 

Regards

Anders

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4437 bytes --]

Anders Blomdell wrote:
> Jan Kiszka wrote:
>> Hi Dmitry,
>>
>> Dmitry Adamushko wrote:
>>
>>> Hi Jan,
>>>
>>> let's make yet another revision of the bits :
>>>
>>> new XN_ISR_HANDLED  == old XN_ISR_HANDLED + old XN_ISR_NO_ENABLE
>>>
>>> ok.
>>>
>>> new XN_ISR_NOENABLE == ~ old XN_ISR_ENABLE
>>>
>>> ok.
>>>
>>> new XN_ISR_PROPAGATE == XN_ISR_CHAINED
>>>
>>> ok.
>>>
>>
>>
>> Just to make sure that you understand my weird ideas: each of the three
>> new XN_ISR_xxx above should be encoded with an individual bit
>>
>>
>>> new XN_ISR_NOINT == ?
>>>
>>> does it suppose the interrupt line to be .end-ed (enabled) and irq
>>> not to be
>>> propagated? Should be so, I guess, if it's different from 5). Then
>>> nucleus
>>> ignores implicit IRQ enable for 5) as well as for 3).
>>>
>>> Do we really need that NOINT then, as it seems to be the same as
>>> ~HANDLED?
>>>
>>> or NOINT == 0 and then it's a scalar value, not a bit.
>>>
>>> So one may consider HANDLED == 1 and NOINT == 0 as really scalar values
>>>
>>> and
>>>
>>> NOENABLE and PROPAGATE as additional bits (used only if needed).
>>>
>>
>>
>> My idea is to urge the user specifying one of the base return types
>> (HANDLED or NOINT) + any of the two additional bits (NOENABLE and
>> PROPAGATE).
>>
>> For correct drivers NOINT could be 0 indeed, but to check that the user
>> picked a new constant we may want to set NOINT != 0. With the old API
>> "return 0" expressed HANDLED + ~ENABLE for the old API. With the new one
>> the user signals no interest and the nucleus may raise a warning that a
>> spurious IRQ occurred. So I would add a debug bit for NOINT here to
>> optionally (on OPT_XENO_DEBUG) detect old-style usage (return 0).
>> Moreover, we gain freedom to move bits in the future when every state is
>> encoded via constants. Or am I too paranoid here?
> After reading the above discussion (of which I understand very little),
> and looking at (what I believe to be) the relevant code:
> 
> +    intr = shirq->handlers;
> +
> +    while (intr)
> +        {
> +        s |= intr->isr(intr);
> +        ++intr->hits;
> +        intr = intr->next;
> +        }
> +    xnintr_shirq_unlock(shirq);
> +
> +    --sched->inesting;
> +
> +    if (s & XN_ISR_ENABLE)
> +       xnarch_end_irq(irq);
> +    else if (s & XN_ISR_CHAINED)
> +       xnarch_chain_irq(irq);
> 
> A number of questions arise:
> 
> 1. What happens if one of the shared handlers leaves the interrupt
> asserted, returns NOENABLE|HANDLED and another return only HANDLED?
> 
> 2. What happens if one returns PROPAGATE and another returns HANDLED?
> 
> As far as I can tell, after all RT handlers havve run, the following
> scenarios are possible:
> 
> 1. The interrupt is deasserted (i.e. it was a RT interrupt)
> 2. The interrupt is still asserted, it will be deasserted later
>    by some RT task (i.e. it was a RT interrupt)
> 3. The interrupt is still asserted and will be deasserted
>    by the Linux IRQ handler.
> 
> IMHO that leads to the conclusion that the IRQ handlers should return a
> scalar
> 
> #define UNHANDLED 0
> #define HANDLED_ENABLE 1
> #define HANDLED_NOENABLE 2
> #define PROPAGATE 3
> 
> and the loop should be
> 
>     s = UNHANDLED
>     while (intr) {
>       tmp = intr->isr(intr);
>       if (tmp > s) { s = tmp; }
>       intr = intr->next;
>     }
>     if (s == PROPAGATE) {
>       xnarch_chain_irq(irq);
>     } else if (s == HANDLED_ENABLE) {
>       xnarch_end_irq(irq);
>     }

This is a very useful suggestion, specifically this escalation of the
return value in the shared case. I would just let UNHANLDED start with 1
for the reasons I explained here:

https://mail.gna.org/public/xenomai-core/2006-02/msg00186.html

Ok, I would say let's got for this and finalise the patch!

> 
> To be really honest, I think that PROPAGATE should be excluded from the
> RT IRQ-handlers, since with the current scheme all RT-handlers has to
> test if the IRQ was a Linux interrupt (otherwise the system will only
> work when the handler that returns PROPAGATE is installed)
> 

Indeed, but I'm with Philippe here: do not exclude the strange corner
case scenarios users may craft with the appropriate care. I could, e.g.,
imagine a scenario where an RT handler takes the arrival time stamp of a
non-RT IRQ and then propagates it.

Jsn


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]