diff mbox series

eal: fix positive error codes from probe/remove

Message ID 20190530132526.3496-1-i.maximets@samsung.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series eal: fix positive error codes from probe/remove |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/Intel-compilation fail Compilation issues

Commit Message

Ilya Maximets May 30, 2019, 1:25 p.m. UTC 
  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

David Marchand June 3, 2019, 8:50 a.m. UTC | #1 
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
>
>
Ilya Maximets June 3, 2019, 3:37 p.m. UTC | #2 
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
David Marchand June 3, 2019, 4:13 p.m. UTC | #3 
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.
Ilya Maximets June 6, 2019, 8:39 a.m. UTC | #4 
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.
diff mbox series

Patch

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;