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

["Dmitry Adamushko" <dmitry.adamushko@domain.hid>]

[-- 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 --]