On 23 February 2017 at 00:27, Bill Fischofer <bill.fischo...@linaro.org> wrote:
>
>
> On Wed, Feb 22, 2017 at 6:56 AM, Christophe Milard
> <christophe.mil...@linaro.org> wrote:
>>
>> Driver registration and probing is implemented for linux-gen ODP.
>>
>> Signed-off-by: Christophe Milard <christophe.mil...@linaro.org>
>> ---
>> platform/linux-generic/drv_driver.c | 348
>> ++++++++++++++++++++++++++++++++++--
>> 1 file changed, 335 insertions(+), 13 deletions(-)
>>
>> diff --git a/platform/linux-generic/drv_driver.c
>> b/platform/linux-generic/drv_driver.c
>> index eb0dc48..0148fc3 100644
>> --- a/platform/linux-generic/drv_driver.c
>> +++ b/platform/linux-generic/drv_driver.c
>> @@ -12,6 +12,7 @@
>> #include <odp/api/std_types.h>
>> #include <odp/api/debug.h>
>> #include <odp/api/rwlock_recursive.h>
>> +#include <odp/api/spinlock.h>
>> #include <odp/drv/driver.h>
>> #include <odp/drv/spec/driver.h>
>> #include <odp_debug_internal.h>
>> @@ -29,6 +30,11 @@ 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;
>> typedef struct _odpdrv_devio_s _odpdrv_devio_t;
>> +typedef struct _odpdrv_driver_s _odpdrv_driver_t;
>> +
>> +static int unbind_device_driver(_odpdrv_device_t *dev,
>> + void (*callback)(odpdrv_device_t
>> odpdrv_dev),
>> + int immediate);
>>
>> /* an enumerator class (list element) */
>> struct _odpdrv_enumr_class_s {
>> @@ -62,6 +68,8 @@ static struct _odpdrv_enumr_lst_t enumr_lst;
>> /* a device (list element) */
>> struct _odpdrv_device_s {
>> odpdrv_device_param_t param;
>> + _odpdrv_driver_t *driver; /* driver for the device (if bound), or
>> NULL*/
>> + _odpdrv_devio_t *devio; /* devio used for device (if bound), or
>> NULL*/
>> void (*enumr_destroy_callback)(void *enum_dev);/*dev destroy
>> callback */
>> struct _odpdrv_device_s *next;
>> } _odpdrv_device_s;
>> @@ -87,6 +95,21 @@ typedef struct _odpdrv_devio_lst_t {
>> } _odpdrv_devio_lst_t;
>> static struct _odpdrv_devio_lst_t devio_lst;
>>
>> +/* a driver (list element) */
>> +struct _odpdrv_driver_s {
>> + odpdrv_driver_param_t param;
>> + _odp_ishm_pool_t *pool;
>> + odp_spinlock_t probelock; /* to avoid concurrent probe on the same
>> drv*/
>
>
> Why use a spinlock rather than a ticketlock here? We seem to have
> standardized on ticketlocks as being more efficient and fair.
I did not know ticket locks were regarded as more efficient. I can
change, no issue:
=> v2
>
>>
>> + struct _odpdrv_driver_s *next;
>> +};
>> +
>> +/* the driver list: */
>> +typedef struct _odpdrv_driver_lst_t {
>> + odp_rwlock_recursive_t lock;
>> + _odpdrv_driver_t *head;
>> +} _odpdrv_driver_lst_t;
>> +static struct _odpdrv_driver_lst_t driver_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.
>> @@ -188,6 +211,30 @@ static void devio_list_write_unlock(void)
>> odp_rwlock_recursive_write_unlock(&devio_lst.lock);
>> }
>>
>> +static void driver_list_read_lock(void)
>> +{
>> + if (init_global_status == DONE)
>> + odp_rwlock_recursive_read_lock(&driver_lst.lock);
>> +}
>> +
>> +static void driver_list_read_unlock(void)
>> +{
>> + if (init_global_status == DONE)
>> + odp_rwlock_recursive_read_unlock(&driver_lst.lock);
>> +}
>> +
>> +static void driver_list_write_lock(void)
>> +{
>> + if (init_global_status == DONE)
>> + odp_rwlock_recursive_write_lock(&driver_lst.lock);
>> +}
>> +
>> +static void driver_list_write_unlock(void)
>> +{
>> + if (init_global_status == DONE)
>> + odp_rwlock_recursive_write_unlock(&driver_lst.lock);
>> +}
>> +
>> odpdrv_enumr_class_t
>> odpdrv_enumr_class_register(odpdrv_enumr_class_param_t
>> *param)
>> {
>> @@ -331,6 +378,8 @@ odpdrv_device_t
>> odpdrv_device_create(odpdrv_device_param_t *param)
>> /* save and set dev init parameters and insert new device in list
>> */
>> dev->param = *param;
>> dev->enumr_destroy_callback = NULL;
>> + dev->driver = NULL;
>> + dev->devio = NULL;
>> dev_list_write_lock();
>> dev->next = device_lst.head;
>> device_lst.head = dev;
>> @@ -384,19 +433,17 @@ int odpdrv_device_destroy(odpdrv_device_t dev,
>> */
>> target->enumr_destroy_callback = callback;
>>
>> - /* TODO: if a driver is bound to the device, unbind it!
>> - * passing the flag andf device_destroy_terminate() as a callback
>> */
>> -
>> - /* no driver is handling this device, or no callback was
>> - * provided: continue removing the device: */
>> - device_destroy_terminate(dev);
>> + /* unbind the driver from the device (if bound).
>> + * The callback is always called. */
>> + unbind_device_driver(target,
>> + device_destroy_terminate,
>> + (flags & ODPDRV_DEV_DESTROY_IMMEDIATE));
>>
>> return 0;
>> }
>>
>> /* This function is called as a callback from the driver, when unbindind
>> - * a device, or directely from odpdrv_device_destroy() if no driver
>> - * was bound to the device.
>> + * a device drom odpdrv_device_destroy()
>> * just call the enumerator callback to cleanup the enumerator part
>> * and free device memory */
>> static void device_destroy_terminate(odpdrv_device_t drv_device)
>> @@ -517,10 +564,238 @@ odpdrv_devio_t
>> odpdrv_devio_register(odpdrv_devio_param_t *param)
>>
>> odpdrv_driver_t odpdrv_driver_register(odpdrv_driver_param_t *param)
>> {
>> - ODP_ERR("NOT Supported yet! Driver %s Registration!\n.",
>> - param->name);
>> + _odpdrv_driver_t *driver;
>> +
>> + /* check for a few compulsory things: */
>> + if ((param->probe == NULL) ||
>> + (param->unbind == NULL))
>> + return ODPDRV_DRIVER_INVALID;
>> +
>> + /* parse the list of already registered drivers to make
>> + * sure no driver with same name already exists:
>> + */
>> + driver_list_read_lock();
>> + driver = driver_lst.head;
>
>
> driver_lst.head needs to be initialized as part of init_global() processing
> along with lock.
no: the initialisation of static variables to 0 is done by the
compiler. Relying on this is needed as the list may be appended at
__constructor__ running (initialisation) time
>
>>
>> + while (driver) {
>> + if ((strncmp(param->name, driver->param.name,
>> + ODPDRV_NAME_SIZE) == 0)) {
>> + ODP_ERR("driver %s already registered!\n",
>> + param->name);
>> + driver_list_read_unlock();
>> + return ODPDRV_DRIVER_INVALID;
>> + }
>> + driver = driver->next;
>> + }
>> + driver_list_read_unlock();
>>
>> - return ODPDRV_DRIVER_INVALID;
>> + /* allocate memory for the new driver:
>> + * 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 drivers
>> + * 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) {
>> + driver = malloc(sizeof(_odpdrv_driver_t));
>> + if (!driver)
>> + return ODPDRV_DRIVER_INVALID;
>> + driver->pool = NULL;
>> + } else {
>> + driver = _odp_ishm_pool_alloc(list_elt_pool,
>> + sizeof(_odpdrv_driver_t));
>> + if (!driver) {
>> + ODP_ERR("_odp_ishm_pool_alloc failed!\n");
>> + return ODPDRV_DRIVER_INVALID;
>> + }
>> + driver->pool = list_elt_pool;
>> + }
>> +
>> + /* save init parameters and insert driver in list */
>> + driver->param = *param;
>> + odp_spinlock_init(&driver->probelock);
>> + driver_list_write_lock();
>> + driver->next = driver_lst.head;
>> + driver_lst.head = driver;
>> + driver_list_write_unlock();
>> +
>> + return (odpdrv_driver_t)driver;
>> +}
>> +
>> +/* Probe, if possible, the given driver with the given device:
>> + * The driver is probed if:
>> + * There exist a devio D such as
>> + * -The name and version of the API provided by D matches one of the
>> requested
>> + * devio {name,version} requested by the driver
>> + * -The enumerator's API (name and version) requested by D is provided
>> + * by the enumerator which enumerated the device.
>> + * This function will return zero if the above condition where met by
>> some
>> + * devio D and the driver probe function returns 0 (success).
>> + * The function will return -1 if some devio D were found, but the driver
>> + * returned a non-zero value when probed (for all of them).
>> + * The function will return -2 if no devio matching the above requirement
>> was
>> + * found.
>> + * The function will return -3 if the device was already bound to a
>> driver */
>> +static int probe_device_driver(_odpdrv_device_t *dev, _odpdrv_driver_t
>> *drv)
>> +{
>> + int i;
>> + int ret = -2;
>> + _odpdrv_devio_t *devio;
>> + _odpdrv_enumr_t *enumr;
>> + _odpdrv_enumr_class_t *enumr_c;
>> +
>> + /* the device already has a driver?: end of story... */
>> + if (dev->driver)
>> + return -3;
>> +
>> + /* look at the different devio this driver can work with: */
>> + for (i = 0; i < ODPDRV_MAX_DEVIOS; i++) {
>> + /* look at each registered devios: */
>> + devio_list_read_lock();
>> + for (devio = devio_lst.head; devio; devio = devio->next) {
>> + /* if devio is no good for this driver, keep
>> searching*/
>> + if ((strncmp(drv->param.devios[i].api_name,
>> + devio->param.api_name,
>> + ODPDRV_NAME_SIZE) != 0) ||
>> + (drv->param.devios[i].api_version !=
>> + devio->param.api_version))
>> + continue;
>> +
>> + /* give a chance to the devio to reject the device
>> + * if it feels it should do so: */
>> + if (devio->param.probe &&
>> + devio->param.probe((odpdrv_device_t)dev))
>> + continue;
>> +
>> + /* grab the device enumerator and its class: */
>> + enumr = (_odpdrv_enumr_t *)
>> + (void *)dev->param.enumerator;
>> + enumr_c = (_odpdrv_enumr_class_t *)
>> + (void
>> *)enumr->param.enumr_class;
>
>
> These casts might be better done with static inline conversion routines. An
> example would look like:
>
> static inline _odpdrv_enumr_t *enumr_hdr(odpdrv_enumr_t enumr)
> {
> return (_odpdrv_enumr_t *)(void *)enumr;
> }
>
> and used as:
>
> enumr = enumr_hdr(dev->param.enumerator);
>
> This reduces clutter as well as preserves flexibility to change the internal
> type representations. The packet system uses routines like this and it
> really helped in recent streamlining changes Petri has been making as we
> were able to change these fairly significantly for improved performance with
> minimal code changes as only the converter functions needed to be updated.
Yes. you are definitively right: => V2
>
>>
>> +
>> + /* if devio is no good for this dev, keep
>> searching */
>> + if ((strncmp(devio->param.enumr_api_name,
>> + enumr->param.api_name,
>> + ODPDRV_NAME_SIZE) != 0) ||
>> + (devio->param.enumr_api_version !=
>> + enumr->param.api_version))
>> + continue;
>> +
>> + /* seems we are good to probe the driver: */
>> + odp_spinlock_lock(&drv->probelock);
>> + if (drv->param.probe((odpdrv_device_t)dev,
>> + (odpdrv_devio_t)devio, i) ==
>> 0) {
>> + /* the driver accepts this device */
>> + odp_spinlock_unlock(&drv->probelock);
>> + devio_list_read_unlock();
>> + ODP_DBG("driver %s will handle device
>> %s(%s)\n",
>> + drv->param.name,
>> + dev->param.address,
>> + enumr_c->param.name);
>> + dev->driver = drv;
>> + dev->devio = devio;
>> + return 0;
>> + }
>> + odp_spinlock_unlock(&drv->probelock);
>> +
>> + /* driver did not accept the device: keep
>> searching */
>> + ret = -1;
>> + }
>> + devio_list_read_unlock();
>> + }
>> + return ret;
>> +}
>> +
>> +/* an empty callback is given to the driver on unprobe, if no real
>> callback is
>> + * needed */
>> +static void empty_unbind_callback(odpdrv_device_t odpdrv_dev ODP_UNUSED)
>> +{
>> +}
>> +
>> +/* unbind the device driver from the device (i.e. "unprobe")
>> + * if the immediate flag is set, the unbind is requested to be immediate,
>> + * i.e. the driver is due to call the callback within its unbind
>> function.
>> + * (if the flag is not set, the callback can be called later on from
>> + * another context. Immediate unbinding may be less graceful then
>> + * non immediate binding)
>> + * The callback function is called in all cases (even if the device was
>> not
>> + * bound)
>> + */
>> +static int unbind_device_driver(_odpdrv_device_t *dev,
>> + void (*callback)(odpdrv_device_t
>> odpdrv_dev),
>> + int immediate)
>> +{
>> + _odpdrv_driver_t *drv;
>> + odpdrv_device_t odpdrv_dev = (odpdrv_device_t)(void *)dev;
>> + int flg = immediate ? ODPDRV_DRV_UNBIND_IMMEDIATE : 0;
>> +
>> + if (!callback)
>> + callback = empty_unbind_callback;
>> +
>> + drv = dev->driver;
>> + if (!drv) { /* nothing to do */
>> + callback(odpdrv_dev);
>> + return 0;
>> + }
>> +
>> + /* note that we assure that a given driver will not be
>> bound/unbound
>> + * concurrentely - but this does not cover the callback */
>> + odp_spinlock_lock(&drv->probelock);
>> + if (drv->param.unbind(odpdrv_dev, callback, flg)) {
>> + ODP_DBG("driver %s could not release device %s\n",
>> + drv->param.name,
>> + dev->param.address);
>> + odp_spinlock_unlock(&drv->probelock);
>> + return -1;
>> + }
>> +
>> + /* unbind succeeded */
>> + dev->driver = NULL;
>> + dev->devio = NULL;
>> + odp_spinlock_unlock(&drv->probelock);
>> + return 0;
>> +}
>> +
>> +/* try to find a driver for the given device, trying all possible
>> registered
>> + * drivers against it:
>> + */
>> +static int probe_device(_odpdrv_device_t *dev)
>> +{
>> + _odpdrv_driver_t *driver;
>> +
>> + /* print the list of registered drivers: */
>> + driver_list_read_lock();
>> + driver = driver_lst.head;
>> + while (driver) {
>> + probe_device_driver(dev, driver);
>
>
> Should the rc be checked here? If not, use (void) on call to keep coverity
> happy.
Yes it should :-). and probing should be stopped after the first success: => V2
>
>>
>> + driver = driver->next;
>> + }
>> + driver_list_read_unlock();
>> +
>> + return 0;
>> +}
>> +
>> +/* try to find a driver for all the registered devices, trying all
>> possible
>> + * drivers-devices combinaison
>> + */
>> +static int probe_all(void)
>> +{
>> + _odpdrv_device_t *dev;
>> +
>> + dev_list_read_lock();
>> + dev = device_lst.head;
>> + while (dev) {
>> + probe_device(dev);
>
>
> Same rc comment here.
I'll put a (void) here: it is not really an error to have devices that
have no ODP drivers
>
>>
>> + dev = dev->next;
>> + }
>> + dev_list_read_unlock();
>> +
>> + return 0;
>> }
>>
>> /* the following function is called each time probing is needed, i.e.
>> @@ -556,6 +831,9 @@ void _odpdrv_driver_probe_drv_items(void)
>> enumr = enumr->next;
>> }
>> enumr_list_read_unlock();
>> +
>> + /* probe drivers for all devices */
>> + probe_all();
>
>
> Same rc comment here.
I'll change the return value of probe_all to void as some devices may
be probed successfully and some others not.
Christophe
>
>>
>> }
>>
>> int odpdrv_print_all(void)
>> @@ -564,6 +842,7 @@ int odpdrv_print_all(void)
>> _odpdrv_enumr_t *enumr;
>> _odpdrv_device_t *dev;
>> _odpdrv_devio_t *devio;
>> + _odpdrv_driver_t *driver;
>>
>> /* we cannot use ODP_DBG before ODP init... */
>> if (init_global_status == UNDONE)
>> @@ -605,11 +884,16 @@ int odpdrv_print_all(void)
>> enumr_c = (_odpdrv_enumr_class_t *)
>> (void
>> *)enumr->param.enumr_class;
>> ODP_DBG(" device: address: %s, from enumerator class: %s "
>> - " API: %s, Version: %d\n",
>> + " API: %s, Version: %d, "
>> + " handled by driver %s, with devio API: %s "
>> + " (version %d)\n",
>> dev->param.address,
>> enumr_c->param.name,
>> enumr->param.api_name,
>> - enumr->param.api_version);
>> + enumr->param.api_version,
>> + dev->driver ? dev->driver->param.name : "<none>",
>> + dev->devio ? dev->devio->param.api_name :
>> "<none>",
>> + dev->devio ? dev->devio->param.api_version : 0);
>> dev = dev->next;
>> }
>> dev_list_read_unlock();
>> @@ -629,6 +913,17 @@ int odpdrv_print_all(void)
>> }
>> devio_list_read_unlock();
>>
>> + /* print the list of registered drivers: */
>> + driver_list_read_lock();
>> + driver = driver_lst.head;
>> + ODP_DBG("The following dev drivers have been registered:\n");
>> + while (driver) {
>> + ODP_DBG(" driver: '%s'\n",
>> + driver->param.name);
>> + driver = driver->next;
>> + }
>> + driver_list_read_unlock();
>> +
>> return 0;
>> }
>>
>> @@ -647,6 +942,7 @@ int _odpdrv_driver_init_global(void)
>> odp_rwlock_recursive_init(&enumr_lst.lock);
>> odp_rwlock_recursive_init(&device_lst.lock);
>> odp_rwlock_recursive_init(&devio_lst.lock);
>> + odp_rwlock_recursive_init(&driver_lst.lock);
>>
>> /* probe things... */
>> _odpdrv_driver_probe_drv_items();
>> @@ -667,10 +963,36 @@ int _odpdrv_driver_term_global(void)
>> _odpdrv_devio_t *devio;
>> _odpdrv_enumr_class_t *enumr_c;
>> _odpdrv_enumr_t *enumr;
>> + _odpdrv_device_t *dev;
>> + _odpdrv_driver_t *driver;
>>
>> if (init_global_status == UNDONE)
>> return 0;
>>
>> + /* unbind any driver from any device: */
>> + dev_list_read_lock();
>> + dev = device_lst.head;
>> + while (dev) {
>> + unbind_device_driver(dev, NULL, 1);
>> + dev = dev->next;
>> + }
>> + dev_list_read_unlock();
>> +
>> + /* and remove all registered drivers: */
>> + driver_list_read_lock();
>> + while (driver_lst.head) {
>> + driver = driver_lst.head;
>> + if (driver->param.remove) {
>> + if (driver->param.remove())
>> + ODP_ERR("driver removal indicated
>> failure!\n");
>> + }
>> + driver_lst.head = driver->next;
>> + if (driver->pool)
>> + _odp_ishm_pool_free(list_elt_pool, driver);
>> + else
>> + free(driver);
>> + }
>> +
>> /* remove all devios which are registered: */
>> devio_list_write_lock();
>> while (devio_lst.head) {
>> --
>> 2.7.4
>>
>
