Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

[Anders Blomdell <anders.blomdell@domain.hid>]

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