From mboxrd@z Thu Jan 1 00:00:00 1970 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) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=GBJHi2UkKyce4cRdyRhZWciI30AVEWJFDMMLX95yrYM=; b=v7PwzeSsmOMTi94kVJVwS4Pf0idGoEEmn1+B4b/L73xhFA9zo4YrvY3z3gDSQzjuam lFjVTCyViNZRelibwpXzySuJSku1NYCQ1NyWpRr8MV22yon5sFEBMRSp0+FDBUkVdnbS vTLRiI3U3KRhsCPiuyJEBGR6OpXe3FCQSr5D6Fk8VKBvtfX7wayehXkgadMBjYo/P8xA uMSLgQ6Sbh27k5BrkbShq+2+sicb7hnNUXYrUKJF8qS7UFx+UkUCNg7dPGHCd5bXiyIg PTkCDaJ+QXQ3iQlkAQsLsuZEi87TlLsoQZyPfOGuGmBaQUt7ByAYb/vayfS35Oe4I+tt RCoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=GBJHi2UkKyce4cRdyRhZWciI30AVEWJFDMMLX95yrYM=; b=lF6vjtdwwi2M9RV0R4hqI6iUjmYR+Q69cKLIjuP8y0arrRl9gzLvIoG3I8Nb2sb2Rs pI4iYLoWO6TjALusRIOc6SifOtptjsSr/a+V8bDzluPs41peJ8GcfLF8SyFyRHXSlblL 55leYhkDFk9wm7ZyRqkb4dlzkyeLgTSK/oCrqSdCYvDmoo3XSE9yOdaoWM2yu0hdFuhE kbZfcNDJ3b4LEoZL0ZYVmh85IxbYIQOR3SEoouFxjyoMbhr+hoE5wNGywA8qPgP4Jlyu XIBPrg4WZwfmEppUBvhS32Q6h73QVSqjREQyBUXt/T1KgAG0A+jJbUGlWtZ0xdKqvSZF VBkg== X-Gm-Message-State: AG10YOSnLlf/OtPkCjgcpbB2Exb9C2flpa5h357tsSV0MHoZKBhZBB7dw/MRcZdgCUsH+UXJ1vXzeY5b2l+3SINy X-Received: by 10.182.86.33 with SMTP id m1mr5267164obz.48.1455008151065; Tue, 09 Feb 2016 00:55:51 -0800 (PST) MIME-Version: 1.0 Received: by 10.76.180.72 with HTTP; Tue, 9 Feb 2016 00:55:31 -0800 (PST) In-Reply-To: <20160208120301.4be9b9c0@jvn> References: <1454076516-21591-1-git-send-email-david.marchand@6wind.com> <1454076516-21591-3-git-send-email-david.marchand@6wind.com> <20160208120301.4be9b9c0@jvn> From: David Marchand Date: Tue, 9 Feb 2016 09:55:31 +0100 Message-ID: To: Jan Viktorin Content-Type: text/plain; charset=UTF-8 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: Tue, 09 Feb 2016 08:55:51 -0000 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