On 22 February 2017 at 22:46, Bill Fischofer <bill.fischo...@linaro.org> wrote:
>
>
> On Wed, Feb 22, 2017 at 6:55 AM, Christophe Milard
> <christophe.mil...@linaro.org> wrote:
>>
>> The enumerator registration functions for the linux-gen ODP
>> implementation.
>>
>> Signed-off-by: Christophe Milard <christophe.mil...@linaro.org>
>> ---
>> platform/linux-generic/drv_driver.c | 135
>> +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 132 insertions(+), 3 deletions(-)
>>
>> diff --git a/platform/linux-generic/drv_driver.c
>> b/platform/linux-generic/drv_driver.c
>> index 50956a7..ee0a75c 100644
>> --- a/platform/linux-generic/drv_driver.c
>> +++ b/platform/linux-generic/drv_driver.c
>> @@ -24,6 +24,7 @@ static enum {UNDONE, IN_PROGRESS, DONE}
>> init_global_status;
>> static _odp_ishm_pool_t *list_elt_pool;
>>
>> typedef struct _odpdrv_enumr_class_s _odpdrv_enumr_class_t;
>> +typedef struct _odpdrv_enumr_s _odpdrv_enumr_t;
>>
>> /* an enumerator class (list element) */
>> struct _odpdrv_enumr_class_s {
>> @@ -40,6 +41,20 @@ typedef struct _odpdrv_enumr_class_lst_t {
>> } _odpdrv_enumr_class_lst_t;
>> static struct _odpdrv_enumr_class_lst_t enumr_class_lst;
>>
>> +/* an enumerator (list element) */
>> +struct _odpdrv_enumr_s {
>> + odpdrv_enumr_param_t param;
>> + int probed;
>> + struct _odpdrv_enumr_s *next;
>> +};
>> +
>> +/* the enumerator list: */
>> +typedef struct _odpdrv_enumr_lst_t {
>> + odp_rwlock_recursive_t lock;
>> + _odpdrv_enumr_t *head;
>> +} _odpdrv_enumr_lst_t;
>> +static struct _odpdrv_enumr_lst_t enumr_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.
>> @@ -69,6 +84,29 @@ static void enumr_class_list_write_unlock(void)
>> odp_rwlock_recursive_write_unlock(&enumr_class_lst.lock);
>> }
>>
>> +static void enumr_list_read_lock(void)
>> +{
>> + if (init_global_status == DONE)
>> + odp_rwlock_recursive_read_lock(&enumr_lst.lock);
>
>
> Same comments here as before with respect to the need for
> init_global_status. This seems unnecessarily complicated.
The problem is the folowing: there are two possible points (in time)
when a driver items (enumerator classes, drivers, devios...) may
register:
1) when ODP is running: this typically happens when a shared module is loaded
2) at early init, before main() is run, i.e. long before
odp_init_global(): this happens when the driver elements are
statically linked with ODP: their constructors are run before main().
This is a gcc construct. nothing we can do about it.
In case 1) we want to protect the lists again concurrent accesses.
in case 2) the ODP mutexes are not even initalised and won't work, but
we are monothreaded at thos time so we can ignore the mutex.
Hence this code
>
>>
>> +}
>> +
>> +static void enumr_list_read_unlock(void)
>> +{
>> + if (init_global_status == DONE)
>> + odp_rwlock_recursive_read_unlock(&enumr_lst.lock);
>> +}
>> +
>> +static void enumr_list_write_lock(void)
>> +{
>> + if (init_global_status == DONE)
>> + odp_rwlock_recursive_write_lock(&enumr_lst.lock);
>> +}
>> +
>> +static void enumr_list_write_unlock(void)
>> +{
>> + if (init_global_status == DONE)
>> + odp_rwlock_recursive_write_unlock(&enumr_lst.lock);
>> +}
>>
>> odpdrv_enumr_class_t
>> odpdrv_enumr_class_register(odpdrv_enumr_class_param_t
>> *param)
>> @@ -133,10 +171,53 @@ odpdrv_enumr_class_t
>> odpdrv_enumr_class_register(odpdrv_enumr_class_param_t
>>
>> odpdrv_enumr_t odpdrv_enumr_register(odpdrv_enumr_param_t *param)
>> {
>> - ODP_ERR("NOT Supported yet! Enumerator API %s Registration!\n.",
>> - param->api_name);
>> + _odpdrv_enumr_t *enumr;
>> + _odpdrv_enumr_class_t *enumr_c;
>> + int found_class = 0;
>> +
>> + /* make sure that the provided enumerator_class does indeed exist:
>> */
>> + enumr_class_list_read_lock();
>> + enumr_c = enumr_class_lst.head;
>
>
> Same question here regarding where is enumr_class_lst.head initialized? It
> seems we're referencing an uninitialized / stale value here. My guess is
> coverity would flag this.
The list heads are static, hence initialised to NULL.
Gcc actually provides means of executing constructor with different
priorities, but I dont really like using that: not sure about
portability:
Assuming we are not using this hack, there is not way I can guarantee
that any of my (ODP) code gets executed before a enumerator class (or
other element) constructor get registered.
I.e. I have to rely on the static zeroing.
>
>>
>> + while (enumr_c) {
>> + if ((_odpdrv_enumr_class_t *)(void *)param->enumr_class ==
>> + enumr_c) {
>> + found_class = 1;
>> + break;
>> + }
>> + enumr_c = enumr_c->next;
>> + }
>> + enumr_class_list_read_unlock();
>> + if (!found_class) {
>> + ODP_ERR("invalid enumerator class provided!\n");
>> + return ODPDRV_ENUMR_INVALID;
>> + }
>> +
>> + /* allocate memory for the new enumerator:
>> + * If init_global has not been done yet, we have a big issue,
>> + * as none of the enumerator classes should be porbed before that!
>
>
> Typo: probed
> Again, not sure if this is something we need to worry about as nothing
> ODP-related should happen before odp_init_global() is called.
typo: => v2
other comment: no: constructor functions will get run earlyer, sadly.
>
>>
>> + * We cannot even issue an error as ODP_* functions have not been
>> + * initialised yet, but this is no good...
>> + */
>> +
>> + if (init_global_status == UNDONE)
>> + return ODPDRV_ENUMR_INVALID;
>> +
>> + enumr = _odp_ishm_pool_alloc(list_elt_pool,
>> + sizeof(_odpdrv_enumr_t));
>> + if (!enumr) {
>> + ODP_ERR("_odp_ishm_pool_alloc failed!\n");
>> + return ODPDRV_ENUMR_INVALID;
>> + }
>> +
>> + /* save init parameters and insert enumerator in list */
>> + enumr->param = *param;
>> + enumr->probed = 0;
>> + enumr_list_write_lock();
>> + enumr->next = enumr_lst.head;
>> + enumr_lst.head = enumr;
>> + enumr_list_write_unlock();
>>
>> - return ODPDRV_ENUMR_INVALID;
>> + return (odpdrv_enumr_t)enumr;
>> }
>>
>> odpdrv_device_t odpdrv_device_create(odpdrv_device_param_t *param)
>> @@ -174,6 +255,7 @@ odpdrv_driver_t
>> odpdrv_driver_register(odpdrv_driver_param_t *param)
>> void _odpdrv_driver_probe_drv_items(void)
>> {
>> _odpdrv_enumr_class_t *enumr_c;
>> + _odpdrv_enumr_t *enumr;
>>
>> /* probe unprobed enumerators: */
>> enumr_class_list_read_lock();
>> @@ -186,11 +268,26 @@ void _odpdrv_driver_probe_drv_items(void)
>> enumr_c = enumr_c->next;
>> }
>> enumr_class_list_read_unlock();
>> +
>> + /* go through the list of registered enumerator probing the new
>> + * (never probed) ones:
>> + */
>> + enumr_list_read_lock();
>> + enumr = enumr_lst.head;
>> + while (enumr) {
>> + if (!enumr->probed) {
>> + enumr->param.probe();
>> + enumr->probed = 1;
>> + }
>> + enumr = enumr->next;
>> + }
>> + enumr_list_read_unlock();
>> }
>>
>> int odpdrv_print_all(void)
>> {
>> _odpdrv_enumr_class_t *enumr_c;
>> + _odpdrv_enumr_t *enumr;
>>
>> /* we cannot use ODP_DBG before ODP init... */
>> if (init_global_status == UNDONE)
>> @@ -207,6 +304,22 @@ int odpdrv_print_all(void)
>> enumr_c = enumr_c->next;
>> }
>> enumr_class_list_read_unlock();
>> +
>> + /* print the list of registered enumerators: */
>> + enumr_list_read_lock();
>> + enumr = enumr_lst.head;
>> + ODP_DBG("The following enumerators have been registered:\n");
>> + while (enumr) {
>> + enumr_c = (_odpdrv_enumr_class_t *)
>> + (void
>> *)enumr->param.enumr_class;
>> + ODP_DBG(" enumerator: class: %s, API: %s, Version: %d\n",
>> + enumr_c->param.name,
>> + enumr->param.api_name,
>> + enumr->param.api_version);
>> + enumr = enumr->next;
>> + }
>> + enumr_list_read_unlock();
>> +
>> return 0;
>> }
>>
>> @@ -222,6 +335,7 @@ int _odpdrv_driver_init_global(void)
>>
>> /* from now, we want to ensure mutex on the list: init lock: */
>> odp_rwlock_recursive_init(&enumr_class_lst.lock);
>> + odp_rwlock_recursive_init(&enumr_lst.lock);
>>
>> /* probe things... */
>> _odpdrv_driver_probe_drv_items();
>> @@ -240,10 +354,25 @@ int _odpdrv_driver_init_local(void)
>> int _odpdrv_driver_term_global(void)
>> {
>> _odpdrv_enumr_class_t *enumr_c;
>> + _odpdrv_enumr_t *enumr;
>>
>> if (init_global_status == UNDONE)
>> return 0;
>>
>> + /* remove all enumerators which are registered: */
>> + enumr_list_write_lock();
>> + while (enumr_lst.head) {
>> + enumr = enumr_lst.head;
>> + if (enumr->param.remove) { /* run remove callback, if any
>> */
>> + if (enumr->param.remove())
>> + ODP_ERR("Enumerator (API %s) removal
>> failed.\n",
>> + enumr->param.api_name);
>> + }
>> + enumr_lst.head = enumr->next;
>> + _odp_ishm_pool_free(list_elt_pool, enumr);
>> + }
>> + enumr_list_write_unlock();
>> +
>> /* remove all enumerator classes which are registered: */
>> enumr_class_list_write_lock();
>> while (enumr_class_lst.head) {
>> --
>> 2.7.4
>>
>
