On Mon, Feb 8, 2016 at 12:03 PM, Jan Viktorin <viktorin at rehivetech.com> wrote: > 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.
Yes, I rushed for this patchset to still be in the proposal window ... Sorry, I will split for next version. > See comments below... Globally, all my answers are "Yes, will do and it will be easier with smaller patches". Just added some comments where appropriate. >> 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? Yes, plus all other drivers are doing this, there were little exception to this convention, so I just aligned here. >> .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? Yes. I will separate crypto updates from netdev updates since crypto framework has a slight different way of initialising. >> 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. I don't think we need NEXT_ABI checks here. I am not breaking anything here, just adding a new symbol. I will introduce this new symbol, then convert all existing pdev/eth_driver to pdev/pci_driver in a second patch. -- David Marchand