Dmitry Adamushko wrote:

 > Good point, leaves us with 2 possible return values for shared handlers:
> 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.
Yes, but it breaks decoupling between shared handlers; interrupts will be deferred for all [shared] handlers until it is properly ended.

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().
But the one returning HANDLED_NOENABLE is likely to leave the interrupt asserted, hence we can't EOI at this point (unless NO_ENABLE means DISABLE).

 > 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 :


+ 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.
I vote for (even though I'm the one who complains the most), BUT I think it is important to keep the rules for using it simple (that's why I worry about the plethora of return-flags).




Reply via email to