Re: [dpdk-dev] [PATCH] eal/vfio: export internal vfio functions

[Thomas Monjalon <thomas@monjalon.net>]

03/04/2018 08:27, Hemant Agrawal:
> On 3/27/2018 9:23 PM, Thomas Monjalon wrote:
> > 14/03/2018 09:00, Hemant Agrawal:
> >> This patch moves some of the internal vfio functions from
> >> eal_vfio.h to rte_vfio.h for common uses with "rte_" prefix.
> >>
> >> This patch also change the FSLMC bus usages from the internal
> >> VFIO functions to external ones with "rte_" prefix
> >>
> >> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> >> ---
> >> --- a/lib/librte_eal/common/include/rte_vfio.h
> >> +++ b/lib/librte_eal/common/include/rte_vfio.h
> >> @@ -28,6 +28,12 @@
> >> +#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 5, 0)
> >> +#define RTE_VFIO_NOIOMMU 8
> >> +#else
> >> +#define RTE_VFIO_NOIOMMU VFIO_NOIOMMU_IOMMU
> >> +#endif
> > I know this is just a move of an existing code,
> > but do you know why this check is against a version number (4.5.0),
> > instead of #ifdef VFIO_NOIOMMU_IOMMU which would be backport-safe?
> Agreed. please check it in v3.

Yes, in v2.

> >> +/**
> >> + * Parse IOMMU group number for a device
> >> + *
> >> + * This function is only relevant to linux and will return
> >> + * an error on BSD.
> >> + *
> >> + * @return
> >> + *   1 on success
> >> + *   0 for non-existent group
> >> + *  <0 for errors
> >> + */
> >> +int __rte_experimental
> >> +rte_vfio_get_group_no(const char *sysfs_base,
> >> +		const char *dev_addr, int *iommu_group_no);
> >> +
> >> +/**
> >> + * Open VFIO container fd or get an existing one
> >> + *
> >> + * This function is only relevant to linux and will return
> >> + * an error on BSD.
> >> + *
> >> + * @return
> >> + *  > 0 container fd
> >> + *  < 0 for errors
> >> + */
> >> +int __rte_experimental
> >> +rte_vfio_get_container_fd(void);
> >> +
> >> +/**
> >> + * Open VFIO group fd or get an existing one
> >> + *
> >> + * This function is only relevant to linux and will return
> >> + * an error on BSD.
> >> + *
> >> + * @return
> >> + *  > 0 group fd
> >> + *  < 0 for errors
> >> + */
> >> +int __rte_experimental
> >> +rte_vfio_get_group_fd(int iommu_group_no);
> > 
> > All these new functions should have some @param documentation.
> 
> added the @param
> 
> > This file is not included in doxygen, probably because @file is missing.
> 
> most of these functions are internal functions. do you think we should 
> add it in doxygen as well?

I think yes. It is an exported header of EAL.
The @file is missing to make it visible in doxygen.

> > About the naming, are you sure about "group_no" instead of "group_num"?
> 
> Agree, but this is already in many places.  I feel this change will be 
> unnecessary.

I don't see any other function using "_no".
What about naming the function "rte_vfio_get_group_no"
as "rte_vfio_get_group_num"?