Re: [dpdk-dev] [PATCH v20.11 2/2] eal: improve device probing API

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

Hello Maxime,

On 08/06/20 17:53 +0200, Maxime Coquelin wrote:
> This patch makes rte_dev_probe() to return the
> rte_device pointer on success and NULL on error
> instead of returning 0 on success and negative
> value on error.
> 
> The goal is to avoid that the calling application
> iterates the devices list afterwards to retrieve
> the pointer. Retrieving the pointer is required
> for calling rte_dev_remove() later.
> 

I agree with the idea. I recall starting to do it on the legacy functions
(rte_eal_hotplug_*), but it was scrapped for API compat.

> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  app/test-pmd/testpmd.c                 |  2 +-
>  drivers/net/failsafe/failsafe.c        |  5 +++--
>  lib/librte_eal/common/eal_common_dev.c | 18 ++++++++++--------
>  lib/librte_eal/include/rte_dev.h       |  4 ++--
>  4 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 4989d22ca8..f777f449a8 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2764,7 +2764,7 @@ attach_port(char *identifier)
>  		return;
>  	}
>  
> -	if (rte_dev_probe(identifier) < 0) {
> +	if (rte_dev_probe(identifier) == NULL) {
>  		TESTPMD_LOG(ERR, "Failed to attach port %s\n", identifier);
>  		return;
>  	}
> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> index 72362f35de..e32effdef2 100644
> --- a/drivers/net/failsafe/failsafe.c
> +++ b/drivers/net/failsafe/failsafe.c
> @@ -341,6 +341,7 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
>  	struct rte_eth_dev *eth_dev;
>  	struct sub_device  *sdev;
>  	struct rte_devargs devargs;
> +	struct rte_device *dev;
>  	uint8_t i;
>  	int ret;
>  
> @@ -378,8 +379,8 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
>  				continue;
>  			}
>  			if (!devargs_already_listed(&devargs)) {
> -				ret = rte_dev_probe(devargs.name);
> -				if (ret < 0) {
> +				dev = rte_dev_probe(devargs.name);
> +				if (dev == NULL) {
>  					ERROR("Failed to probe devargs %s",
>  					      devargs.name);
>  					continue;
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index 9e4f09d83e..72baae2e48 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -120,7 +120,9 @@ rte_eal_hotplug_add(const char *busname, const char *devname,
>  	if (ret != 0)
>  		return ret;
>  
> -	ret = rte_dev_probe(devargs);
> +	if (rte_dev_probe(devargs) == NULL)
> +		ret = -1;
> +
>  	free(devargs);
>  
>  	return ret;
> @@ -192,7 +194,7 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
>  	return ret;
>  }
>  
> -int
> +struct rte_device *
>  rte_dev_probe(const char *devargs)
>  {
>  	struct eal_dev_mp_req req;
> @@ -212,12 +214,12 @@ rte_dev_probe(const char *devargs)
>  		if (ret != 0) {
>  			RTE_LOG(ERR, EAL,
>  				"Failed to send hotplug request to primary\n");
> -			return -ENOMSG;
> +			return NULL;

Is is a problem to keep the ENOMSG through rte_errno?

>  		}
>  		if (req.result != 0)
>  			RTE_LOG(ERR, EAL,
>  				"Failed to hotplug add device\n");
> -		return req.result;
> +		return NULL;
>  	}
>  
>  	/* attach a shared device from primary start from here: */
> @@ -236,7 +238,7 @@ rte_dev_probe(const char *devargs)
>  		 * process.
>  		 */
>  		if (ret != -EEXIST)
> -			return ret;
> +			return dev;
>  	}
>  
>  	/* primary send attach sync request to secondary. */
> @@ -261,11 +263,11 @@ rte_dev_probe(const char *devargs)
>  
>  		/* for -EEXIST, we don't need to rollback. */
>  		if (ret == -EEXIST)
> -			return ret;
> +			return dev;
>  		goto rollback;
>  	}
>  
> -	return 0;
> +	return dev;
>  
>  rollback:
>  	req.t = EAL_DEV_REQ_TYPE_ATTACH_ROLLBACK;
> @@ -282,7 +284,7 @@ rte_dev_probe(const char *devargs)
>  			"Failed to rollback device attach on primary."
>  			"Devices in secondary may not sync with primary\n");
>  
> -	return ret;
> +	return NULL;
>  }
>  
>  int
> diff --git a/lib/librte_eal/include/rte_dev.h b/lib/librte_eal/include/rte_dev.h
> index c8d985fb5c..9cf7c7fd71 100644
> --- a/lib/librte_eal/include/rte_dev.h
> +++ b/lib/librte_eal/include/rte_dev.h
> @@ -148,9 +148,9 @@ int rte_eal_hotplug_add(const char *busname, const char *devname,
>   * @param devargs
>   *   Device arguments including bus, class and driver properties.
>   * @return
> - *   0 on success, negative on error.
> + *   Generic device pointer on success, NULL on error.

If rte_errno is set, mapping codes to meanings would be helpful here.

Acked-by: Gaetan Rivet <grive@u256.net>

-- 
Gaëtan