[v2,(v20.11),2/2] eal: improve device probing API
diff mbox series

Message ID 20200625080430.1392037-3-maxime.coquelin@redhat.com
State Deferred
Delegated to: Thomas Monjalon
Headers show
Series
  • rte_dev_probe() API change
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/travis-robot warning Travis build: failed
ci/checkpatch success coding style OK

Commit Message

Maxime Coquelin June 25, 2020, 8:04 a.m. UTC
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.

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(-)

Comments

Gaëtan Rivet June 29, 2020, 9:57 a.m. UTC | #1
On 25/06/20 10:04 +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.
> 
> 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/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.

Hello Maxime,

Do you plan on a v3 with rte_errno support in the new API or do you
prefer to keep it this way after all?
Thomas Monjalon Aug. 5, 2020, 11:33 p.m. UTC | #2
25/06/2020 10:04, Maxime Coquelin:
> 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.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> --- 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.
>   */
> -int rte_dev_probe(const char *devargs);
> +struct rte_device *rte_dev_probe(const char *devargs);

Sorry for not catching it earlier, I think this change is against
the idea of having a generic devargs syntax.
One string could identify multiple devices.
And a successful probe does not mean there is a new rte_device
(can be an update, allowing more ports on the same device).
Maxime Coquelin Aug. 6, 2020, 10:29 a.m. UTC | #3
On 8/6/20 1:33 AM, Thomas Monjalon wrote:
> 25/06/2020 10:04, Maxime Coquelin:
>> 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.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>> --- 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.
>>   */
>> -int rte_dev_probe(const char *devargs);
>> +struct rte_device *rte_dev_probe(const char *devargs);
> 
> Sorry for not catching it earlier, I think this change is against
> the idea of having a generic devargs syntax.
> One string could identify multiple devices.

That sounds fragile to me. What if one of the multiple devices fails to
probe? Do the other ones are going to be removed?

> And a successful probe does not mean there is a new rte_device
> (can be an update, allowing more ports on the same device).

This should be done by a separate API in my opinion, ports may be seen
as a sub-function of the device.

But anyway, I am fine with dropping it. It is not blocking vDPA probing,
just making it more cumbersome.

Regards,
Maxime
Thomas Monjalon Aug. 6, 2020, 11:43 a.m. UTC | #4
06/08/2020 12:29, Maxime Coquelin:
> On 8/6/20 1:33 AM, Thomas Monjalon wrote:
> > 25/06/2020 10:04, Maxime Coquelin:
> >> 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.
> >>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> ---
> >> --- 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.
> >>   */
> >> -int rte_dev_probe(const char *devargs);
> >> +struct rte_device *rte_dev_probe(const char *devargs);
> > 
> > Sorry for not catching it earlier, I think this change is against
> > the idea of having a generic devargs syntax.
> > One string could identify multiple devices.
> 
> That sounds fragile to me. What if one of the multiple devices fails to
> probe? Do the other ones are going to be removed?

No, what is correctly probed should not be rolled back in my opinion.

> > And a successful probe does not mean there is a new rte_device
> > (can be an update, allowing more ports on the same device).
> 
> This should be done by a separate API in my opinion, ports may be seen
> as a sub-function of the device.
> 
> But anyway, I am fine with dropping it. It is not blocking vDPA probing,
> just making it more cumbersome.

Apps should not iterate on devices.
vDPA should have an event mechanism like in ethdev:
a callback with RTE_ETH_EVENT_NEW is called for each new port probed.

Patch
diff mbox series

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;
 		}
 		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.
  */
-int rte_dev_probe(const char *devargs);
+struct rte_device *rte_dev_probe(const char *devargs);
 
 /**
  * Hotplug remove a given device from a specific bus.