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

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1: Type: text/plain, Size: 4437 bytes --]

Anders Blomdell wrote:
> Jan Kiszka wrote:
>> Hi Dmitry,
>>
>> Dmitry Adamushko wrote:
>>
>>> Hi Jan,
>>>
>>> let's make yet another revision of the bits :
>>>
>>> new XN_ISR_HANDLED  == old XN_ISR_HANDLED + old XN_ISR_NO_ENABLE
>>>
>>> ok.
>>>
>>> new XN_ISR_NOENABLE == ~ old XN_ISR_ENABLE
>>>
>>> ok.
>>>
>>> new XN_ISR_PROPAGATE == XN_ISR_CHAINED
>>>
>>> ok.
>>>
>>
>>
>> Just to make sure that you understand my weird ideas: each of the three
>> new XN_ISR_xxx above should be encoded with an individual bit
>>
>>
>>> new XN_ISR_NOINT == ?
>>>
>>> does it suppose the interrupt line to be .end-ed (enabled) and irq
>>> not to be
>>> propagated? Should be so, I guess, if it's different from 5). Then
>>> nucleus
>>> ignores implicit IRQ enable for 5) as well as for 3).
>>>
>>> Do we really need that NOINT then, as it seems to be the same as
>>> ~HANDLED?
>>>
>>> or NOINT == 0 and then it's a scalar value, not a bit.
>>>
>>> So one may consider HANDLED == 1 and NOINT == 0 as really scalar values
>>>
>>> and
>>>
>>> NOENABLE and PROPAGATE as additional bits (used only if needed).
>>>
>>
>>
>> My idea is to urge the user specifying one of the base return types
>> (HANDLED or NOINT) + any of the two additional bits (NOENABLE and
>> PROPAGATE).
>>
>> For correct drivers NOINT could be 0 indeed, but to check that the user
>> picked a new constant we may want to set NOINT != 0. With the old API
>> "return 0" expressed HANDLED + ~ENABLE for the old API. With the new one
>> the user signals no interest and the nucleus may raise a warning that a
>> spurious IRQ occurred. So I would add a debug bit for NOINT here to
>> optionally (on OPT_XENO_DEBUG) detect old-style usage (return 0).
>> Moreover, we gain freedom to move bits in the future when every state is
>> encoded via constants. Or am I too paranoid here?
> After reading the above discussion (of which I understand very little),
> and looking at (what I believe to be) the relevant code:
> 
> +    intr = shirq->handlers;
> +
> +    while (intr)
> +        {
> +        s |= intr->isr(intr);
> +        ++intr->hits;
> +        intr = intr->next;
> +        }
> +    xnintr_shirq_unlock(shirq);
> +
> +    --sched->inesting;
> +
> +    if (s & XN_ISR_ENABLE)
> +       xnarch_end_irq(irq);
> +    else if (s & XN_ISR_CHAINED)
> +       xnarch_chain_irq(irq);
> 
> 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?
> 
> As far as I can tell, after all RT handlers havve run, the following
> scenarios are possible:
> 
> 1. The interrupt is deasserted (i.e. it was a RT interrupt)
> 2. The interrupt is still asserted, it will be deasserted later
>    by some RT task (i.e. it was a RT interrupt)
> 3. The interrupt is still asserted and will be deasserted
>    by the Linux IRQ handler.
> 
> IMHO that leads to the conclusion that the IRQ handlers should return a
> scalar
> 
> #define UNHANDLED 0
> #define HANDLED_ENABLE 1
> #define HANDLED_NOENABLE 2
> #define PROPAGATE 3
> 
> and the loop should be
> 
>     s = UNHANDLED
>     while (intr) {
>       tmp = intr->isr(intr);
>       if (tmp > s) { s = tmp; }
>       intr = intr->next;
>     }
>     if (s == PROPAGATE) {
>       xnarch_chain_irq(irq);
>     } else if (s == HANDLED_ENABLE) {
>       xnarch_end_irq(irq);
>     }

This is a very useful suggestion, specifically this escalation of the
return value in the shared case. I would just let UNHANLDED start with 1
for the reasons I explained here:

https://mail.gna.org/public/xenomai-core/2006-02/msg00186.html

Ok, I would say let's got for this and finalise the patch!

> 
> To be really honest, I think that PROPAGATE should be excluded from the
> RT IRQ-handlers, since with the current scheme all RT-handlers has to
> test if the IRQ was a Linux interrupt (otherwise the system will only
> work when the handler that returns PROPAGATE is installed)
> 

Indeed, but I'm with Philippe here: do not exclude the strange corner
case scenarios users may craft with the appropriate care. I could, e.g.,
imagine a scenario where an RT handler takes the arrival time stamp of a
non-RT IRQ and then propagates it.

Jsn


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]