Re: [dpdk-dev] [RFC PATCH 2/6] common/mlx5: use class enable check helper function

["Gaëtan Rivet" <grive@u256.net>]

On 10/06/20 17:17 +0000, Parav Pandit wrote:
> Currently mlx5_class_get() returns enabled single valid class.
> To support multiple class and to improve readability of code, change it
> to mlx5_class_enabled().
> With this function, each class enablement can be checked, to load class
> specific driver.
> 
> Signed-off-by: Parav Pandit <parav@mellanox.com>
> ---
>  drivers/common/mlx5/mlx5_common.c               | 12 +++++++-----
>  drivers/common/mlx5/mlx5_common.h               |  5 +++--
>  drivers/common/mlx5/rte_common_mlx5_version.map |  2 +-
>  drivers/net/mlx5/linux/mlx5_os.c                |  3 ++-
>  drivers/vdpa/mlx5/mlx5_vdpa.c                   |  2 +-
>  5 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_common.c b/drivers/common/mlx5/mlx5_common.c
> index db94d4aa8..96c415842 100644
> --- a/drivers/common/mlx5/mlx5_common.c
> +++ b/drivers/common/mlx5/mlx5_common.c
> @@ -37,22 +37,24 @@ mlx5_class_check_handler(__rte_unused const char *key, const char *value,
>  	return 0;
>  }
>  
> -enum mlx5_class
> -mlx5_class_get(struct rte_devargs *devargs)
> +bool
> +mlx5_class_enabled(const struct rte_devargs *devargs, enum mlx5_class dev_class)
>  {
>  	struct rte_kvargs *kvlist;
>  	const char *key = MLX5_CLASS_ARG_NAME;
> +	/* Default NET CLASS is enabled if user didn't specify the class */
>  	enum mlx5_class ret = MLX5_CLASS_NET;
>  
>  	if (devargs == NULL)
> -		return ret;
> +		return dev_class == MLX5_CLASS_NET ? true : false;
>  	kvlist = rte_kvargs_parse(devargs->args, NULL);
>  	if (kvlist == NULL)
> -		return ret;
> +		return dev_class == MLX5_CLASS_NET ? true : false;
>  	if (rte_kvargs_count(kvlist, key))
>  		rte_kvargs_process(kvlist, key, mlx5_class_check_handler, &ret);
>  	rte_kvargs_free(kvlist);
> -	return ret;
> +
> +	return (ret & dev_class) ? true : false;
>  }

I think it would be simpler to transform the enum mlx5_class into a
value that could support multiple states simultaneously, instead of
going through this function.

As it is this function cannot properly work. You return CLASS_NET on
devargs NULL (fine), but also on kvlist error, meaning on devargs
invalid keys passed to the device. An error signal should be worked out
at this point, but it does not play well with the binary true/false.

I think you should instead change enum mlx5_class into a bitfield.
Return some u8 (I guess u32 if you want to keep the enum width) with the proper bits set,
and MLX5_CLASS_INVALID bit set on error (EOM if kvlist malloc failed, or other
errors due to invalid inputs).

>  
>  
> diff --git a/drivers/common/mlx5/mlx5_common.h b/drivers/common/mlx5/mlx5_common.h
> index 8e679c699..1d59873c8 100644
> --- a/drivers/common/mlx5/mlx5_common.h
> +++ b/drivers/common/mlx5/mlx5_common.h
> @@ -202,13 +202,14 @@ int mlx5_dev_to_pci_addr(const char *dev_path, struct rte_pci_addr *pci_addr);
>  #define MLX5_CLASS_ARG_NAME "class"
>  
>  enum mlx5_class {
> +	MLX5_CLASS_INVALID,
>  	MLX5_CLASS_NET,
>  	MLX5_CLASS_VDPA,
> -	MLX5_CLASS_INVALID,
>  };
>  
>  __rte_internal
> -enum mlx5_class mlx5_class_get(struct rte_devargs *devargs);
> +bool mlx5_class_enabled(const struct rte_devargs *devargs,
> +			enum mlx5_class dev_class);
>  __rte_internal
>  void mlx5_translate_port_name(const char *port_name_in,
>  			      struct mlx5_switch_info *port_info_out);
> diff --git a/drivers/common/mlx5/rte_common_mlx5_version.map b/drivers/common/mlx5/rte_common_mlx5_version.map
> index 350e77140..01fa0cc25 100644
> --- a/drivers/common/mlx5/rte_common_mlx5_version.map
> +++ b/drivers/common/mlx5/rte_common_mlx5_version.map
> @@ -1,7 +1,7 @@
>  INTERNAL {
>  	global:
>  
> -	mlx5_class_get;
> +	mlx5_class_enabled;
>  
>  	mlx5_create_mr_ext;
>  
> diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
> index 92422dbe6..06772b7ae 100644
> --- a/drivers/net/mlx5/linux/mlx5_os.c
> +++ b/drivers/net/mlx5/linux/mlx5_os.c
> @@ -1377,11 +1377,12 @@ mlx5_os_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  	struct mlx5_dev_config dev_config;
>  	int ret;
>  
> -	if (mlx5_class_get(pci_dev->device.devargs) != MLX5_CLASS_NET) {
> +	if (!mlx5_class_enabled(pci_dev->device.devargs, MLX5_CLASS_NET)) {
>  		DRV_LOG(DEBUG, "Skip probing - should be probed by other mlx5"
>  			" driver.");
>  		return 1;
>  	}
> +
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>  		mlx5_pmd_socket_init();
>  	ret = mlx5_init_once();
> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
> index 1113d6cef..96776b64e 100644
> --- a/drivers/vdpa/mlx5/mlx5_vdpa.c
> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
> @@ -451,7 +451,7 @@ mlx5_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  	struct mlx5_hca_attr attr;
>  	int ret;
>  
> -	if (mlx5_class_get(pci_dev->device.devargs) != MLX5_CLASS_VDPA) {
> +	if (!mlx5_class_enabled(pci_dev->device.devargs, MLX5_CLASS_VDPA)) {
>  		DRV_LOG(DEBUG, "Skip probing - should be probed by other mlx5"
>  			" driver.");
>  		return 1;
> -- 
> 2.25.4
> 

-- 
Gaëtan