From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wes1-so2.wedos.net (wes1-so2.wedos.net [46.28.106.16]) by dpdk.org (Postfix) with ESMTP id C655B9E7 for ; Mon, 8 Feb 2016 12:03:03 +0100 (CET) Received: from jvn (dynamic-109-81-211-84.ipv4.broadband.iol.cz [109.81.211.84]) by wes1-so2.wedos.net (Postfix) with ESMTPSA id 3pzPbC11NCzYg; Mon, 8 Feb 2016 12:03:03 +0100 (CET) Date: Mon, 8 Feb 2016 12:03:01 +0100 From: Jan Viktorin To: David Marchand Message-ID: <20160208120301.4be9b9c0@jvn> In-Reply-To: <1454076516-21591-3-git-send-email-david.marchand@6wind.com> References: <1454076516-21591-1-git-send-email-david.marchand@6wind.com> <1454076516-21591-3-git-send-email-david.marchand@6wind.com> Organization: RehiveTech X-Mailer: Claws Mail 3.13.0 (GTK+ 2.24.28; x86_64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH 2/9] pci: register all pdev as pci drivers X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Feb 2016 11:03:04 -0000 On Fri, 29 Jan 2016 15:08:29 +0100 David Marchand 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 > --- > 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@RehiveTech.com System Architect Web: www.RehiveTech.com RehiveTech Brno, Czech Republic