Re: [lng-odp] [API-NEXT PATCH 08/21] linux-gen: drv: device creation and deletion

[Christophe Milard]

On 22 February 2017 at 23:28, Bill Fischofer  wrote:
>
>
> On Wed, Feb 22, 2017 at 6:55 AM, Christophe Milard
>  wrote:
>>
>> Functions to create and remove devices are populated to do
>> more proper things.
>>
>> Signed-off-by: Christophe Milard 
>> ---
>>  platform/linux-generic/drv_driver.c | 172
>> ++--
>>  1 file changed, 164 insertions(+), 8 deletions(-)
>>
>> diff --git a/platform/linux-generic/drv_driver.c
>> b/platform/linux-generic/drv_driver.c
>> index f8844f5..48a90a2 100644
>> --- a/platform/linux-generic/drv_driver.c
>> +++ b/platform/linux-generic/drv_driver.c
>> @@ -19,12 +19,15 @@
>>
>>  static enum {UNDONE, IN_PROGRESS, DONE} init_global_status;
>>
>> +static void device_destroy_terminate(odpdrv_device_t device);
>> +
>>  /* 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;
>>  typedef struct _odpdrv_enumr_s _odpdrv_enumr_t;
>> +typedef struct _odpdrv_device_s _odpdrv_device_t;
>>
>>  /* an enumerator class (list element) */
>>  struct _odpdrv_enumr_class_s {
>> @@ -55,6 +58,20 @@ typedef struct _odpdrv_enumr_lst_t {
>>  } _odpdrv_enumr_lst_t;
>>  static struct _odpdrv_enumr_lst_t enumr_lst;
>>
>> +/* a device (list element) */
>> +struct _odpdrv_device_s {
>> +   odpdrv_device_param_t param;
>> +   void (*enumr_destroy_callback)(void *enum_dev);/*dev destroy
>> callback */
>> +   struct _odpdrv_device_s *next;
>> +} _odpdrv_device_s;
>> +
>> +/* the device list (all devices, from all enumerators): */
>> +typedef struct _odpdrv_device_lst_t {
>> +   odp_rwlock_recursive_t lock;
>> +   _odpdrv_device_t *head;
>> +} _odpdrv_device_lst_t;
>> +static struct _odpdrv_device_lst_t device_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.
>> @@ -108,6 +125,30 @@ static void enumr_list_write_unlock(void)
>> odp_rwlock_recursive_write_unlock(_lst.lock);
>>  }
>>
>> +static void dev_list_read_lock(void)
>> +{
>> +   if (init_global_status == DONE)
>
>
> Same comment as for earlier locking routines regarding the necessity of
> these guards.

Hope my previous answer made that clear :-)

>
>>
>> +   odp_rwlock_recursive_read_lock(_lst.lock);
>> +}
>> +
>> +static void dev_list_read_unlock(void)
>> +{
>> +   if (init_global_status == DONE)
>> +   odp_rwlock_recursive_read_unlock(_lst.lock);
>> +}
>> +
>> +static void dev_list_write_lock(void)
>> +{
>> +   if (init_global_status == DONE)
>> +   odp_rwlock_recursive_write_lock(_lst.lock);
>> +}
>> +
>> +static void dev_list_write_unlock(void)
>> +{
>> +   if (init_global_status == DONE)
>> +   odp_rwlock_recursive_write_unlock(_lst.lock);
>> +}
>> +
>>  odpdrv_enumr_class_t
>> odpdrv_enumr_class_register(odpdrv_enumr_class_param_t
>>  *param)
>>  {
>> @@ -222,24 +263,119 @@ odpdrv_enumr_t
>> odpdrv_enumr_register(odpdrv_enumr_param_t *param)
>>
>>  odpdrv_device_t odpdrv_device_create(odpdrv_device_param_t *param)
>>  {
>> -   ODP_ERR("odpdrv_device_create not Supported yet! devaddress:
>> %s\n.",
>> -   param->address);
>> -   return ODPDRV_DEVICE_INVALID;
>> +   _odpdrv_device_t *dev;
>> +
>> +   /* If init_global has not been done yet, we have a big issue. */
>> +   if (init_global_status == UNDONE)
>> +   return ODPDRV_DEVICE_INVALID;
>> +
>> +   /* make sure that the provided device address does not already
>> exist: */
>> +   dev_list_read_lock();
>> +   dev = device_lst.head;
>
>
> Same question as for other lists. Where is the head field initialized? This
> should be done in the init routine along with lock initialization.

hope my previous answer made that clear

Christophe

>
>>
>> +   while (dev) {
>> +   if (strcmp(param->address, dev->param.address) == 0) {
>> +   ODP_ERR("device already exists!\n");
>> +   dev_list_read_unlock();
>> +   return ODPDRV_DEVICE_INVALID;
>> +   }
>> +   dev = dev->next;
>> +   }
>> +   dev_list_read_unlock();
>> +
>> +   dev = _odp_ishm_pool_alloc(list_elt_pool,
>> +  sizeof(_odpdrv_device_t));
>> +   if (!dev) {
>> +   ODP_ERR("_odp_ishm_pool_alloc failed!\n");
>> +   return ODPDRV_DEVICE_INVALID;
>> +   }
>> +
>> +   /* save and set dev init parameters and insert new device in list
>> */
>> +   dev->param = *param;
>> +   dev->enumr_destroy_callback = NULL;
>> +   

Re: [lng-odp] [API-NEXT PATCH 08/21] linux-gen: drv: device creation and deletion

[Bill Fischofer]

On Wed, Feb 22, 2017 at 6:55 AM, Christophe Milard <
christophe.mil...@linaro.org> wrote:

> Functions to create and remove devices are populated to do
> more proper things.
>
> Signed-off-by: Christophe Milard 
> ---
>  platform/linux-generic/drv_driver.c | 172 ++
> --
>  1 file changed, 164 insertions(+), 8 deletions(-)
>
> diff --git a/platform/linux-generic/drv_driver.c
> b/platform/linux-generic/drv_driver.c
> index f8844f5..48a90a2 100644
> --- a/platform/linux-generic/drv_driver.c
> +++ b/platform/linux-generic/drv_driver.c
> @@ -19,12 +19,15 @@
>
>  static enum {UNDONE, IN_PROGRESS, DONE} init_global_status;
>
> +static void device_destroy_terminate(odpdrv_device_t device);
> +
>  /* 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;
>  typedef struct _odpdrv_enumr_s _odpdrv_enumr_t;
> +typedef struct _odpdrv_device_s _odpdrv_device_t;
>
>  /* an enumerator class (list element) */
>  struct _odpdrv_enumr_class_s {
> @@ -55,6 +58,20 @@ typedef struct _odpdrv_enumr_lst_t {
>  } _odpdrv_enumr_lst_t;
>  static struct _odpdrv_enumr_lst_t enumr_lst;
>
> +/* a device (list element) */
> +struct _odpdrv_device_s {
> +   odpdrv_device_param_t param;
> +   void (*enumr_destroy_callback)(void *enum_dev);/*dev destroy
> callback */
> +   struct _odpdrv_device_s *next;
> +} _odpdrv_device_s;
> +
> +/* the device list (all devices, from all enumerators): */
> +typedef struct _odpdrv_device_lst_t {
> +   odp_rwlock_recursive_t lock;
> +   _odpdrv_device_t *head;
> +} _odpdrv_device_lst_t;
> +static struct _odpdrv_device_lst_t device_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.
> @@ -108,6 +125,30 @@ static void enumr_list_write_unlock(void)
> odp_rwlock_recursive_write_unlock(_lst.lock);
>  }
>
> +static void dev_list_read_lock(void)
> +{
> +   if (init_global_status == DONE)
>

Same comment as for earlier locking routines regarding the necessity of
these guards.


> +   odp_rwlock_recursive_read_lock(_lst.lock);
> +}
> +
> +static void dev_list_read_unlock(void)
> +{
> +   if (init_global_status == DONE)
> +   odp_rwlock_recursive_read_unlock(_lst.lock);
> +}
> +
> +static void dev_list_write_lock(void)
> +{
> +   if (init_global_status == DONE)
> +   odp_rwlock_recursive_write_lock(_lst.lock);
> +}
> +
> +static void dev_list_write_unlock(void)
> +{
> +   if (init_global_status == DONE)
> +   odp_rwlock_recursive_write_unlock(_lst.lock);
> +}
> +
>  odpdrv_enumr_class_t odpdrv_enumr_class_register(
> odpdrv_enumr_class_param_t
>  *param)
>  {
> @@ -222,24 +263,119 @@ odpdrv_enumr_t 
> odpdrv_enumr_register(odpdrv_enumr_param_t
> *param)
>
>  odpdrv_device_t odpdrv_device_create(odpdrv_device_param_t *param)
>  {
> -   ODP_ERR("odpdrv_device_create not Supported yet! devaddress:
> %s\n.",
> -   param->address);
> -   return ODPDRV_DEVICE_INVALID;
> +   _odpdrv_device_t *dev;
> +
> +   /* If init_global has not been done yet, we have a big issue. */
> +   if (init_global_status == UNDONE)
> +   return ODPDRV_DEVICE_INVALID;
> +
> +   /* make sure that the provided device address does not already
> exist: */
> +   dev_list_read_lock();
> +   dev = device_lst.head;
>

Same question as for other lists. Where is the head field initialized? This
should be done in the init routine along with lock initialization.


> +   while (dev) {
> +   if (strcmp(param->address, dev->param.address) == 0) {
> +   ODP_ERR("device already exists!\n");
> +   dev_list_read_unlock();
> +   return ODPDRV_DEVICE_INVALID;
> +   }
> +   dev = dev->next;
> +   }
> +   dev_list_read_unlock();
> +
> +   dev = _odp_ishm_pool_alloc(list_elt_pool,
> +  sizeof(_odpdrv_device_t));
> +   if (!dev) {
> +   ODP_ERR("_odp_ishm_pool_alloc failed!\n");
> +   return ODPDRV_DEVICE_INVALID;
> +   }
> +
> +   /* save and set dev init parameters and insert new device in list
> */
> +   dev->param = *param;
> +   dev->enumr_destroy_callback = NULL;
> +   dev_list_write_lock();
> +   dev->next = device_lst.head;
> +   device_lst.head = dev;
> +   dev_list_write_unlock();
> +
> +   /* todo: probe for drivers */
> +
> +   return (odpdrv_device_t)dev;
>  }
>
>  int odpdrv_device_destroy(odpdrv_device_t dev,
>   void (*callback)(void *enum_dev), uint32_t 

[lng-odp] [API-NEXT PATCH 08/21] linux-gen: drv: device creation and deletion

[Christophe Milard]

Functions to create and remove devices are populated to do
more proper things.

Signed-off-by: Christophe Milard 
---
 platform/linux-generic/drv_driver.c | 172 ++--
 1 file changed, 164 insertions(+), 8 deletions(-)

diff --git a/platform/linux-generic/drv_driver.c 
b/platform/linux-generic/drv_driver.c
index f8844f5..48a90a2 100644
--- a/platform/linux-generic/drv_driver.c
+++ b/platform/linux-generic/drv_driver.c
@@ -19,12 +19,15 @@
 
 static enum {UNDONE, IN_PROGRESS, DONE} init_global_status;
 
+static void device_destroy_terminate(odpdrv_device_t device);
+
 /* 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;
 typedef struct _odpdrv_enumr_s _odpdrv_enumr_t;
+typedef struct _odpdrv_device_s _odpdrv_device_t;
 
 /* an enumerator class (list element) */
 struct _odpdrv_enumr_class_s {
@@ -55,6 +58,20 @@ typedef struct _odpdrv_enumr_lst_t {
 } _odpdrv_enumr_lst_t;
 static struct _odpdrv_enumr_lst_t enumr_lst;
 
+/* a device (list element) */
+struct _odpdrv_device_s {
+   odpdrv_device_param_t param;
+   void (*enumr_destroy_callback)(void *enum_dev);/*dev destroy callback */
+   struct _odpdrv_device_s *next;
+} _odpdrv_device_s;
+
+/* the device list (all devices, from all enumerators): */
+typedef struct _odpdrv_device_lst_t {
+   odp_rwlock_recursive_t lock;
+   _odpdrv_device_t *head;
+} _odpdrv_device_lst_t;
+static struct _odpdrv_device_lst_t device_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.
@@ -108,6 +125,30 @@ static void enumr_list_write_unlock(void)
odp_rwlock_recursive_write_unlock(_lst.lock);
 }
 
+static void dev_list_read_lock(void)
+{
+   if (init_global_status == DONE)
+   odp_rwlock_recursive_read_lock(_lst.lock);
+}
+
+static void dev_list_read_unlock(void)
+{
+   if (init_global_status == DONE)
+   odp_rwlock_recursive_read_unlock(_lst.lock);
+}
+
+static void dev_list_write_lock(void)
+{
+   if (init_global_status == DONE)
+   odp_rwlock_recursive_write_lock(_lst.lock);
+}
+
+static void dev_list_write_unlock(void)
+{
+   if (init_global_status == DONE)
+   odp_rwlock_recursive_write_unlock(_lst.lock);
+}
+
 odpdrv_enumr_class_t odpdrv_enumr_class_register(odpdrv_enumr_class_param_t
 *param)
 {
@@ -222,24 +263,119 @@ odpdrv_enumr_t 
odpdrv_enumr_register(odpdrv_enumr_param_t *param)
 
 odpdrv_device_t odpdrv_device_create(odpdrv_device_param_t *param)
 {
-   ODP_ERR("odpdrv_device_create not Supported yet! devaddress: %s\n.",
-   param->address);
-   return ODPDRV_DEVICE_INVALID;
+   _odpdrv_device_t *dev;
+
+   /* If init_global has not been done yet, we have a big issue. */
+   if (init_global_status == UNDONE)
+   return ODPDRV_DEVICE_INVALID;
+
+   /* make sure that the provided device address does not already exist: */
+   dev_list_read_lock();
+   dev = device_lst.head;
+   while (dev) {
+   if (strcmp(param->address, dev->param.address) == 0) {
+   ODP_ERR("device already exists!\n");
+   dev_list_read_unlock();
+   return ODPDRV_DEVICE_INVALID;
+   }
+   dev = dev->next;
+   }
+   dev_list_read_unlock();
+
+   dev = _odp_ishm_pool_alloc(list_elt_pool,
+  sizeof(_odpdrv_device_t));
+   if (!dev) {
+   ODP_ERR("_odp_ishm_pool_alloc failed!\n");
+   return ODPDRV_DEVICE_INVALID;
+   }
+
+   /* save and set dev init parameters and insert new device in list */
+   dev->param = *param;
+   dev->enumr_destroy_callback = NULL;
+   dev_list_write_lock();
+   dev->next = device_lst.head;
+   device_lst.head = dev;
+   dev_list_write_unlock();
+
+   /* todo: probe for drivers */
+
+   return (odpdrv_device_t)dev;
 }
 
 int odpdrv_device_destroy(odpdrv_device_t dev,
  void (*callback)(void *enum_dev), uint32_t flags)
 {
-   if (dev == ODPDRV_DEVICE_INVALID)
+   _odpdrv_device_t *device = (_odpdrv_device_t *)(void *)dev;
+   _odpdrv_device_t *_dev;
+   _odpdrv_device_t *target = NULL;
+
+   if (dev == ODPDRV_DEVICE_INVALID) {
ODP_ERR("Invalid device\n");
-   if (callback != NULL)
-   ODP_ERR("Callback not supported yet\n");
-   if (flags != 0)
-   ODP_ERR("flags not supported yet\n");
+   return -1;
+   }
+
+   if (flags & ODPDRV_DEV_DESTROY_IMMEDIATE)
+