Re: [dpdk-dev] [PATCH 2/9] pci: register all pdev as pci drivers

[Jan Viktorin <viktorin@rehivetech.com>]

On Fri, 29 Jan 2016 15:08:29 +0100
David Marchand <david.marchand@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@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(&eth_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@RehiveTech.com
  System Architect            Web:    www.RehiveTech.com
  RehiveTech
  Brno, Czech Republic