From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 36AB412A8 for ; Thu, 21 Apr 2016 14:08:45 +0200 (CEST) Received: from [107.15.76.160] (helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1atDPP-0007Bh-TQ; Thu, 21 Apr 2016 08:08:40 -0400 Date: Thu, 21 Apr 2016 08:08:35 -0400 From: Neil Horman To: David Marchand Cc: "dev@dpdk.org" , Thomas Monjalon , Stephen Hemminger , "Richardson, Bruce" , Panu Matilainen , Christian Ehrhardt , Wenzhuo Lu , "Zhang, Helin" Message-ID: <20160421120835.GA22931@hmsreliant.think-freely.org> 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=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-Spam-Score: -1.0 (-) X-Spam-Status: No Subject: Re: [dpdk-dev] [PATCH v3 01/13] e1000: move pci device ids to driver 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: Thu, 21 Apr 2016 12:08:45 -0000 On Thu, Apr 21, 2016 at 09:27:18AM +0200, David Marchand wrote: > 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 ? > Thats an option, yes, I'd rename it rte_pci_vendor_ids.h perhaps, but you get the idea. > Or, we could rely on linux kernel headers (pci_ids.h), rather than > maintain a header in dpdk. Thats also a good idea. > 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 ? > If the dependency is unpalitable, I suppose its fine to duplicate the header file. My more direct point was really more just that you didn't need to duplicate the vendor id macro multiple times within the dpdk project. You can still define the device ids internally to each pmd if you don't need it in other locations, and the duplication against other projects there is ok by me. > > >> > 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. > Ok, so if I'm reading you correctly, you're indicating that test_pci is just there to test the pci common infrastructure code, and happens to use a few well known and supported pci id's (igb and ixgbe) to do that. i.e. theres no need for exhaustive pci device enumeration there, right? If thats the case then you can just create a data structure with the RTE_PCI_DEV macro and stop aliasing it to all the device specific DECL macros, no? My main concern here is really just the needless aliasing of those macros. > > - 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. > Ok, but how does that relate to the use of device specific PCI ennumeration values? Looking at the KNI code I see no reason that they are required any longer (or previously were), and if you're going to move them around you may as well clean up the usage at the same time. > 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 :-) > These all sound good :) > I don't mind doing trivial changes, but I don't have time for more on > this series. > Um, I'm not sure what to say here. The whole point of review is to help improve the code. If you don't have time to do anything non-trivial, Why are we reviewing it? Neil > > -- > David Marchand >