[dpdk-dev,RFC,6/6] eal: removing eth_driver

  This patch demonstrates how eth_driver can be replaced with appropriate
changes for rte_xxx_driver from the PMD itself. It uses ixgbe_ethernet as
an example.

A large set of changes exists in the rte_ethdev.c - primarily because too
much PCI centric code (names, assumption of rte_pci_device) still exists
in it. Most, except symbol naming, has been changed in this patch.

This proposes that:
 - PMD would declare the rte_xxx_driver. In case of ixgbe, it would be
   rte_pci_driver.
 - Probe and remove continue to exists in rte_pci_driver. But, the
   rte_driver has new hooks for init and uninit. The rationale is that
   once a ethernet or cryto device is created, the rte_driver->init would
   be responsible for initializing the device.
   -- Eth_dev -> rte_driver -> rte_pci_driver
                          |    `-> probe/remove
                          `--> init/uninit
 - necessary changes in the rte_eth_dev have also been done so that it
   refers to the rte_device and rte_driver rather than rte_xxx_*. This
   would imply, ethernet device is 'linked' to a rte_device/rte_driver
   which in turn is a rte_xxx_device/rte_xxx_driver type.
   - for all operations related to extraction relvant xxx type,
     container_of would have to be used.

Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 49 +++++++++++++++++++++-------------------
 lib/librte_ether/rte_ethdev.c    | 36 +++++++++++++++++------------
 lib/librte_ether/rte_ethdev.h    |  6 ++---
 3 files changed, 51 insertions(+), 40 deletions(-)
  

On Thu, Nov 17, 2016 at 6:30 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> This patch demonstrates how eth_driver can be replaced with appropriate
> changes for rte_xxx_driver from the PMD itself. It uses ixgbe_ethernet as
> an example.
>
> A large set of changes exists in the rte_ethdev.c - primarily because too
> much PCI centric code (names, assumption of rte_pci_device) still exists
> in it. Most, except symbol naming, has been changed in this patch.
>
> This proposes that:
>  - PMD would declare the rte_xxx_driver. In case of ixgbe, it would be
>    rte_pci_driver.
>  - Probe and remove continue to exists in rte_pci_driver. But, the
>    rte_driver has new hooks for init and uninit. The rationale is that
>    once a ethernet or cryto device is created, the rte_driver->init would
>    be responsible for initializing the device.
>    -- Eth_dev -> rte_driver -> rte_pci_driver
>                           |    `-> probe/remove
>                           `--> init/uninit

Hmm, from my perspective this moves struct rte_driver a step closer to
struct rte_eth_dev instead of decoupling them. It is up to the
rte_driver->probe if it wants to allocate a struct rte_eth_dev,
rte_crypto_dev or the famous rte_foo_dev.

Instead of explicitly modelling rte_eth_dev specifics like init, unit
or dev_private_size I think we should delegate this to the
rte_driver->probe instead. Most of what is in rte_eth_dev_pci_probe()
today is anyway a rte_eth_dev_allocate_priv() anyway. I already have
some patches in this area in my patch stack.


>  - necessary changes in the rte_eth_dev have also been done so that it
>    refers to the rte_device and rte_driver rather than rte_xxx_*. This
>    would imply, ethernet device is 'linked' to a rte_device/rte_driver
>    which in turn is a rte_xxx_device/rte_xxx_driver type.
>    - for all operations related to extraction relvant xxx type,
>      container_of would have to be used.
>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c | 49 +++++++++++++++++++++-------------------
>  lib/librte_ether/rte_ethdev.c    | 36 +++++++++++++++++------------
>  lib/librte_ether/rte_ethdev.h    |  6 ++---
>  3 files changed, 51 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index edc9b22..acead31 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -1419,7 +1419,7 @@ eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev)
>                 return 0;
>         }
>
> -       pci_dev = eth_dev->pci_dev;
> +       pci_dev = container_of(eth_dev->device, struct rte_pci_device, device);
>
>         rte_eth_copy_pci_info(eth_dev, pci_dev);
>
> @@ -1532,7 +1532,9 @@ static int
>  eth_ixgbevf_dev_uninit(struct rte_eth_dev *eth_dev)
>  {
>         struct ixgbe_hw *hw;
> -       struct rte_pci_device *pci_dev = eth_dev->pci_dev;
> +       struct rte_pci_device *pci_dev;
> +
> +       pci_dev = container_of(eth_dev->device, struct rte_pci_device, device);
>
>         PMD_INIT_FUNC_TRACE();
>
> @@ -1562,32 +1564,33 @@ eth_ixgbevf_dev_uninit(struct rte_eth_dev *eth_dev)
>         return 0;
>  }
>
> -static struct eth_driver rte_ixgbe_pmd = {
> -       .pci_drv = {
> -               .id_table = pci_id_ixgbe_map,
> -               .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
> -                       RTE_PCI_DRV_DETACHABLE,
> -               .probe = rte_eth_dev_pci_probe,
> -               .remove = rte_eth_dev_pci_remove,
> +static struct rte_pci_driver rte_ixgbe_pci_driver = {
> +       .id_table = pci_id_ixgbe_map,
> +       .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
> +                    RTE_PCI_DRV_DETACHABLE,
> +       .probe = rte_eth_dev_pci_probe,
> +       .remove = rte_eth_dev_pci_remove,
> +       .driver = {
> +               .driver_init_t= eth_ixgbe_dev_init,
> +               .driver_uninit_t= eth_ixgbe_dev_uninit,
> +               .dev_private_size = sizeof(struct ixgbe_adapter),
>         },
> -       .eth_dev_init = eth_ixgbe_dev_init,
> -       .eth_dev_uninit = eth_ixgbe_dev_uninit,
> -       .dev_private_size = sizeof(struct ixgbe_adapter),
>  };
>
>  /*
>   * virtual function driver struct
>   */
> -static struct eth_driver rte_ixgbevf_pmd = {
> -       .pci_drv = {
> -               .id_table = pci_id_ixgbevf_map,
> -               .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_DETACHABLE,
> -               .probe = rte_eth_dev_pci_probe,
> -               .remove = rte_eth_dev_pci_remove,
> +static struct rte_pci_driver rte_ixgbevf_pci_driver = {
> +       .id_table = pci_id_ixgbevf_map,
> +       .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_DETACHABLE,
> +       .probe = rte_eth_dev_pci_probe,
> +       .remove = rte_eth_dev_pci_remove,
> +       .driver = {
> +               /* rte_driver hooks */
> +               .init = eth_ixgbevf_dev_init,
> +               .uninit = eth_ixgbevf_dev_uninit,
> +               .dev_private_size = sizeof(struct ixgbe_adapter),
>         },
> -       .eth_dev_init = eth_ixgbevf_dev_init,
> -       .eth_dev_uninit = eth_ixgbevf_dev_uninit,
> -       .dev_private_size = sizeof(struct ixgbe_adapter),
>  };
>
>  static int
> @@ -7592,7 +7595,7 @@ ixgbevf_dev_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
>         ixgbevf_dev_interrupt_action(dev);
>  }
>
> -RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd.pci_drv);
> +RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pci_driver);
>  RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
> -RTE_PMD_REGISTER_PCI(net_ixgbe_vf, rte_ixgbevf_pmd.pci_drv);
> +RTE_PMD_REGISTER_PCI(net_ixgbe_vf, rte_ixgbevf_pci_driver);
>  RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe_vf, pci_id_ixgbevf_map);
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index fde8112..3535ff4 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -235,13 +235,13 @@ int
>  rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>                       struct rte_pci_device *pci_dev)
>  {
> -       struct eth_driver    *eth_drv;
> +       struct rte_driver *drv;
>         struct rte_eth_dev *eth_dev;
>         char ethdev_name[RTE_ETH_NAME_MAX_LEN];
>
>         int diag;
>
> -       eth_drv = (struct eth_driver *)pci_drv;
> +       drv = pci_drv->driver;
>
>         rte_eal_pci_device_name(&pci_dev->addr, ethdev_name,
>                         sizeof(ethdev_name));
> @@ -252,13 +252,13 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>
>         if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>                 eth_dev->data->dev_private = rte_zmalloc("ethdev private structure",
> -                                 eth_drv->dev_private_size,
> +                                 drv->dev_private_size,
>                                   RTE_CACHE_LINE_SIZE);
>                 if (eth_dev->data->dev_private == NULL)
>                         rte_panic("Cannot allocate memzone for private port data\n");
>         }
> -       eth_dev->pci_dev = pci_dev;
> -       eth_dev->driver = eth_drv;
> +       eth_dev->device = pci_dev->device;
> +       eth_dev->driver = drv;
>         eth_dev->data->rx_mbuf_alloc_failed = 0;
>
>         /* init user callbacks */
> @@ -270,7 +270,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>         eth_dev->data->mtu = ETHER_MTU;
>
>         /* Invoke PMD device initialization function */
> -       diag = (*eth_drv->eth_dev_init)(eth_dev);
> +       diag = (*drv->init)(eth_dev);
>         if (diag == 0)
>                 return 0;
>
> @@ -287,7 +287,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>  int
>  rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev)
>  {
> -       const struct eth_driver *eth_drv;
> +       const struct rte_driver *drv;
>         struct rte_eth_dev *eth_dev;
>         char ethdev_name[RTE_ETH_NAME_MAX_LEN];
>         int ret;
> @@ -302,11 +302,11 @@ rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev)
>         if (eth_dev == NULL)
>                 return -ENODEV;
>
> -       eth_drv = (const struct eth_driver *)pci_dev->driver;
> +       drv = pci_dev->driver;
>
>         /* Invoke PMD device uninit function */
> -       if (*eth_drv->eth_dev_uninit) {
> -               ret = (*eth_drv->eth_dev_uninit)(eth_dev);
> +       if (*drv->uninit) {
> +               ret = (*drv->uninit)(eth_dev);
>                 if (ret)
>                         return ret;
>         }
> @@ -317,7 +317,7 @@ rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev)
>         if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>                 rte_free(eth_dev->data->dev_private);
>
> -       eth_dev->pci_dev = NULL;
> +       eth_dev->device = NULL;
>         eth_dev->driver = NULL;
>         eth_dev->data = NULL;
>
> @@ -1556,7 +1556,7 @@ rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info)
>
>         RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
>         (*dev->dev_ops->dev_infos_get)(dev, dev_info);
> -       dev_info->pci_dev = dev->pci_dev;
> +       dev_info->device = dev->device;
>         dev_info->driver_name = dev->data->drv_name;
>         dev_info->nb_rx_queues = dev->data->nb_rx_queues;
>         dev_info->nb_tx_queues = dev->data->nb_tx_queues;
> @@ -2537,6 +2537,7 @@ rte_eth_dev_rx_intr_ctl(uint8_t port_id, int epfd, int op, void *data)
>  {
>         uint32_t vec;
>         struct rte_eth_dev *dev;
> +       struct rte_pci_device *pci_dev;
>         struct rte_intr_handle *intr_handle;
>         uint16_t qid;
>         int rc;
> @@ -2544,6 +2545,10 @@ rte_eth_dev_rx_intr_ctl(uint8_t port_id, int epfd, int op, void *data)
>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>
>         dev = &rte_eth_devices[port_id];
> +       /* TODO intr_handle is currently in rte_pci_device;
> +        * Below is incorrect until that time
> +        */
> +       pci_dev = container_of(dev->device, struct rte_pci_device, device);
>         intr_handle = &dev->pci_dev->intr_handle;
>         if (!intr_handle->intr_vec) {
>                 RTE_PMD_DEBUG_TRACE("RX Intr vector unset\n");
> @@ -2572,7 +2577,7 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
>         const struct rte_memzone *mz;
>
>         snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
> -                dev->driver->pci_drv.driver.name, ring_name,
> +                dev->driver->name, ring_name,
>                  dev->data->port_id, queue_id);
>
>         mz = rte_memzone_lookup(z_name);
> @@ -2593,6 +2598,7 @@ rte_eth_dev_rx_intr_ctl_q(uint8_t port_id, uint16_t queue_id,
>  {
>         uint32_t vec;
>         struct rte_eth_dev *dev;
> +       struct rte_pci_device *pci_dev;
>         struct rte_intr_handle *intr_handle;
>         int rc;
>
> @@ -2604,7 +2610,9 @@ rte_eth_dev_rx_intr_ctl_q(uint8_t port_id, uint16_t queue_id,
>                 return -EINVAL;
>         }
>
> -       intr_handle = &dev->pci_dev->intr_handle;
> +       /* TODO; Until intr_handle is available in rte_device, below is incorrect */
> +       pci_dev = container_of(dev->device, struct rte_pci_device, device);
> +       intr_handle = &pci_dev->intr_handle;
>         if (!intr_handle->intr_vec) {
>                 RTE_PMD_DEBUG_TRACE("RX Intr vector unset\n");
>                 return -EPERM;
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 38641e8..2b1d826 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -876,7 +876,7 @@ struct rte_eth_conf {
>   * Ethernet device information
>   */
>  struct rte_eth_dev_info {
> -       struct rte_pci_device *pci_dev; /**< Device PCI information. */
> +       struct rte_device *device; /**< Device PCI information. */

We already the situation that virtual devices don't set the pci_dev
field. I wonder if it really makes sense to replace it with a struct
rte_device because that is not adding a lot of value (only numa_node).
If we break ABI we might want to add numa_node and dev_flags as
suggested by Stephen Hemminger already. If we choose to not break ABI
we can delegate the population of the pci_dev field to dev_infos_get.
I already have that patch in my patch stack too.

The problem with rte_eth_dev_info is that it doesn't support
extensions. Maybe its time to add rte_eth_dev_info_ex() ... or
rte_eth_dev_xinfo() if you don't like the IB verbs API.


>         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. */
> @@ -1623,9 +1623,9 @@ struct rte_eth_dev {
>         eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */
>         eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */
>         struct rte_eth_dev_data *data;  /**< Pointer to device data */
> -       const struct eth_driver *driver;/**< Driver for this device */
> +       const struct rte_driver *driver;/**< Driver for this device */
>         const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
> -       struct rte_pci_device *pci_dev; /**< PCI info. supplied by probing */
> +       struct rte_device *device; /**< Device instance */
>         /** User application callbacks for NIC interrupts */
>         struct rte_eth_dev_cb_list link_intr_cbs;
>         /**
> --
> 2.7.4
>
  

sorry for delay in responding; somehow I didn't notice this email.

On Thursday 17 November 2016 06:23 PM, Jan Blunck wrote:
> On Thu, Nov 17, 2016 at 6:30 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>> This patch demonstrates how eth_driver can be replaced with appropriate
>> changes for rte_xxx_driver from the PMD itself. It uses ixgbe_ethernet as
>> an example.
>>
>> A large set of changes exists in the rte_ethdev.c - primarily because too
>> much PCI centric code (names, assumption of rte_pci_device) still exists
>> in it. Most, except symbol naming, has been changed in this patch.
>>
>> This proposes that:
>>  - PMD would declare the rte_xxx_driver. In case of ixgbe, it would be
>>    rte_pci_driver.
>>  - Probe and remove continue to exists in rte_pci_driver. But, the
>>    rte_driver has new hooks for init and uninit. The rationale is that
>>    once a ethernet or cryto device is created, the rte_driver->init would
>>    be responsible for initializing the device.
>>    -- Eth_dev -> rte_driver -> rte_pci_driver
>>                           |    `-> probe/remove
>>                           `--> init/uninit
>
> Hmm, from my perspective this moves struct rte_driver a step closer to
> struct rte_eth_dev instead of decoupling them. It is up to the
> rte_driver->probe if it wants to allocate a struct rte_eth_dev,
> rte_crypto_dev or the famous rte_foo_dev.

That 'closeness' was my intention - to make rte_eth_dev an 
implementation of rte_device type.

rte_eth_dev == rte_cryptodev == rte_anyother_functional_device
- for the above context. All would include rte_device.

As for rte_driver->probe(), it still comes in the rte_driver->init()'s 
role to initialize the 'generic' functional device associated with the 
driver. And, allowing bus specific driver (like PCI) for its individual 
initialization using rte_xxx_driver->probe.

>
> Instead of explicitly modelling rte_eth_dev specifics like init, unit
> or dev_private_size I think we should delegate this to the
> rte_driver->probe instead. Most of what is in rte_eth_dev_pci_probe()
> today is anyway a rte_eth_dev_allocate_priv() anyway. I already have
> some patches in this area in my patch stack.

Can be done - either way rte_pci_driver->probe() ends up calling 
driver->init() (or erstwhile eth_driver->eth_dev_init()).

But, I still think it is better to keep them separate.
A PCI device is type of rte_device, physically.
A ethernet device is type of rte_device, logically.
They both should exist independently. It will help in splitting the 
functionality from physical layout in future - if need be.

>
>
>>  - necessary changes in the rte_eth_dev have also been done so that it
>>    refers to the rte_device and rte_driver rather than rte_xxx_*. This
>>    would imply, ethernet device is 'linked' to a rte_device/rte_driver
>>    which in turn is a rte_xxx_device/rte_xxx_driver type.
>>    - for all operations related to extraction relvant xxx type,
>>      container_of would have to be used.
>>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>> ---
>>  drivers/net/ixgbe/ixgbe_ethdev.c | 49 +++++++++++++++++++++-------------------
>>  lib/librte_ether/rte_ethdev.c    | 36 +++++++++++++++++------------
>>  lib/librte_ether/rte_ethdev.h    |  6 ++---
>>  3 files changed, 51 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index edc9b22..acead31 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -1419,7 +1419,7 @@ eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev)
>>                 return 0;
>>         }
>>
>> -       pci_dev = eth_dev->pci_dev;
>> +       pci_dev = container_of(eth_dev->device, struct rte_pci_device, device);
>>
>>         rte_eth_copy_pci_info(eth_dev, pci_dev);
>>
>> @@ -1532,7 +1532,9 @@ static int
>>  eth_ixgbevf_dev_uninit(struct rte_eth_dev *eth_dev)
>>  {
>>         struct ixgbe_hw *hw;
>> -       struct rte_pci_device *pci_dev = eth_dev->pci_dev;
>> +       struct rte_pci_device *pci_dev;
>> +
>> +       pci_dev = container_of(eth_dev->device, struct rte_pci_device, device);
>>
>>         PMD_INIT_FUNC_TRACE();
>>
>> @@ -1562,32 +1564,33 @@ eth_ixgbevf_dev_uninit(struct rte_eth_dev *eth_dev)
>>         return 0;
>>  }
>>
>> -static struct eth_driver rte_ixgbe_pmd = {
>> -       .pci_drv = {
>> -               .id_table = pci_id_ixgbe_map,
>> -               .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
>> -                       RTE_PCI_DRV_DETACHABLE,
>> -               .probe = rte_eth_dev_pci_probe,
>> -               .remove = rte_eth_dev_pci_remove,
>> +static struct rte_pci_driver rte_ixgbe_pci_driver = {
>> +       .id_table = pci_id_ixgbe_map,
>> +       .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
>> +                    RTE_PCI_DRV_DETACHABLE,
>> +       .probe = rte_eth_dev_pci_probe,
>> +       .remove = rte_eth_dev_pci_remove,
>> +       .driver = {
>> +               .driver_init_t= eth_ixgbe_dev_init,
>> +               .driver_uninit_t= eth_ixgbe_dev_uninit,
>> +               .dev_private_size = sizeof(struct ixgbe_adapter),
>>         },
>> -       .eth_dev_init = eth_ixgbe_dev_init,
>> -       .eth_dev_uninit = eth_ixgbe_dev_uninit,
>> -       .dev_private_size = sizeof(struct ixgbe_adapter),
>>  };
>>
>>  /*
>>   * virtual function driver struct
>>   */
>> -static struct eth_driver rte_ixgbevf_pmd = {
>> -       .pci_drv = {
>> -               .id_table = pci_id_ixgbevf_map,
>> -               .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_DETACHABLE,
>> -               .probe = rte_eth_dev_pci_probe,
>> -               .remove = rte_eth_dev_pci_remove,
>> +static struct rte_pci_driver rte_ixgbevf_pci_driver = {
>> +       .id_table = pci_id_ixgbevf_map,
>> +       .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_DETACHABLE,
>> +       .probe = rte_eth_dev_pci_probe,
>> +       .remove = rte_eth_dev_pci_remove,
>> +       .driver = {
>> +               /* rte_driver hooks */
>> +               .init = eth_ixgbevf_dev_init,
>> +               .uninit = eth_ixgbevf_dev_uninit,
>> +               .dev_private_size = sizeof(struct ixgbe_adapter),
>>         },
>> -       .eth_dev_init = eth_ixgbevf_dev_init,
>> -       .eth_dev_uninit = eth_ixgbevf_dev_uninit,
>> -       .dev_private_size = sizeof(struct ixgbe_adapter),
>>  };
>>
>>  static int
>> @@ -7592,7 +7595,7 @@ ixgbevf_dev_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
>>         ixgbevf_dev_interrupt_action(dev);
>>  }
>>
>> -RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd.pci_drv);
>> +RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pci_driver);
>>  RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
>> -RTE_PMD_REGISTER_PCI(net_ixgbe_vf, rte_ixgbevf_pmd.pci_drv);
>> +RTE_PMD_REGISTER_PCI(net_ixgbe_vf, rte_ixgbevf_pci_driver);
>>  RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe_vf, pci_id_ixgbevf_map);
>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>> index fde8112..3535ff4 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -235,13 +235,13 @@ int
>>  rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>>                       struct rte_pci_device *pci_dev)
>>  {
>> -       struct eth_driver    *eth_drv;
>> +       struct rte_driver *drv;
>>         struct rte_eth_dev *eth_dev;
>>         char ethdev_name[RTE_ETH_NAME_MAX_LEN];
>>
>>         int diag;
>>
>> -       eth_drv = (struct eth_driver *)pci_drv;
>> +       drv = pci_drv->driver;
>>
>>         rte_eal_pci_device_name(&pci_dev->addr, ethdev_name,
>>                         sizeof(ethdev_name));
>> @@ -252,13 +252,13 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>>
>>         if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>>                 eth_dev->data->dev_private = rte_zmalloc("ethdev private structure",
>> -                                 eth_drv->dev_private_size,
>> +                                 drv->dev_private_size,
>>                                   RTE_CACHE_LINE_SIZE);
>>                 if (eth_dev->data->dev_private == NULL)
>>                         rte_panic("Cannot allocate memzone for private port data\n");
>>         }
>> -       eth_dev->pci_dev = pci_dev;
>> -       eth_dev->driver = eth_drv;
>> +       eth_dev->device = pci_dev->device;
>> +       eth_dev->driver = drv;
>>         eth_dev->data->rx_mbuf_alloc_failed = 0;
>>
>>         /* init user callbacks */
>> @@ -270,7 +270,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>>         eth_dev->data->mtu = ETHER_MTU;
>>
>>         /* Invoke PMD device initialization function */
>> -       diag = (*eth_drv->eth_dev_init)(eth_dev);
>> +       diag = (*drv->init)(eth_dev);
>>         if (diag == 0)
>>                 return 0;
>>
>> @@ -287,7 +287,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>>  int
>>  rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev)
>>  {
>> -       const struct eth_driver *eth_drv;
>> +       const struct rte_driver *drv;
>>         struct rte_eth_dev *eth_dev;
>>         char ethdev_name[RTE_ETH_NAME_MAX_LEN];
>>         int ret;
>> @@ -302,11 +302,11 @@ rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev)
>>         if (eth_dev == NULL)
>>                 return -ENODEV;
>>
>> -       eth_drv = (const struct eth_driver *)pci_dev->driver;
>> +       drv = pci_dev->driver;
>>
>>         /* Invoke PMD device uninit function */
>> -       if (*eth_drv->eth_dev_uninit) {
>> -               ret = (*eth_drv->eth_dev_uninit)(eth_dev);
>> +       if (*drv->uninit) {
>> +               ret = (*drv->uninit)(eth_dev);
>>                 if (ret)
>>                         return ret;
>>         }
>> @@ -317,7 +317,7 @@ rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev)
>>         if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>>                 rte_free(eth_dev->data->dev_private);
>>
>> -       eth_dev->pci_dev = NULL;
>> +       eth_dev->device = NULL;
>>         eth_dev->driver = NULL;
>>         eth_dev->data = NULL;
>>
>> @@ -1556,7 +1556,7 @@ rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info)
>>
>>         RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
>>         (*dev->dev_ops->dev_infos_get)(dev, dev_info);
>> -       dev_info->pci_dev = dev->pci_dev;
>> +       dev_info->device = dev->device;
>>         dev_info->driver_name = dev->data->drv_name;
>>         dev_info->nb_rx_queues = dev->data->nb_rx_queues;
>>         dev_info->nb_tx_queues = dev->data->nb_tx_queues;
>> @@ -2537,6 +2537,7 @@ rte_eth_dev_rx_intr_ctl(uint8_t port_id, int epfd, int op, void *data)
>>  {
>>         uint32_t vec;
>>         struct rte_eth_dev *dev;
>> +       struct rte_pci_device *pci_dev;
>>         struct rte_intr_handle *intr_handle;
>>         uint16_t qid;
>>         int rc;
>> @@ -2544,6 +2545,10 @@ rte_eth_dev_rx_intr_ctl(uint8_t port_id, int epfd, int op, void *data)
>>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>>         dev = &rte_eth_devices[port_id];
>> +       /* TODO intr_handle is currently in rte_pci_device;
>> +        * Below is incorrect until that time
>> +        */
>> +       pci_dev = container_of(dev->device, struct rte_pci_device, device);
>>         intr_handle = &dev->pci_dev->intr_handle;
>>         if (!intr_handle->intr_vec) {
>>                 RTE_PMD_DEBUG_TRACE("RX Intr vector unset\n");
>> @@ -2572,7 +2577,7 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
>>         const struct rte_memzone *mz;
>>
>>         snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
>> -                dev->driver->pci_drv.driver.name, ring_name,
>> +                dev->driver->name, ring_name,
>>                  dev->data->port_id, queue_id);
>>
>>         mz = rte_memzone_lookup(z_name);
>> @@ -2593,6 +2598,7 @@ rte_eth_dev_rx_intr_ctl_q(uint8_t port_id, uint16_t queue_id,
>>  {
>>         uint32_t vec;
>>         struct rte_eth_dev *dev;
>> +       struct rte_pci_device *pci_dev;
>>         struct rte_intr_handle *intr_handle;
>>         int rc;
>>
>> @@ -2604,7 +2610,9 @@ rte_eth_dev_rx_intr_ctl_q(uint8_t port_id, uint16_t queue_id,
>>                 return -EINVAL;
>>         }
>>
>> -       intr_handle = &dev->pci_dev->intr_handle;
>> +       /* TODO; Until intr_handle is available in rte_device, below is incorrect */
>> +       pci_dev = container_of(dev->device, struct rte_pci_device, device);
>> +       intr_handle = &pci_dev->intr_handle;
>>         if (!intr_handle->intr_vec) {
>>                 RTE_PMD_DEBUG_TRACE("RX Intr vector unset\n");
>>                 return -EPERM;
>> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
>> index 38641e8..2b1d826 100644
>> --- a/lib/librte_ether/rte_ethdev.h
>> +++ b/lib/librte_ether/rte_ethdev.h
>> @@ -876,7 +876,7 @@ struct rte_eth_conf {
>>   * Ethernet device information
>>   */
>>  struct rte_eth_dev_info {
>> -       struct rte_pci_device *pci_dev; /**< Device PCI information. */
>> +       struct rte_device *device; /**< Device PCI information. */
>
> We already the situation that virtual devices don't set the pci_dev
> field. I wonder if it really makes sense to replace it with a struct
> rte_device because that is not adding a lot of value (only numa_node).

Sorry, I couldn't understand which way you are pointing:
  - continuing with 'rte_pci_device' in rte_eth_dev_info.
  - completely removing both, rte_pci_device and rte_device

In either case, I am ok. I went through the code usage of 
rte_eth_dev_info and it is mostly being used for getting information. I 
couldn't point out a situation where based on available info 
(rte_eth_dev_info), we need to extract back the device it is 
representing. Is that understanding correct?

If yes, I can remove this (after checking that this member is not being 
used).

> If we break ABI we might want to add numa_node and dev_flags as
> suggested by Stephen Hemminger already. If we choose to not break ABI
> we can delegate the population of the pci_dev field to dev_infos_get.
> I already have that patch in my patch stack too.

We can't avoid the ABI breakage - it is anyway going to happen.
As for 'dev_flags', I am assuming you are referring to moving 
'drv_flags' from rte_pci_driver. And you are suggesting moving that to 
'rte_driver' - is that correct understanding?

I don't know if drv_flags have any significance in rte_device. I thought 
they are driver specific flags (mmap, etc). Or, maybe they are just 
placed in driver for acting on all compatible devices.

>
> The problem with rte_eth_dev_info is that it doesn't support
> extensions. Maybe its time to add rte_eth_dev_info_ex() ... or
> rte_eth_dev_xinfo() if you don't like the IB verbs API.

I have no idea about IB verbs. And as for extensions, I will have to see 
- I don't prefer mixing that with current set. Though, the idea is nice.

>
>
>>         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. */
>> @@ -1623,9 +1623,9 @@ struct rte_eth_dev {
>>         eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */
>>         eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */
>>         struct rte_eth_dev_data *data;  /**< Pointer to device data */
>> -       const struct eth_driver *driver;/**< Driver for this device */
>> +       const struct rte_driver *driver;/**< Driver for this device */
>>         const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
>> -       struct rte_pci_device *pci_dev; /**< PCI info. supplied by probing */
>> +       struct rte_device *device; /**< Device instance */
>>         /** User application callbacks for NIC interrupts */
>>         struct rte_eth_dev_cb_list link_intr_cbs;
>>         /**
>> --
>> 2.7.4
>>
>

-
Shreyansh