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

[Jan Kiszka <jan.kiszka@domain.hid>]

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