From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Marchand Subject: Re: [PATCH 2/9] pci: register all pdev as pci drivers Date: Tue, 9 Feb 2016 09:55:31 +0100 Message-ID: References: <1454076516-21591-1-git-send-email-david.marchand@6wind.com> <1454076516-21591-3-git-send-email-david.marchand@6wind.com> <20160208120301.4be9b9c0@jvn> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: "dev@dpdk.org" To: Jan Viktorin Return-path: Received: from mail-ob0-f175.google.com (mail-ob0-f175.google.com [209.85.214.175]) by dpdk.org (Postfix) with ESMTP id 93AAB95D0 for ; Tue, 9 Feb 2016 09:55:51 +0100 (CET) Received: by mail-ob0-f175.google.com with SMTP id is5so179340222obc.0 for ; Tue, 09 Feb 2016 00:55:51 -0800 (PST) In-Reply-To: <20160208120301.4be9b9c0@jvn> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Feb 8, 2016 at 12:03 PM, Jan Viktorin wrote: > 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. 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