From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <43FB6102.1070004@domain.hid> Date: Tue, 21 Feb 2006 19:50:42 +0100 From: Anders Blomdell MIME-Version: 1.0 Subject: Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge) References: <43F4E2D3.4010705@domain.hid> <43F793AF.4080805@domain.hid> <43F9C9C1.60101@domain.hid> <43FAD322.4060001@domain.hid> <43FAF94C.4080709@domain.hid> <43FAFE58.5060201@domain.hid> <43FB480E.9020808@domain.hid> <43FB529B.3040207@domain.hid> In-Reply-To: <43FB529B.3040207@domain.hid> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: "Xenomai life and development \(bug reports, patches, discussions\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: xenomai@xenomai.org 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