| Message ID | 1487768164-43184-3-git-send-email-christophe.milard@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | driver items registration and probing | expand |
On Wed, Feb 22, 2017 at 6:55 AM, Christophe Milard < christophe.milard@linaro.org> wrote: > The functions to register and probe enumerator classes are added. > > Signed-off-by: Christophe Milard <christophe.milard@linaro.org> > --- > platform/linux-generic/Makefile.am | 1 + > platform/linux-generic/_modules.c | 4 + > platform/linux-generic/drv_driver.c | 212 > ++++++++++++++++++++- > .../linux-generic/include/drv_driver_internal.h | 22 +++ > platform/linux-generic/include/odp_internal.h | 5 + > platform/linux-generic/odp_init.c | 21 +- > 6 files changed, 260 insertions(+), 5 deletions(-) > create mode 100644 platform/linux-generic/include/drv_driver_internal.h > > diff --git a/platform/linux-generic/Makefile.am > b/platform/linux-generic/Makefile.am > index 66ff53d..b7d1b1a 100644 > --- a/platform/linux-generic/Makefile.am > +++ b/platform/linux-generic/Makefile.am > @@ -133,6 +133,7 @@ noinst_HEADERS = \ > ${srcdir}/include/_ishm_internal.h \ > ${srcdir}/include/_ishmphy_internal.h \ > ${srcdir}/include/_ishmpool_internal.h \ > + ${srcdir}/include/drv_driver_internal.h\ > ${srcdir}/include/odp_align_internal.h \ > ${srcdir}/include/odp_atomic_internal.h \ > ${srcdir}/include/odp_buffer_inlines.h \ > diff --git a/platform/linux-generic/_modules.c > b/platform/linux-generic/_modules.c > index 6bb854e..b23c81f 100644 > --- a/platform/linux-generic/_modules.c > +++ b/platform/linux-generic/_modules.c > @@ -9,6 +9,7 @@ > #include <odp/api/std_types.h> > #include <odp/api/debug.h> > #include <odp_debug_internal.h> > +#include <drv_driver_internal.h> > #include <libconfig.h> > #include <dlfcn.h> > > @@ -40,6 +41,9 @@ static int load_modules(void) > ODP_DBG("module %s loaded.\n", module_name); > } > > + /* give a chance top the driver interface to probe for new things: > */ > Garbled phrasing. I assume the intent here was "Give the top driver interface a chance to probe for new things" ? > + _odpdrv_driver_probe_drv_items(); > + > return 0; > } > > diff --git a/platform/linux-generic/drv_driver.c > b/platform/linux-generic/drv_driver.c > index 529da48..50956a7 100644 > --- a/platform/linux-generic/drv_driver.c > +++ b/platform/linux-generic/drv_driver.c > @@ -4,20 +4,131 @@ > * SPDX-License-Identifier: BSD-3-Clause > */ > > +#include <string.h> > + > #include <odp_config_internal.h> > +#include <_ishmpool_internal.h> > > #include <odp/api/std_types.h> > #include <odp/api/debug.h> > +#include <odp/api/rwlock_recursive.h> > #include <odp/drv/driver.h> > +#include <odp/drv/spec/driver.h> > #include <odp_debug_internal.h> > +#include <drv_driver_internal.h> > + > +static enum {UNDONE, IN_PROGRESS, DONE} init_global_status; > + > +/* pool from which different list elements are alocated: */ > +#define ELT_POOL_SIZE (1 << 20) /* 1Mb */ > +static _odp_ishm_pool_t *list_elt_pool; > + > +typedef struct _odpdrv_enumr_class_s _odpdrv_enumr_class_t; > + > +/* an enumerator class (list element) */ > +struct _odpdrv_enumr_class_s { > + odpdrv_enumr_class_param_t param; > + int probed; > + _odp_ishm_pool_t *pool; > + struct _odpdrv_enumr_class_s *next; > +}; > + > +/* the enumerator class list: */ > +typedef struct _odpdrv_enumr_class_lst_t { > + odp_rwlock_recursive_t lock; > + _odpdrv_enumr_class_t *head; > +} _odpdrv_enumr_class_lst_t; > +static struct _odpdrv_enumr_class_lst_t enumr_class_lst; > + > +/* some driver elements (such as enumeraor classes, drivers, devio) may > + * register before init_global and init_local complete. Mutex will fail > + * in this cases but should be used later on. > + * These functions disable the usage of Mutex while it is global init i.e. > + * while single threaded*/ > I don't understand why these tests are needed. You've put the driver init stuff at the end of odp_init_global() so all the other machinery is available to you at this point. So these guards seem unnecessary. > +static void enumr_class_list_read_lock(void) > +{ > + if (init_global_status == DONE) > + odp_rwlock_recursive_read_lock(&enumr_class_lst.lock); > +} > + > +static void enumr_class_list_read_unlock(void) > +{ > + if (init_global_status == DONE) > + odp_rwlock_recursive_read_unlock(&enumr_class_lst.lock); > +} > + > +static void enumr_class_list_write_lock(void) > +{ > + if (init_global_status == DONE) > + odp_rwlock_recursive_write_lock(&enumr_class_lst.lock); > +} > + > +static void enumr_class_list_write_unlock(void) > +{ > + if (init_global_status == DONE) > + odp_rwlock_recursive_write_unlock(&enumr_class_lst.lock); > +} > + > > odpdrv_enumr_class_t odpdrv_enumr_class_register(od > pdrv_enumr_class_param_t > *param) > { > - ODP_ERR("NOT Supported yet! Enumerator Class %s Registration!\n.", > - param->name); > + _odpdrv_enumr_class_t *enumr_c; > > - return ODPDRV_ENUMR_CLASS_INVALID; > + /* parse the list of already registered enumerator class to make > + * sure no enumerator with identical name already exists: > + */ > + enumr_class_list_read_lock(); > + enumr_c = enumr_class_lst.head; > + while (enumr_c) { > + if (strncmp(param->name, enumr_c->param.name, > + ODPDRV_NAME_SIZE) == 0) { > + ODP_ERR("enumerator class %s already exists!\n", > + param->name); > + enumr_class_list_read_unlock(); > + return ODPDRV_ENUMR_CLASS_INVALID; > + } > + enumr_c = enumr_c->next; > + } > + enumr_class_list_read_unlock(); > + > + /* allocate memory for the new enumerator class: > + * If init_global has not been done yet, then, we cannot allocate > + * from any _ishm pool (ishm has not even been initialised at this > + * stage...this happens when statically linked enumerator classes > + * register: their __constructor__ function is run before main() > + * is called). But any malloc performed here(before init_global) > + * will be inherited by any odpthreads (process or pthreads) as we > + * are still running in the ODP instantiation processes and all > + * other processes are guaranteed to be descendent of this one... > + * If init_global has been done, then we allocate from the _ishm > pool > + * to guarantee visibility from any ODP thread. > + */ > + > + if (init_global_status == UNDONE) { > + enumr_c = malloc(sizeof(_odpdrv_enumr_class_t)); > + if (!enumr_c) > + return ODPDRV_ENUMR_CLASS_INVALID; > + enumr_c->pool = NULL; > + } else { > + enumr_c = _odp_ishm_pool_alloc(list_elt_pool, > + > sizeof(_odpdrv_enumr_class_t)); > + if (!enumr_c) { > + ODP_ERR("_odp_ishm_pool_alloc failed!\n"); > + return ODPDRV_ENUMR_CLASS_INVALID; > + } > + enumr_c->pool = list_elt_pool; > + } > + > + /* save init parameters and insert enumerator class in list */ > + enumr_c->param = *param; > + enumr_c->probed = 0; > + enumr_class_list_write_lock(); > + enumr_c->next = enumr_class_lst.head; > + enumr_class_lst.head = enumr_c; > + enumr_class_list_write_unlock(); > + > + return (odpdrv_enumr_class_t)enumr_c; > } > > odpdrv_enumr_t odpdrv_enumr_register(odpdrv_enumr_param_t *param) > @@ -57,8 +168,101 @@ odpdrv_driver_t odpdrv_driver_register(odpdrv_driver_param_t > *param) > return ODPDRV_DRIVER_INVALID; > } > > +/* the following function is called each time probing is needed, i.e. > + * at init or after loading a new module as a module can be anything, > + * including enumerators or drivers */ > +void _odpdrv_driver_probe_drv_items(void) > +{ > + _odpdrv_enumr_class_t *enumr_c; > + > + /* probe unprobed enumerators: */ > + enumr_class_list_read_lock(); > + enumr_c = enumr_class_lst.head; > Where is enumr_class_lst.head initialized? You've initialized the lock part of this struct in _odpdrv_init_global() but not the head. So this may contain uninitialized / stale values from a previous ODP instance. > + while (enumr_c) { > + if (!enumr_c->probed) { > + enumr_c->param.probe(); > + enumr_c->probed = 1; > + } > + enumr_c = enumr_c->next; > + } > + enumr_class_list_read_unlock(); > +} > + > int odpdrv_print_all(void) > { > - ODP_ERR("odpdrv_print_all not Supported yet!\n."); > + _odpdrv_enumr_class_t *enumr_c; > + > + /* we cannot use ODP_DBG before ODP init... */ > + if (init_global_status == UNDONE) > + return 0; > + > + ODP_DBG("ODP Driver status:\n"); > + > + /* print the list of registered enumerator classes: */ > + enumr_class_list_read_lock(); > + enumr_c = enumr_class_lst.head; > + ODP_DBG("The following enumerator classes have been > registered:\n"); > + while (enumr_c) { > + ODP_DBG(" class: %s\n", enumr_c->param.name); > + enumr_c = enumr_c->next; > + } > + enumr_class_list_read_unlock(); > + return 0; > +} > + > +int _odpdrv_driver_init_global(void) > +{ > + /* create a memory pool to for list elements: */ > + list_elt_pool = _odp_ishm_pool_create(NULL, ELT_POOL_SIZE, > + 0, ELT_POOL_SIZE, 0); > + > + /* remember that init global is being done so the further list > allocs > + * are made from the list_elt_pool: */ > + init_global_status = IN_PROGRESS; > + > + /* from now, we want to ensure mutex on the list: init lock: */ > + odp_rwlock_recursive_init(&enumr_class_lst.lock); > + > + /* probe things... */ > + _odpdrv_driver_probe_drv_items(); > + > + return 0; > +} > + > +int _odpdrv_driver_init_local(void) > +{ > + /* remember that init global is done, so list mutexes are used from > + * now */ > + init_global_status = DONE; > Not clear what this does. Also, having the init() function manipulate a global variable is not good form. The idea behind the init_local() calls is that these routines do thread-local initialization. Remember this routine may be called in parallel from multiple threads if the app is launching threads since each of them is going to be calling odp_init_local(). > + return 0; > +} > + > +int _odpdrv_driver_term_global(void) > +{ > + _odpdrv_enumr_class_t *enumr_c; > + > + if (init_global_status == UNDONE) > + return 0; > I'm not sure what this guard is protecting against. If an app calls odp_term_global() before it calls odp_init_global() that is an application programming error and we need not worry about such things. > + > + /* remove all enumerator classes which are registered: */ > + enumr_class_list_write_lock(); > + while (enumr_class_lst.head) { > + enumr_c = enumr_class_lst.head; > + if (enumr_c->param.remove) { /* run remove callback, if > any */ > + if (enumr_c->param.remove()) > + ODP_ERR("Enumerator class %s removal > failed.\n", > + enumr_c->param.name); > + } > + enumr_class_lst.head = enumr_c->next; > + if (enumr_c->pool) > + _odp_ishm_pool_free(list_elt_pool, enumr_c); > + else > + free(enumr_c); > + } > + enumr_class_list_write_unlock(); > + > + /* destroy the list element pool: */ > + _odp_ishm_pool_destroy(list_elt_pool); > + > return 0; > } > diff --git a/platform/linux-generic/include/drv_driver_internal.h > b/platform/linux-generic/include/drv_driver_internal.h > new file mode 100644 > index 0000000..eb06c1b > --- /dev/null > +++ b/platform/linux-generic/include/drv_driver_internal.h > @@ -0,0 +1,22 @@ > +/* Copyright (c) 2017, Linaro Limited > + * All rights reserved. > + * > + * SPDX-License-Identifier: BSD-3-Clause > + */ > + > +#ifndef DRV_DRIVER_INTERNAL_H_ > +#define DRV_DRIVER_INTERNAL_H_ > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +#include <sys/types.h> > + > +void _odpdrv_driver_probe_drv_items(void); > + > +#ifdef __cplusplus > +} > +#endif > + > +#endif > diff --git a/platform/linux-generic/include/odp_internal.h > b/platform/linux-generic/include/odp_internal.h > index 05c8a42..1760b56 100644 > --- a/platform/linux-generic/include/odp_internal.h > +++ b/platform/linux-generic/include/odp_internal.h > @@ -71,6 +71,7 @@ enum init_stage { > CLASSIFICATION_INIT, > TRAFFIC_MNGR_INIT, > NAME_TABLE_INIT, > + DRIVER_INIT, > MODULES_INIT, > ALL_INIT /* All init stages completed */ > }; > @@ -130,6 +131,10 @@ int _odp_ishm_init_local(void); > int _odp_ishm_term_global(void); > int _odp_ishm_term_local(void); > > +int _odpdrv_driver_init_global(void); > +int _odpdrv_driver_init_local(void); > +int _odpdrv_driver_term_global(void); > + > int _odp_modules_init_global(void); > > int cpuinfo_parser(FILE *file, system_info_t *sysinfo); > diff --git a/platform/linux-generic/odp_init.c > b/platform/linux-generic/odp_init.c > index 685e02f..c59cc28 100644 > --- a/platform/linux-generic/odp_init.c > +++ b/platform/linux-generic/odp_init.c > @@ -266,6 +266,12 @@ int odp_init_global(odp_instance_t *instance, > } > stage = NAME_TABLE_INIT; > > + if (_odpdrv_driver_init_global()) { > + ODP_ERR("ODP drivers init failed\n"); > + goto init_failed; > + } > + stage = DRIVER_INIT; > + > if (_odp_modules_init_global()) { > ODP_ERR("ODP modules init failed\n"); > goto init_failed; > @@ -296,6 +302,13 @@ int _odp_term_global(enum init_stage stage) > switch (stage) { > case ALL_INIT: > case MODULES_INIT: > + case DRIVER_INIT: > + if (_odpdrv_driver_term_global()) { > + ODP_ERR("driver term failed.\n"); > + rc = -1; > + } > + /* Fall through */ > + > case NAME_TABLE_INIT: > if (_odp_int_name_tbl_term_global()) { > ODP_ERR("Name table term failed.\n"); > @@ -445,7 +458,13 @@ int odp_init_local(odp_instance_t instance, > odp_thread_type_t thr_type) > ODP_ERR("ODP schedule local init failed.\n"); > goto init_fail; > } > - /* stage = SCHED_INIT; */ > + stage = SCHED_INIT; > + > + if (_odpdrv_driver_init_local()) { > + ODP_ERR("ODP driver local init failed.\n"); > + goto init_fail; > + } > + /* stage = DRIVER_INIT; */ > > return 0; > > -- > 2.7.4 > >
On 23 February 2017 at 04:57, Bill Fischofer <bill.fischofer@linaro.org> wrote: > > > On Wed, Feb 22, 2017 at 6:55 AM, Christophe Milard < > christophe.milard@linaro.org> wrote: > >> The functions to register and probe enumerator classes are added. >> >> Signed-off-by: Christophe Milard <christophe.milard@linaro.org> >> --- >> platform/linux-generic/Makefile.am | 1 + >> platform/linux-generic/_modules.c | 4 + >> platform/linux-generic/drv_driver.c | 212 >> ++++++++++++++++++++- >> .../linux-generic/include/drv_driver_internal.h | 22 +++ >> platform/linux-generic/include/odp_internal.h | 5 + >> platform/linux-generic/odp_init.c | 21 +- >> 6 files changed, 260 insertions(+), 5 deletions(-) >> create mode 100644 platform/linux-generic/include/drv_driver_internal.h >> >> diff --git a/platform/linux-generic/Makefile.am >> b/platform/linux-generic/Makefile.am >> index 66ff53d..b7d1b1a 100644 >> --- a/platform/linux-generic/Makefile.am >> +++ b/platform/linux-generic/Makefile.am >> @@ -133,6 +133,7 @@ noinst_HEADERS = \ >> ${srcdir}/include/_ishm_internal.h \ >> ${srcdir}/include/_ishmphy_internal.h \ >> ${srcdir}/include/_ishmpool_internal.h \ >> + ${srcdir}/include/drv_driver_internal.h\ >> ${srcdir}/include/odp_align_internal.h \ >> ${srcdir}/include/odp_atomic_internal.h \ >> ${srcdir}/include/odp_buffer_inlines.h \ >> diff --git a/platform/linux-generic/_modules.c >> b/platform/linux-generic/_modules.c >> index 6bb854e..b23c81f 100644 >> --- a/platform/linux-generic/_modules.c >> +++ b/platform/linux-generic/_modules.c >> @@ -9,6 +9,7 @@ >> #include <odp/api/std_types.h> >> #include <odp/api/debug.h> >> #include <odp_debug_internal.h> >> +#include <drv_driver_internal.h> >> #include <libconfig.h> >> #include <dlfcn.h> >> >> @@ -40,6 +41,9 @@ static int load_modules(void) >> ODP_DBG("module %s loaded.\n", module_name); >> } >> >> + /* give a chance top the driver interface to probe for new >> things: */ >> > > Garbled phrasing. I assume the intent here was "Give the top driver > interface a chance to probe for new things" ? > > >> + _odpdrv_driver_probe_drv_items(); >> + >> return 0; >> } >> >> diff --git a/platform/linux-generic/drv_driver.c >> b/platform/linux-generic/drv_driver.c >> index 529da48..50956a7 100644 >> --- a/platform/linux-generic/drv_driver.c >> +++ b/platform/linux-generic/drv_driver.c >> @@ -4,20 +4,131 @@ >> * SPDX-License-Identifier: BSD-3-Clause >> */ >> >> +#include <string.h> >> + >> #include <odp_config_internal.h> >> +#include <_ishmpool_internal.h> >> >> #include <odp/api/std_types.h> >> #include <odp/api/debug.h> >> +#include <odp/api/rwlock_recursive.h> >> #include <odp/drv/driver.h> >> +#include <odp/drv/spec/driver.h> >> #include <odp_debug_internal.h> >> +#include <drv_driver_internal.h> >> + >> +static enum {UNDONE, IN_PROGRESS, DONE} init_global_status; >> + >> +/* pool from which different list elements are alocated: */ >> +#define ELT_POOL_SIZE (1 << 20) /* 1Mb */ >> +static _odp_ishm_pool_t *list_elt_pool; >> + >> +typedef struct _odpdrv_enumr_class_s _odpdrv_enumr_class_t; >> + >> +/* an enumerator class (list element) */ >> +struct _odpdrv_enumr_class_s { >> + odpdrv_enumr_class_param_t param; >> + int probed; >> + _odp_ishm_pool_t *pool; >> + struct _odpdrv_enumr_class_s *next; >> +}; >> + >> +/* the enumerator class list: */ >> +typedef struct _odpdrv_enumr_class_lst_t { >> + odp_rwlock_recursive_t lock; >> + _odpdrv_enumr_class_t *head; >> +} _odpdrv_enumr_class_lst_t; >> +static struct _odpdrv_enumr_class_lst_t enumr_class_lst; >> + >> +/* some driver elements (such as enumeraor classes, drivers, devio) may >> + * register before init_global and init_local complete. Mutex will fail >> + * in this cases but should be used later on. >> + * These functions disable the usage of Mutex while it is global init >> i.e. >> + * while single threaded*/ >> > > I don't understand why these tests are needed. You've put the driver init > stuff at the end of odp_init_global() so all the other machinery is > available to you at this point. So these guards seem unnecessary. > Is this flag solving problem described as "driver probe time...", which to cover both statically and dynamically linked modules' situation? at the very beginning: statically linked plugin modules __constructor__ (register) ... odp_init_global(): _odpdrv_driver_init_global(): _odp_modules_init_global(): dynamically linked plugin modules __constructor__ (register), and kick the probing ... One possible alternative may be creating two lists: registered_list: ring-buffer based, no system intialization required, all plugin modules register by only adding themselves into this list but no further operations. (malloc, shm, etc). probed_list: lock protected, probing will actually walk through registered_list and do initialization like shm allocate and then move them into probed list. In this way: eliminate this flag and also the probed flag in structure? > > >> +static void enumr_class_list_read_lock(void) >> +{ >> + if (init_global_status == DONE) >> + odp_rwlock_recursive_read_lock(&enumr_class_lst.lock); >> +} >> + >> +static void enumr_class_list_read_unlock(void) >> +{ >> + if (init_global_status == DONE) >> + odp_rwlock_recursive_read_unlock(&enumr_class_lst.lock); >> +} >> + >> +static void enumr_class_list_write_lock(void) >> +{ >> + if (init_global_status == DONE) >> + odp_rwlock_recursive_write_lock(&enumr_class_lst.lock); >> +} >> + >> +static void enumr_class_list_write_unlock(void) >> +{ >> + if (init_global_status == DONE) >> + odp_rwlock_recursive_write_unlock(&enumr_class_lst.lock); >> +} >> + >> >> odpdrv_enumr_class_t odpdrv_enumr_class_register(od >> pdrv_enumr_class_param_t >> *param) >> { >> - ODP_ERR("NOT Supported yet! Enumerator Class %s Registration!\n.", >> - param->name); >> + _odpdrv_enumr_class_t *enumr_c; >> >> - return ODPDRV_ENUMR_CLASS_INVALID; >> + /* parse the list of already registered enumerator class to make >> + * sure no enumerator with identical name already exists: >> + */ >> + enumr_class_list_read_lock(); >> + enumr_c = enumr_class_lst.head; >> + while (enumr_c) { >> + if (strncmp(param->name, enumr_c->param.name, >> + ODPDRV_NAME_SIZE) == 0) { >> + ODP_ERR("enumerator class %s already exists!\n", >> + param->name); >> + enumr_class_list_read_unlock(); >> + return ODPDRV_ENUMR_CLASS_INVALID; >> + } >> + enumr_c = enumr_c->next; >> + } >> + enumr_class_list_read_unlock(); >> + >> + /* allocate memory for the new enumerator class: >> + * If init_global has not been done yet, then, we cannot allocate >> + * from any _ishm pool (ishm has not even been initialised at this >> + * stage...this happens when statically linked enumerator classes >> + * register: their __constructor__ function is run before main() >> + * is called). But any malloc performed here(before init_global) >> + * will be inherited by any odpthreads (process or pthreads) as we >> + * are still running in the ODP instantiation processes and all >> + * other processes are guaranteed to be descendent of this one... >> + * If init_global has been done, then we allocate from the _ishm >> pool >> + * to guarantee visibility from any ODP thread. >> + */ >> + >> + if (init_global_status == UNDONE) { >> + enumr_c = malloc(sizeof(_odpdrv_enumr_class_t)); >> + if (!enumr_c) >> + return ODPDRV_ENUMR_CLASS_INVALID; >> + enumr_c->pool = NULL; >> + } else { >> + enumr_c = _odp_ishm_pool_alloc(list_elt_pool, >> + >> sizeof(_odpdrv_enumr_class_t)); >> + if (!enumr_c) { >> + ODP_ERR("_odp_ishm_pool_alloc failed!\n"); >> + return ODPDRV_ENUMR_CLASS_INVALID; >> + } >> + enumr_c->pool = list_elt_pool; >> + } >> + >> + /* save init parameters and insert enumerator class in list */ >> + enumr_c->param = *param; >> + enumr_c->probed = 0; >> + enumr_class_list_write_lock(); >> + enumr_c->next = enumr_class_lst.head; >> + enumr_class_lst.head = enumr_c; >> + enumr_class_list_write_unlock(); >> + >> + return (odpdrv_enumr_class_t)enumr_c; >> } >> >> odpdrv_enumr_t odpdrv_enumr_register(odpdrv_enumr_param_t *param) >> @@ -57,8 +168,101 @@ odpdrv_driver_t odpdrv_driver_register(odpdrv_driver_param_t >> *param) >> return ODPDRV_DRIVER_INVALID; >> } >> >> +/* the following function is called each time probing is needed, i.e. >> + * at init or after loading a new module as a module can be anything, >> + * including enumerators or drivers */ >> +void _odpdrv_driver_probe_drv_items(void) >> +{ >> + _odpdrv_enumr_class_t *enumr_c; >> + >> + /* probe unprobed enumerators: */ >> + enumr_class_list_read_lock(); >> + enumr_c = enumr_class_lst.head; >> > > Where is enumr_class_lst.head initialized? You've initialized the lock > part of this struct in _odpdrv_init_global() but not the head. So this may > contain uninitialized / stale values from a previous ODP instance. > > >> + while (enumr_c) { >> + if (!enumr_c->probed) { >> + enumr_c->param.probe(); >> + enumr_c->probed = 1; >> + } >> + enumr_c = enumr_c->next; >> + } >> + enumr_class_list_read_unlock(); >> +} >> + >> int odpdrv_print_all(void) >> { >> - ODP_ERR("odpdrv_print_all not Supported yet!\n."); >> + _odpdrv_enumr_class_t *enumr_c; >> + >> + /* we cannot use ODP_DBG before ODP init... */ >> + if (init_global_status == UNDONE) >> + return 0; >> + >> + ODP_DBG("ODP Driver status:\n"); >> + >> + /* print the list of registered enumerator classes: */ >> + enumr_class_list_read_lock(); >> + enumr_c = enumr_class_lst.head; >> + ODP_DBG("The following enumerator classes have been >> registered:\n"); >> + while (enumr_c) { >> + ODP_DBG(" class: %s\n", enumr_c->param.name); >> + enumr_c = enumr_c->next; >> + } >> + enumr_class_list_read_unlock(); >> + return 0; >> +} >> + >> +int _odpdrv_driver_init_global(void) >> +{ >> + /* create a memory pool to for list elements: */ >> + list_elt_pool = _odp_ishm_pool_create(NULL, ELT_POOL_SIZE, >> + 0, ELT_POOL_SIZE, 0); >> + >> + /* remember that init global is being done so the further list >> allocs >> + * are made from the list_elt_pool: */ >> + init_global_status = IN_PROGRESS; >> + >> + /* from now, we want to ensure mutex on the list: init lock: */ >> + odp_rwlock_recursive_init(&enumr_class_lst.lock); >> + >> + /* probe things... */ >> + _odpdrv_driver_probe_drv_items(); >> + >> + return 0; >> +} >> + >> +int _odpdrv_driver_init_local(void) >> +{ >> + /* remember that init global is done, so list mutexes are used >> from >> + * now */ >> + init_global_status = DONE; >> > > Not clear what this does. Also, having the init() function manipulate a > global variable is not good form. The idea behind the init_local() calls is > that these routines do thread-local initialization. Remember this routine > may be called in parallel from multiple threads if the app is launching > threads since each of them is going to be calling odp_init_local(). > > >> + return 0; >> +} >> + >> +int _odpdrv_driver_term_global(void) >> +{ >> + _odpdrv_enumr_class_t *enumr_c; >> + >> + if (init_global_status == UNDONE) >> + return 0; >> > > I'm not sure what this guard is protecting against. If an app calls > odp_term_global() before it calls odp_init_global() that is an application > programming error and we need not worry about such things. > > >> + >> + /* remove all enumerator classes which are registered: */ >> + enumr_class_list_write_lock(); >> + while (enumr_class_lst.head) { >> + enumr_c = enumr_class_lst.head; >> + if (enumr_c->param.remove) { /* run remove callback, if >> any */ >> + if (enumr_c->param.remove()) >> + ODP_ERR("Enumerator class %s removal >> failed.\n", >> + enumr_c->param.name); >> + } >> + enumr_class_lst.head = enumr_c->next; >> + if (enumr_c->pool) >> + _odp_ishm_pool_free(list_elt_pool, enumr_c); >> + else >> + free(enumr_c); >> + } >> + enumr_class_list_write_unlock(); >> + >> + /* destroy the list element pool: */ >> + _odp_ishm_pool_destroy(list_elt_pool); >> + >> return 0; >> } >> diff --git a/platform/linux-generic/include/drv_driver_internal.h >> b/platform/linux-generic/include/drv_driver_internal.h >> new file mode 100644 >> index 0000000..eb06c1b >> --- /dev/null >> +++ b/platform/linux-generic/include/drv_driver_internal.h >> @@ -0,0 +1,22 @@ >> +/* Copyright (c) 2017, Linaro Limited >> + * All rights reserved. >> + * >> + * SPDX-License-Identifier: BSD-3-Clause >> + */ >> + >> +#ifndef DRV_DRIVER_INTERNAL_H_ >> +#define DRV_DRIVER_INTERNAL_H_ >> + >> +#ifdef __cplusplus >> +extern "C" { >> +#endif >> + >> +#include <sys/types.h> >> + >> +void _odpdrv_driver_probe_drv_items(void); >> + >> +#ifdef __cplusplus >> +} >> +#endif >> + >> +#endif >> diff --git a/platform/linux-generic/include/odp_internal.h >> b/platform/linux-generic/include/odp_internal.h >> index 05c8a42..1760b56 100644 >> --- a/platform/linux-generic/include/odp_internal.h >> +++ b/platform/linux-generic/include/odp_internal.h >> @@ -71,6 +71,7 @@ enum init_stage { >> CLASSIFICATION_INIT, >> TRAFFIC_MNGR_INIT, >> NAME_TABLE_INIT, >> + DRIVER_INIT, >> MODULES_INIT, >> ALL_INIT /* All init stages completed */ >> }; >> @@ -130,6 +131,10 @@ int _odp_ishm_init_local(void); >> int _odp_ishm_term_global(void); >> int _odp_ishm_term_local(void); >> >> +int _odpdrv_driver_init_global(void); >> +int _odpdrv_driver_init_local(void); >> +int _odpdrv_driver_term_global(void); >> + >> int _odp_modules_init_global(void); >> >> int cpuinfo_parser(FILE *file, system_info_t *sysinfo); >> diff --git a/platform/linux-generic/odp_init.c >> b/platform/linux-generic/odp_init.c >> index 685e02f..c59cc28 100644 >> --- a/platform/linux-generic/odp_init.c >> +++ b/platform/linux-generic/odp_init.c >> @@ -266,6 +266,12 @@ int odp_init_global(odp_instance_t *instance, >> } >> stage = NAME_TABLE_INIT; >> >> + if (_odpdrv_driver_init_global()) { >> + ODP_ERR("ODP drivers init failed\n"); >> + goto init_failed; >> + } >> + stage = DRIVER_INIT; >> + >> if (_odp_modules_init_global()) { >> ODP_ERR("ODP modules init failed\n"); >> goto init_failed; >> @@ -296,6 +302,13 @@ int _odp_term_global(enum init_stage stage) >> switch (stage) { >> case ALL_INIT: >> case MODULES_INIT: >> + case DRIVER_INIT: >> + if (_odpdrv_driver_term_global()) { >> + ODP_ERR("driver term failed.\n"); >> + rc = -1; >> + } >> + /* Fall through */ >> + >> case NAME_TABLE_INIT: >> if (_odp_int_name_tbl_term_global()) { >> ODP_ERR("Name table term failed.\n"); >> @@ -445,7 +458,13 @@ int odp_init_local(odp_instance_t instance, >> odp_thread_type_t thr_type) >> ODP_ERR("ODP schedule local init failed.\n"); >> goto init_fail; >> } >> - /* stage = SCHED_INIT; */ >> + stage = SCHED_INIT; >> + >> + if (_odpdrv_driver_init_local()) { >> + ODP_ERR("ODP driver local init failed.\n"); >> + goto init_fail; >> + } >> + /* stage = DRIVER_INIT; */ >> >> return 0; >> >> -- >> 2.7.4 >> >> >
I'd like to revise my some of my earlier review comments now that I understand what's going on here with respect to the use of constructors. I don't like trying to "fake out" various ODP calls as as part of constructor processing as I think that's both risky and not very extensible (or portable across different ODP implementations). What I'd prefer to see is that the constructor simply adds itself to a global static list of routines that should be called implicitly at odp_init_global() time. That way all of these routines can have assurance that the full ODP environment is available to them. This doesn't have a dynamic linked list or anything fancy. It can be a simple static array whose maximum size is specified at ODP configuration time. That controls the maximum number of statically defined drivers supported by this particular ODP implementation. Whether they are actually statically bound or implicitly dynamically loaded (based on a static configuration) I leave to you. On Thu, Feb 23, 2017 at 4:13 AM, Yi He <yi.he@linaro.org> wrote: > > On 23 February 2017 at 04:57, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > >> >> >> On Wed, Feb 22, 2017 at 6:55 AM, Christophe Milard < >> christophe.milard@linaro.org> wrote: >> >>> The functions to register and probe enumerator classes are added. >>> >>> Signed-off-by: Christophe Milard <christophe.milard@linaro.org> >>> --- >>> platform/linux-generic/Makefile.am | 1 + >>> platform/linux-generic/_modules.c | 4 + >>> platform/linux-generic/drv_driver.c | 212 >>> ++++++++++++++++++++- >>> .../linux-generic/include/drv_driver_internal.h | 22 +++ >>> platform/linux-generic/include/odp_internal.h | 5 + >>> platform/linux-generic/odp_init.c | 21 +- >>> 6 files changed, 260 insertions(+), 5 deletions(-) >>> create mode 100644 platform/linux-generic/include/drv_driver_internal.h >>> >>> diff --git a/platform/linux-generic/Makefile.am >>> b/platform/linux-generic/Makefile.am >>> index 66ff53d..b7d1b1a 100644 >>> --- a/platform/linux-generic/Makefile.am >>> +++ b/platform/linux-generic/Makefile.am >>> @@ -133,6 +133,7 @@ noinst_HEADERS = \ >>> ${srcdir}/include/_ishm_internal.h \ >>> ${srcdir}/include/_ishmphy_internal.h \ >>> ${srcdir}/include/_ishmpool_internal.h \ >>> + ${srcdir}/include/drv_driver_internal.h\ >>> ${srcdir}/include/odp_align_internal.h \ >>> ${srcdir}/include/odp_atomic_internal.h \ >>> ${srcdir}/include/odp_buffer_inlines.h \ >>> diff --git a/platform/linux-generic/_modules.c >>> b/platform/linux-generic/_modules.c >>> index 6bb854e..b23c81f 100644 >>> --- a/platform/linux-generic/_modules.c >>> +++ b/platform/linux-generic/_modules.c >>> @@ -9,6 +9,7 @@ >>> #include <odp/api/std_types.h> >>> #include <odp/api/debug.h> >>> #include <odp_debug_internal.h> >>> +#include <drv_driver_internal.h> >>> #include <libconfig.h> >>> #include <dlfcn.h> >>> >>> @@ -40,6 +41,9 @@ static int load_modules(void) >>> ODP_DBG("module %s loaded.\n", module_name); >>> } >>> >>> + /* give a chance top the driver interface to probe for new >>> things: */ >>> >> >> Garbled phrasing. I assume the intent here was "Give the top driver >> interface a chance to probe for new things" ? >> >> >>> + _odpdrv_driver_probe_drv_items(); >>> + >>> return 0; >>> } >>> >>> diff --git a/platform/linux-generic/drv_driver.c >>> b/platform/linux-generic/drv_driver.c >>> index 529da48..50956a7 100644 >>> --- a/platform/linux-generic/drv_driver.c >>> +++ b/platform/linux-generic/drv_driver.c >>> @@ -4,20 +4,131 @@ >>> * SPDX-License-Identifier: BSD-3-Clause >>> */ >>> >>> +#include <string.h> >>> + >>> #include <odp_config_internal.h> >>> +#include <_ishmpool_internal.h> >>> >>> #include <odp/api/std_types.h> >>> #include <odp/api/debug.h> >>> +#include <odp/api/rwlock_recursive.h> >>> #include <odp/drv/driver.h> >>> +#include <odp/drv/spec/driver.h> >>> #include <odp_debug_internal.h> >>> +#include <drv_driver_internal.h> >>> + >>> +static enum {UNDONE, IN_PROGRESS, DONE} init_global_status; >>> + >>> +/* pool from which different list elements are alocated: */ >>> +#define ELT_POOL_SIZE (1 << 20) /* 1Mb */ >>> +static _odp_ishm_pool_t *list_elt_pool; >>> + >>> +typedef struct _odpdrv_enumr_class_s _odpdrv_enumr_class_t; >>> + >>> +/* an enumerator class (list element) */ >>> +struct _odpdrv_enumr_class_s { >>> + odpdrv_enumr_class_param_t param; >>> + int probed; >>> + _odp_ishm_pool_t *pool; >>> + struct _odpdrv_enumr_class_s *next; >>> +}; >>> + >>> +/* the enumerator class list: */ >>> +typedef struct _odpdrv_enumr_class_lst_t { >>> + odp_rwlock_recursive_t lock; >>> + _odpdrv_enumr_class_t *head; >>> +} _odpdrv_enumr_class_lst_t; >>> +static struct _odpdrv_enumr_class_lst_t enumr_class_lst; >>> + >>> +/* some driver elements (such as enumeraor classes, drivers, devio) may >>> + * register before init_global and init_local complete. Mutex will fail >>> + * in this cases but should be used later on. >>> + * These functions disable the usage of Mutex while it is global init >>> i.e. >>> + * while single threaded*/ >>> >> >> I don't understand why these tests are needed. You've put the driver init >> stuff at the end of odp_init_global() so all the other machinery is >> available to you at this point. So these guards seem unnecessary. >> > > Is this flag solving problem described as "driver probe time...", which to > cover both statically and dynamically linked modules' situation? > > at the very beginning: statically linked plugin modules > __constructor__ (register) ... > odp_init_global(): > _odpdrv_driver_init_global(): > _odp_modules_init_global(): dynamically linked plugin modules > __constructor__ (register), and kick the probing ... > > One possible alternative may be creating two lists: > > registered_list: ring-buffer based, no system intialization required, all > plugin modules register by only adding themselves into this list but no > further operations. (malloc, shm, etc). > probed_list: lock protected, probing will actually walk through > registered_list and do initialization like shm allocate and then move them > into probed list. > > In this way: eliminate this flag and also the probed flag in structure? > > >> > >> >>> +static void enumr_class_list_read_lock(void) >>> +{ >>> + if (init_global_status == DONE) >>> + odp_rwlock_recursive_read_lock(&enumr_class_lst.lock); >>> +} >>> + >>> +static void enumr_class_list_read_unlock(void) >>> +{ >>> + if (init_global_status == DONE) >>> + odp_rwlock_recursive_read_unlock(&enumr_class_lst.lock); >>> +} >>> + >>> +static void enumr_class_list_write_lock(void) >>> +{ >>> + if (init_global_status == DONE) >>> + odp_rwlock_recursive_write_lock(&enumr_class_lst.lock); >>> +} >>> + >>> +static void enumr_class_list_write_unlock(void) >>> +{ >>> + if (init_global_status == DONE) >>> + odp_rwlock_recursive_write_un >>> lock(&enumr_class_lst.lock); >>> +} >>> + >>> >>> odpdrv_enumr_class_t odpdrv_enumr_class_register(od >>> pdrv_enumr_class_param_t >>> *param) >>> { >>> - ODP_ERR("NOT Supported yet! Enumerator Class %s >>> Registration!\n.", >>> - param->name); >>> + _odpdrv_enumr_class_t *enumr_c; >>> >>> - return ODPDRV_ENUMR_CLASS_INVALID; >>> + /* parse the list of already registered enumerator class to make >>> + * sure no enumerator with identical name already exists: >>> + */ >>> + enumr_class_list_read_lock(); >>> + enumr_c = enumr_class_lst.head; >>> + while (enumr_c) { >>> + if (strncmp(param->name, enumr_c->param.name, >>> + ODPDRV_NAME_SIZE) == 0) { >>> + ODP_ERR("enumerator class %s already exists!\n", >>> + param->name); >>> + enumr_class_list_read_unlock(); >>> + return ODPDRV_ENUMR_CLASS_INVALID; >>> + } >>> + enumr_c = enumr_c->next; >>> + } >>> + enumr_class_list_read_unlock(); >>> + >>> + /* allocate memory for the new enumerator class: >>> + * If init_global has not been done yet, then, we cannot allocate >>> + * from any _ishm pool (ishm has not even been initialised at >>> this >>> + * stage...this happens when statically linked enumerator classes >>> + * register: their __constructor__ function is run before main() >>> + * is called). But any malloc performed here(before init_global) >>> + * will be inherited by any odpthreads (process or pthreads) as >>> we >>> + * are still running in the ODP instantiation processes and all >>> + * other processes are guaranteed to be descendent of this one... >>> + * If init_global has been done, then we allocate from the _ishm >>> pool >>> + * to guarantee visibility from any ODP thread. >>> + */ >>> + >>> + if (init_global_status == UNDONE) { >>> + enumr_c = malloc(sizeof(_odpdrv_enumr_class_t)); >>> + if (!enumr_c) >>> + return ODPDRV_ENUMR_CLASS_INVALID; >>> + enumr_c->pool = NULL; >>> + } else { >>> + enumr_c = _odp_ishm_pool_alloc(list_elt_pool, >>> + >>> sizeof(_odpdrv_enumr_class_t)); >>> + if (!enumr_c) { >>> + ODP_ERR("_odp_ishm_pool_alloc failed!\n"); >>> + return ODPDRV_ENUMR_CLASS_INVALID; >>> + } >>> + enumr_c->pool = list_elt_pool; >>> + } >>> + >>> + /* save init parameters and insert enumerator class in list */ >>> + enumr_c->param = *param; >>> + enumr_c->probed = 0; >>> + enumr_class_list_write_lock(); >>> + enumr_c->next = enumr_class_lst.head; >>> + enumr_class_lst.head = enumr_c; >>> + enumr_class_list_write_unlock(); >>> + >>> + return (odpdrv_enumr_class_t)enumr_c; >>> } >>> >>> odpdrv_enumr_t odpdrv_enumr_register(odpdrv_enumr_param_t *param) >>> @@ -57,8 +168,101 @@ odpdrv_driver_t odpdrv_driver_register(odpdrv_driver_param_t >>> *param) >>> return ODPDRV_DRIVER_INVALID; >>> } >>> >>> +/* the following function is called each time probing is needed, i.e. >>> + * at init or after loading a new module as a module can be anything, >>> + * including enumerators or drivers */ >>> +void _odpdrv_driver_probe_drv_items(void) >>> +{ >>> + _odpdrv_enumr_class_t *enumr_c; >>> + >>> + /* probe unprobed enumerators: */ >>> + enumr_class_list_read_lock(); >>> + enumr_c = enumr_class_lst.head; >>> >> >> Where is enumr_class_lst.head initialized? You've initialized the lock >> part of this struct in _odpdrv_init_global() but not the head. So this may >> contain uninitialized / stale values from a previous ODP instance. >> >> >>> + while (enumr_c) { >>> + if (!enumr_c->probed) { >>> + enumr_c->param.probe(); >>> + enumr_c->probed = 1; >>> + } >>> + enumr_c = enumr_c->next; >>> + } >>> + enumr_class_list_read_unlock(); >>> +} >>> + >>> int odpdrv_print_all(void) >>> { >>> - ODP_ERR("odpdrv_print_all not Supported yet!\n."); >>> + _odpdrv_enumr_class_t *enumr_c; >>> + >>> + /* we cannot use ODP_DBG before ODP init... */ >>> + if (init_global_status == UNDONE) >>> + return 0; >>> + >>> + ODP_DBG("ODP Driver status:\n"); >>> + >>> + /* print the list of registered enumerator classes: */ >>> + enumr_class_list_read_lock(); >>> + enumr_c = enumr_class_lst.head; >>> + ODP_DBG("The following enumerator classes have been >>> registered:\n"); >>> + while (enumr_c) { >>> + ODP_DBG(" class: %s\n", enumr_c->param.name); >>> + enumr_c = enumr_c->next; >>> + } >>> + enumr_class_list_read_unlock(); >>> + return 0; >>> +} >>> + >>> +int _odpdrv_driver_init_global(void) >>> +{ >>> + /* create a memory pool to for list elements: */ >>> + list_elt_pool = _odp_ishm_pool_create(NULL, ELT_POOL_SIZE, >>> + 0, ELT_POOL_SIZE, 0); >>> + >>> + /* remember that init global is being done so the further list >>> allocs >>> + * are made from the list_elt_pool: */ >>> + init_global_status = IN_PROGRESS; >>> + >>> + /* from now, we want to ensure mutex on the list: init lock: */ >>> + odp_rwlock_recursive_init(&enumr_class_lst.lock); >>> + >>> + /* probe things... */ >>> + _odpdrv_driver_probe_drv_items(); >>> + >>> + return 0; >>> +} >>> + >>> +int _odpdrv_driver_init_local(void) >>> +{ >>> + /* remember that init global is done, so list mutexes are used >>> from >>> + * now */ >>> + init_global_status = DONE; >>> >> >> Not clear what this does. Also, having the init() function manipulate a >> global variable is not good form. The idea behind the init_local() calls is >> that these routines do thread-local initialization. Remember this routine >> may be called in parallel from multiple threads if the app is launching >> threads since each of them is going to be calling odp_init_local(). >> >> >>> + return 0; >>> +} >>> + >>> +int _odpdrv_driver_term_global(void) >>> +{ >>> + _odpdrv_enumr_class_t *enumr_c; >>> + >>> + if (init_global_status == UNDONE) >>> + return 0; >>> >> >> I'm not sure what this guard is protecting against. If an app calls >> odp_term_global() before it calls odp_init_global() that is an application >> programming error and we need not worry about such things. >> >> >>> + >>> + /* remove all enumerator classes which are registered: */ >>> + enumr_class_list_write_lock(); >>> + while (enumr_class_lst.head) { >>> + enumr_c = enumr_class_lst.head; >>> + if (enumr_c->param.remove) { /* run remove callback, if >>> any */ >>> + if (enumr_c->param.remove()) >>> + ODP_ERR("Enumerator class %s removal >>> failed.\n", >>> + enumr_c->param.name); >>> + } >>> + enumr_class_lst.head = enumr_c->next; >>> + if (enumr_c->pool) >>> + _odp_ishm_pool_free(list_elt_pool, enumr_c); >>> + else >>> + free(enumr_c); >>> + } >>> + enumr_class_list_write_unlock(); >>> + >>> + /* destroy the list element pool: */ >>> + _odp_ishm_pool_destroy(list_elt_pool); >>> + >>> return 0; >>> } >>> diff --git a/platform/linux-generic/include/drv_driver_internal.h >>> b/platform/linux-generic/include/drv_driver_internal.h >>> new file mode 100644 >>> index 0000000..eb06c1b >>> --- /dev/null >>> +++ b/platform/linux-generic/include/drv_driver_internal.h >>> @@ -0,0 +1,22 @@ >>> +/* Copyright (c) 2017, Linaro Limited >>> + * All rights reserved. >>> + * >>> + * SPDX-License-Identifier: BSD-3-Clause >>> + */ >>> + >>> +#ifndef DRV_DRIVER_INTERNAL_H_ >>> +#define DRV_DRIVER_INTERNAL_H_ >>> + >>> +#ifdef __cplusplus >>> +extern "C" { >>> +#endif >>> + >>> +#include <sys/types.h> >>> + >>> +void _odpdrv_driver_probe_drv_items(void); >>> + >>> +#ifdef __cplusplus >>> +} >>> +#endif >>> + >>> +#endif >>> diff --git a/platform/linux-generic/include/odp_internal.h >>> b/platform/linux-generic/include/odp_internal.h >>> index 05c8a42..1760b56 100644 >>> --- a/platform/linux-generic/include/odp_internal.h >>> +++ b/platform/linux-generic/include/odp_internal.h >>> @@ -71,6 +71,7 @@ enum init_stage { >>> CLASSIFICATION_INIT, >>> TRAFFIC_MNGR_INIT, >>> NAME_TABLE_INIT, >>> + DRIVER_INIT, >>> MODULES_INIT, >>> ALL_INIT /* All init stages completed */ >>> }; >>> @@ -130,6 +131,10 @@ int _odp_ishm_init_local(void); >>> int _odp_ishm_term_global(void); >>> int _odp_ishm_term_local(void); >>> >>> +int _odpdrv_driver_init_global(void); >>> +int _odpdrv_driver_init_local(void); >>> +int _odpdrv_driver_term_global(void); >>> + >>> int _odp_modules_init_global(void); >>> >>> int cpuinfo_parser(FILE *file, system_info_t *sysinfo); >>> diff --git a/platform/linux-generic/odp_init.c >>> b/platform/linux-generic/odp_init.c >>> index 685e02f..c59cc28 100644 >>> --- a/platform/linux-generic/odp_init.c >>> +++ b/platform/linux-generic/odp_init.c >>> @@ -266,6 +266,12 @@ int odp_init_global(odp_instance_t *instance, >>> } >>> stage = NAME_TABLE_INIT; >>> >>> + if (_odpdrv_driver_init_global()) { >>> + ODP_ERR("ODP drivers init failed\n"); >>> + goto init_failed; >>> + } >>> + stage = DRIVER_INIT; >>> + >>> if (_odp_modules_init_global()) { >>> ODP_ERR("ODP modules init failed\n"); >>> goto init_failed; >>> @@ -296,6 +302,13 @@ int _odp_term_global(enum init_stage stage) >>> switch (stage) { >>> case ALL_INIT: >>> case MODULES_INIT: >>> + case DRIVER_INIT: >>> + if (_odpdrv_driver_term_global()) { >>> + ODP_ERR("driver term failed.\n"); >>> + rc = -1; >>> + } >>> + /* Fall through */ >>> + >>> case NAME_TABLE_INIT: >>> if (_odp_int_name_tbl_term_global()) { >>> ODP_ERR("Name table term failed.\n"); >>> @@ -445,7 +458,13 @@ int odp_init_local(odp_instance_t instance, >>> odp_thread_type_t thr_type) >>> ODP_ERR("ODP schedule local init failed.\n"); >>> goto init_fail; >>> } >>> - /* stage = SCHED_INIT; */ >>> + stage = SCHED_INIT; >>> + >>> + if (_odpdrv_driver_init_local()) { >>> + ODP_ERR("ODP driver local init failed.\n"); >>> + goto init_fail; >>> + } >>> + /* stage = DRIVER_INIT; */ >>> >>> return 0; >>> >>> -- >>> 2.7.4 >>> >>> >> >
diff --git a/platform/linux-generic/Makefile.am b/platform/linux-generic/Makefile.am index 66ff53d..b7d1b1a 100644 --- a/platform/linux-generic/Makefile.am +++ b/platform/linux-generic/Makefile.am @@ -133,6 +133,7 @@ noinst_HEADERS = \ ${srcdir}/include/_ishm_internal.h \ ${srcdir}/include/_ishmphy_internal.h \ ${srcdir}/include/_ishmpool_internal.h \ + ${srcdir}/include/drv_driver_internal.h\ ${srcdir}/include/odp_align_internal.h \ ${srcdir}/include/odp_atomic_internal.h \ ${srcdir}/include/odp_buffer_inlines.h \ diff --git a/platform/linux-generic/_modules.c b/platform/linux-generic/_modules.c index 6bb854e..b23c81f 100644 --- a/platform/linux-generic/_modules.c +++ b/platform/linux-generic/_modules.c @@ -9,6 +9,7 @@ #include <odp/api/std_types.h> #include <odp/api/debug.h> #include <odp_debug_internal.h> +#include <drv_driver_internal.h> #include <libconfig.h> #include <dlfcn.h> @@ -40,6 +41,9 @@ static int load_modules(void) ODP_DBG("module %s loaded.\n", module_name); } + /* give a chance top the driver interface to probe for new things: */ + _odpdrv_driver_probe_drv_items(); + return 0; } diff --git a/platform/linux-generic/drv_driver.c b/platform/linux-generic/drv_driver.c index 529da48..50956a7 100644 --- a/platform/linux-generic/drv_driver.c +++ b/platform/linux-generic/drv_driver.c @@ -4,20 +4,131 @@ * SPDX-License-Identifier: BSD-3-Clause */ +#include <string.h> + #include <odp_config_internal.h> +#include <_ishmpool_internal.h> #include <odp/api/std_types.h> #include <odp/api/debug.h> +#include <odp/api/rwlock_recursive.h> #include <odp/drv/driver.h> +#include <odp/drv/spec/driver.h> #include <odp_debug_internal.h> +#include <drv_driver_internal.h> + +static enum {UNDONE, IN_PROGRESS, DONE} init_global_status; + +/* pool from which different list elements are alocated: */ +#define ELT_POOL_SIZE (1 << 20) /* 1Mb */ +static _odp_ishm_pool_t *list_elt_pool; + +typedef struct _odpdrv_enumr_class_s _odpdrv_enumr_class_t; + +/* an enumerator class (list element) */ +struct _odpdrv_enumr_class_s { + odpdrv_enumr_class_param_t param; + int probed; + _odp_ishm_pool_t *pool; + struct _odpdrv_enumr_class_s *next; +}; + +/* the enumerator class list: */ +typedef struct _odpdrv_enumr_class_lst_t { + odp_rwlock_recursive_t lock; + _odpdrv_enumr_class_t *head; +} _odpdrv_enumr_class_lst_t; +static struct _odpdrv_enumr_class_lst_t enumr_class_lst; + +/* some driver elements (such as enumeraor classes, drivers, devio) may + * register before init_global and init_local complete. Mutex will fail + * in this cases but should be used later on. + * These functions disable the usage of Mutex while it is global init i.e. + * while single threaded*/ +static void enumr_class_list_read_lock(void) +{ + if (init_global_status == DONE) + odp_rwlock_recursive_read_lock(&enumr_class_lst.lock); +} + +static void enumr_class_list_read_unlock(void) +{ + if (init_global_status == DONE) + odp_rwlock_recursive_read_unlock(&enumr_class_lst.lock); +} + +static void enumr_class_list_write_lock(void) +{ + if (init_global_status == DONE) + odp_rwlock_recursive_write_lock(&enumr_class_lst.lock); +} + +static void enumr_class_list_write_unlock(void) +{ + if (init_global_status == DONE) + odp_rwlock_recursive_write_unlock(&enumr_class_lst.lock); +} + odpdrv_enumr_class_t odpdrv_enumr_class_register(odpdrv_enumr_class_param_t *param) { - ODP_ERR("NOT Supported yet! Enumerator Class %s Registration!\n.", - param->name); + _odpdrv_enumr_class_t *enumr_c; - return ODPDRV_ENUMR_CLASS_INVALID; + /* parse the list of already registered enumerator class to make + * sure no enumerator with identical name already exists: + */ + enumr_class_list_read_lock(); + enumr_c = enumr_class_lst.head; + while (enumr_c) { + if (strncmp(param->name, enumr_c->param.name, + ODPDRV_NAME_SIZE) == 0) { + ODP_ERR("enumerator class %s already exists!\n", + param->name); + enumr_class_list_read_unlock(); + return ODPDRV_ENUMR_CLASS_INVALID; + } + enumr_c = enumr_c->next; + } + enumr_class_list_read_unlock(); + + /* allocate memory for the new enumerator class: + * If init_global has not been done yet, then, we cannot allocate + * from any _ishm pool (ishm has not even been initialised at this + * stage...this happens when statically linked enumerator classes + * register: their __constructor__ function is run before main() + * is called). But any malloc performed here(before init_global) + * will be inherited by any odpthreads (process or pthreads) as we + * are still running in the ODP instantiation processes and all + * other processes are guaranteed to be descendent of this one... + * If init_global has been done, then we allocate from the _ishm pool + * to guarantee visibility from any ODP thread. + */ + + if (init_global_status == UNDONE) { + enumr_c = malloc(sizeof(_odpdrv_enumr_class_t)); + if (!enumr_c) + return ODPDRV_ENUMR_CLASS_INVALID; + enumr_c->pool = NULL; + } else { + enumr_c = _odp_ishm_pool_alloc(list_elt_pool, + sizeof(_odpdrv_enumr_class_t)); + if (!enumr_c) { + ODP_ERR("_odp_ishm_pool_alloc failed!\n"); + return ODPDRV_ENUMR_CLASS_INVALID; + } + enumr_c->pool = list_elt_pool; + } + + /* save init parameters and insert enumerator class in list */ + enumr_c->param = *param; + enumr_c->probed = 0; + enumr_class_list_write_lock(); + enumr_c->next = enumr_class_lst.head; + enumr_class_lst.head = enumr_c; + enumr_class_list_write_unlock(); + + return (odpdrv_enumr_class_t)enumr_c; } odpdrv_enumr_t odpdrv_enumr_register(odpdrv_enumr_param_t *param) @@ -57,8 +168,101 @@ odpdrv_driver_t odpdrv_driver_register(odpdrv_driver_param_t *param) return ODPDRV_DRIVER_INVALID; } +/* the following function is called each time probing is needed, i.e. + * at init or after loading a new module as a module can be anything, + * including enumerators or drivers */ +void _odpdrv_driver_probe_drv_items(void) +{ + _odpdrv_enumr_class_t *enumr_c; + + /* probe unprobed enumerators: */ + enumr_class_list_read_lock(); + enumr_c = enumr_class_lst.head; + while (enumr_c) { + if (!enumr_c->probed) { + enumr_c->param.probe(); + enumr_c->probed = 1; + } + enumr_c = enumr_c->next; + } + enumr_class_list_read_unlock(); +} + int odpdrv_print_all(void) { - ODP_ERR("odpdrv_print_all not Supported yet!\n."); + _odpdrv_enumr_class_t *enumr_c; + + /* we cannot use ODP_DBG before ODP init... */ + if (init_global_status == UNDONE) + return 0; + + ODP_DBG("ODP Driver status:\n"); + + /* print the list of registered enumerator classes: */ + enumr_class_list_read_lock(); + enumr_c = enumr_class_lst.head; + ODP_DBG("The following enumerator classes have been registered:\n"); + while (enumr_c) { + ODP_DBG(" class: %s\n", enumr_c->param.name); + enumr_c = enumr_c->next; + } + enumr_class_list_read_unlock(); + return 0; +} + +int _odpdrv_driver_init_global(void) +{ + /* create a memory pool to for list elements: */ + list_elt_pool = _odp_ishm_pool_create(NULL, ELT_POOL_SIZE, + 0, ELT_POOL_SIZE, 0); + + /* remember that init global is being done so the further list allocs + * are made from the list_elt_pool: */ + init_global_status = IN_PROGRESS; + + /* from now, we want to ensure mutex on the list: init lock: */ + odp_rwlock_recursive_init(&enumr_class_lst.lock); + + /* probe things... */ + _odpdrv_driver_probe_drv_items(); + + return 0; +} + +int _odpdrv_driver_init_local(void) +{ + /* remember that init global is done, so list mutexes are used from + * now */ + init_global_status = DONE; + return 0; +} + +int _odpdrv_driver_term_global(void) +{ + _odpdrv_enumr_class_t *enumr_c; + + if (init_global_status == UNDONE) + return 0; + + /* remove all enumerator classes which are registered: */ + enumr_class_list_write_lock(); + while (enumr_class_lst.head) { + enumr_c = enumr_class_lst.head; + if (enumr_c->param.remove) { /* run remove callback, if any */ + if (enumr_c->param.remove()) + ODP_ERR("Enumerator class %s removal failed.\n", + enumr_c->param.name); + } + enumr_class_lst.head = enumr_c->next; + if (enumr_c->pool) + _odp_ishm_pool_free(list_elt_pool, enumr_c); + else + free(enumr_c); + } + enumr_class_list_write_unlock(); + + /* destroy the list element pool: */ + _odp_ishm_pool_destroy(list_elt_pool); + return 0; } diff --git a/platform/linux-generic/include/drv_driver_internal.h b/platform/linux-generic/include/drv_driver_internal.h new file mode 100644 index 0000000..eb06c1b --- /dev/null +++ b/platform/linux-generic/include/drv_driver_internal.h @@ -0,0 +1,22 @@ +/* Copyright (c) 2017, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +#ifndef DRV_DRIVER_INTERNAL_H_ +#define DRV_DRIVER_INTERNAL_H_ + +#ifdef __cplusplus +extern "C" { +#endif + +#include <sys/types.h> + +void _odpdrv_driver_probe_drv_items(void); + +#ifdef __cplusplus +} +#endif + +#endif diff --git a/platform/linux-generic/include/odp_internal.h b/platform/linux-generic/include/odp_internal.h index 05c8a42..1760b56 100644 --- a/platform/linux-generic/include/odp_internal.h +++ b/platform/linux-generic/include/odp_internal.h @@ -71,6 +71,7 @@ enum init_stage { CLASSIFICATION_INIT, TRAFFIC_MNGR_INIT, NAME_TABLE_INIT, + DRIVER_INIT, MODULES_INIT, ALL_INIT /* All init stages completed */ }; @@ -130,6 +131,10 @@ int _odp_ishm_init_local(void); int _odp_ishm_term_global(void); int _odp_ishm_term_local(void); +int _odpdrv_driver_init_global(void); +int _odpdrv_driver_init_local(void); +int _odpdrv_driver_term_global(void); + int _odp_modules_init_global(void); int cpuinfo_parser(FILE *file, system_info_t *sysinfo); diff --git a/platform/linux-generic/odp_init.c b/platform/linux-generic/odp_init.c index 685e02f..c59cc28 100644 --- a/platform/linux-generic/odp_init.c +++ b/platform/linux-generic/odp_init.c @@ -266,6 +266,12 @@ int odp_init_global(odp_instance_t *instance, } stage = NAME_TABLE_INIT; + if (_odpdrv_driver_init_global()) { + ODP_ERR("ODP drivers init failed\n"); + goto init_failed; + } + stage = DRIVER_INIT; + if (_odp_modules_init_global()) { ODP_ERR("ODP modules init failed\n"); goto init_failed; @@ -296,6 +302,13 @@ int _odp_term_global(enum init_stage stage) switch (stage) { case ALL_INIT: case MODULES_INIT: + case DRIVER_INIT: + if (_odpdrv_driver_term_global()) { + ODP_ERR("driver term failed.\n"); + rc = -1; + } + /* Fall through */ + case NAME_TABLE_INIT: if (_odp_int_name_tbl_term_global()) { ODP_ERR("Name table term failed.\n"); @@ -445,7 +458,13 @@ int odp_init_local(odp_instance_t instance, odp_thread_type_t thr_type) ODP_ERR("ODP schedule local init failed.\n"); goto init_fail; } - /* stage = SCHED_INIT; */ + stage = SCHED_INIT; + + if (_odpdrv_driver_init_local()) { + ODP_ERR("ODP driver local init failed.\n"); + goto init_fail; + } + /* stage = DRIVER_INIT; */ return 0;
The functions to register and probe enumerator classes are added. Signed-off-by: Christophe Milard <christophe.milard@linaro.org> --- platform/linux-generic/Makefile.am | 1 + platform/linux-generic/_modules.c | 4 + platform/linux-generic/drv_driver.c | 212 ++++++++++++++++++++- .../linux-generic/include/drv_driver_internal.h | 22 +++ platform/linux-generic/include/odp_internal.h | 5 + platform/linux-generic/odp_init.c | 21 +- 6 files changed, 260 insertions(+), 5 deletions(-) create mode 100644 platform/linux-generic/include/drv_driver_internal.h -- 2.7.4