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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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@intel.com>
> Tested-by: Qun Wan <qun.wan@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 :)

>  	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 */
>  };