From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: Date: Tue, 21 Feb 2006 18:48:50 +0200 From: "Dmitry Adamushko" Subject: Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge) In-Reply-To: <43FAFE58.5060201@domain.hid> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="----=_Part_15956_28632741.1140540530511" 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> 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 ------=_Part_15956_28632741.1140540530511 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline > 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 =3D> some overhead that likely affects non-shared aware code t= oo. 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 ------=_Part_15956_28632741.1140540530511 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline
> Good point, leaves us with 2 possible return values for shared handler= s:
>
>   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 reenven= ting
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 handle= d by
>
> a) Documenting the potential conflict in the same place when describin= g
> 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 an= d
> PROPAGATE features as *additional* bits from HANDLED and
> NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all valid b= it
> combination present as constants can be more helpful for the user. We<= br> > 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 precis= ely").

I agree with you on PROPAGATE case, but NO_ENABLE that, as pointed out abov= e,
should (IMHO and at least, in theory) only mean "keep the IRQ line dis= abled"
(and have nothing to do with .end-ing the IRQ line) would be better support= ed.
But this is, again as was pointed out above, about some redesigning of the<= br> current code =3D> some overhead that likely affects non-shared aware cod= e 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 ------=_Part_15956_28632741.1140540530511--