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

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

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

> 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).

HANDLED_NOEBNABLE could be supported too. There would be no need in
reenventing
a wheel, just do it the way Linux does it.
But it's about some additional re-designing of the current codebase
(e.g. nested calling for irq_enable/disable())
I'm not sure we do need it for something else rather than irq sharing code
but it affects the rest of the code.

And we have a kind of wrong concept :

XN_ISR_ENABLE (or NOENABLE) corresponds to xnarch_end_irq().

But the later one is not only about enabling the line, but
on some archs - about .end-ing it too (sending EOI).

And to support HANDLED_NOENABLE properly, those 2 have to be decoupled, i.e.
EOI should always be sent from xnintr_shirq_handler().

> Yes, "should". And this "should" is best be handled by
>
> a) Documenting the potential conflict in the same place when describing
> the return values
>
> b) Placing some debug warning in the nucleus' IRQ trampoline function to
> bail out (once per line) when running into such situation
>
> But I'm against any further runtime restrictions, especially as most
> drivers will never return anything else than NOT_HANDLED or HANDLED.
> Actually, this was the reason why I tried to separate the NO_ENABLE and
> PROPAGATE features as *additional* bits from HANDLED and
> NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all valid bit
> combination present as constants can be more helpful for the user. We
> just have to draw some line between the standard values and the
> additional gurus return codes
>
(documentation: "don't use NO_ENABLE or
> PROPAGATE unless you understand their side-effects and pitfalls
precisely").

I agree with you on PROPAGATE case, but NO_ENABLE that, as pointed out
above,
should (IMHO and at least, in theory) only mean "keep the IRQ line disabled"
(and have nothing to do with .end-ing the IRQ line) would be better
supported.
But this is, again as was pointed out above, about some redesigning of the
current code => some overhead that likely affects non-shared aware code too.


So on one hand,

I'm ready to re-work code with :

HANDLED and UNHANDLED (or NOINT)

+ 2 additional bits : NOENABLE and PROPAGATE.

and document it like you suggested "don't use NO_ENABLE or
PROPAGATE with shared interrupts
unless you understand their side-effects and pitfalls precisely";

on the other hand,

I'd say that I'm almost ready to vote against merging the irq sharing code
at all as it looks to be a rather partial solution.


--
Best regards,
Dmitry Adamushko

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