[dpdk-dev] [PATCH] devargs: fix scan mode configuration in add

[dpdk-dev] [PATCH] devargs: fix scan mode configuration in add

[Gaetan Rivet]

When adding rte_devargs to the list, a check is performed on the
intended bus that should use this rte_devargs. This bus configuration is
for the time being only set once when the first rte_devargs is added to
it. If the bus configuration has previously been set, then the rte_devargs
insertion fails.

Failure occuring upon detection of the set configuration is an API
change. While rules will certainly be enforced in the next rte_devargs
API, none were previously enforced and this should be respected until
this API is deprecated.

The bus configuration is meant to evolve soon, but in the meantime it
should strictly follow the current rte_eal_devargs_add API.

The rte_devargs unit tests are failing due to this API change. Revert
this evolution, it will be reintroduced properly in the next release if
necessary.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_devargs.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 33e9f0a..6ac88d6 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -170,22 +170,12 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	bus = devargs->bus;
 	if (devargs->type == RTE_DEVTYPE_BLACKLISTED_PCI)
 		devargs->policy = RTE_DEV_BLACKLISTED;
-	if (devargs->policy == RTE_DEV_WHITELISTED) {
-		if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
+	if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
+		if (devargs->policy == RTE_DEV_WHITELISTED)
 			bus->conf.scan_mode = RTE_BUS_SCAN_WHITELIST;
-		} else if (bus->conf.scan_mode == RTE_BUS_SCAN_BLACKLIST) {
-			fprintf(stderr, "ERROR: incompatible device policy and bus scan mode\n");
-			goto fail;
-		}
-	} else if (devargs->policy == RTE_DEV_BLACKLISTED) {
-		if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
+		else if (devargs->policy == RTE_DEV_BLACKLISTED)
 			bus->conf.scan_mode = RTE_BUS_SCAN_BLACKLIST;
-		} else if (bus->conf.scan_mode == RTE_BUS_SCAN_WHITELIST) {
-			fprintf(stderr, "ERROR: incompatible device policy and bus scan mode\n");
-			goto fail;
-		}
 	}
-
 	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
 	return 0;
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH] devargs: fix scan mode configuration in add

[Thomas Monjalon]

02/08/2017 10:55, Gaetan Rivet:
> When adding rte_devargs to the list, a check is performed on the
> intended bus that should use this rte_devargs. This bus configuration is
> for the time being only set once when the first rte_devargs is added to
> it. If the bus configuration has previously been set, then the rte_devargs
> insertion fails.

Sorry, I don't understand which logic is changed :)
Maybe it would be easier with an example?

> Failure occuring upon detection of the set configuration is an API
> change. While rules will certainly be enforced in the next rte_devargs
> API, none were previously enforced and this should be respected until
> this API is deprecated.
> 
> The bus configuration is meant to evolve soon, but in the meantime it
> should strictly follow the current rte_eal_devargs_add API.
> 
> The rte_devargs unit tests are failing due to this API change. Revert
> this evolution, it will be reintroduced properly in the next release if
> necessary.

So this is a revert.
I think the title start with "revert" instead of "fix" as you seem
to go back to the old behaviour.

Re: [dpdk-dev] [PATCH] devargs: fix scan mode configuration in add

[Gaëtan Rivet]

On Wed, Aug 02, 2017 at 04:32:38PM +0200, Thomas Monjalon wrote:
> 02/08/2017 10:55, Gaetan Rivet:
> > When adding rte_devargs to the list, a check is performed on the
> > intended bus that should use this rte_devargs. This bus configuration is
> > for the time being only set once when the first rte_devargs is added to
> > it. If the bus configuration has previously been set, then the rte_devargs
> > insertion fails.
> 
> Sorry, I don't understand which logic is changed :)
> Maybe it would be easier with an example?
> 

Ok, I will send a more explicit v2.

> > Failure occuring upon detection of the set configuration is an API
> > change. While rules will certainly be enforced in the next rte_devargs
> > API, none were previously enforced and this should be respected until
> > this API is deprecated.
> > 
> > The bus configuration is meant to evolve soon, but in the meantime it
> > should strictly follow the current rte_eal_devargs_add API.
> > 
> > The rte_devargs unit tests are failing due to this API change. Revert
> > this evolution, it will be reintroduced properly in the next release if
> > necessary.
> 
> So this is a revert.
> I think the title start with "revert" instead of "fix" as you seem
> to go back to the old behaviour.

This API change introduced by the bus configuration is a bug. It was
meant to prepare the change, not enact it. Reverting to the old behavior
is fixing this mistake.

But revert is fine if you prefer.

-- 
Gaëtan Rivet
6WIND

[dpdk-dev] [PATCH v2] devargs: fix policy check in rte_eal_devargs_add

[Gaetan Rivet]

Do not fail upon detecting a set bus configuration when adding an
rte_devargs. i.e. adding a blacklisted PCI device while the bus is
configured in whitelist mode.

This failure condition has been introduced when the bus policies
were implemented. This implementation however was meant to strictly
follow the existing API. This check was not and should not be performed
until the current API has been properly deprecated.

Fixes: 02823c1db0bc ("devargs: parse bus policies")

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---

v2:

  - reword commitlog to better explain my intent.

 lib/librte_eal/common/eal_common_devargs.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 33e9f0a..6ac88d6 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -170,22 +170,12 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	bus = devargs->bus;
 	if (devargs->type == RTE_DEVTYPE_BLACKLISTED_PCI)
 		devargs->policy = RTE_DEV_BLACKLISTED;
-	if (devargs->policy == RTE_DEV_WHITELISTED) {
-		if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
+	if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
+		if (devargs->policy == RTE_DEV_WHITELISTED)
 			bus->conf.scan_mode = RTE_BUS_SCAN_WHITELIST;
-		} else if (bus->conf.scan_mode == RTE_BUS_SCAN_BLACKLIST) {
-			fprintf(stderr, "ERROR: incompatible device policy and bus scan mode\n");
-			goto fail;
-		}
-	} else if (devargs->policy == RTE_DEV_BLACKLISTED) {
-		if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
+		else if (devargs->policy == RTE_DEV_BLACKLISTED)
 			bus->conf.scan_mode = RTE_BUS_SCAN_BLACKLIST;
-		} else if (bus->conf.scan_mode == RTE_BUS_SCAN_WHITELIST) {
-			fprintf(stderr, "ERROR: incompatible device policy and bus scan mode\n");
-			goto fail;
-		}
 	}
-
 	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
 	return 0;
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH v2] devargs: fix policy check in rte_eal_devargs_add

[Thomas Monjalon]

02/08/2017 19:12, Gaetan Rivet:
> Do not fail upon detecting a set bus configuration when adding an
> rte_devargs. i.e. adding a blacklisted PCI device while the bus is
> configured in whitelist mode.
> 
> This failure condition has been introduced when the bus policies
> were implemented. This implementation however was meant to strictly
> follow the existing API. This check was not and should not be performed
> until the current API has been properly deprecated.
> 
> Fixes: 02823c1db0bc ("devargs: parse bus policies")
> 
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Adding a precision:
The same kind of check is already done when parsing EAL options.

Applied, thanks