Re: [PATCH v1 1/3] eal: enable uio_pci_generic support

["Zhou, Danny" <danny.zhou-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

Thomas, thanks for review and I added comments inline.

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Wednesday, February 18, 2015 9:40 PM
> To: Zhou, Danny
> Cc: dev-VfR2kkLFssw@public.gmane.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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Tested-by: Qun Wan <qun.wan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> [...]
> > --- 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 */
> >  };