[Xenomai-core] [RFC, Experimental Patch] nested irq disable calls

[Xenomai-core] [RFC, Experimental Patch] nested irq disable calls

[Dmitry Adamushko]

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

Hi there,

the following patches illustrate _experimental_ implementation of nested irq
disable calls.
This new feature would allow us to have scalar return values of ISR and
avoid the need
for NOENABLE bit.

[ Ok, repeating myself one more time... we would have NONE, HANDLED,
PROPAGATE and it would be possible
to call xnintr_disable_nosync()/even xnintr_disable() from the ISR to defer
the IRQ line
enabling. ]

The pre-requirement : implement as much code as possible on the Xeno layer
with zero or
minimal changes on the ipipe layer (at least, they should not be intrusive
and difficult to maintain for
different archs).


2 main issues which are quite arguable when it comes to possible
implementations :

1) we need to store the "depth" counter and IRQ_DISABLED bit somewhere
(actually, this bit is not that necessary
as the non-zero "depth" counter indicates the same).

So where? There is a sole per-IRQ table that's available for all possible
configs - ipipe_domain::irqs[NR_IRQS].

So the minor changes below don't make any structure bigger. Read the comment
about the 3-d byte.


----------------------------- IPIPE changes
-------------------------------------------

--- ipipe.h    2006-02-27 15:10:41.000000000 +0100
+++ ipipe-exp.h    2006-03-02 12:08:27.000000000 +0100
@@ -62,6 +62,15 @@
 #define IPIPE_SHARED_FLAG    6
 #define IPIPE_EXCLUSIVE_FLAG    31    /* ipipe_catch_event() is the reason
why. */

+/*
+ * The 3-d byte of ipipe_domain::irqs[IRQ]::control is reserved for use
+ * by upper layers and must not be changed on the ipipe layer.
+ *
+ * e.g. experimantal support for nested irq disable calls in Xeno is based
on it.
+ */
+#define IPIPE_RESERVED_AREA_SHIFT    16
+#define IPIPE_RESERVED_AREA_MASK    (0xff << IPIPE_RESERVED_AREA_SHIFT)
+
 #define IPIPE_HANDLE_MASK    (1 << IPIPE_HANDLE_FLAG)
 #define IPIPE_PASS_MASK        (1 << IPIPE_PASS_FLAG)
 #define IPIPE_ENABLE_MASK    (1 << IPIPE_ENABLE_FLAG)
@@ -190,6 +199,12 @@ struct ipipe_domain_attr {

 #define __ipipe_cpudata_irq_hits(ipd,cpuid,irq)
((ipd)->cpudata[cpuid].irq_counters[irq].total_hits)

+#define __ipipe_get_reserved_area(ipd,irq) \
+    (((ipd)->irqs[irq].control & IPIPE_RESERVED_AREA_MASK) >>
IPIPE_RESERVED_AREA_SHIFT)
+
+#define __ipipe_set_reserved_area(ipd,irq,value) \
+    ((ipd)->irqs[irq].control &= (((value) << IPIPE_RESERVED_AREA_SHIFT) &
IPIPE_RESERVED_AREA_MASK)
+
 #define __ipipe_set_irq_bit(ipd,cpuid,irq) \
 do { \
     if (!test_bit(IPIPE_LOCK_FLAG,&(ipd)->irqs[irq].control)) { \


---------------------------------------------------------------------------------------

2) We need somehow to force the .end handler of the PIC (only PPC arch make
uses of it at the moment;
other archs - use .enable instead as .ending for those is just .enabling) to
_not_ re-enable the IRQ line
when the ISR has disabled the IRQ explicitly.

So there are 2 possible ways (at least, I can see only 2 now) :

    2.1) make changes for all PIC handlers supported for a given arch if
their .end handler is about re-enabling the IRQ line :

    BEFORE

    static void openpic_end_irq(unsigned int irq_nr)
    {
    if (!(irq_desc[irq_nr].status & (IRQ_DISABLED|IRQ_INPROGRESS))
        && irq_desc[irq_nr].action)
        openpic_enable_irq(irq_nr);
    }


    AFTER

    static void openpic_end_irq(unsigned int irq_nr)
    {
    if (!ipipe_root_domain_p()
        &&
!test_bit(IPIPE_DISABLED_FLAG,&ipipe_current_domain->irqs[irq_nr].control))
        return;


    if (!ipipe_root_domain_p() || !(irq_desc[irq_nr].status &
(IRQ_DISABLED|IRQ_INPROGRESS))
        && irq_desc[irq_nr].action)
        openpic_enable_irq(irq_nr);
    }


    2.2) do some trick.

    That's how this patch does. But, well, I do _not_ like this way.

    Look at the xnarch_end_irq()'s implementation below.

---
p.s.

So I tend to think that 2.2 is, at the very least, ugly if even it seems to
be safe at the first glance.

2.1 would be better probably. But then the ipipe layer must know at least
the DISABLED bit. What concerns me is that the logic is implemented mostly
in Xeno but the bits of this interface (check for DISABLED bit in the .end
handlers) is visible for ipipe. Bad localization, I mean.
Maybe something different from ipipe_set/get_reserved_area() should be
introduced to make DISABLED bit and "depth" counter invisible for the Xeno.
Smth like ipipe_irq_depth_inc/dec() and
ipipe_irq_set/clear_disabled() .. I'm not sure yet.

>From another point of view, this new feature seems not to be too intrusive
and not something really affecting the "fast path" so it could be used by
default, I guess. Or maybe we don't need it at all?

Any comments please?

---


--------------------------------- XENO changes
------------------------------------------------

diff -urp xenomai/include/asm-generic/hal.h
xenomai-nirq-2/include/asm-generic/hal.h
--- xenomai/include/asm-generic/hal.h    2006-02-27 14:42:59.000000000 +0100
+++ xenomai-nirq-2/include/asm-generic/hal.h    2006-03-02 12:12:
55.000000000 +0100
@@ -106,6 +106,9 @@ typedef rwlock_t rthal_rwlock_t;

 #define rthal_cpudata_irq_hits(ipd,cpu,irq)
__ipipe_cpudata_irq_hits(ipd,cpu,irq)

+#define rthal_get_reserved_area(ipd,irq)
__ipipe_get_reserved_area(ipd,irq)
+#define rthal_set_reserved_area(ipd,irq,value)
__ipipe_set_reserved_area(ipd,irq,value)
+
 #define rthal_local_irq_disable()
ipipe_stall_pipeline_from(&rthal_domain)
 #define rthal_local_irq_enable()
ipipe_unstall_pipeline_from(&rthal_domain)
 #define rthal_local_irq_save(x)        ((x) =
!!ipipe_test_and_stall_pipeline_from(&rthal_domain))
diff -urp xenomai/include/asm-generic/system.h
xenomai-nirq-2/include/asm-generic/system.h
--- xenomai/include/asm-generic/system.h    2006-02-27 14:42:59.000000000+0100
+++ xenomai-nirq-2/include/asm-generic/system.h    2006-03-03 11:28:
09.000000000 +0100
@@ -490,6 +490,14 @@ static inline unsigned long long xnarch_

 #ifdef XENO_INTR_MODULE

+#ifdef CONFIG_SMP
+static xnlock_t xnarch_intr_lock = XNARCH_LOCK_UNLOCKED;
+#endif
+
+/* Operating the per-IRQ reserved area providen by the ipipe layer. */
+#define XNARCH_IRQ_DISABLED    0x80
+#define XNARCH_IRQ_DEPTH_MASK    0x7f
+
 static inline int xnarch_hook_irq (unsigned irq,
                    rthal_irq_handler_t handler,
                    rthal_irq_ackfn_t ackfn,
@@ -507,19 +515,81 @@ static inline int xnarch_release_irq (un
 static inline int xnarch_enable_irq (unsigned irq)

 {
-    return rthal_irq_enable(irq);
+    int ret = 0;
+    char area;
+    spl_t s;
+
+    xnlock_get_irqsave(&xnarch_intr_lock,s);
+
+    area = rthal_get_reserved_area(&rthal_domain,irq);
+    switch (area & XNARCH_IRQ_DEPTH_MASK)
+    {
+    case 0:
+        xnlogerr("xnintr_enable() : depth == 0. "
+             "Unbalanced enable/disable calls for IRQ%d!\n",irq);
+        break;
+    case 1:
+        area &= ~XNARCH_IRQ_DISABLED;
+        ret = rthal_irq_enable(irq);
+    default:
+        rthal_set_reserved_area(&rthal_domain,irq,--area);
+    }
+
+    xnlock_put_irqrestore(&xnarch_intr_lock,s);
+    return ret;
 }

 static inline int xnarch_disable_irq (unsigned irq)

 {
-    return rthal_irq_disable(irq);
+    char area, depth;
+    int ret = 0;
+    spl_t s;
+
+    xnlock_get_irqsave(&xnarch_intr_lock,s);
+
+    area = rthal_get_reserved_area(&rthal_domain,irq);
+    depth = area & XNARCH_IRQ_DEPTH_MASK;
+
+    if (depth == XNARCH_IRQ_DEPTH_MASK)
+    {
+    xnlogerr("xnintr_disable_nosync() : depth == %d (MAX_ALLOWED)."
+         "Too many nested disable calls for IRQ%d.\n",depth,irq);
+    goto unlock_and_exit;
+    }
+
+    if (!depth)
+    {
+    area |= XNARCH_IRQ_DISABLED;
+    ret = rthal_irq_disable(irq);
+    }
+
+    rthal_set_reserved_area(&rthal_domain,irq,++area);
+
+unlock_and_exit:
+    xnlock_put_irqrestore(&xnarch_intr_lock,s);
+    return ret;
 }

 static inline int xnarch_end_irq (unsigned irq)

 {
-     return rthal_irq_end(irq);
+    int rt_off = rthal_get_reserved_area(&rthal_domain,irq) &
XNARCH_IRQ_DISABLED,
+    linux_off = rthal_irq_descp(irq)->status & IRQ_DISABLED,
+    ret;
+
+    if (rt_off)
+    set_bit(IRQ_DISABLED, &rthal_irq_descp(irq)->status);
+
+    /* We must not re-enable the IRQ line here in case it has been
explicitly
+       disabled by the ISR. To this end, we _temporary_ set the
IRQ_DISABLED bit
+       so that the .end handler of the PIC will not re-enable the IRQ line.
*/
+    ret = rthal_irq_end(irq);
+
+    if (!linux_off)
+    clear_bit(IRQ_DISABLED, &rthal_irq_descp(irq)->status);
+
+    return ret;
 }


 static inline void xnarch_chain_irq (unsigned irq)
diff -urp xenomai/include/nucleus/intr.h
xenomai-nirq-2/include/nucleus/intr.h
--- xenomai/include/nucleus/intr.h    2006-02-27 14:42:59.000000000 +0100
+++ xenomai-nirq-2/include/nucleus/intr.h    2006-03-03 11:26:14.000000000+0100
@@ -25,10 +25,7 @@
 /* Possible return values of ISR. */
 #define XN_ISR_NONE        0x1
 #define XN_ISR_HANDLED     0x2
-/* Additional bits. */
 #define XN_ISR_PROPAGATE 0x100
-#define XN_ISR_NOENABLE  0x200
-#define XN_ISR_BITMASK     ~0xff

 /* Creation flags. */
 #define XN_ISR_SHARED     0x1
diff -urp xenomai/ksrc/nucleus/intr.c xenomai-nirq-2/ksrc/nucleus/intr.c
--- xenomai/ksrc/nucleus/intr.c    2006-02-27 14:42:59.000000000 +0100
+++ xenomai-nirq-2/ksrc/nucleus/intr.c    2006-03-03 10:44:17.000000000+0100
@@ -46,6 +46,11 @@ static void xnintr_irq_handler(unsigned
 /* Helper functions. */
 static int xnintr_shirq_attach(xnintr_t *intr, void *cookie);
 static int xnintr_shirq_detach(xnintr_t *intr);
+static void xnintr_synchronize(xnintr_t *intr);
+
+#else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
+
+#define xnintr_synchronize(irq)    xnarch_memory_barrier()

 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */

@@ -72,27 +77,18 @@ static int xnintr_shirq_detach(xnintr_t
  * return this value when it determines that the interrupt request has not
been
  * issued by the dedicated hardware device.
  *
- * In addition, one of the following bits may be set by the ISR :
- *
- * NOTE: use these bits with care and only when you do understand their
effect
- * on the system.
- * The ISR is not encouraged to use these bits in case it shares the IRQ
line
- * with other ISRs in the real-time domain.
- *
  * - XN_ISR_PROPAGATE tells the nucleus to require the real-time control
  * 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.
  *
- * - XN_ISR_NOENABLE causes the nucleus to ask the real-time control
- * layer _not_ to re-enable the IRQ line (read the following section).
- * xnarch_end_irq() must be called to re-enable the IRQ line later.
- *
  * The nucleus re-enables the IRQ line by default. Over some real-time
  * control layers which mask and acknowledge IRQs, this operation is
  * necessary to revalidate the interrupt channel so that more interrupts
  * can be notified.
+ * The ISR may issue xnintr_disable_nosync() call in order to defer
+ * the IRQ line enabling until later.
  *
  * A count of interrupt receipts is tracked into the interrupt
  * descriptor, and reset to zero each time the interrupt object is
@@ -345,12 +341,22 @@ int xnintr_enable (xnintr_t *intr)
  * Rescheduling: never.
  */

-int xnintr_disable (xnintr_t *intr)
+int xnintr_disable_nosync (xnintr_t *intr)

 {
     return xnarch_disable_irq(intr->irq);
 }

+
+int xnintr_disable (xnintr_t *intr)
+
+{
+    int ret = xnintr_disable_nosync(intr);
+    xnintr_synchronize(intr);
+
+    return ret;
+}
+
 /*!
  * \fn xnarch_cpumask_t xnintr_affinity (xnintr_t *intr, xnarch_cpumask_t
cpumask)
  * \brief Set interrupt's processor affinity.
@@ -405,9 +411,9 @@ static void xnintr_irq_handler (unsigned
     s = intr->isr(intr);
     ++intr->hits;

-    if (s & XN_ISR_PROPAGATE)
+    if (s == XN_ISR_PROPAGATE)
     xnarch_chain_irq(irq);
-    else if (!(s & XN_ISR_NOENABLE))
+    else
     xnarch_end_irq(irq);

     if (--sched->inesting == 0 && xnsched_resched_p())
@@ -487,7 +493,9 @@ static void xnintr_shirq_handler (unsign

     while (intr)
         {
-    s |= intr->isr(intr) & XN_ISR_BITMASK;
+    int temp = intr->isr(intr);
+    if (temp > s)
+        s = temp;
         ++intr->hits;
         intr = intr->next;
         }
@@ -495,9 +503,9 @@ static void xnintr_shirq_handler (unsign

     --sched->inesting;

-    if (s & XN_ISR_PROPAGATE)
+    if (s == XN_ISR_PROPAGATE)
     xnarch_chain_irq(irq);
-    else if (!(s & XN_ISR_NOENABLE))
+    else
     xnarch_end_irq(irq);

     if (sched->inesting == 0 && xnsched_resched_p())
@@ -535,19 +543,19 @@ static void xnintr_edge_shirq_handler (u

     while (intr != end)
     {
-    int ret = intr->isr(intr),
-        code = ret & ~XN_ISR_BITMASK,
-        bits = ret & XN_ISR_BITMASK;
+    int ret = intr->isr(intr);

-    if (code == XN_ISR_HANDLED)
+    if (ret == XN_ISR_HANDLED)
         {
         ++intr->hits;
         end = NULL;
-        s |= bits;
         }
-    else if (code == XN_ISR_NONE && end == NULL)
+    else if (ret == XN_ISR_NONE && end == NULL)
         end = intr;

+    if (ret > s)
+        s = ret;
+
     if (counter++ > MAX_EDGEIRQ_COUNTER)
         break;

@@ -562,9 +570,9 @@ static void xnintr_edge_shirq_handler (u
     if (counter > MAX_EDGEIRQ_COUNTER)
     xnlogerr("xnintr_edge_shirq_handler() : failed to get the IRQ%d line
free.\n", irq);

-    if (s & XN_ISR_PROPAGATE)
+    if (s == XN_ISR_PROPAGATE)
     xnarch_chain_irq(irq);
-    else if (!(s & XN_ISR_NOENABLE))
+    else
     xnarch_end_irq(irq);

     if (sched->inesting == 0 && xnsched_resched_p())
@@ -645,7 +653,7 @@ unlock_and_exit:
     return err;
 }

-int xnintr_shirq_detach (xnintr_t *intr)
+static int xnintr_shirq_detach (xnintr_t *intr)
 {
     xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
     xnintr_t *e, **p = &shirq->handlers;
@@ -695,6 +703,11 @@ int xnintr_shirq_detach (xnintr_t *intr)
     return err;
 }

+static void xnintr_synchronize (xnintr_t *intr)
+{
+    xnintr_shirq_spin(&xnshirqs[intr->irq]);
+}
+
 int xnintr_mount(void)
 {
     int i;


-----------------------------------------------------------------------------------------------




--
Best regards,
Dmitry Adamushko

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

Re: [Xenomai-core] [RFC, Experimental Patch] nested irq disable calls

[Philippe Gerum]

Dmitry Adamushko wrote:
> 
> Hi there,
> 
> the following patches illustrate _experimental_ implementation of nested 
> irq disable calls.
> This new feature would allow us to have scalar return values of ISR and 
> avoid the need
> for NOENABLE bit.
> 
> [ Ok, repeating myself one more time... we would have NONE, HANDLED, 
> PROPAGATE and it would be possible
> to call xnintr_disable_nosync()/even xnintr_disable() from the ISR to 
> defer the IRQ line
> enabling. ]
> 
> The pre-requirement : implement as much code as possible on the Xeno 
> layer with zero or
> minimal changes on the ipipe layer (at least, they should not be 
> intrusive and difficult to maintain for
> different archs).
> 
> 
> 2 main issues which are quite arguable when it comes to possible 
> implementations :
> 
> 1) we need to store the "depth" counter and IRQ_DISABLED bit somewhere 
> (actually, this bit is not that necessary
> as the non-zero "depth" counter indicates the same).
> 
> So where? There is a sole per-IRQ table that's available for all 
> possible configs - ipipe_domain::irqs[NR_IRQS].
> 
> So the minor changes below don't make any structure bigger. Read the 
> comment about the 3-d byte.
> 
> 
> ----------------------------- IPIPE changes 
> -------------------------------------------
> 
> --- ipipe.h    2006-02-27 15:10:41.000000000 +0100
> +++ ipipe-exp.h    2006-03-02 12:08:27.000000000 +0100
> @@ -62,6 +62,15 @@
>  #define IPIPE_SHARED_FLAG    6
>  #define IPIPE_EXCLUSIVE_FLAG    31    /* ipipe_catch_event() is the 
> reason why. */
>  
> +/*
> + * The 3-d byte of ipipe_domain::irqs[IRQ]::control is reserved for use
> + * by upper layers and must not be changed on the ipipe layer.
> + * 
> +

<snip>

> 2) We need somehow to force the .end handler of the PIC (only PPC arch 
> make uses of it at the moment;
> other archs - use .enable instead as .ending for those is just 
> .enabling) to _not_ re-enable the IRQ line
> when the ISR has disabled the IRQ explicitly.
> 
> So there are 2 possible ways (at least, I can see only 2 now) :
> 
>     2.1) make changes for all PIC handlers supported for a given arch if 
> their .end handler is about re-enabling the IRQ line :
>    
>     BEFORE
>    
>     static void openpic_end_irq(unsigned int irq_nr)
>     {
>     if (!(irq_desc[irq_nr].status & (IRQ_DISABLED|IRQ_INPROGRESS))
>         && irq_desc[irq_nr].action)
>         openpic_enable_irq(irq_nr);
>     }
> 
>    
>     AFTER
>    
>     static void openpic_end_irq(unsigned int irq_nr)
>     {
>     if (!ipipe_root_domain_p()
>         && 
> !test_bit(IPIPE_DISABLED_FLAG,&ipipe_current_domain->irqs[irq_nr].control))
>         return;
>     

- !test_bit(IPIPE_DISABLED_FLAG,&ipipe_current_domain->irqs[irq_nr].control))
+ test_bit(IPIPE_DISABLED_FLAG,&ipipe_current_domain->irqs[irq_nr].control))

?

Additionally, there is another issue we discussed once with Anders, which is 
related to not sending EOI twice after the shared IRQ already ended by a RT domain 
has been fully propagated down the pipeline to Linux; some kind of 
test_and_clear_temporary_disable flag, would do, I guess. The other way would be 
to test_and_set some "ended" flag for the outstanding IRQ when the ->end() routine 
is entered, clearing this flag before pipelining the IRQ in __ipipe_walk_pipeline().

Actually, I'm now starting to wonder why we would want to permanently disable an 
IRQ line from a RT domain, which is known to be used by Linux. Is this what 
IPIPE_DISABLED_FLAG is expected to be used for, or is it only there to handle the 
transient disabled state discussed above?

>    
>     if (!ipipe_root_domain_p() || !(irq_desc[irq_nr].status & 
> (IRQ_DISABLED|IRQ_INPROGRESS))
>         && irq_desc[irq_nr].action)
>         openpic_enable_irq(irq_nr);
>     }
>    

There is another way for most archs, which is to add such code to the ->end() 
routine override in ipipe-root.c; this would be simpler and safer than fixing such 
routine for each and every kind of interrupt controller. x86 directly pokes into 
the PIC code and does not overrides IRQ control routines, though.

> 
>     2.2) do some trick.
>    
>     That's how this patch does. But, well, I do _not_ like this way.
>    

Same feeling here; this blurs the line between Adeos and Xenomai, which is bad.

>     Look at the xnarch_end_irq()'s implementation below.
>    
> ---
> p.s.
> 
> So I tend to think that 2.2 is, at the very least, ugly if even it seems 
> to be safe at the first glance.
> 
> 2.1 would be better probably. But then the ipipe layer must know at 
> least the DISABLED bit. What concerns me is that the logic is 
> implemented mostly in Xeno but the bits of this interface (check for 
> DISABLED bit in the .end handlers) is visible for ipipe. Bad 
> localization, I mean.
> Maybe something different from ipipe_set/get_reserved_area() should be 
> introduced to make DISABLED bit and "depth" counter invisible for the 
> Xeno. Smth like ipipe_irq_depth_inc/dec() and
> ipipe_irq_set/clear_disabled() .. I'm not sure yet.

IRQ sharing is an exception case for dealing with legacy hw; let's not introduce 
old new legacy code into Adeos which is only a minimalistic virtualization layer, 
just for the purpose of it; especially since we can implement this support at a 
higher level, the one that actually wants to provide the full semantics of IRQ 
sharing. Adeos is better at providing simple mechanisms, policies are Xenomai's 
business there.

However, there must be a mean to deal with the particular ->end() issue at Adeos 
level, but this should be decoupled from the disable nesting count issue.

> 
>  >From another point of view, this new feature seems not to be too 
> intrusive and not something really affecting the "fast path" so it could 
> be used by default, I guess. Or maybe we don't need it at all?
> 

The disable nesting count at Xenomai level is needed to let ISRs act independently 
from each others wrt interrupt masking - or at the very least, to let them think 
they do. This is strictly a Xenomai issue, not an Adeos one. If we keep it at this 
level, then using the xnintr struct to store the nesting counter becomes an 
option, I guess.

-- 

Philippe.

Re: [Xenomai-core] [RFC, Experimental Patch] nested irq disable calls

[Dmitry Adamushko]

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

> >
> >     BEFORE
> >
> >     static void openpic_end_irq(unsigned int irq_nr)
> >     {
> >     if (!(irq_desc[irq_nr].status & (IRQ_DISABLED|IRQ_INPROGRESS))
> >         && irq_desc[irq_nr].action)
> >         openpic_enable_irq(irq_nr);
> >     }
> >
> >
> >     AFTER
> >
> >     static void openpic_end_irq(unsigned int irq_nr)
> >     {
> >     if (!ipipe_root_domain_p()
> >         &&
> >
!test_bit(IPIPE_DISABLED_FLAG,&ipipe_current_domain->irqs[irq_nr].control))
> >         return;
> >
>
> -
!test_bit(IPIPE_DISABLED_FLAG,&ipipe_current_domain->irqs[irq_nr].control))
> +
test_bit(IPIPE_DISABLED_FLAG,&ipipe_current_domain->irqs[irq_nr].control))
>
> ?

Yep.


> Additionally, there is another issue we discussed once with Anders, which
is
> related to not sending EOI twice after the shared IRQ already ended by a
RT domain
> has been fully propagated down the pipeline to Linux;
> some kind of test_and_clear_temporary_disable flag, would do, I guess. The
other way would be
> to test_and_set some "ended" flag for the outstanding IRQ when the ->end()
routine
> is entered, clearing this flag before pipelining the IRQ in
__ipipe_walk_pipeline().
>
> Actually, I'm now starting to wonder why we would want to permanently
disable an
> IRQ line from a RT domain, which is known to be used by Linux.
> Is this what IPIPE_DISABLED_FLAG is expected to be used for, or is it only
there to handle the
> transient disabled state discussed above?

Why "permanently"? I would see the following scenario - an ISR wants to
_temporary_ defer an IRQ line enabling
until some later stage (e.g. rt_task which is a bottom half).
This is the only reason why xnarch_end_irq() or some later step in it (in
this case ->end() ) must be aware
of IPIPE_DISABLED_FLAG.

Why the currently used approach is not that good for it (NOENABLE) ?

1)   it actually defers (for some PICs) not only "enabling" but sending an
EOI too;
    As a consequence :

2)   rthal_end_irq() (on PPC and not just xnintr_enable() or
rthal_enable_irq()) must be called in bottom half
    to re-endable the IRQ line;

3)   does not co-exist well with the shared interrupts support (I don't mean
sharing between RT and not-RT doamins here).
    Although it's not a common case if a few ISRs on the same shared line
want to defer enabling, esp. for
    real-time domain;

4)   it's a bit and if we would like to use only scalar values one day then
something
    like HANDLED, HANDLED_NOENABLE would be needed;

The support for nested irq enable/disable calls would resolve all the
"restrictions" above but the question is
whether we really need to resolve them.

In the same vein, I'd like to know you vision of the "nested irq
enable/disable calls support". Any use cases?

>
> >
> >     if (!ipipe_root_domain_p() || !(irq_desc[irq_nr].status &
> > (IRQ_DISABLED|IRQ_INPROGRESS))
> >         && irq_desc[irq_nr].action)
> >         openpic_enable_irq(irq_nr);
> >     }
> >
>
> There is another way for most archs, which is to add such code to the
->end()
> routine override in ipipe-root.c; this would be simpler and safer than
fixing such
> routine for each and every kind of interrupt controller. x86 directly
pokes into
> the PIC code and does not overrides IRQ control routines, though.

I didn't know about them as I mostly looked at the x86 implementation.

This gives as control over per-domain irq locking/unlocking
(ipipe_irq_lock/unlock()),
__ipipe_std_irq_dtype[irq].end() is always called unconditionally (as a
result, .enable() for some PICs).
That said, the IRQ line is actually on, the interrupts are just not handled
but accumulated in the pipe-log.

Actually, why is ipipe_irq_unlock(irq) necessary in
__ipipe_override_irq_end()? ipipe_irq_lock() is not
called in __ipipe_ack_irq(). Is it locked somewhere else? At least, I
haven't found explicit ipipe_irq_lock()
or __ipipe_lock_irq() calls anywhere else.


> [skip-skip-skip]
>
> >
> >  >From another point of view, this new feature seems not to be too
> > intrusive and not something really affecting the "fast path" so it could
> > be used by default, I guess. Or maybe we don't need it at all?
> >
>
> The disable nesting count at Xenomai level is needed to let ISRs act
independently
> from each others wrt interrupt masking - or at the very least, to let them
think
> they do. This is strictly a Xenomai issue, not an Adeos one. If we keep it
at this
> level, then using the xnintr struct to store the nesting counter becomes
an
> option, I guess.

Let's start from defining possible use cases with nested irq enable/disable
calls then.
Maybe we just don't need them at all (at least the same way Linux deals with
them)?

>
> --
>
> Philippe.


--
Best regards,
Dmitry Adamushko

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

Re: [Xenomai-core] [RFC, Experimental Patch] nested irq disable calls

[Philippe Gerum]

Dmitry Adamushko wrote:

<snip>

>  > Additionally, there is another issue we discussed once with Anders, 
> which is
>  > related to not sending EOI twice after the shared IRQ already ended 
> by a RT domain
>  > has been fully propagated down the pipeline to Linux;
>  > some kind of test_and_clear_temporary_disable flag, would do, I 
> guess. The other way would be
>  > to test_and_set some "ended" flag for the outstanding IRQ when the 
> ->end() routine
>  > is entered, clearing this flag before pipelining the IRQ in 
> __ipipe_walk_pipeline().
>  >
>  > Actually, I'm now starting to wonder why we would want to permanently 
> disable an
>  > IRQ line from a RT domain, which is known to be used by Linux.
>  > Is this what IPIPE_DISABLED_FLAG is expected to be used for, or is it 
> only there to handle the
>  > transient disabled state discussed above?
> 
> Why "permanently"?

This is what I thought you wanted to provide support for in order to address all
the pending issues, reading your previous post, but it seems I was wrong.

 > I would see the following scenario - an ISR wants to
> _temporary_ defer an IRQ line enabling
> until some later stage (e.g. rt_task which is a bottom half).
> This is the only reason why xnarch_end_irq() or some later step in it 
> (in this case ->end() ) must be aware
> of IPIPE_DISABLED_FLAG.
> 
> Why the currently used approach is not that good for it (NOENABLE) ?
> 
> 1)   it actually defers (for some PICs) not only "enabling" but sending 
> an EOI too;
>     As a consequence :
>

This is no more the case for ppc over which Adeos had this bug Anders reported and 
fixed. For each arch/pic pairs, ->ack() should now send eoi and likely mask the 
outstanding IRQ, whilst ->end() should only unmask it as needed. AFAICT after a 
brief inspection, x86, ppc, ia64, and blackfin ports look ok regarding this. (I 
did not check neither the ARM nor ppc64 ports yet, though). But in any case, this 
is the way Adeos is expected to behave, and it should be fixed iff it doesn't.

> 2)   rthal_end_irq() (on PPC and not just xnintr_enable() or 
> rthal_enable_irq()) must be called in bottom half
>     to re-endable the IRQ line;
> 
> 3)   does not co-exist well with the shared interrupts support (I don't 
> mean sharing between RT and not-RT doamins here).
>     Although it's not a common case if a few ISRs on the same shared 
> line want to defer enabling, esp. for
>     real-time domain;
>

Talking about intra-domain sharing, I assume that such deferral only makes sense 
for implementing full top/bottom-halves approach involving threads. If we stick to 
the simplest form where all the interrupt processing is done from within the ISRs, 
we only have two issues to deal with:

1. Make sure that interrupt ending (i.e. unmasking) occurs only once at hw level;
2. Make sure not to spuriously re-enable an IRQ line from some ISR which would 
have been disabled by a previously called ISR. Said differently, ensure that ISRs 
  which are part of the same chain of shared IRQ handlers do not step on each 
others toes with respect to the PIC status.

Additionally, the way we solve these issues should be compatible with sharing IRQs 
between different domain handlers. 1. is easy to achieve using a logic that starts 
right before the outstanding IRQ is pipelined, and ends after the first call to 
end the interrupt is seen. 2. is also trivial: if some domain in the pipeline 
wants to disable the IRQ line, it certainly does not want Linux or any other 
domain down the pipeline to either play or end the current IRQ. So the solution is 
to stop propagating the IRQ down the pipeline after it has been disabled, and we 
should be done. In any case, it's a local decision which must be taken by the ISR 
code itself and enforced by the nucleus; I don't see any need for some global 
logic there.

> 4)   it's a bit and if we would like to use only scalar values one day 
> then something
>     like HANDLED, HANDLED_NOENABLE would be needed;
> 
> The support for nested irq enable/disable calls would resolve all the 
> "restrictions" above but the question is
> whether we really need to resolve them.
>
> In the same vein, I'd like to know you vision of the "nested irq 
> enable/disable calls support". Any use cases?
> 

I tend to think that it really depends on the complexity we want to be able to
deal with when it comes to interrupt sharing in a single domain (i.e. the
real-time one). Either,

1) ending the IRQ must happen from within any of the shared interrupt handler 
connected to the interrupt line;

2) ending the IRQ might be delegated to threads in order to support a complete top
and bottom-halves approach.

In case of scenario #1, we need to make sure that unmasking occurs only once:
first because it's safer, second because it's cheaper (e.g. fiddling with the x86
XT-PIC through the ISA bus is expensive, so we'd better do this once as needed, 
since well, freaking x86+ISA is the major reason why we are currently banging our 
heads on the wall devising an interrupt sharing scheme in the first place...).
Instead of trying to check for the unmask-unless-disabled condition, it seems
possible to rather check for the unmask-if-not-done one. IOW, the first handler to
call ->end() would actually unmask, subsequent invocations would lead to a no-op.
As you already stated, the pieces of code implementing such guard against multiple 
ending calls would have to be handled by Adeos directly. Solving the "spurious 
re-enabling" issue without resorting to braindead tricks seems out of reach. This 
is where the disable nesting count would help, I guess.

Having ISRs return scalar values would be possible too, provided that handlers
would then be allowed to call the ->end() routine as many times as they wish, 
while the chained handlers are invoked in loop; i.e. we could get rid of NOENABLE,
just asking each ISR to explicitely end the interrupt as part of their common duty.

2) is more complex, because it's an actual deferral of the ending call unlike 1). 
There, we have to handle the case where all ISRs sharing the same interrupt line 
would run _before_ any threaded bottom-half could execute. IOW, relying on the 
unmask-if-not-done guard flag which is only valid while the domain is in interrupt 
processing state is no more an option, because the logic has to span over non-ISR 
contexts, and some ISR different from the one implementing the top half might well 
decide to end the IRQ under our feet before the bottom half thread had a chance to 
execute. In order to sanely deal with this issue, a disable nesting count is 
likely the best approach.

One way to implement it could be as follows:

- ISRs ask for incrementing (disable) or decrementing if positive (enable) the 
disable nesting count associated with the interrupt line, either by calling a 
specific support routine, or returning a proper action flag to the nucleus. The 
action of decrementing the disable count down to zero would _not_ trigger the 
actual ending at hw level, just bookkeeping the status.
- xnintr_end would be introduced, in order to better fit with the actual semantics 
(i.e. ending != enabling an interrupt). This call would check the current disable 
count for the IRQ and invoke the ->end() routine if zero. ISRs (and bottom halves) 
would be required to use xnintr_end after normal operations, but would still be 
allowed to call xnintr_disable, e.g. in case of emergency situation.
- Upon return from the last ISR, the nucleus would call xnintr_end.
- xnintr_disable would increment the disable count and forward the request to the 
hw as a result of transitioning from 0 to 1.
- xnintr_enable would do the exact opposite of xnintr_disable.
- bottom-halves could then call xnintr_end when they are done with processing the 
outstanding IRQ.

Sharing the count between the enable/disable and ending actions simply reflects 
the reality: ->end() routines have to check for the disable state before 
proceeding, because most of the time, they both deal with the IRQ masking state.
Unlike 1), such implementation could be fully provided by Xenomai, remaining 
unknown from Adeos. We would still need to implement the unmask-if-not-done bit at 
Adeos level to deal with extra-domain sharing, though.

To sum up, I agree with you that addressing #2 directly through a disable nesting 
count would solve those issues quite more elegantly.

>  >
>  > >
>  > >     if (!ipipe_root_domain_p() || !(irq_desc[irq_nr].status &
>  > > (IRQ_DISABLED|IRQ_INPROGRESS))
>  > >         && irq_desc[irq_nr].action)
>  > >         openpic_enable_irq(irq_nr);
>  > >     }
>  > >
>  >
>  > There is another way for most archs, which is to add such code to the 
> ->end()
>  > routine override in ipipe-root.c; this would be simpler and safer 
> than fixing such
>  > routine for each and every kind of interrupt controller. x86 directly 
> pokes into
>  > the PIC code and does not overrides IRQ control routines, though.
> 
> I didn't know about them as I mostly looked at the x86 implementation.
> 
> This gives as control over per-domain irq locking/unlocking 
> (ipipe_irq_lock/unlock()),
> __ipipe_std_irq_dtype[irq].end() is always called unconditionally (as a 
> result, .enable() for some PICs).
> That said, the IRQ line is actually on, the interrupts are just not 
> handled but accumulated in the pipe-log.
> 
> Actually, why is ipipe_irq_unlock(irq) necessary in 
> __ipipe_override_irq_end()? ipipe_irq_lock() is not
> called in __ipipe_ack_irq(). Is it locked somewhere else? At least, I 
> haven't found explicit ipipe_irq_lock()
> or __ipipe_lock_irq() calls anywhere else.

Basically because as documented in __do_IRQ, the ->end() handler has to deal with
interrupts which got disabled while the handler was running. If for some reason,
some IRQ handler decides to disable its own IRQ line, then the lock bit would be
raised in the pipeline for this IRQ too as a result of calling disable_irq().
Therefore, ->end() must unlock the IRQ at pipeline level whenever it eventually 
decides to unmask.

>  
> 
>  > [skip-skip-skip]
>  >
>  > >
>  > >  >From another point of view, this new feature seems not to be too
>  > > intrusive and not something really affecting the "fast path" so it 
> could
>  > > be used by default, I guess. Or maybe we don't need it at all?
>  > >
>  >
>  > The disable nesting count at Xenomai level is needed to let ISRs act 
> independently
>  > from each others wrt interrupt masking - or at the very least, to let 
> them think
>  > they do. This is strictly a Xenomai issue, not an Adeos one. If we 
> keep it at this
>  > level, then using the xnintr struct to store the nesting counter 
> becomes an
>  > option, I guess.
> 
> Let's start from defining possible use cases with nested irq 
> enable/disable calls then.
> Maybe we just don't need them at all (at least the same way Linux deals 
> with them)?
> 
>  >
>  > --
>  >
>  > Philippe.
> 
> 
> -- 
> Best regards,
> Dmitry Adamushko


-- 

Philippe.

Re: [Xenomai-core] [RFC, Experimental Patch] nested irq disable calls

[Dmitry Adamushko]

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

<snip>

> > I would see the following scenario - an ISR wants to
> > _temporary_ defer an IRQ line enabling
> > until some later stage (e.g. rt_task which is a bottom half).
> > This is the only reason why xnarch_end_irq() or some later step in it
> > (in this case ->end() ) must be aware
> > of IPIPE_DISABLED_FLAG.
> >
> > Why the currently used approach is not that good for it (NOENABLE) ?
> >
> > 1)   it actually defers (for some PICs) not only "enabling" but sending
> > an EOI too;
> >     As a consequence :
> >
>
> This is no more the case for ppc over which Adeos had this bug Anders
reported and
> fixed.

I thought that the actual reason of that problem was the use of
rthal_enable_irq() in xnintr_irq_handler()
to play the IRQ .ending phase, not taking into account that .ending is not
always == .enabling.
As a consequence, it didn't work for some ppc/PICs where the EOI is sent by
.end and not .ack and
that's why rthal_end_irq() has been recently introduced.

> For each arch/pic pairs, ->ack() should now send eoi and likely mask the
> outstanding IRQ, whilst ->end() should only unmask it as needed.
> AFAICT after a brief inspection, x86, ppc, ia64, and blackfin ports look
ok regarding this. (I
> did not check neither the ARM nor ppc64 ports yet, though).
> But in any case, this is the way Adeos is expected to behave, and it
should be fixed iff it doesn't.

This actually makes the implemention of nested irq disable calls simpler.

But is such a rework of the PIC logic always possible and correct, hw-wise?

let's consider 2 cases :

1)
.ack = NULL (do nothing)
.end = send EOI

2)
.ack = { mask and send EOI }
.end = unmask

I guess, both 1) and 2) keep the line "disabled" until .end takes place.
But 2) requires 2 more opertions (mask and unmask) and, hence, it gives high
overhead, esp. on some sluggish archs with not memory-mapped i/o ports.

Are those variants always safely exchangable?

> <snip>
> To sum up, I agree with you that addressing #2 directly through a disable
nesting
> count would solve those issues quite more elegantly.

Good.

> > Actually, why is ipipe_irq_unlock(irq) necessary in
> > __ipipe_override_irq_end()? ipipe_irq_lock() is not
> > called in __ipipe_ack_irq(). Is it locked somewhere else? At least, I
> > haven't found explicit ipipe_irq_lock()
> > or __ipipe_lock_irq() calls anywhere else.
>
> Basically because as documented in __do_IRQ, the ->end() handler has to
deal with
> interrupts which got disabled while the handler was running. If for some
reason,
> some IRQ handler decides to disable its own IRQ line, then the lock bit
would be
> raised in the pipeline for this IRQ too as a result of calling
disable_irq().
> Therefore, ->end() must unlock the IRQ at pipeline level whenever it
eventually
> decides to unmask.

So roughly, __ipipe_irq_lock() is coupled with ->disable() while
__ipipe_irq_unlock() -
with ->enable().
Actually, I was confused by the fact that .ack normally does the same thing
as ->disable(), i.e. masking, but there is no corresponding
__ipipe_irq_lock() call there.
IOW, a number of __ipipe_irq_lock() calls doesn't correlate to a number of
__ipipe_irq_unlock()
calls as 1:1.

And actually this saves us from a possible problem, I guess.
.ack and .end may be issued from different domains and that, in turn, leads
to calling
__ipipe_irq_lock() and __ipipe_irq_unlock() from different domains too. As a
result, a given
IRQ line is enabled (at the hw layer) but remains to be "LOCKED" in the
domain where .ack
took place.

The same may happen if a rt ISR calls ->enable() and then asks nucleus to
propagate an interrupt
down to the Linux domain where .end takes place.


> --
>
>Philippe.
>

--
Best regards,
Dmitry Adamushko

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