Jan Kiszka wrote:
Anders Blomdell wrote:
Dmitry Adamushko wrote:
> 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.
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().
Agree
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).
I guess this is what Dmitry meant: explicitly call disable() if one or
more ISRs returned NOENABLE - at least on archs where end != enable.
Will this work? We could then keep on using the existing IRQ-enable API
from bottom-half IRQ tasks. But what about NOENABLE+PROPAGATE? Will this
special case still mean NOT to end the ISR (as Linux will do)?
Bah, we are running in circles, I'm afraid. I think it's better to call
NOENABLE NOEOI, which will indeed mean to not end this line (as it is
the current situation anyway, isn't it?), and leave the user with what
(s)he can do with such a feature. We found out that there are trillions
of ways to shoot oneself into the foot with NOENABLE and PROPAGATE, and
we cannot prevent most of them. So let's stop trying, at least for this
patch!
> 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.
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).
And I'm with you here: My original proposal (2 base-states + 2 bits)
created 8 expressible states while your version only knows 4 states -
those which make sense most (and 2 of them are still the ones recommand
for the masses).
For RTDM I'm now almost determined to rework the API in way that only
HANDLED/UNHANDLED (or what ever their names will be) get exported, any
additional guru features will remain excluded as long as we have no
clean usage policy for them.
You have my vote for this.
--
Anders
_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core
