The eCos, QNX, ChorusOs irq handling API

[till]

I'm glad Sergei joined this discussion as Eric and me seemed to
have deadlocked a little bit. New arguments are badly needed.

I like several points Sergei brought up...

Valette Eric wrote:

> Sergei Organov wrote:
>
>> Valette Eric <eric.valette at free.fr> writes:
>
>
>> And I think it shouldn't. I think it should call driver code that in 
>> turn
>> should disconnect handler if it is asked to. How to find a driver 
>> that owns
>> the IRQ is another issue that IMHO requires more general handling at 
>> upper
>> level (apparently through some kind of driver registration).
>
>
> Nice try : to replace a simple function pointer you propose to add 
> driver registration routines... It surely simplify the API that as you 
> seem to recognise IRQ management API will not be complete without that...

On the point of driver unloding: I absolutely agree with Sergei on that 
this is NOT
the job of irqOff. I thought (based on Erics comments in the header 
files) that this
routine should disable the IRQ at the device level - Erics API says 
NOTHING about
a driver giving up all its resources and stop as a side effect of this call.
This is simply not acceptable.

Since we are talking about a real-time executive here and not a 
multi-user general
purpose system, I believe that we can live without driver registration. 
Driver unloading
is very useful for debugging/development and a simple drvXXXCleanup() entry
point does the job (when you use CEXP, this can even be a C++ destructor 
:-).

>
>> Yes, it could be used for that purpose. But for me it looks like a 
>> limited
>> solution you've invented to overcome absence of generic drivers API.
>
>
> Do you think we shall provide it to avoid the On/Off :-) 

Again: on/off, if it's there (and I believe they are not really 
necessary) should
do what it says: enable/disable the IRQ at the device and nothing else

>
>
>>>>>     2) There is no way for code outside a driver to stop interrupt 
>>>>> for a
>>>>>        while and then reenable them without having knowledge of 
>>>>> all the
>>>>>        devices. This type of functionnality is necessary for 
>>>>> profilling,
>>>>>        system debuggers, irq monitoring,
>>>>
>>>>
>>>> Why? cyg_interrupt_mask(vector) will do.
>>>
>>>
>>> 1) If tasks start manipulating the PIC because you expose this API, 
>>> and are
>>> prempted by other tasks performing the same thing vevrything will be 
>>> rapidly
>>> broken (To not even mention SMP problems with this API). You better 
>>> disable
>>> the real cause of the interrupt (device) because the PIC are shared 
>>> among all
>>> interrupts while the device is not.
>>
>>
>>
>> The PIC is shared resource, so *implementation* of API routines that 
>> needs to
>> access the PIC should take care to use appropriate mutual exclusion
>> primitives. Anyway, I don't see how the on/off interface helps here, 
>> sorry.
>
I agree with S. again.

>
> They cannot has there will be separate call to set the mask and 
> restore it later on. If you prevent any PIC access during this 
> sequence, then you cannot handle any other interrupts???

Sure, but if you have to manipulate the masks you have to assert mutual 
exclusion
which means that you cannot handle interrupts (that critical section 
better be short).

>
>>> 2) What if I want to mask a set of interrupt via a masks (as most 
>>> moderm PIC
>>> implement it)?
>>
>>
>>
>> With this particular API it obviously requires multiple calls to the 
>> routine.
>> While I think it's rarely needed and thus multiple calls could be 
>> acceptable,
>> I see two solution for this problem:
>>
>> 1. Design API that accepts bit masks.
>>
>> 2. Let BSPs provide specific optimized routines for more advanced 
>> hardware
>>    management.
>>
>> I personally prefer (2).
>
>
> First I think applying bit masks is the only way to implement the 
> notion of IRQ priority on many PICs.  Therefore applying masks is 
> mandatory contrary to what you suggest... Now, the masks are totally 
> PIC dependent and so even if you provide an API, any usage inside a 
> drivers will be surrounded by a zillion of ifdefs one for each PIC... 
> And also when there are cascaded hardware 5openPic + 8259, SIU +CPM), 
> there are several masks that should be applyied at diffrent time 
> isnide the handler routine...

I don't understand. IMO implementing priorities by masks is the job of 
the BSP/PIC driver
and is hidden from the 'normal' driver writer. For most other cases, 
just disabling one vector
is probably fine.

>
>>>>>     3) not acknowledging the irq by generic layer means that a 
>>>>> driver can
>>>>>        break any form of irq priorities if the PIC must be 
>>>>> acknowledged
>>>>>        before issuing the next interrupt.
>>>>
>>>>
>>>> A driver could break anything no matter if acknowledge is performed 
>>>> by the
>>>> generic layer or not. Anyway, if BSP writer decides to ack irqs in the
>>>> generic layer, he can make interface 
>>>> 'cyg_interrupt_acknowledge(vector)'
>>>> routine to be a no-op.
>>>>
>>>>
>>>>>        And while it seems necessary to acknowledge explicitely, 
>>>>> there is
>>>>>        no API to mask the current IRQ at PIC level
>>>>
>>>>
>>>> I believe cyg_interrupt_mask()/cyg_interrupt_unmask() are exactly 
>>>> for this
>>>> purpose, so the rest of your sentence below makes no sense.
>>>
>>>
>>> So each driver will then have to include the sequence :
>>>     1) Get the current mask
>>>     2) Apply a new mask than at least mask the current IRQ. BTW 
>>> sometimes
>>>         you want to mask more than one IRQ source so the proposed 
>>> eCos API
>>>         is not sufficient 
>>
Why would you want to do such a thing in a driver ISR???

>>>
>>>     3) send an EOI
>>>     4) reenable IRQ a processor level
>>>     5) handle the hardware
>>>     6) restore the original mask
>>>
>>> Great. Do you think It cannot be factorized as already done on Ix86 
>>> and PPC?
>>> And do you really think you can avoid this sequence?
>>
>>
>>
>> I really think that it is *a* sequence, not *the* sequence, and I 
>> don't see
>> why this one should be preferred over other possible sequences. 
>
Well, but here I tend to agree with Eric. The 1-6 sequence is certainly 
one that
makes sense in most cases while ensuring low latency. Strangely, Eric,
you didn't implement OpenPIC interrupts that way, however. There, you
send EOI only after running the user ISR - shouldn't we fix this??

>> The simplest
>> possible sequence is:
>>
>>         1) handle the hardware
>>         2) do something useful
>>         3) acknowledge the IRQ
>
>
> If you do that, then no other interrupt will occur before 3 and thus 
> you increase the global interrupt latency of the system by a single 
> driver... 

BTW: Eric, that is how you handle (non ISA) IRQs on PPC/shared, yep.

>
>
>>
>> where exactly one of (1), (2) or (3) could in fact be a no-op. I can 
>> build
>> quite a few other likely sequences as well, and in fact I believe the 
>> sequence
>> you've provided is one of the least likely sequences in real-time 
>> applications
>> where it's a common practice to do absolute minimum of work in ISRs 
>> deferring
>> as much as possible to threads level.
>
>
>
> Well it is the sequence I have seen in Linux, ChorusOs, BSD, ... But 
> anyway 1,2,3 represent actually 5 instructions on Ix86, not much more 
> on PCC *BUT* That way I can say the interrupt latency is low...
>
>
>> While I myself didn't carefully examine which implications SMP may 
>> have on the
>> discussed API, I think that it is *the implementation* of the API 
>> should take
>> care of SMP. 
>
>
> You simply cannot implememt SMP safe drivers API if you impose the 
> usage of PIC bitmask manipulation in handlers as several handler may 
> be executed concurently meaning that you have no mean the correct mask 
> that need to be restored before returning from hdl... 

Of course, bitmask manipulation (i.e. 'disable(vector)' / 
'enable(vector)') has to be SMP safe.
BTW: one example of a driver who needs to do that is the tundra universe 
PCI-VME bridge.
It encorporates a "PIC" itself and hence wants to re-enable it's PCI IRQ 
at the OpenPIC ASAP
(to support VME interrupt priorities in software)

>
>
>> Average driver writer will need to understand only API (the specific 
>> hardware
>> the driver should manage aside), and suggested API is not platform/PIC
>> dependent. The implementation of the API will manage PIC (if any), 
>> not driver
>> writer.
>
>
> Not if you impose to call routine that manipulate the PIC to correctly 
> hanlde an interrupt...
>
>> Flexibility does matter here. Having slightly more complex handler 
>> worth the
>> flexibility the approach provides, IMHO.
>
>
> I disagree. Flexibility for you would probably be seen by many other 
> on this list as unecessary complexity...
>
>> 1. Because it seems you believe there is an advantage. Don't you?
>> 2. Because on(irq_number), unlike your p->on(irq_connect_data), could 
>> be even
>>    an inline routine thus reducing to a few asm instructions, and I 
>> *do care*
>>    about performance of such operation.
>
>
> There is no performance impact has this code *again* is not called by 
> handler... You already forgot it? 

Sorry, I still don't see the great benefit of the on/off routines. 
Especially since it's explicitely
left to the driver implementor to make them no-ops. Hence, you can not 
really rely on them
really disabling/enabling interrupts at the device...

>
>
>> The more we discuss the issue the more feeling I have that it's not only
>> useless, but also misleading. Maybe I'm the only so stupid here who 
>> doesn't
>> understand what it is for?
>
>
> For sure you are probably the only one who proposed many things 
> regarding the IRQ API without even suggesting that you would care to 
> really implement anything and not even taking care to see if the API 
> implementation is realistic or not on 8XX, motorola_shared or Ix86 :-) 

Please don't let us get personal here. Not only does this not matter, I 
believe, on the contrary,
that the "USER's" opinion about the API should be of utmost importance 
to us. Anyone
(Sergei not being precisely "anyone") who takes his/her time to think 
about this and share
their thoughs is welcome.

I'd love to see more contributions by other people rather than talking a 
lot myself or
to continue argueing with Eric about the benefits of his current 
implementation.

Cheers
-- Till

>
>
>
>