From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Marchand Subject: Re: [PATCH v3 01/13] e1000: move pci device ids to driver Date: Thu, 21 Apr 2016 09:27:18 +0200 Message-ID: References: <1453120248-28274-1-git-send-email-david.marchand@6wind.com> <1461156236-25349-1-git-send-email-david.marchand@6wind.com> <1461156236-25349-2-git-send-email-david.marchand@6wind.com> <20160420132908.GA21072@hmsreliant.think-freely.org> <20160420181511.GB21072@hmsreliant.think-freely.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: "dev@dpdk.org" , Thomas Monjalon , Stephen Hemminger , "Richardson, Bruce" , Panu Matilainen , Christian Ehrhardt , Wenzhuo Lu , "Zhang, Helin" To: Neil Horman Return-path: Received: from mail-wm0-f51.google.com (mail-wm0-f51.google.com [74.125.82.51]) by dpdk.org (Postfix) with ESMTP id 3B5B02BE2 for ; Thu, 21 Apr 2016 09:27:38 +0200 (CEST) Received: by mail-wm0-f51.google.com with SMTP id n3so117440723wmn.0 for ; Thu, 21 Apr 2016 00:27:38 -0700 (PDT) In-Reply-To: <20160420181511.GB21072@hmsreliant.think-freely.org> 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 Wed, Apr 20, 2016 at 8:15 PM, Neil Horman wrote: > On Wed, Apr 20, 2016 at 03:39:59PM +0200, David Marchand wrote: >> On Wed, Apr 20, 2016 at 3:29 PM, Neil Horman wrote: >> >> +#ifndef RTE_PCI_DEV_ID_DECL_EM >> >> +#define RTE_PCI_DEV_ID_DECL_EM(vend, dev) >> >> +#endif >> >> + >> >> +#ifndef PCI_VENDOR_ID_INTEL >> >> +/** Vendor ID used by Intel devices */ >> >> +#define PCI_VENDOR_ID_INTEL 0x8086 >> >> +#endif >> >> + >> > This is broken, PCI_VENDOR_ID_INTEL should be defined in a central location for >> > all pci drivers, not redefined in every compilation unit. And you can likely >> >> Well we can keep the vendors in a common header, but I don't see the benefit. >> > ? > The fact that you won't have to do this > #ifndef PCI_VENDOR_ID_INTEL > #define PCI_VENDOR_ID_INTEL ... > #endif > in every pmd Ok, so you would keep the rte_pci_dev_ids.h with just the vendors in it ? Or, we could rely on linux kernel headers (pci_ids.h), rather than maintain a header in dpdk. But this would add a dependency build on dpdk and I am not sure there is something equivalent on freebsd (afaics all drivers seem to duplicate the pci vendor id). Any freebsd gourou reading this ? >> > just remvoe the RTE_PCI_DEV_ID_DECL_* macros from each patch and use the >> > RTE_PCI_DEV macro, as all the DECL macros just evaluate to that anyway. >> >> app/test/test_pci.c:#define RTE_PCI_DEV_ID_DECL_IGB(vend, dev) >> {RTE_PCI_DEVICE(vend, dev)}, >> lib/librte_eal/linuxapp/kni/kni_misc.c: #define >> RTE_PCI_DEV_ID_DECL_IGB(vend, dev) case (dev): >> >> All this stuff is because of pci tests and kni code. >> > You're going to have to explain a bit further than that. Why does the kni code > and pci testing code require that each driver have their own macro that just > maps to the same macro underneath? Looking at the test_pci code, it appears its > done because we used to contain all our pci device ids in a single file, and the > device specific macros were used to selectively enable devices that were there > for testing. But the point of this series is in part to separate out the device > ids to their own locations, so it seems like you will have to fix up the pci > tests anyway (and additionally it would be nice to include more than just EM, > IGB, and IXGBE in thsoe tests anyway, though I understand thats outside the > scope of this series) - test_pci.c should be about testing pci infrastructure. Relying on igb / ixgbe (or whatever pci device if we extend the list to all pci ids) being present on the system to run successfully is flawed but I have no better idea. - kni implements specific ethtool stuff based on pci ids. kni contains duplicated code from the kernel and it uses those ids to drive to the right ops. The solutions I have imagined so far : * use a shared header for the devices that it supports * drop the use of pci ids between kni library and kni kernel driver, instead use the pmd name that would be resolved internally by the kni library at RTE_KNI_IOCTL_CREATE time, and use this in the kni kernel driver * drop kni :-) I don't mind doing trivial changes, but I don't have time for more on this series. -- David Marchand