PowerPC exceptions and interrupts

[Sebastian Huber]

Pavel Pisa wrote:
> On Tuesday 20 May 2008 09:22, Sebastian Huber wrote:
>   
> [...]
>   
>> static rtems_status_code rtems_interrupt_handler_install( 
>> 	rtems_vector_number vector,
>> 	rtems_interrupt_handler handler, 
>> 	void *arg
>> 	)
>>     
>
> I think personally, that additional function
>  rtems_interrupt_handler_unique_install() 
> for unique handler install is not good idea.
> I would prefer to add some mode/flags field into
> regular rtems_interrupt_handler_install() function.
> You implementation uses this approach anyway.
> I would suggest to use masks provided by defines
> for these flags. You need sometimes specific modifications/
> modes for interrupts, not only RTEMS_IRQ_SHARE_ALLOWED.
> The Linux generic IRQ support converged to allow specify
> if IRQ is triggered on rising, falling edge or level directly
> in request_irq() function for GPIO IRQ sources for example.
>
>   
Ok, good idea.
> The Linux adds ability to provide some user recognizable name
> for handler as well. This is not mandatory for RTEMS, but
> it could be of great help, if you need to check what is
> registered in the system and to display IRQ counters.
>
>   
Hm, nice feature but this requires memory.
> So my proposal is
>
> static rtems_status_code rtems_interrupt_handler_install( 
> 	rtems_vector_number vector,
> 	rtems_interrupt_handler handler, 
> 	rtems_interrupt_flags   flags, 
> 	char *informative_name,
> 	void *arg
> 	)
>
>   
>> static rtems_status_code rtems_interrupt_handler_remove( 
>> 	rtems_vector_number vector,
>> 	rtems_interrupt_handler handler 
>> 	)
>>     
>
> As for the remove, I would again suggest to follow proven
> way, which is used in Linux. It has significant rationale.
> If you have shared interrupt source (consider PCI for example)
> and you can have multiple instances of same hardware (multiple
> ETHERNET cards for example) sharing same IRQ line, then
> you cannot control, which of multiple registration
> with same handler would be unregisterred. This could lead
> to fatal problems, that state structure for some card is
> destroyed, but handler for different one is unregisterred
> => handler with arg pointing to the free/reused memory
> would be used => disaster situation.
>
> On the other hand, if device context structure pointer
> ("arg") is used for selection which shared registration
> should be removed, than it finds correct registration instance.
>
> This means, that I would propose with Linux and other OS
> proven prototype
>
> static rtems_status_code rtems_interrupt_handler_remove( 
> 	rtems_vector_number vector,
> 	void *arg
> 	)
>
> If you ensure safety even more, handler parameter could
> be left there as well. In such case, the functionality
> could be extended next way
>
>    handler == NULL .. then only match for arg is checked
>    arg == NULL .. then only match for handler is checked
>                   and all instances with this handler are removed
>   
Ok, lets do it.
>    (handler == NULL) && (arg == NULL) .. all registration
>                   for given vector are removed
>
>   
I am a bit uneasy with this, because this allows you to remove alien 
handlers (= private hander routines and arguments from other drivers).
> As for low level stuff and relation to the rtems_irq_connect_data,
> I have feeling, that your approach is upside down.
> It think, that rtems_irq_connect_data and IRQ controllers
> low level stuff should be base on which user friendly
>  rtems_interrupt_handler_install() and
>  rtems_status_code rtems_interrupt_handler_remove()
> should be based upon and not vice versa.
>   
Yes. A rtems(!)_irq_connect_data is superfluous. Data structures are up 
to the implementation.
> The IRQ controller support can gain from ability to have direct
> and fast aces to some (even architecture extendable) information
> context for each interrupt source
>   typedef void (*rtems_irq_enable) (const struct __rtems_irq_connect_data__*);
>   typedef void (*rtems_irq_disable) (const struct __rtems_irq_connect_data__*);
>   typedef int  (*rtems_irq_is_enabled) (const struct __rtems_irq_connect_data__*);
>   
The is_enabled() function is not needed since you can check this in your 
enable or disable function. We can merge the other two functions into 
enable( enable, connect_data).

But do we really need this?
> [...]
>
>   
>> typedef void(*rtems_interrupt_handler )(void *)
>>     
>
> If we speak about generic support, then to allow
> correct sharing for all obscure controllers and
> even for edge triggered sources, then
>   rtems_interrupt_handler
> prototype has to be modified to return information,
> if the even has been recognized by device driver or not
>   typedef int rtems_interrupt_handled_state;
>   typedef rtems_interrupt_handled_state (*rtems_interrupt_handler )(void *)
>
> The next algorithm is required to handle correctly shared edge
> triggered sources
>
>    rtems_interrupt_handled_state again;
>    rtems_interrupt_handled_state res;
>    do {
>      again = 0;
>      for_each(vector->rtems_irq_connect_data_list, connect) {
>        res=connect->hndl(connect->handle);
>        if(res == RTEMS_INTERRUPT_HANDLED)
>          again = 1;
>      }
>    } while(again);
>
> [...]
>   
I am to inexperienced to comment this.

-- 
embedded brains GmbH
Sebastian Huber           Obere Lagerstr. 30
D-82178 Puchheim          Germany
Tel. : +49-89-18 90 80 79-6
Fax  : +49-89-18 90 80 79-9
email: Sebastian.Huber at embedded-brains.de
PGP public key available on request

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.