From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v1 1/3] eal: enable uio_pci_generic support Date: Wed, 18 Feb 2015 14:39:46 +0100 Message-ID: <22608095.lA2lToQDvY@xps13> References: <1422523699-17181-1-git-send-email-danny.zhou@intel.com> <1422523699-17181-2-git-send-email-danny.zhou@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev-VfR2kkLFssw@public.gmane.org To: Danny Zhou Return-path: In-Reply-To: <1422523699-17181-2-git-send-email-danny.zhou-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" Hi Danny, I wanted to apply this patchset which was reviewed. But when having a quick overview, I've seen some strange additions. 2015-01-29 17:28, Danny Zhou: > 1) Unify procedure to retrieve BAR resource mapping information. > 2) Setup bus master bit in NIC's PCIe configuration space for uio_pci_generic. > > Signed-off-by: Danny Zhou > Tested-by: Qun Wan [...] > --- a/lib/librte_eal/common/include/rte_pci.h > +++ b/lib/librte_eal/common/include/rte_pci.h > @@ -148,6 +148,7 @@ struct rte_pci_device { > struct rte_pci_id id; /**< PCI ID. */ > struct rte_pci_resource mem_resource[PCI_MAX_RESOURCE]; /**< PCI Memory Resource */ > struct rte_intr_handle intr_handle; /**< Interrupt handle */ > + char driver_name[BUFSIZ]; /**< driver name */ Why not embedding this field in driver struct? The name and comment should be more precise. There is also driver->name and hotplug patchset is adding a kernel driver name. Please bring the light in all these driver names :) > const struct rte_pci_driver *driver; /**< Associated driver */ [...] > --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c > +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c > +#define IORESOURCE_MEM 0x00000200 Please comment this value. > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h > @@ -50,8 +50,14 @@ enum rte_intr_handle_type { > > /** Handle for interrupts. */ > struct rte_intr_handle { > - int vfio_dev_fd; /**< VFIO device file descriptor */ > - int fd; /**< file descriptor */ > + union { > + int vfio_dev_fd; /**< VFIO device file descriptor */ > + }; > + union { > + int uio_cfg_fd; /**< UIO config file descriptor > + for uio_pci_generic */ > + }; Apart the indent, it seems there is a mistake here. Why 2 unions with 1 field each? > + int fd; /**< interrupt event file descriptor */ > enum rte_intr_handle_type type; /**< handle type */ > };