[dpdk-dev] [RFC PATCH 2/4] Add the new common device header and C file.

[Neil Horman]

On Fri, Apr 10, 2015 at 07:25:32PM +0000, Wiles, Keith wrote:
> 
> 
> On 4/9/15, 6:53 AM, "Neil Horman" <nhorman at tuxdriver.com> wrote:
> 
> >On Wed, Apr 08, 2015 at 03:58:38PM -0500, Keith Wiles wrote:
> >> Move a number of device specific define, structures and functions
> >> into a generic device base set of files for all device not just
> >>Ethernet.
> >> 
> >> Signed-off-by: Keith Wiles <keith.wiles at intel.com>
> >> ---
> >>  lib/librte_eal/common/eal_common_device.c         | 185 +++++++
> >>  lib/librte_eal/common/include/rte_common_device.h | 617
> >>++++++++++++++++++++++
> >>  2 files changed, 802 insertions(+)
> >>  create mode 100644 lib/librte_eal/common/eal_common_device.c
> >>  create mode 100644 lib/librte_eal/common/include/rte_common_device.h
> >> 
> >> diff --git a/lib/librte_eal/common/eal_common_device.c
> >>b/lib/librte_eal/common/eal_common_device.c
> >> new file mode 100644
> >> index 0000000..a9ef925
> >> --- /dev/null
> >> +++ b/lib/librte_eal/common/eal_common_device.c
> >> @@ -0,0 +1,185 @@
> >> +/*-
> >> + *   BSD LICENSE
> >> + *
> >> + *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
> >> + *   Copyright(c) 2014 6WIND S.A.
> >> + *   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 Intel Corporation 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.
> >> + */
> >> +
> >> +#include <stdio.h>
> >> +#include <string.h>
> >> +#include <inttypes.h>
> >> +#include <sys/queue.h>
> >> +
> >> +#include <rte_dev.h>
> >> +#include <rte_devargs.h>
> >> +#include <rte_debug.h>
> >> +#include <rte_devargs.h>
> >> +#include <rte_log.h>
> >> +#include <rte_malloc.h>
> >> +#include <rte_errno.h>
> >> +
> >> +#include "rte_common_device.h"
> >> +
> >> +void *
> >> +rte_dev_add_callback(struct rte_dev_rxtx_callback ** cbp,
> >> +		void * fn, void *user_param)
> >> +{
> >> +	struct rte_dev_rxtx_callback *cb;
> >> +
> >> +	cb = rte_zmalloc(NULL, sizeof(*cb), 0);
> >> +
> >> +	if (cb == NULL) {
> >> +		rte_errno = ENOMEM;
> >> +		return NULL;
> >> +	}
> >> +
> >> +	cb->fn.vp = fn;
> >> +	cb->param = user_param;
> >> +	cb->next = *cbp;
> >> +	*cbp = cb;
> >> +	return cb;
> >> +}
> >> +
> >> +int
> >> +rte_dev_remove_callback(struct rte_dev_rxtx_callback ** cbp,
> >> +		struct rte_dev_rxtx_callback *user_cb)
> >> +{
> >> +	struct rte_dev_rxtx_callback *cb = *cbp;
> >> +	struct rte_dev_rxtx_callback *prev_cb;
> >> +
> >> +	/* Reset head pointer and remove user cb if first in the list. */
> >> +	if (cb == user_cb) {
> >> +		*cbp = user_cb->next;
> >> +		return 0;
> >> +	}
> >> +
> >> +	/* Remove the user cb from the callback list. */
> >> +	do {
> >> +		prev_cb = cb;
> >> +		cb = cb->next;
> >> +
> >> +		if (cb == user_cb) {
> >> +			prev_cb->next = user_cb->next;
> >> +			return 0;
> >> +		}
> >> +	} while (cb != NULL);
> >> +
> >> +	/* Callback wasn't found. */
> >> +	return (-EINVAL);
> >> +}
> >> +
> >> +int
> >> +rte_dev_callback_register(struct rte_dev_cb_list * cb_list,
> >> +		rte_spinlock_t * lock,
> >> +		enum rte_dev_event_type event,
> >> +		rte_dev_cb_fn cb_fn, void *cb_arg)
> >> +{
> >> +	struct rte_dev_callback *cb;
> >> +
> >> +	rte_spinlock_lock(lock);
> >> +
> >> +	TAILQ_FOREACH(cb, cb_list, next) {
> >> +		if (cb->cb_fn == cb_fn &&
> >> +			cb->cb_arg == cb_arg &&
> >> +			cb->event == event) {
> >> +			break;
> >> +		}
> >> +	}
> >> +
> >> +	/* create a new callback. */
> >> +	if (cb == NULL && (cb = rte_zmalloc("INTR_USER_CALLBACK",
> >> +			sizeof(struct rte_dev_callback), 0)) != NULL) {
> >> +		cb->cb_fn = cb_fn;
> >> +		cb->cb_arg = cb_arg;
> >> +		cb->event = event;
> >> +		TAILQ_INSERT_TAIL(cb_list, cb, next);
> >> +	}
> >> +
> >> +	rte_spinlock_unlock(lock);
> >> +	return ((cb == NULL) ? -ENOMEM : 0);
> >> +}
> >> +
> >> +int
> >> +rte_dev_callback_unregister(struct rte_dev_cb_list * cb_list,
> >> +		rte_spinlock_t * lock,
> >> +		enum rte_dev_event_type event,
> >> +		rte_dev_cb_fn cb_fn, void *cb_arg)
> >> +{
> >> +	int ret;
> >> +	struct rte_dev_callback *cb, *next;
> >> +
> >> +	rte_spinlock_lock(lock);
> >> +
> >> +	ret = 0;
> >> +	for (cb = TAILQ_FIRST(cb_list); cb != NULL; cb = next) {
> >> +
> >> +		next = TAILQ_NEXT(cb, next);
> >> +
> >> +		if (cb->cb_fn != cb_fn || cb->event != event ||
> >> +				(cb->cb_arg != (void *)-1 &&
> >> +				cb->cb_arg != cb_arg))
> >> +			continue;
> >> +
> >> +		/*
> >> +		 * if this callback is not executing right now,
> >> +		 * then remove it.
> >> +		 */
> >> +		if (cb->active == 0) {
> >> +			TAILQ_REMOVE(cb_list, cb, next);
> >> +			rte_free(cb);
> >> +		} else {
> >> +			ret = -EAGAIN;
> >> +		}
> >> +	}
> >> +
> >> +	rte_spinlock_unlock(lock);
> >> +	return (ret);
> >> +}
> >> +
> >> +void
> >> +rte_dev_callback_process(struct rte_dev_cb_list * cb_list, uint8_t
> >>port_id,
> >> +	enum rte_dev_event_type event, rte_spinlock_t *lock)
> >> +{
> >> +	struct rte_dev_callback *cb;
> >> +	struct rte_dev_callback dev_cb;
> >> +
> >> +	rte_spinlock_lock(lock);
> >> +	TAILQ_FOREACH(cb, cb_list, next) {
> >> +		if (cb->cb_fn == NULL || cb->event != event)
> >> +			continue;
> >> +		dev_cb = *cb;
> >> +		cb->active = 1;
> >> +		rte_spinlock_unlock(lock);
> >> +		dev_cb.cb_fn(port_id, dev_cb.event, dev_cb.cb_arg);
> >> +		rte_spinlock_lock(lock);
> >> +		cb->active = 0;
> >> +	}
> >> +	rte_spinlock_unlock(lock);
> >> +}
> >
> >
> >This is a bit...odd.   The rte_eth callbacks had some context because you
> >knew
> >when the callback was going to be issued (at the end of an rx and tx
> >operation).
> >Here, by making the process routine generic, you're removed that context,
> >and so
> >the caller has no guarantee as to when their callbacks will be called.
> >If its
> >to be done at the end of the generic transmit and routine functions, that
> >would
> >be fine, but I don't see that happening here.
> 
> No able to follow you here as this routine is called from the rte_ethdev.c
> _rte_eth_dev_callback_process(). We could remove the
> _rte_eth_dev_callback_process function and have the rest of the code call
> this routine instead. No context is lost here and the callbacks are called
> in the same location as before.
> 
Its ok, ignore me here.  I hadn't looked ahead to how you used this in the
ethdev case, that makes sense.  Though I'm still a bit concerned about how this
might be used in other device cases, but that can wait until you post the QAT
RFC patches.

> > 
> >> diff --git a/lib/librte_eal/common/include/rte_common_device.h
> >>b/lib/librte_eal/common/include/rte_common_device.h
> >> new file mode 100644
> >> index 0000000..5baefb6
> >> --- /dev/null
> >> +++ b/lib/librte_eal/common/include/rte_common_device.h
> >> @@ -0,0 +1,617 @@
> >> +/*-
> >> + *   BSD LICENSE
> >> + *
> >> + *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
> >> + *   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 Intel Corporation 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, WHPDF 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_COMMON_DEVICE_H_
> >> +#define _RTE_COMMON_DEVICE_H_
> >> +
> >> +/**
> >> + * @file
> >> + *
> >> + * Common Device Helpers in RTE
> >> + */
> >> +
> >> +#ifdef __cplusplus
> >> +extern "C" {
> >> +#endif
> >> +
> >> +#include <stdint.h>
> >> +#include <stdio.h>
> >> +#include <string.h>
> >> +#include <rte_spinlock.h>
> >> +
> >> +/* Macros for checking for restricting functions to primary instance
> >>only */
> >> +#define PROC_PRIMARY_OR_ERR_RET(retval) do { \
> >> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \
> >> +		PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \
> >> +		return (retval); \
> >> +	} \
> >> +} while(0)
> >> +#define PROC_PRIMARY_OR_RET() do { \
> >> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \
> >> +		PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \
> >> +		return; \
> >> +	} \
> >> +} while(0)
> >> +
> >> +/* Macros to check for invalid function pointers in dev_ops structure
> >>*/
> >> +#define FUNC_PTR_OR_ERR_RET(func, retval) do { \
> >> +	if ((func) == NULL) { \
> >> +		PMD_DEBUG_TRACE("Function not supported\n"); \
> >> +		return (retval); \
> >> +	} \
> >> +} while(0)
> >> +#define FUNC_PTR_OR_RET(func) do { \
> >> +	if ((func) == NULL) { \
> >> +		PMD_DEBUG_TRACE("Function not supported\n"); \
> >> +		return; \
> >> +	} \
> >> +} while(0)
> >> +
> >> +enum {
> >> +	DEV_DETACHED = 0,
> >> +	DEV_ATTACHED
> >> +};
> >> +
> >> +/*
> >> + * The device type
> >> + */
> >> +enum rte_dev_type {
> >> +	RTE_DEV_UNKNOWN,	/**< unknown device type */
> >> +	RTE_DEV_PCI,
> >> +		/**< Physical function and Virtual function of PCI devices */
> >> +	RTE_DEV_VIRTUAL,	/**< non hardware device */
> >> +	RTE_DEV_MAX			/**< max value of this enum */
> >> +};
> >> +
> >> +/**
> >> + * The device event type for interrupt, and maybe others in the future.
> >> + */
> >> +enum rte_dev_event_type {
> >> +	RTE_DEV_EVENT_UNKNOWN,  /**< unknown event type */
> >> +	RTE_DEV_EVENT_INTR_LSC, /**< lsc interrupt event */
> >> +	RTE_DEV_EVENT_MAX       /**< max value of this enum */
> >> +};
> >> +
> >> +struct rte_dev_callback;
> >> +
> >> +/** @internal Structure to keep track of registered callback */
> >> +TAILQ_HEAD(rte_dev_cb_list, rte_dev_callback);
> >> +
> >> +struct rte_mbuf;
> >> +
> >> +typedef uint16_t (*dev_rx_burst_t)(void *rxq,
> >> +				   struct rte_mbuf **rx_pkts,
> >> +				   uint16_t nb_pkts);
> >> +/**< @internal Retrieve input packets from a receive queue of a
> >>device. */
> >> +
> >> +typedef uint16_t (*dev_tx_burst_t)(void *txq,
> >> +				   struct rte_mbuf **tx_pkts,
> >> +				   uint16_t nb_pkts);
> >> +/**< @internal Send output packets on a transmit queue of a device. */
> >> +
> >> +typedef void (*rte_dev_cb_fn)(uint8_t port_id, \
> >> +		enum rte_dev_event_type event, void *cb_arg);
> >> +/**< user application callback to be registered for interrupts */
> >> +
> >> +#define RTE_DEV_NAME_MAX_LEN (32)
> >> +
> >> +/**
> >> + * @internal
> >> + * Common set of members for all devices included at the top of
> >>'struct rte_xyz_dev'
> >> + */
> >> +#define RTE_COMMON_DEV(_t)
> >>                 	\
> >> +    dev_rx_burst_t              rx_pkt_burst;   /**< Pointer to PMD
> >>receive function. */    \
> >> +    dev_tx_burst_t              tx_pkt_burst;   /**< Pointer to PMD
> >>transmit function. */   \
> >> +    struct rte_##_t##dev_data   *data;          /**< Pointer to device
> >>data */              \
> >> +    struct rte_##_t##global     *gbl_data;      /**< Pointer to device
> >>global data */       \
> >> +    const struct _t##driver     *driver;        /**< Driver for this
> >>device */              \
> >> +    struct _t##dev_ops          *dev_ops;       /**< Functions
> >>exported by PMD */           \
> >
> >This doesn't make any sense to me.  You've made a generic macro that
> >creates
> >what is ostensibly supposed to be common code point, but then you include
> >some
> >string concatenation so that the common parts are actually unique to a
> >given
> >device class.  Why would you do that?  At that point the common parts
> >aren't
> >common by definition, and should go in a structure specific to that
> >device class
> 
> The reason for these being specific to the device is the device may have
> extra members private to the device. I could have added a private pointer
> to the structure to make then generic to allow the device to have its own
> private members.
> 
Well, yes, that would make more sense to me.  The macro is called
RTE_COMMON_DEV.  It seems like it should only create members that are common to
all devices, with common names.

> >
> >> +    struct rte_pci_device       *pci_dev;       /**< PCI info.
> >>supplied by probing */       \
> >> +    /** User application callback for interrupts if present */
> >>                     \
> >> +    struct rte_dev_cb_list   link_intr_cbs;
> >>              		\
> >> +    /**        
> >>                     \
> >> +     * User-supplied functions called from rx_burst to post-process
> >>                     \
> >> +     * received packets before passing them to the user
> >>                     \
> >> +     */        
> >>                     \
> >> +    struct rte_dev_rxtx_callback **post_rx_burst_cbs;
> >>                  	\
> >> +    /**        
> >>                     \
> >> +     * User-supplied functions called from tx_burst to pre-process
> >>                     \
> >> +     * received packets before passing them to the driver for
> >>transmission.                 \
> >> +     */        
> >>                     \
> >> +    struct rte_dev_rxtx_callback **pre_tx_burst_cbs;
> >>                  	\
> >> +    enum rte_dev_type       	dev_type;       /**< Flag indicating the
> >>device type */     \
> >> +    uint8_t                     attached        /**< Flag indicating
> >>the port is attached */
> >> +
> >
> >I'm also confused here.  Looking at this, this is effectively a complete
> >renaming of the rte_eth_dev device structure.  Once again, why not just
> >leave
> >rte_eth_dev alone, and develop the crypto device from scratch?  If your
> >intent
> >is for it to have so much in common with an ethernet device that you can
> >effectively just rename the existing structure, why not just call it an
> >ethernet
> >device?
> 
> My goal was not to have everything common to a ethdev, but to move items
> out of the ethdev we can have in common. I feel you are not understanding
> something and I can not figure out what it is. If I remove the concat part
> maybe this will get my point across better.
> 

I know what your goal is.  All I'm saying is that, in pursuing that goal,
you seem to have decided that all the data in the rte_ethdev is common to all
devices.

> >
> >> +/**
> >> + * @internal
> >> + * Common set of members for all devices included at the top of
> >>'struct rte_xyz_dev_data'
> >> + */
> >> +#define RTE_COMMON_DEV_DATA
> >>     	\
> >> +    char name[RTE_DEV_NAME_MAX_LEN]; /**< Unique identifier name */
> >>         \
> >> +               
> >>         \
> >> +    void **rx_queues;               /**< Array of pointers to RX
> >>queues. */     \
> >> +    void **tx_queues;               /**< Array of pointers to TX
> >>queues. */     \
> >> +    uint16_t nb_rx_queues;          /**< Number of RX queues. */
> >>         \
> >> +    uint16_t nb_tx_queues;          /**< Number of TX queues. */
> >>         \
> >> +               
> >>         \
> >> +    uint16_t mtu;                   /**< Maximum Transmission Unit. */
> >>         \
> >> +    uint8_t unit_id;                /**< Unit/Port ID for this
> >>instance */      \
> >> +    uint8_t _pad0;             		/* alignment filler */
> >>      \
> >> +               
> >>         \
> >> +    /* 64bit alignment starts here */
> >>         \
> >> +    void    *dev_private;           /**< PMD-specific private data */
> >>         \
> >> +    uint64_t rx_mbuf_alloc_failed;  /**< RX ring mbuf allocation
> >>failures. */   \
> >> +    uint32_t min_rx_buf_size;       /**< Common rx buffer size handled
> >>by all queues */ \
> >> +    uint32_t _pad1					/**< Align to a 64bit boundary */
> >> +
> >> +/**
> >> + * @internal
> >> + * Common set of members for all devices included at the top of
> >>'struct rte_xyz_dev_info'
> >> + */
> >> +#define RTE_COMMON_DEV_INFO
> >>     	\
> >> +    struct rte_pci_device   *pci_dev;       /**< Device PCI
> >>information. */     \
> >> +    const char              *driver_name;   /**< Device Driver name.
> >>*/         \
> >> +    unsigned int            if_index;       /**< Index to bound host
> >>interface, or 0 if none. */ \
> >> +        /* Use if_indextoname() to translate into an interface name.
> >>*/         \
> >> +    uint32_t _pad0
> >> +
> >> +#define port_id		unit_id
> >> +
> >> +/**
> >> + * @internal
> >> + * Common set of members for all devices included at the top of
> >>'struct rte_xyz_global'
> >> + */
> >> +#define RTE_COMMON_GLOBAL(_t)
> >> \
> >> +    struct rte_##_t##dev * devs;/**< Device information array */
> >>\
> >> +    struct rte_##_t##dev_data * data;  /**< Device private data */
> >>\
> >Again, if you're going to make something common, It doesn't make sense to
> >then
> >give it a name that is going to be unique to the device class
> >instantiating the
> >macro.
> 
> OK, will try to update the code to be non-specific to a given device.
> 
Thank you.

> >
> >> +    uint8_t     nb_ports;        /**< Number of ports/units found */
> >> \
> >> +    uint8_t     max_ports;       /**< Max number of ports or units */
> >> \
> >> +    uint16_t    dflt_mtu;        /**< Default MTU if needed by device
> >>*/\
> >> +    uint16_t    dev_size;        /**< Internal size of device struct
> >>*/	\
> >> +    uint16_t    data_size;       /**< Internal size of data structure
> >>*/\
> >> +    const char * mz_dev_data     /**< String constant for device data
> >>*/
> >> +
> >> +/**
> >> + * @internal
> >> + * Common overlay type structures with all devices.
> >> + */
> >> +struct rte_dev_info {
> >> +    RTE_COMMON_DEV_INFO;
> >> +};
> >> +
> >> +/**
> >> + * @internal
> >> + * The generic data structure associated with each device.
> >> + *
> >> + * Pointers to burst-oriented packet receive and transmit functions are
> >> + * located at the beginning of the structure, along with the pointer to
> >> + * where all the data elements for the particular device are stored in
> >>shared
> >> + * memory. This split allows the function pointer and driver data to
> >>be per-
> >> + * process, while the actual configuration data for the device is
> >>shared.
> >> + */
> >> +struct rte_dev {
> >> +    RTE_COMMON_DEV();
> >> +};
> >> +
> >> +/**
> >> + * @internal
> >> + * The data part, with no function pointers, associated with each
> >>device.
> >> + *
> >> + * This structure is safe to place in shared memory to be common among
> >>different
> >> + * processes in a multi-process configuration.
> >> + */
> >> +struct rte_dev_data {
> >> +    RTE_COMMON_DEV_DATA;
> >> +};
> >> +
> >> +/**
> >> + * @internal
> >> + * This structure is attempting to mirror the rte_xyz_global
> >>structures.
> >> + *
> >> + * Be careful as this structure is over layered on top of the xyzdev
> >>structures.
> >> + */
> >> +struct rte_dev_global {
> >> +    RTE_COMMON_GLOBAL();
> >> +};
> >> +
> >I don't understand, you created macros that allow for naming specifics,
> >then
> >don't use it?  If your intent was for that data to be generically named
> >(which
> >would make sense), then you should remove the parameter from the macro so
> >that
> >can't change.
> >
> >> +/**
> >> + * Get the rte_pkt_dev structure device pointer for the device.
> >> + *
> >> + * @param   gbl     pointer to device specific global structure.
> >> + * @param   port_id Port ID value to select the device structure.
> >> + *
> >> + * @return
> >> + *   - The rte_pkt_dev structure pointer for the given port ID.
> >> + */
> >> +static inline void *
> >> +rte_get_dev(void * _gbl, uint8_t port_id)
> >> +{
> >> +    struct rte_dev_global * gbl = (struct rte_dev_global *)_gbl;
> >> +
> >> +    return (void *)((uint8_t *)gbl->devs + (port_id * gbl->dev_size));
> >> +}
> >> +
> >How will this be helpful?  the caller will need to know what type of
> >struct to
> >cast this to (since it can be named different for different dev classes).
> >doesn't that sort of thing undo the notion of commonality?
> 
> Not really the point was to have a routine able to return the correct
> device structure pointer and the developer should understand which global
> structure he is passing into the routine and what type is being returned.
> 
Ok, I get that, but if the caller has to know how to cast this to an appropriate
type, then they will need to know the type of device they are communicating
with, and understand all its member data and methods, which defeats the ability
for a user of the device to write truly generic code to access devices, which,
when we spoke offline (and I think earlier in this thread), you indicated was a
goal of yours.

Neil