The eCos, QNX, ChorusOs irq handling API

[Sergei Organov]

Valette Eric <eric.valette at free.fr> writes:
> Sergei Organov wrote:
> > Valette Eric <eric.valette at free.fr> writes:
> >
> 
> >>Sergei Organov wrote:
> >>>The API seems to be complete and rather simple. Overall, that's roughly
> >>>what I'd like to see in the RTEMS.
> >>
> >>The APi is not simple I think and also not complete :
> >>
> >>	1) While there is a way to get the previous handler for an irq
> >>	   (meaning you intend to replace it dynamically), there is no way to
> >>	   notify the previous driver to clean up the ressources it may have
> >>	   allocated to handle the interrupts (e.g tasks, memory, ...).
> >>	   Handled by irqOff routine calling is my API. If you think anyway
> >>	   the driver should coordinate, I would answer that for profiling or
> >>	   irq couting it is not necessary,
> > So the driver API should be designed to manage it.
> 
> How does the eCos API manage it which is my point?

eCos *IRQ API* doesn't manage driver resources. And it shouldn't, IMHO. IRQ
API is used by *drivers implementations*, and eCos IRQ API allows drivers to
disconnect their handlers from IRQs. *Drivers API* is another issue that I'm
not discussing here.

> > As you correctly mentioned yourself, IRQ is only one resource a driver may
> > need, so putting a routine that would handle cleanup of driver resources
> > in IRQ API is just plain wrong. What if a driver doesn't need IRQs at all?
> 
> What I say is that an API that enables to disconnect a handler should have
> provision for a callBack to call the initial driver code, and also ask it
> politely to shut down the interrupt the way he wants...

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

> I do still think irqOff could be used for that purpose...

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.

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

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

> >>	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
> 	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. The simplest
possible sequence is:

        1) handle the hardware
        2) do something useful
        3) acknowledge the IRQ

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.

Anyway, the sequence you've asked about could be implemented once in BSP as an
ISR that every driver that needs exactly this sequence will be able to install
instead of its own handler (and its own handler as a call-back for this ISR).

IMHO, the rule of thumb here is: put into generic code only those
functionality that any handler will always need (CPU context save/restore
is a good example) and provide helper routines to simplify handling of most
likely scenarios.

> 
> I would like to point out that any primitive that handle PIC masks called
> into an IRQ handler will prevent it to be SMP safe... Who care of SMP
> support? At least I do especially whem I'm designing a new API.

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. And I don't see how eCos-like API prevents implementation from
taking care of SMP. Neither I can see how irqOn/irqOff pointers may help here.

> 
> > Anyway, it's up to BSP writer to select particular implementation. We are
> > talking about interfaces here.
> 
> But wrong interface prevent to easilly write correct code :-) Never let the
> handler manage the PIC or average driver writer will make mistakes that will
> break the whole system...

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.

> Managing the PICs means that any driver as an open door to harm other
> drivers. In the current ix86 and PPC code, the handler do not manipulate PIC
> and therefore the code is simpler to write : you do not need to read the PIC
> manual before actually starting to handle the hardware you really want to
> mange. Did you read the SIU/CPM manual, the OpenPIC specs, the 8259 manual?
> I did but if I can avoid driver writer to read it, I'm quite satisfied.

With suggested API driver writers will need to know only vector number and the
API itself. The rest will be done by BSP routines that the driver will call
when appropriate. If a driver doesn't adhere to the assumptions of the
framework, the driver is broken no matter what particular API we select.

> 
> > I'm trying to convince you that one can avoid the duplication using usual
> > routines. There is no reason to store pointers to routines and call them
> > through these pointers. It only increases the cost, not decreases it, I
> > believe.
> 
> Code size for each handler is increased by sveral instructions so globally
> you increase the code size

But handler attach code (plus data required for attachment) is decreased.

> while making the writing of handler more difficult...

Flexibility does matter here. Having slightly more complex handler worth the
flexibility the approach provides, IMHO.

> 
> >>   One could mesure the data size of IRQ related data and code size on
> >>   Intel before and after the implementation of the new IRQ,
> > Did you?
> 
> No but I though about it when designing it and I do say as author it reduced
> the size because each irq handler had the PIC manipulation sequence
> mentionned above... Prove me I'm wrong :-)

Then the old implementation was bad. The fact that your implementation is
better than the old one doesn't mean it's the best possible implementation.

> 
> >>2) The code corresponding to the On/Off routine has to exist anyway as you
> >>   need to program the hardware... It indeed force you need to cleanly
> >>   isolate it, and export it so that other part of the code can use it,
> > The code has to exist, but why it should be called through
> 
> > p->on(irq_connect_data)/p->off(irq_connect_data) instead of
> > on(irq_number)/off(irq_number)? What's an advantage?
> 
> Frankly why are you pissing me of for such detail?

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.

> irq_connect_data contains the relevant irq_number...

I know.

> 
> >>Or maybe you do not care because anyway you never wrote a system that can
> >>dynamically replace drivers and use expansive hardware to perform system
> >>debugging via jtag?
> > My primary goal is to minimize overhead for really small systems. What I've
> > wrote and what I haven't has nothing to do with the issues we discuss.
> 
> Again the cost is minimal : NB IRQ*sizeof word*2 and as pointed out, as all
> PIC manipulation is done by generic layer is remove code that needs to be
> writen in each handler... And more important is facilitate implementation of
> more sophisticated services like system debugging, irq monitoring, ...

If you were able to demonstrate even single case where this could be an
advantage, I'd probably forget about the cost. But for otherwise useless thing
even 1 byte of overhead is too much, I believe ;-)

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?

-- 
Sergei.