[v2,(v20.11),2/2] eal: improve device probing API
Checks
Commit Message
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
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?
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).
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
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.
@@ -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;
}
@@ -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;
@@ -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
@@ -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.