[dpdk-dev] [RFC PATCH 2/6] eal: introduce bus-device-driver structure

Shreyansh Jain shreyansh.jain at nxp.com
Thu Nov 17 14:00:00 CET 2016


Hello Jan,

Thanks for comments. Replies inline.

On Thursday 17 November 2016 04:49 PM, Jan Blunck wrote:
> On Thu, Nov 17, 2016 at 6:30 AM, Shreyansh Jain <shreyansh.jain at nxp.com> wrote:
>> A device is connected to a bus and services by a driver associated with
>> the bus. It is responsibility of the bus to identify the devices (scan)
>> and then assign each device to a matching driver.
>>
>> A PMD would allocate a rte_xxx_driver and rte_xxx_device.
>> rte_xxx_driver has rte_driver and rte_bus embedded. Similarly, rte_xxx_device
>> has rte_device and rte_bus embedded.
>
> I don't think so: the rte_xxx_device embeds the generic rte_device and
> references a the rte_bus
> that it is attached to.

You mean?

  struct rte_pci_device {
    struct rte_device device;
    struct rte_bus *bus;
    ...
}

If yes then I have a different view.
'device' is connected to a bus. pci_device is just a type of device. 
Only way it should know about the bus is through the parent rte_device.
rte_device can reference the bus.

>
>> When a ethernet or crypto device (rte_eth_dev, rte_cryptodev) is allocated,
>> it contains a reference of rte_device and rte_driver.
>> Each ethernet device implementation would use container_of for finding the
>> enclosing structure of rte_xxx_*.
>>
>>                             +-------------------+
>>  +--------------+           |rte_pci_device     |
>>  |rte_eth_dev   |           |+-----------------+|
>>  |+------------+|   .-------->rte_device       ||
>>  ||rte_device*-----'        |+-----------------+|
>>  |+------------+|           ||rte_bus          ||
>>  |              |           |+-----------------+|
>>  /              /           +-------------------+
>>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain at nxp.com>
>> ---
>>  lib/librte_eal/common/include/rte_bus.h | 243 ++++++++++++++++++++++++++++++++
>>  lib/librte_eal/common/include/rte_dev.h |  36 ++---
>>  2 files changed, 261 insertions(+), 18 deletions(-)
>>  create mode 100644 lib/librte_eal/common/include/rte_bus.h
>>
>> diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
>> new file mode 100644
>> index 0000000..dc3aeb8
>> --- /dev/null
>> +++ b/lib/librte_eal/common/include/rte_bus.h
>> @@ -0,0 +1,243 @@
>> +/*-
>> + *   BSD LICENSE
>> + *
>> + *   Copyright(c) 2016 NXP
>> + *   All rights reserved.
>> + *
>> + *   Redistribution and use in source and binary forms, with or without
>> + *   modification, are permitted provided that the following conditions
>> + *   are met:
>> + *
>> + *     * Redistributions of source code must retain the above copyright
>> + *       notice, this list of conditions and the following disclaimer.
>> + *     * Redistributions in binary form must reproduce the above copyright
>> + *       notice, this list of conditions and the following disclaimer in
>> + *       the documentation and/or other materials provided with the
>> + *       distribution.
>> + *     * Neither the name of NXP nor the names of its
>> + *       contributors may be used to endorse or promote products derived
>> + *       from this software without specific prior written permission.
>> + *
>> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +
>> +#ifndef _RTE_BUS_H_
>> +#define _RTE_BUS_H_
>> +
>> +/**
>> + * @file
>> + *
>> + * RTE PMD Bus Abstraction interfaces
>> + *
>> + * This file exposes APIs and Interfaces for Bus Abstraction over the devices
>> + * drivers in EAL.
>> + */
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +#include <stdio.h>
>> +#include <sys/queue.h>
>> +
>> +#include <rte_log.h>
>> +#include <rte_dev.h>
>> +
>> +
>> +/** Double linked list of buses */
>> +TAILQ_HEAD(rte_bus_list, rte_bus);
>> +
>> +/**
>> + * Bus specific scan for devices attached on the bus.
>> + * For each bus object, the scan would be reponsible for finding devices and
>> + * adding them to its private device list.
>> + *
>> + * Successful detection of a device results in rte_device object which is
>> + * embedded within the respective device type (rte_pci_device, for example).
>> + * Thereafter, PCI specific bus would need to perform
>> + * container_of(rte_pci_device) to obtain PCI device object.
>> + *
>> + * Scan failure of a bus is not treated as exit criteria for application. Scan
>> + * for all other buses would still continue.
>> + *
>> + * @param void
>> + * @return
>> + *     0 for successful scan
>> + *     !0 (<0) for unsuccessful scan with error value
>> + */
>> +typedef int (* bus_scan_t)(void);
>> +
>> +/**
>> + * Bus specific match for devices and drivers which can service them.
>> + * For each scanned device, during probe the match would link the devices with
>> + * drivers which can service the device.
>> + *
>> + * It is the work of each bus handler to obtain the specific device object
>> + * using container_of (or typecasting, as a less preferred way).
>> + *
>> + * @param drv
>> + *     Driver object attached to the bus
>> + * @param dev
>> + *     Device object which is being probed.
>> + * @return
>> + *     0 for successful match
>> + *     !0 for unsuccessful match
>> + */
>> +typedef int (* bus_match_t)(struct rte_driver *drv, struct rte_device *dev);
>
> Do you think this should do match & probe?

It is only matching as of now.
rte_eal_probe() from eal.c calls this function for the bus and probe for 
each device of that bus. (Look at 'pci_probe_all_drivers' called from 
rte_eal_pci_probe).

>
> I believe it is better to separate this into two functions to match()
> and probe(). The
> result of matching tells if the driver would want to claim the device
> in general. But
> probe()ing the device should only happen if the device isn't claimed
> by a driver yet and
> is not blacklisted.

Agree. This is what rte_eal_pci_probe() is doing for PCI. Similar 
implementation would exists in bus specific area (this is a different 
debate where...) for probe on each driver (associated with that bus).

>
>> +
>> +/**
>> + * Dump the devices on the bus.
>> + * Each bus type can define its own definition of information to dump.
>> + *
>> + * @param bus
>> + *     Handle for bus, device from which are to be dumped.
>> + * @param f
>> + *     Handle to output device or file.
>> + * @return void
>> + */
>> +typedef void (* bus_dump_t)(struct rte_bus *bus, FILE *f);
>> +
>> +/**
>> + * Search for a specific device in device list of the bus
>> + * This would rely on the bus specific addressing. Each implementation would
>> + * extract its specific device type and perform address compare.
>> + *
>> + * @param dev
>> + *     device handle to search for.
>> + * @return
>> + *     rte_device handle for matched device, or NULL
>> + */
>> +typedef struct rte_device * (* bus_device_get_t)(struct rte_device *dev);
>
> From my experience it is better to delegate this to a helper:
>
> int bus_for_each_dev(struct rte_bus *bus, struct rte_device *start,
> int (*match)(struct rte_device *dev, void *data), void *data);
>
> Based on that you can easily implement all kinds of functions like
> bus_find_dev(), bus_find_dev_by_name(), bus_dump(), ...

Interesting idea. Thanks. I will have a look on this.

>
>> +
>> +struct rte_bus {
>> +       TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list */
>> +       struct rte_driver_list driver_list; /**< List of all drivers of bus */
>
> TAILQ_HEAD?

Yes. That should be TAILQ_HEAD. I just reused the definition create in 
rte_dev.h using TAILQ_HEAD. I will change this.

>
>> +       struct rte_device_list device_list; /**< List of all devices on bus */
>
> TAILQ_HEAD?

Same as above. I will change this.

>
>> +       const char *name;            /**< Name of the bus */
>> +       /* Mandatory hooks */
>> +       bus_scan_t *scan;            /**< Hook for scanning for devices */
>> +       bus_match_t *match;          /**< Hook for matching device & driver */
>> +       /* Optional hooks */
>> +       bus_dump_t *dump_dev;        /**< Hook for dumping devices on bus */
>> +       bus_device_get_t *find_dev;  /**< Search for a device on bus */
>> +};
>> +
>> +/** @internal
>> + * Add a device to a bus.
>> + *
>> + * @param bus
>> + *     Bus on which device is to be added
>> + * @param dev
>> + *     Device handle
>> + * @return
>> + *     None
>> + */
>> +void
>> +rte_eal_bus_add_device(struct rte_bus *bus, struct rte_device *dev);
>
> Why do we need this? From my understanding the rte_bus->scan() is
> adding the devices to the rte_bus->device_list.

Plugging a device *after* scan has been completed. Hotplugging.
Specially for vdev, this would be required, in my opinion.

>
>> +/** @internal
>> + * Remove a device from its bus.
>> + *
>> + * @param dev
>> + *     Device handle to remove
>> + * @return
>> + *     None
>> + */
>> +void
>> +rte_eal_bus_remove_device(struct rte_device *dev);
>> +
>> +/** @internal
>> + * Associate a driver with a bus.
>> + *
>> + * @param bus
>> + *     Bus on which driver is to be added
>> + * @param dev
>> + *     Driver handle
>> + * @return
>> + *     None
>> + */
>> +void
>> +rte_eal_bus_add_driver(struct rte_bus *bus, struct rte_driver *drv);
>> +
>
> What happens if a driver is added at runtime to a bus? Does that immediately
> trigger a match & probe of unclaimed devices?

My take:
  - scan the bus for any newly plugged devices. Might be required in 
case a device is logical entity represented by multiple entries in sysfs.
  -- add only those which are not already added - maybe address/id match
  - Trigger driver probe for all devices which don't have a driver 
assigned to them (unclaimed, as you stated).

(This is part of my further changes - I think I forgot to put them as 
note in cover letter).

>
>> +/** @internal
>> + * Disassociate a driver from its bus.
>> + *
>> + * @param dev
>> + *     Driver handle to remove
>> + * @return
>> + *     None
>> + */
>> +void
>> +rte_eal_bus_remove_driver(struct rte_driver *drv);
>> +
>> +/**
>> + * Register a Bus handler.
>> + *
>> + * @param driver
>> + *   A pointer to a rte_bus structure describing the bus
>> + *   to be registered.
>> + */
>> +void rte_eal_bus_register(struct rte_bus *bus);
>> +
>> +/**
>> + * Unregister a Bus handler.
>> + *
>> + * @param driver
>> + *   A pointer to a rte_bus structure describing the bus
>> + *   to be unregistered.
>> + */
>> +void rte_eal_bus_unregister(struct rte_bus *bus);
>> +
>> +/**
>> + * Obtain handle for bus given its name.
>> + *
>> + * @param bus_name
>> + *     Name of the bus handle to search
>> + * @return
>> + *     Pointer to Bus object if name matches any registered bus object
>> + *     NULL, if no matching bus found
>> + */
>> +struct rte_bus * rte_eal_get_bus(const char *bus_name);
>> +
>> +/**
>> + * Register a device driver.
>> + *
>> + * @param driver
>> + *   A pointer to a rte_dev structure describing the driver
>> + *   to be registered.
>> + */
>> +void rte_eal_driver_register(struct rte_driver *driver);
>> +
>> +/**
>> + * Unregister a device driver.
>> + *
>> + * @param driver
>> + *   A pointer to a rte_dev structure describing the driver
>> + *   to be unregistered.
>> + */
>> +void rte_eal_driver_unregister(struct rte_driver *driver);
>> +
>> +/** Helper for Bus registration */
>> +#define RTE_PMD_REGISTER_BUS(nm, bus) \
>> +RTE_INIT(businitfn_ ##nm); \
>> +static void businitfn_ ##nm(void) \
>> +{\
>> +       (bus).name = RTE_STR(nm);\
>> +       rte_eal_bus_register(&bus); \
>> +}
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +
>> +#endif /* _RTE_BUS_H */
>> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
>> index 8840380..b08bab5 100644
>> --- a/lib/librte_eal/common/include/rte_dev.h
>> +++ b/lib/librte_eal/common/include/rte_dev.h
>> @@ -116,12 +116,14 @@ TAILQ_HEAD(rte_device_list, rte_device);
>>
>>  /* Forward declaration */
>>  struct rte_driver;
>> +struct rte_bus;
>>
>>  /**
>>   * A structure describing a generic device.
>>   */
>>  struct rte_device {
>>         TAILQ_ENTRY(rte_device) next; /**< Next device */
>> +       struct rte_bus *bus;          /**< Bus on which device is placed */
>>         struct rte_driver *driver;    /**< Associated driver */
>>         int numa_node;                /**< NUMA node connection */
>>         struct rte_devargs *devargs;  /**< Device user arguments */
>> @@ -144,31 +146,29 @@ void rte_eal_device_insert(struct rte_device *dev);
>>  void rte_eal_device_remove(struct rte_device *dev);
>>
>>  /**
>> - * A structure describing a device driver.
>> + * @internal
>> + * TODO
>>   */
>> -struct rte_driver {
>> -       TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
>> -       const char *name;                   /**< Driver name. */
>> -       const char *alias;              /**< Driver alias. */
>> -};
>> +typedef int (*driver_init_t)(struct rte_device *eth_dev);
>>
>>  /**
>> - * Register a device driver.
>> - *
>> - * @param driver
>> - *   A pointer to a rte_dev structure describing the driver
>> - *   to be registered.
>> + * @internal
>> + * TODO
>>   */
>> -void rte_eal_driver_register(struct rte_driver *driver);
>> +typedef int (*driver_uninit_t)(struct rte_device *eth_dev);
>>
>>  /**
>> - * Unregister a device driver.
>> - *
>> - * @param driver
>> - *   A pointer to a rte_dev structure describing the driver
>> - *   to be unregistered.
>> + * A structure describing a device driver.
>>   */
>> -void rte_eal_driver_unregister(struct rte_driver *driver);
>> +struct rte_driver {
>> +       TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
>> +       struct rte_bus *bus;           /**< Bus which drivers services */
>
> I think this should be TAILQ_ENTRY instead.

rte_bus
       `-> device_list-. <- TAILQ_HEAD (rte_device)
                       |
                 rte_pci_device:TAILQ_ENTRY(rte_device)

rte_device just references the bus it belong to.

Am I missing something?

>
>> +       const char *name;              /**< Driver name. */
>> +       const char *alias;             /**< Driver alias. */
>> +       driver_init_t *init;           /**< Driver initialization */
>> +       driver_uninit_t *uninit;       /**< Driver uninitialization */
>
> Shouldn't this be probe() and remove()?

rte_xxx_driver already includes probe and remove.
Mandate for init is to allocate the ethernet or cryptodev (or some other 
functional unit). Whereas, probe is responsible for driver specific 
intialization - PCI specific, in case of PCI driver.

Initially I had thought of moving rte_pci_driver->probe() to rte_driver. 
But, I couldn't understand where would functional device initialization 
exist. It cannot be rte_xxx_driver. It should be generic place.

Such functional device initialization is in path of the probe function, 
but so does driver specific information. If we remove 
rte_pci_driver->probe and move to rte_driver->probe(), it would only 
mean rte_driver calling a hook within the PCI driver specific domain.

>
>> +       unsigned int dev_private_size; /**< Size of device private data ??*/
>
> I don't think that dev_private_size is really required at this level.
> First of all this is related to the rte_eth_dev structure and
> therefore it really depends on the driver_init_t (aka probe()) if it
> is actually allocating an rte_eth_dev or not. Anyway that is up to the
> drivers probe() function.

I moved it based on input through ML (or IRC, I don't remember).
My understanding: if this is common for all rte_xxx_driver instances 
(for allocating their ethernet/crypto structure), it would actually make 
sense to keep at this level so that init() can use it for allocating 
necessary space before handling over the rte_xxx_driver.

But again, I am OK keeping it in the underlying layer - as you have 
rightly pointed out, it would only be used at rte_xxx_driver layer.

>
>> +};
>>
>>  /**
>>   * Initalize all the registered drivers in this process
>> --
>> 2.7.4
>>
>

-
Shreyansh


More information about the dev mailing list