rtems-CVS-050303 i.MX/MX1/csb336 BUG and BUGFIX

[Pavel Pisa]

Hello Ralf,


On Friday 04 March 2005 06:15, Ralf Corsepius wrote:
> > and uninitialized memory contents is used as base address for the vector
> > table
> >
> >       /* Initialize the vector table contents with default handler */
> >       for (i=0; i<BSP_MAX_INT; i++)
> >         *(vectorTable + i) = (long)(default_int_handler);
>
> Worse, this code probably is non functionial.
>
> I think,
> volatile rtems_irq_hdl* vectorTable
> is required, because as far as I understand the this code is supposed to
> initialize hardware addresses.

This is the second level IRQ/exception table.

As I understand to ARM, ARM has the first exception table only eight
entries long. One of them is fast IRQ (FIQ) and other is normal (IRQ).
CPU has only hardware two bit masks (for FIQ and IRQ).
The CPU takes one of these two vectors through virtual addresses
(6<<2) and (7<<2). Many of ARM boards do not have RAM at physical
address 0, so some piece from RAM from other absolute address is remapped
there. This need not be true for ARMv6+, because these versions
are able to provide caching at hardware address level in the new
modified MMU mode.

The second level table is interpretted by software only. The first level
handler reads vector number from independent IRQ controller mapped
as peripheral. Handler code adjusts acceptance levels in the IRQ controller,
uses vector as index into VECTOR_TABLE and calls real ISR routine.
As you see, all manipulation and interpretation of the second level table
is only by means of software and if it is only through one mapping of page,
than there is no need to add special synchronization into code.
You are right that volatile would be appropriate there, but it is
not critical, in which part of _CPU_ISR_install_vector function processing
is value really stored into RAM.

So, even, that I agree, that ARM RTEMS is not fully mature and clean,
I do not see so fundamental flaw in the fixed code. I think, that it would
be better, if VECTOR_TABLE  macro is eliminated and real pointer variable
  volatile rtems_irq_hdl** vectorTableAddress;
to the start of the second level table is used in all ARM variants.
Than each BSP gets responsibility to setup value of this variable.
But this could make problem if incorrect RAM cleanup and initialization order
would be taken and can be more prone to some errornous operation..

> > Next oneliner patch solves described problem
---------------------------------------------------------------------------------------------
diff --minimal -r -u -P -p rtems/c/src/lib/libcpu/arm/mc9328mxl/irq/irq.h.orig rtems/c/src/lib/libcpu/arm/mc9328mxl/irq/irq.h
--- rtems/c/src/lib/libcpu/arm/mc9328mxl/irq/irq.h.orig 2005-03-04 00:57:56.000000000 +0100
+++ rtems/c/src/lib/libcpu/arm/mc9328mxl/irq/irq.h 2005-03-04 01:00:05.000000000 +0100
@@ -114,7 +114,7 @@ typedef void (*rtems_irq_enable)    (con
 typedef void (*rtems_irq_disable)   (const struct __rtems_irq_connect_data__*);
 typedef int  (*rtems_irq_is_enabled)(const struct __rtems_irq_connect_data__*);
 
-extern rtems_irq_hdl bsp_vector_table;
+extern rtems_irq_hdl bsp_vector_table[BSP_MAX_INT];
 #define VECTOR_TABLE bsp_vector_table
                 
 typedef struct __rtems_irq_connect_data__ {

---------------------------------------------------------------------------------------------

> > But I have big problem to analyze and find this hidden bug.
>
> Me thinks, this code needs to be redesigned - :(

I agree, that there is always space to clean code,
but I am happy with suitable working solution at this moment.

By the way, should I put bug and solution into RTEMS GNAT
or someone other can do that? What is preferred way of login
into RTEMS GNAT?

Best wishes
                Pavel Pisa