eal: fix positive error codes from probe/remove
Checks
Commit Message
According to API, 'rte_dev_probe()' and 'rte_dev_remove()' and their
'hotplug' equivalents must return 0 or negative error code. Bus code
returns positive values if device wasn't recognized by any driver, so
the result of 'bus->plug/unplug()' must be converted.
Positive on remove means that device not found by driver.
Positive on probe means that there are no suitable buses/drivers,
i.e. device is not supported.
CC: stable@dpdk.org
Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
lib/librte_eal/common/eal_common_dev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Comments
On Thu, May 30, 2019 at 3:26 PM Ilya Maximets <i.maximets@samsung.com>
wrote:
> According to API, 'rte_dev_probe()' and 'rte_dev_remove()' and their
> 'hotplug' equivalents must return 0 or negative error code. Bus code
>
About this first part, existing callers in dpdk are not consistent with the
api which might explain why this was not seen earlier.
How about fixing the existing callers?
returns positive values if device wasn't recognized by any driver, so
> the result of 'bus->plug/unplug()' must be converted.
>
The problem is in local_dev_probe() (resp. local_dev_remove()) itself,
since this internal api announces it should return < 0 on error.
> Positive on remove means that device not found by driver.
> Positive on probe means that there are no suitable buses/drivers,
> i.e. device is not supported.
>
> CC: stable@dpdk.org
> Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
> Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> lib/librte_eal/common/eal_common_dev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_dev.c
> b/lib/librte_eal/common/eal_common_dev.c
> index 824b8f926..f9cae8e26 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -233,7 +233,7 @@ rte_dev_probe(const char *devargs)
> * process.
> */
> if (ret != -EEXIST)
> - return ret;
> + return (ret < 0) ? ret : -ENOTSUP;
> }
>
> /* primary send attach sync request to secondary. */
> @@ -319,7 +319,7 @@ local_dev_remove(struct rte_device *dev)
> if (ret) {
> RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
> dev->name);
> - return ret;
> + return (ret < 0) ? ret : -ENOENT;
> }
>
> return 0;
> --
> 2.17.1
>
>
On 03.06.2019 11:50, David Marchand wrote:
>
>
> On Thu, May 30, 2019 at 3:26 PM Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com>> wrote:
>
> According to API, 'rte_dev_probe()' and 'rte_dev_remove()' and their
> 'hotplug' equivalents must return 0 or negative error code. Bus code
>
>
> About this first part, existing callers in dpdk are not consistent with the api which might explain why this was not seen earlier.
> How about fixing the existing callers?
Do you mean replacing all the 'rte_dev_probe() != 0' with 'rte_dev_probe() < 0'
around the codebase?
>
>
> returns positive values if device wasn't recognized by any driver, so
> the result of 'bus->plug/unplug()' must be converted.
>
>
> The problem is in local_dev_probe() (resp. local_dev_remove()) itself, since this internal api announces it should return < 0 on error.
>
>
>
> Positive on remove means that device not found by driver.
> Positive on probe means that there are no suitable buses/drivers,
> i.e. device is not supported.
>
> CC: stable@dpdk.org <mailto:stable@dpdk.org>
> Fixes: a3ee360f4440 ("eal: add hotplug add/remove device")
> Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com>>
> ---
> lib/librte_eal/common/eal_common_dev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index 824b8f926..f9cae8e26 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -233,7 +233,7 @@ rte_dev_probe(const char *devargs)
> * process.
> */
> if (ret != -EEXIST)
> - return ret;
> + return (ret < 0) ? ret : -ENOTSUP;
> }
>
> /* primary send attach sync request to secondary. */
> @@ -319,7 +319,7 @@ local_dev_remove(struct rte_device *dev)
> if (ret) {
> RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
> dev->name);
> - return ret;
> + return (ret < 0) ? ret : -ENOENT;
> }
>
> return 0;
> --
> 2.17.1
>
>
>
> --
> David Marchand
On Mon, Jun 3, 2019 at 5:37 PM Ilya Maximets <i.maximets@samsung.com> wrote:
> On 03.06.2019 11:50, David Marchand wrote:
> >
> >
> > On Thu, May 30, 2019 at 3:26 PM Ilya Maximets <i.maximets@samsung.com
> <mailto:i.maximets@samsung.com>> wrote:
> >
> > According to API, 'rte_dev_probe()' and 'rte_dev_remove()' and their
> > 'hotplug' equivalents must return 0 or negative error code. Bus code
> >
> >
> > About this first part, existing callers in dpdk are not consistent with
> the api which might explain why this was not seen earlier.
> > How about fixing the existing callers?
>
> Do you mean replacing all the 'rte_dev_probe() != 0' with 'rte_dev_probe()
> < 0'
> around the codebase?
>
Yes.
It is not necessary to this patch so I can handle it if you don't have time.
But dpdk should show a good example by respecting its own apis description.
On 03.06.2019 19:13, David Marchand wrote:
>
>
> On Mon, Jun 3, 2019 at 5:37 PM Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com>> wrote:
>
> On 03.06.2019 11:50, David Marchand wrote:
> >
> >
> > On Thu, May 30, 2019 at 3:26 PM Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com> <mailto:i.maximets@samsung.com <mailto:i.maximets@samsung.com>>> wrote:
> >
> > According to API, 'rte_dev_probe()' and 'rte_dev_remove()' and their
> > 'hotplug' equivalents must return 0 or negative error code. Bus code
> >
> >
> > About this first part, existing callers in dpdk are not consistent with the api which might explain why this was not seen earlier.
> > How about fixing the existing callers?
>
> Do you mean replacing all the 'rte_dev_probe() != 0' with 'rte_dev_probe() < 0'
> around the codebase?
>
>
> Yes.
> It is not necessary to this patch so I can handle it if you don't have time.
> But dpdk should show a good example by respecting its own apis description.
I agree. I'll send v2 with fixed users.
> The problem is in local_dev_probe() (resp. local_dev_remove()) itself, since
> this internal api announces it should return < 0 on error.
Hmm. I missed that private internal API defined for local_* functions.
I'll move the check from rte_dev_probe() to local_dev_probe().
Best regards, Ilya Maximets.
@@ -233,7 +233,7 @@ rte_dev_probe(const char *devargs)
* process.
*/
if (ret != -EEXIST)
- return ret;
+ return (ret < 0) ? ret : -ENOTSUP;
}
/* primary send attach sync request to secondary. */
@@ -319,7 +319,7 @@ local_dev_remove(struct rte_device *dev)
if (ret) {
RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
dev->name);
- return ret;
+ return (ret < 0) ? ret : -ENOENT;
}
return 0;