Anders Blomdell wrote:
> Dmitry Adamushko wrote:
>>
>> On 21/02/06, *Anders Blomdell* <[EMAIL PROTECTED]
>> <mailto:[EMAIL PROTECTED]>> wrote:
>>
>>     Dmitry Adamushko wrote:
>>      >
>>      > N.B. Amongst other things, some thoughts about CHAINED with shared
>>      > interrupts.
>>      >
>>      >
>>      > On 20/02/06, *Anders Blomdell* < [EMAIL PROTECTED]
>>     <mailto:[EMAIL PROTECTED]>
>>      > <mailto:[EMAIL PROTECTED]
>>     <mailto:[EMAIL PROTECTED]>>> wrote:
>>      >
>>      >
>>      >
>>      >     A number of questions arise:
>>      >
>>      >     1. What happens if one of the shared handlers leaves the
>>     interrupt
>>      >     asserted,
>>      >     returns NOENABLE|HANDLED and another return only HANDLED?
>>      >
>>      >     2. What happens if one returns PROPAGATE and another returns
>>     HANDLED?
>>      >
>>      >
>>      > Yep, each ISR may return a different value and all of them are
>>      > accumulated in the "s" variable ( s |= intr->isr(intr); ).
>>      >
>>      > So the loop may end up with "s" which contains all of the
>>     possible bits:
>>      >
>>      > (e.g.
>>      >
>>      > isr1 - HANDLED | ENABLE
>>      > isr2 - HANDLED (don't want the irq to be enabled)
>>      > isr3 - CHAINED
>>      >
>>      > )
>>      >
>>      > s = HANDLED | ENABLE | CHAINED;
>>      >
>>      > Then CHAINED will be ignored because of the following code :
>>      >
>>      > +    if (s & XN_ISR_ENABLE)
>>      > +       xnarch_end_irq(irq);
>>      > +    else if (s & XN_ISR_CHAINED)    (*)
>>      > +       xnarch_chain_irq(irq);
>>     Which is the worst way possible of prioritizing them, if a Linux
>>     interrupt is
>>     active when we get there with ENABLE|CHAINED, interrupts will be
>>     enabled with
>>     the Linux interrupt still asserted -> the IRQ-handlers will be
>>     called once more,
>>     probably returning ENABLE|CHAINED again -> infinite loop...
>>
>>      >
>>      > the current code in the CVS doen not contain "else" in (*), so
>> that
>>      > ENABLE | CHAINED is possible, though it's a wrong combination.
>>      >
>>      > This said, we suppose that one knows what he is doing.
>>      >
>>      > In the case of a single ISR per line, it's not that difficult to
>>      > achieve. But if there are a few ISRs, then one should analize and
>>     take
>>      > into account all possible return values of all the ISRs, as each
>>     of them
>>      > may affect others (e.g. if one returns CHAINED when another -
>>     HANDLED |
>>      > ENABLE).
>>     Which is somewhat contrary to the concept of shared interrupts, if
>>     we have to
>>     take care of the global picture, why make them shared in the first
>>     place?
>>     (I like the concept of shared interrupts, but it is important that
>>     the framework
>>     gives a separation of concerns)
>>
>>
>> Unfortunately, it looks to me that the current picture (even with your
>> scalar values) requires from the user who develops a given IRQ to take
>> into account the possible return values of other ISRs.
>>
>> As I pointed out, the situation when 2 ISRs return HANDLED_NOENABLE
>> may lead to problems on some archs.
> 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).
> 

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").

Jan

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to