Module: xenomai-rpm
Branch: for-upstream
Commit: b8c893aa452db6f9727337d7c923f2aaec63b19b
URL:    
http://git.xenomai.org/?p=xenomai-rpm.git;a=commit;h=b8c893aa452db6f9727337d7c923f2aaec63b19b

Author: Philippe Gerum <r...@xenomai.org>
Date:   Sun Nov  7 16:08:55 2010 +0100

hal/generic, nucleus/intr: synchronize before releasing interrupt

At this chance, the patch also fixes the following issues:

- do not alter the irq descriptor (e.g. cookie and stats) if the
attachment fails early.

- do not set irq affinity before the validity checks, and set it only
for the first handler introduced in the shared list.

---

 ksrc/arch/generic/hal.c |   10 ++-
 ksrc/nucleus/intr.c     |  147 ++++++++++++++++++++++++++++++++++-------------
 2 files changed, 113 insertions(+), 44 deletions(-)

diff --git a/ksrc/arch/generic/hal.c b/ksrc/arch/generic/hal.c
index 6583975..061336d 100644
--- a/ksrc/arch/generic/hal.c
+++ b/ksrc/arch/generic/hal.c
@@ -209,11 +209,13 @@ int rthal_irq_request(unsigned irq,
 
 int rthal_irq_release(unsigned irq)
 {
-    if (irq >= IPIPE_NR_IRQS)
-       return -EINVAL;
+       if (irq >= IPIPE_NR_IRQS)
+               return -EINVAL;
 
-    return rthal_virtualize_irq(&rthal_domain,
-                               irq, NULL, NULL, NULL, IPIPE_PASS_MASK);
+       rthal_synchronize_irq(irq);
+
+       return rthal_virtualize_irq(&rthal_domain,
+                                   irq, NULL, NULL, NULL, IPIPE_PASS_MASK);
 }
 
 /**
diff --git a/ksrc/nucleus/intr.c b/ksrc/nucleus/intr.c
index 0d6f64b..0cf66f4 100644
--- a/ksrc/nucleus/intr.c
+++ b/ksrc/nucleus/intr.c
@@ -41,6 +41,8 @@
 
 DEFINE_PRIVATE_XNLOCK(intrlock);
 
+static unsigned long teardown[BITS_TO_LONGS(XNARCH_NR_IRQS)];
+
 #ifdef CONFIG_XENO_OPT_STATS
 xnintr_t nkclock;           /* Only for statistics */
 static int xnintr_count = 1; /* Number of attached xnintr objects + nkclock */
@@ -314,7 +316,7 @@ static inline int xnintr_irq_attach(xnintr_t *intr)
 {
        xnintr_irq_t *shirq = &xnirqs[intr->irq];
        xnintr_t *prev, **p = &shirq->handlers;
-       int err;
+       int ret;
 
        if ((prev = *p) != NULL) {
                /* Check on whether the shared mode is allowed. */
@@ -324,11 +326,15 @@ static inline int xnintr_irq_attach(xnintr_t *intr)
                        (intr->flags & XN_ISR_EDGE)))
                        return -EBUSY;
 
-               /* Get a position at the end of the list to insert the new 
element. */
-               while (prev) {
+               do {
+                       /*
+                        * Get a position at the end of the list to
+                        * insert the new element.
+                        */
                        p = &prev->next;
                        prev = *p;
-               }
+               } while (prev);
+               ret = 1;        /* More than one handler will be present. */
        } else {
                /* Initialize the corresponding interrupt channel */
                void (*handler) (unsigned, void *) = &xnintr_irq_handler;
@@ -342,26 +348,27 @@ static inline int xnintr_irq_attach(xnintr_t *intr)
                }
                shirq->unhandled = 0;
 
-               err = xnarch_hook_irq(intr->irq, handler,
+               ret = xnarch_hook_irq(intr->irq, handler,
                                      (rthal_irq_ackfn_t)intr->iack, intr);
-               if (err)
-                       return err;
+               if (ret)
+                       return ret;
        }
 
        intr->next = NULL;
 
-       /* Add the given interrupt object. No need to synchronise with the IRQ
-          handler, we are only extending the chain. */
+       /*
+        * Add the given interrupt object. No need to synchronise with
+        * the IRQ handler, we are only extending the chain.
+        */
        *p = intr;
 
-       return 0;
+       return ret;
 }
 
 static inline int xnintr_irq_detach(xnintr_t *intr)
 {
        xnintr_irq_t *shirq = &xnirqs[intr->irq];
        xnintr_t *e, **p = &shirq->handlers;
-       int err = 0;
 
        while ((e = *p) != NULL) {
                if (e == intr) {
@@ -369,21 +376,20 @@ static inline int xnintr_irq_detach(xnintr_t *intr)
                        xnlock_get(&shirq->lock);
                        *p = e->next;
                        xnlock_put(&shirq->lock);
-
                        xnintr_sync_stat_references(intr);
-
-                       /* Release the IRQ line if this was the last user */
-                       if (shirq->handlers == NULL)
-                               err = xnarch_release_irq(intr->irq);
-
-                       return err;
+                       return shirq->handlers == NULL;
                }
                p = &e->next;
        }
 
        xnlogerr("attempted to detach a non previously attached interrupt "
                 "object.\n");
-       return err;
+       return 0;
+}
+
+static inline int xnintr_irq_release(xnintr_t *intr)
+{
+       return xnarch_release_irq(intr->irq);
 }
 
 #else /* !CONFIG_XENO_OPT_SHIRQ */
@@ -416,14 +422,19 @@ static inline int xnintr_irq_attach(xnintr_t *intr)
 
 static inline int xnintr_irq_detach(xnintr_t *intr)
 {
+       xnintr_sync_stat_references(intr);
+
+       return 1;
+}
+
+static inline int xnintr_irq_release(xnintr_t *intr)
+{
        int irq = intr->irq, ret;
 
        xnlock_get(&xnirqs[irq].lock);
        ret = xnarch_release_irq(irq);
        xnlock_put(&xnirqs[irq].lock);
 
-       xnintr_sync_stat_references(intr);
-
        return ret;
 }
 
@@ -453,7 +464,7 @@ static void xnintr_irq_handler(unsigned irq, void *cookie)
 #ifdef CONFIG_SMP
        /*
         * In SMP case, we have to reload the cookie under the per-IRQ
-        * lock to avoid racing with xnintr_detach.  However, we
+        * lock to avoid racing with xnintr_irq_release.  However, we
         * assume that no CPU migration will occur while running the
         * interrupt service routine, so the scheduler pointer will
         * remain valid throughout this function.
@@ -464,7 +475,10 @@ static void xnintr_irq_handler(unsigned irq, void *cookie)
                goto unlock_and_exit;
        }
 #else
-       /* cookie always valid, attach/detach happens with IRQs disabled */
+       /*
+        * cookie always valid, xnintr_detach disables the line before
+        * releasing the interrupt.
+        */
        intr = cookie;
 #endif
        s = intr->isr(intr);
@@ -600,7 +614,8 @@ int __init xnintr_mount(void)
  * to enable IRQ-sharing of edge-triggered interrupts.
  *
  * @return 0 is returned on success. Otherwise, -EINVAL is returned if
- * @a irq is not a valid interrupt number.
+ * @a irq is not a valid interrupt number, or @a flags contains
+ * invalid bits.
  *
  * Environments:
  *
@@ -619,6 +634,9 @@ int xnintr_init(xnintr_t *intr,
        if (irq >= XNARCH_NR_IRQS)
                return -EINVAL;
 
+       if (flags & ~(XN_ISR_SHARED|XN_ISR_EDGE))
+               return -EINVAL;
+               
        intr->irq = irq;
        intr->isr = isr;
        intr->iack = iack;
@@ -703,7 +721,7 @@ EXPORT_SYMBOL_GPL(xnintr_destroy);
  * Rescheduling: never.
  *
  * @note Attaching an interrupt resets the tracked number of receipts
- * to zero.
+ * to zero. Interrupts must be enabled on entry.
  */
 
 int xnintr_attach(xnintr_t *intr, void *cookie)
@@ -714,27 +732,52 @@ int xnintr_attach(xnintr_t *intr, void *cookie)
        trace_mark(xn_nucleus, irq_attach, "irq %u name %s",
                   intr->irq, intr->name);
 
-       intr->cookie = cookie;
-       memset(&intr->stat, 0, sizeof(intr->stat));
-
-#ifdef CONFIG_SMP
-       xnarch_set_irq_affinity(intr->irq, nkaffinity);
-#endif /* CONFIG_SMP */
-
+       XENO_BUGON(NUCLEUS, spltest());
+retry:
        xnlock_get_irqsave(&intrlock, s);
 
        if (__testbits(intr->flags, XN_ISR_ATTACHED)) {
                ret = -EBUSY;
-               goto out;
+               goto fail;
+       }
+
+       /*
+        * If a teardown is in progress for this interrupt on a remote
+        * CPU running xnintr_detach, relax a bit then retry. Note
+        * that we can't have preempted any ongoing xnintr_detach on
+        * the local CPU, since teardown is done with interrupts off.
+        *
+        * CAUTION: when a teardown is detected, we must re-enable
+        * interrupts shortly to accept any critical IPI from the
+        * remote CPU in xnintr_detach.
+        */
+       if (test_bit(intr->irq, teardown)) {
+               xnlock_put_irqrestore(&intrlock, s);
+               goto retry;
        }
 
        ret = xnintr_irq_attach(intr);
-       if (ret)
-               goto out;
+       if (ret < 0)
+               goto fail;
+
+       intr->cookie = cookie;
+       memset(&intr->stat, 0, sizeof(intr->stat));
 
        __setbits(intr->flags, XN_ISR_ATTACHED);
        xnintr_stat_counter_inc();
-out:
+
+       xnlock_put_irqrestore(&intrlock, s);
+#ifdef CONFIG_SMP
+       /*
+        * If this is the first handler we installed for this IRQ, the
+        * line must still be disabled and we won't override any prior
+        * setting: we may set the affinity.
+        */
+       if (ret == 0)
+               xnarch_set_irq_affinity(intr->irq, nkaffinity);
+#endif /* CONFIG_SMP */
+       return 0;
+fail:
        xnlock_put_irqrestore(&intrlock, s);
 
        return ret;
@@ -760,8 +803,9 @@ EXPORT_SYMBOL_GPL(xnintr_attach);
  * the interrupt, or if the interrupt object was not attached. In both
  * cases, no action is performed.
  *
- * @note The caller <b>must not</b> hold nklock when invoking this service,
- * this would cause deadlocks.
+ * @note The caller <b>must not</b> hold nklock when invoking this
+ * service, this would cause deadlocks. Additionally, interrupts must
+ * be enabled on entry to this service.
  *
  * Environments:
  *
@@ -789,10 +833,33 @@ int xnintr_detach(xnintr_t *intr)
        __clrbits(intr->flags, XN_ISR_ATTACHED);
 
        ret = xnintr_irq_detach(intr);
-       if (ret)
+       xnintr_stat_counter_dec();
+       if (ret == 0)   /* still busy? */
                goto out;
 
-       xnintr_stat_counter_dec();
+       /*
+        * We have no more handlers on this interrupt line, so we may
+        * disable it. When paired with interrupt synchronization in
+        * xnarch_release_irq, this guarantees that no handler could
+        * be running concurrently on any remote CPU before we tell
+        * the pipeline to release the IRQ.
+        */
+       xnarch_disable_irq(intr->irq);
+       /*
+        * We use the teardown flag to prevent further attachment on
+        * the same IRQ line, after we dropped the lock to release the
+        * interrupt. We must drop the intrlock to prevent other CPUs
+        * from answering the critical IPI request sent when releasing
+        * the interrupt (we don't want any of them to spin on this
+        * lock with interrupts off waiting for us, while we wait for
+        * them to ack the IPI). However, we keep the interrupts off
+        * to prevent deadlocking with xnintr_attach on the local CPU.
+        */
+       __set_bit(intr->irq, teardown);
+       xnlock_put(&intrlock);
+       ret = xnintr_irq_release(intr);
+       xnlock_get(&intrlock);
+       __clear_bit(intr->irq, teardown);
  out:
        xnlock_put_irqrestore(&intrlock, s);
 


_______________________________________________
Xenomai-git mailing list
Xenomai-git@gna.org
https://mail.gna.org/listinfo/xenomai-git

Reply via email to