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. Jan
signature.asc
Description: OpenPGP digital signature
