Sebastian Huber commented on a discussion on bsps/sparc/leon3/start/amba.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1498#note_159770 > ambapp_grlib_root_register( &grlib_bus_config ); > } > > -RTEMS_SYSINIT_ITEM( Thank you for tracking this down to a reproducible fatal error. The failure is real and I can reproduce it. The diagnosis needs a correction though, and the fix should go to a different place. ## The linker set contract `c-user/linker_sets.md` states the design: > They can be used to initialize and include only those RTEMS managers and > other components which are used by the application. [...] in case the > application does not use message queues, there will be no reference to the > `rtems_message_queue_create()` function and the constructor is not > registered, thus nothing of the message queue handler will be in the final > executable. Three rules follow: 1. The system initialization item lives in the module which it initializes. 2. The module enters the image if and only if an object already in the image references a symbol of it. 3. A module whose initialization is mandatory needs a real link-time reference from an object which is always in the image. A driver manager item which disappears is rule 2 at work. The defect is a broken rule 3. ## The missing link-time dependency This is the chain on leon3 with `RTEMS_DRVMGR_STARTUP`: ``` start.o -> boot_card() -> bsp_start() bspstart.o, always in the image <-- MISSING REFERENCE --> amba.o root bus item, calls ambapp_grlib_root_register() -> ambapp_bus_grlib.o calls drvmgr_bus_register() -> drvmgr.o the five driver manager system initialization items -> drvmgr_drivers[] drvmgr_def_drivers.o, weak -> gptimer.o, apbuart_cons.o ``` Every arrow below the gap is a symbol reference and works today. Only the top one is absent. `amba.c` defines `LEON3_IrqCtrl_Regs` and `LEON3_Timer_Regs` only when `LEON3_IRQAMP_BASE` and `LEON3_GPTIMER_BASE` are undefined. On leon3 and ut700 those globals exist, `eirq.o` and `bsp_isr_handler.o` reference them, the linker extracts `amba.o` and the rest of the chain follows. On gr712rc and gr740 both bases are defined, the globals do not exist and `amba.o` has no incoming reference at all. One absent reference removes the root bus, the driver manager and every driver. So the statement "it affected all BSPs" is too broad. Items for the driver manager and the root bus in `ticker.exe`: | BSP | `LEON3_IRQAMP_BASE` and `LEON3_GPTIMER_BASE` | main | this MR | fix below | | --- | --- | ---: | ---: | ---: | | sparc/leon3 | undefined | 6 | 6 | 6 | | sparc/ut700 | undefined | 6 | 6 | 6 | | sparc/gr712rc | defined | 0 | 6 | 6 | | sparc/gr740 | defined | 0 | 6 | 6 | gr712rc carries the same defect as gr740. For the record, the fatal error path: with `RTEMS_DRVMGR_STARTUP` the classic `bsps/sparc/leon3/clock/ckinit.c` compiles out and `bsps/shared/grlib/btimer/tlib_ckinit.c` takes over. `Clock_driver_support_find_timer()` finds `tlib_dev_head` empty and raises the fatal error. On `sparc-rtems7-sis -gr740 -extirq 10 -m 4` the unpatched `ticker.exe` reports fatal source 6 and fatal code 514, which is `BSP_FATAL_CODE_BLOCK(2) + 2`, that is `LEON3_FATAL_CLOCK_INITIALIZATION`. `bspstart.o` also registers `bsp_interrupt_initialize` at `RTEMS_SYSINIT_DRVMGR_LEVEL_1`. That item is in the image but never runs either. The clock fatal error only arrives first. ## Why the current patch is a workaround The patch anchors the items on the application object instead of repairing the reference. That has four consequences. 1. It breaks rule 1. The five items leave `drvmgr.o`. `drvmgr.o` becomes reachable without `drvmgr_drivers[]` being reachable. 2. It breaks the riscv/griscv build. That BSP defines no `drvmgr_drivers[]`. On main only `fileio.exe` fails to link. With this MR every executable fails: ``` drvmgr.c:(.text._DRV_Manager_initialization+0x8): undefined reference to `drvmgr_drivers' ``` `base_sp.exe`, `capture.exe` and `minimum.norun.exe` all fail. 3. It puts `#if defined( LEON3 ) || defined( NOELV )` and `#include <grlib/ambapp_bus_grlib.h>` into the portable kernel. `cpukit` must not name a BSP family. The patch also has to promote `ambapp_grlib_root_initialize()` to a public BSP function for this single purpose. 4. `NOELV` is defined nowhere in the tree, so the second arm of that condition is dead. `RTEMS_DRVMGR_STARTUP` is a BSP build option, not an application option. The anchor belongs to the BSP. ## Proposed fix `bsps/sparc/leon3/start/bspstart.c` is always in the image, because `boot_card()` calls `bsp_start()`. It already carries a `RTEMS_SYSINIT_DRVMGR_LEVEL_1` item for `bsp_interrupt_initialize`, so it already states that this BSP depends on the driver manager. `bsps/sparc/leon2/start/bspstart.c` holds its root bus item the same way. Move the `#ifdef RTEMS_DRVMGR_STARTUP` block from `amba.c` to `bspstart.c`. `ambapp_grlib_root_initialize()` stays static. Its call to `ambapp_plb()` becomes the missing reference and pulls `amba.o` in. No `cpukit` change is needed, so please drop the whole first commit except the list macro rename. ```diff diff --git a/bsps/sparc/leon3/start/amba.c b/bsps/sparc/leon3/start/amba.c index 8a3edecb78a..8801378e8a6 100644 --- a/bsps/sparc/leon3/start/amba.c +++ b/bsps/sparc/leon3/start/amba.c @@ -98,42 +98,6 @@ struct ambapp_bus *ambapp_plb( void ) return plb; } -/* If RTEMS_DRVMGR_STARTUP is defined extra code is added that - * registers the GRLIB AMBA PnP bus driver as root driver. - */ -#ifdef RTEMS_DRVMGR_STARTUP -#include <drvmgr/drvmgr.h> -#include <grlib/ambapp_bus_grlib.h> - -/* Driver resources configuration for AMBA root bus. It is declared weak - * so that the user may override it, if the defualt settings are not - * enough. - */ -struct drvmgr_bus_res grlib_drv_resources __attribute__(( weak )) = { - .next = NULL, - .resource = { - DRVMGR_RES_EMPTY, - } -}; - -/* GRLIB AMBA bus configuration (the LEON3 root bus configuration) */ -struct grlib_config grlib_bus_config; - -static void ambapp_grlib_root_initialize( void ) -{ - /* Register Root bus, Use GRLIB AMBA PnP bus as root bus for LEON3 */ - grlib_bus_config.abus = ambapp_plb(); - grlib_bus_config.resources = &grlib_drv_resources; - ambapp_grlib_root_register( &grlib_bus_config ); -} - -RTEMS_SYSINIT_ITEM( - ambapp_grlib_root_initialize, - RTEMS_SYSINIT_BSP_START, - RTEMS_SYSINIT_ORDER_SECOND -); -#endif - #if !defined( LEON3_IRQAMP_BASE ) irqamp *LEON3_IrqCtrl_Regs; struct ambapp_dev *LEON3_IrqCtrl_Adev; diff --git a/bsps/sparc/leon3/start/bspstart.c b/bsps/sparc/leon3/start/bspstart.c index d457b1fe0f2..d58e80d4de7 100644 --- a/bsps/sparc/leon3/start/bspstart.c +++ b/bsps/sparc/leon3/start/bspstart.c @@ -118,3 +118,39 @@ RTEMS_SYSINIT_ITEM( RTEMS_SYSINIT_ORDER_LAST_BUT_5 ); #endif + +/* If RTEMS_DRVMGR_STARTUP is defined extra code is added that + * registers the GRLIB AMBA PnP bus driver as root driver. + */ +#ifdef RTEMS_DRVMGR_STARTUP +#include <drvmgr/drvmgr.h> +#include <grlib/ambapp_bus_grlib.h> + +/* Driver resources configuration for AMBA root bus. It is declared weak + * so that the user may override it, if the defualt settings are not + * enough. + */ +struct drvmgr_bus_res grlib_drv_resources __attribute__(( weak )) = { + .next = NULL, + .resource = { + DRVMGR_RES_EMPTY, + } +}; + +/* GRLIB AMBA bus configuration (the LEON3 root bus configuration) */ +struct grlib_config grlib_bus_config; + +static void ambapp_grlib_root_initialize( void ) +{ + /* Register Root bus, Use GRLIB AMBA PnP bus as root bus for LEON3 */ + grlib_bus_config.abus = ambapp_plb(); + grlib_bus_config.resources = &grlib_drv_resources; + ambapp_grlib_root_register( &grlib_bus_config ); +} + +RTEMS_SYSINIT_ITEM( + ambapp_grlib_root_initialize, + RTEMS_SYSINIT_BSP_START, + RTEMS_SYSINIT_ORDER_SECOND +); +#endif ``` Verified on sparc/leon3, sparc/ut700, sparc/gr712rc and sparc/gr740 with `RTEMS_DRVMGR_STARTUP = True`. All four images hold the six items. On `sparc-rtems7-sis -gr740 -extirq 10 -m 4`, `ticker.exe` prints `*** BEGIN OF TEST CLOCK TICK ***` and `TA1 - rtems_clock_get_tod - 09:00:00 12/31/1988`. ## Smaller points - Neither body states a problem. The first opens with `This would lead to`, which refers to nothing. State the wrong behaviour: the linker drops the system initialization items because no object references them. - The macro rename is sound. `LIST_HEAD` collides with `<sys/queue.h>` and with `cpukit/libfs/src/jffs2/include/linux/list.h`. Please name the collision in the commit message, record the break of an installed header, and prefix `BUS_LIST_*`, `DEV_LIST_*` and `DRV_LIST_*` in `drvmgr.h` too. ## Two defects outside this MR Each deserves its own issue. - `sparc/leon2` with `RTEMS_DRVMGR_STARTUP = True` does not compile: `bsps/shared/grlib/uart/cons.c:57:21: error: 'BSP_NUMBER_OF_TERMIOS_PORTS' undeclared here (not in a function)`. - `riscv/griscv` has neither a `drvmgr_drivers[]` table nor a root bus registration. Its driver manager support is incomplete. -- View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1498#note_159770 You're receiving this email because of your account on gitlab.rtems.org. Unsubscribe from this thread: https://gitlab.rtems.org/-/namespace/49/sent_notifications/5-6ygoewv2giqzk23515x497apx-1d/unsubscribe | Manage all notifications: https://gitlab.rtems.org/-/profile/notifications | Help: https://gitlab.rtems.org/help
_______________________________________________ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
