On Fri, 29 Jan 2016 15:08:29 +0100
David Marchand <david.marchand at 6wind.com> wrote:
> pdev drivers are actually pci drivers.
> Wrappers for ethdev and crypto pci drivers, that assume a 1 to 1
> association between pci device and upper device, have been added so that
> current drivers are not impacted.
It took me a while to find out what's going on in this patch. I could
see several not-so-related changes down the e-mail. I'd suggest to split
it this way:
1) separate coding style fixes
2) rename init/uninit to probe/remove (preserve the 'static' there)
3) remove rte_eth_driver_register invocations
4) separate VDEV and PDEV for cryptodev
5) play with the NEXT_ABI (remove those 'static' keywords?)
A more detailed commit log would help too. But this would
be automatically solved by the separation, I think.
See comments below...
Regards
Jan
>
> Signed-off-by: David Marchand <david.marchand at 6wind.com>
> ---
> drivers/crypto/qat/rte_qat_cryptodev.c | 7 +++++--
> drivers/net/bnx2x/bnx2x_ethdev.c | 16 ++++++++++------
> drivers/net/cxgbe/cxgbe_ethdev.c | 5 +++--
> drivers/net/e1000/em_ethdev.c | 4 +++-
> drivers/net/e1000/igb_ethdev.c | 14 +++++++++-----
> drivers/net/enic/enic_ethdev.c | 5 +++--
> drivers/net/fm10k/fm10k_ethdev.c | 4 +++-
> drivers/net/i40e/i40e_ethdev.c | 5 +++--
> drivers/net/i40e/i40e_ethdev_vf.c | 6 +++---
> drivers/net/ixgbe/ixgbe_ethdev.c | 10 ++++++----
> drivers/net/nfp/nfp_net.c | 7 ++++---
> drivers/net/virtio/virtio_ethdev.c | 4 +++-
> drivers/net/vmxnet3/vmxnet3_ethdev.c | 5 +++--
> lib/librte_cryptodev/rte_cryptodev.c | 18 ++++++++++--------
> lib/librte_cryptodev/rte_cryptodev_pmd.h | 14 ++++++++++++++
> lib/librte_cryptodev/rte_cryptodev_version.map | 10 +++++++++-
> lib/librte_ether/rte_ethdev.c | 16 +++++++++-------
> lib/librte_ether/rte_ethdev.h | 15 +++++++++++++++
> lib/librte_ether/rte_ether_version.map | 8 ++++++++
> 19 files changed, 123 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/crypto/qat/rte_qat_cryptodev.c
> b/drivers/crypto/qat/rte_qat_cryptodev.c
> index e500c1e..6853aee 100644
> --- a/drivers/crypto/qat/rte_qat_cryptodev.c
> +++ b/drivers/crypto/qat/rte_qat_cryptodev.c
> @@ -113,10 +113,12 @@ crypto_qat_dev_init(__attribute__((unused)) struct
> rte_cryptodev_driver *crypto_
> }
>
> static struct rte_cryptodev_driver rte_qat_pmd = {
> - {
> + .pci_drv = {
I believe that you are making the driver independent on the exact
location of the pci_drv member in the rte_cryptodev_drivers struct. Is
it true? Why is that important?
> .name = "rte_qat_pmd",
> .id_table = pci_id_qat_map,
> .drv_flags = RTE_PCI_DRV_NEED_MAPPING,
> + .devinit = rte_cryptodev_pci_probe,
> + .devuninit = rte_cryptodev_pci_remove,
> },
> .cryptodev_init = crypto_qat_dev_init,
> .dev_private_size = sizeof(struct qat_pmd_private),
> @@ -126,7 +128,8 @@ static int
> rte_qat_pmd_init(const char *name __rte_unused, const char *params
> __rte_unused)
> {
> PMD_INIT_FUNC_TRACE();
> - return rte_cryptodev_pmd_driver_register(&rte_qat_pmd, PMD_PDEV);
> + rte_eal_pci_register(&rte_qat_pmd.pci_drv);
> + return 0;
So, I finally discovered that this change somehow separates the PCI
(PDEV) and VDEV initialization. Is that correct?
> }
>
> static struct rte_driver pmd_qat_drv = {
> diff --git a/drivers/net/bnx2x/bnx2x_ethdev.c
> b/drivers/net/bnx2x/bnx2x_ethdev.c
> index 69df02e..916b9da 100644
> --- a/drivers/net/bnx2x/bnx2x_ethdev.c
> +++ b/drivers/net/bnx2x/bnx2x_ethdev.c
> @@ -501,6 +501,8 @@ static struct eth_driver rte_bnx2x_pmd = {
> .name = "rte_bnx2x_pmd",
> .id_table = pci_id_bnx2x_map,
> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
> + .devinit = rte_eth_dev_pci_probe,
> + .devuninit = rte_eth_dev_pci_remove,
> },
> .eth_dev_init = eth_bnx2x_dev_init,
> .dev_private_size = sizeof(struct bnx2x_softc),
> @@ -514,24 +516,26 @@ static struct eth_driver rte_bnx2xvf_pmd = {
> .name = "rte_bnx2xvf_pmd",
> .id_table = pci_id_bnx2xvf_map,
> .drv_flags = RTE_PCI_DRV_NEED_MAPPING,
> + .devinit = rte_eth_dev_pci_probe,
> + .devuninit = rte_eth_dev_pci_remove,
> },
> .eth_dev_init = eth_bnx2xvf_dev_init,
> .dev_private_size = sizeof(struct bnx2x_softc),
> };
>
> -static int rte_bnx2x_pmd_init(const char *name __rte_unused, const char
> *params __rte_unused)
> +static int rte_bnx2x_pmd_init(const char *name __rte_unused,
> + const char *params __rte_unused)
> {
> PMD_INIT_FUNC_TRACE();
> - rte_eth_driver_register(&rte_bnx2x_pmd);
> -
> + rte_eal_pci_register(&rte_bnx2x_pmd.pci_drv);
Here, I think you are trying to remove calls to the
rte_eth_driver_register as it does not do anything important.
Similar below (snipped)...
> return 0;
> }
>
> -static int rte_bnx2xvf_pmd_init(const char *name __rte_unused, const char
> *params __rte_unused)
> +static int rte_bnx2xvf_pmd_init(const char *name __rte_unused,
> + const char *params __rte_unused)
Coding style fixes should be in a separate patch. More occurences below
(snipped).
> {
> PMD_INIT_FUNC_TRACE();
> [snip]
>
> diff --git a/lib/librte_cryptodev/rte_cryptodev.c
> b/lib/librte_cryptodev/rte_cryptodev.c
> index f09f67e..1950020 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -313,9 +313,9 @@ rte_cryptodev_pmd_virtual_dev_init(const char *name,
> size_t dev_private_size,
> return cryptodev;
> }
>
> -static int
> -rte_cryptodev_init(struct rte_pci_driver *pci_drv,
> - struct rte_pci_device *pci_dev)
> +int
> +rte_cryptodev_pci_probe(struct rte_pci_driver *pci_drv,
> + struct rte_pci_device *pci_dev)
> {
> struct rte_cryptodev_driver *cryptodrv;
> struct rte_cryptodev *cryptodev;
> @@ -375,8 +375,8 @@ rte_cryptodev_init(struct rte_pci_driver *pci_drv,
> return -ENXIO;
> }
>
> -static int
> -rte_cryptodev_uninit(struct rte_pci_device *pci_dev)
> +int
> +rte_cryptodev_pci_remove(struct rte_pci_device *pci_dev)
> {
> const struct rte_cryptodev_driver *cryptodrv;
> struct rte_cryptodev *cryptodev;
> @@ -418,26 +418,28 @@ rte_cryptodev_uninit(struct rte_pci_device *pci_dev)
> return 0;
> }
>
> +#ifndef RTE_NEXT_ABI
> int
> rte_cryptodev_pmd_driver_register(struct rte_cryptodev_driver *cryptodrv,
> enum pmd_type type)
> {
> /* Call crypto device initialization directly if device is virtual */
> if (type == PMD_VDEV)
> - return rte_cryptodev_init((struct rte_pci_driver *)cryptodrv,
> + return rte_cryptodev_pci_probe((struct rte_pci_driver
> *)cryptodrv,
> NULL);
>
> /*
> * Register PCI driver for physical device intialisation during
> * PCI probing
> */
> - cryptodrv->pci_drv.devinit = rte_cryptodev_init;
> - cryptodrv->pci_drv.devuninit = rte_cryptodev_uninit;
> + cryptodrv->pci_drv.devinit = rte_cryptodev_pci_probe;
> + cryptodrv->pci_drv.devuninit = rte_cryptodev_pci_remove;
>
> rte_eal_pci_register(&cryptodrv->pci_drv);
>
> return 0;
> }
> +#endif
>
>
> uint16_t
> diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h
> b/lib/librte_cryptodev/rte_cryptodev_pmd.h
> index 8270afa..1c5ee14 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_pmd.h
> +++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h
> @@ -499,6 +499,7 @@ extern int
> rte_cryptodev_pmd_release_device(struct rte_cryptodev *cryptodev);
>
>
> +#ifndef RTE_NEXT_ABI
> /**
> * Register a Crypto [Poll Mode] driver.
> *
> @@ -527,6 +528,7 @@ rte_cryptodev_pmd_release_device(struct rte_cryptodev
> *cryptodev);
> extern int
> rte_cryptodev_pmd_driver_register(struct rte_cryptodev_driver *crypto_drv,
> enum pmd_type type);
> +#endif
>
> /**
> * Executes all the user application registered callbacks for the specific
> @@ -541,6 +543,18 @@ rte_cryptodev_pmd_driver_register(struct
> rte_cryptodev_driver *crypto_drv,
> void rte_cryptodev_pmd_callback_process(struct rte_cryptodev *dev,
> enum rte_cryptodev_event_type event);
>
> +/**
> + * Wrapper for use by pci drivers as a .devinit function to attach to a
> crypto
> + * interface.
> + */
> +int rte_cryptodev_pci_probe(struct rte_pci_driver *pci_drv,
> + struct rte_pci_device *pci_dev);
> +
> +/**
> + * Wrapper for use by pci drivers as a .devuninit function to detach a crypto
> + * interface.
> + */
> +int rte_cryptodev_pci_remove(struct rte_pci_device *pci_dev);
>
> #ifdef __cplusplus
> }
> diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map
> b/lib/librte_cryptodev/rte_cryptodev_version.map
> index ff8e93d..3f838a4 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_version.map
> +++ b/lib/librte_cryptodev/rte_cryptodev_version.map
> @@ -29,4 +29,12 @@ DPDK_2.2 {
> rte_cryptodev_queue_pair_stop;
>
> local: *;
> -};
> \ No newline at end of file
> +};
> +
> +DPDK_2.3 {
> + global:
> +
> + rte_cryptodev_pci_probe;
> + rte_cryptodev_pci_remove;
> +
> +} DPDK_2.2;
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 756b234..17e4f4d 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -239,9 +239,9 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
> return 0;
> }
>
> -static int
> -rte_eth_dev_init(struct rte_pci_driver *pci_drv,
> - struct rte_pci_device *pci_dev)
> +int
> +rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
> + struct rte_pci_device *pci_dev)
As I've suggested at the beginning, what about "first just rename then
make it public (non-static)"? I don't see the connection between the
rename and the NEXT_ABI conditional.
> {
> struct eth_driver *eth_drv;
> struct rte_eth_dev *eth_dev;
> @@ -293,8 +293,8 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv,
> return diag;
> }
>
> -static int
> -rte_eth_dev_uninit(struct rte_pci_device *pci_dev)
> +int
> +rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev)
> {
> const struct eth_driver *eth_drv;
> struct rte_eth_dev *eth_dev;
> @@ -334,6 +334,7 @@ rte_eth_dev_uninit(struct rte_pci_device *pci_dev)
> return 0;
> }
>
> +#ifndef RTE_NEXT_ABI
> /**
> * Register an Ethernet [Poll Mode] driver.
> *
> @@ -351,10 +352,11 @@ rte_eth_dev_uninit(struct rte_pci_device *pci_dev)
> void
> rte_eth_driver_register(struct eth_driver *eth_drv)
> {
> - eth_drv->pci_drv.devinit = rte_eth_dev_init;
> - eth_drv->pci_drv.devuninit = rte_eth_dev_uninit;
> + eth_drv->pci_drv.devinit = rte_eth_dev_pci_probe;
> + eth_drv->pci_drv.devuninit = rte_eth_dev_pci_remove;
> rte_eal_pci_register(ð_drv->pci_drv);
> }
> +#endif
>
> int
> rte_eth_dev_is_valid_port(uint8_t port_id)
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 8710dd7..af051d1 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -1774,6 +1774,7 @@ struct eth_driver {
> unsigned int dev_private_size; /**< Size of device private data. */
> };
>
> +#ifndef RTE_NEXT_ABI
> /**
> * @internal
> * A function invoked by the initialization function of an Ethernet driver
> @@ -1785,6 +1786,7 @@ struct eth_driver {
> * the Ethernet driver.
> */
> void rte_eth_driver_register(struct eth_driver *eth_drv);
> +#endif
>
> /**
> * Configure an Ethernet device.
> @@ -3880,6 +3882,19 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev
> *eth_dev, const char *name,
> uint16_t queue_id, size_t size,
> unsigned align, int socket_id);
>
> +/**
> + * Wrapper for use by pci drivers as a .devinit function to attach to a
> ethdev
> + * interface.
> + */
> +int rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
> + struct rte_pci_device *pci_dev);
> +
> +/**
> + * Wrapper for use by pci drivers as a .devuninit function to detach a ethdev
> + * interface.
> + */
> +int rte_eth_dev_pci_remove(struct rte_pci_device *pci_dev);
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/lib/librte_ether/rte_ether_version.map
> b/lib/librte_ether/rte_ether_version.map
> index d8db24d..07b2d8b 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -117,3 +117,11 @@ DPDK_2.2 {
>
> local: *;
> };
> +
> +DPDK_2.3 {
> + global:
> +
> + rte_eth_dev_pci_probe;
> + rte_eth_dev_pci_remove;
> +
> +} DPDK_2.2;
--
Jan Viktorin E-mail: Viktorin at RehiveTech.com
System Architect Web: www.RehiveTech.com
RehiveTech
Brno, Czech Republic
