Thomas, thanks for review and I added comments inline.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Wednesday, February 18, 2015 9:40 PM
> To: Zhou, Danny
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1 1/3] eal: enable uio_pci_generic support
>
> 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 <danny.zhou at intel.com>
> > Tested-by: Qun Wan <qun.wan at intel.com>
> [...]
> > --- 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 :)
>
This driver_name is the name of kernel driver(e.g. vfio_pci, igb_uio,
uio_pci_generic) while the driver->name is
a user-defined name for user space driver. I am going to change it to
kernel_driver_name with precise comment in V2
patch, and when the V2 patch is applied, I think the function
pci_get_kernel_driver_by_path() in hotplug patchset is not
necessary then as it could directly retrieve the kernel driver name from this
variable.
> > 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.
Will do.
>
> > --- 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?
It is a mistake I made during code merge, will fix it in V2.
>
> > + int fd; /**< interrupt event file descriptor */
> > enum rte_intr_handle_type type; /**< handle type */
> > };
