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

[Anders Blomdell <anders.blomdell@domain.hid>]

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