diff mbox

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

Message ID dfbb11930ec2f85254fe439ec7dfd594a0a6330b.1501664109.git.gaetan.rivet@6wind.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Gaëtan Rivet Aug. 2, 2017, 8:55 a.m. UTC 
  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(-)

Comments

Thomas Monjalon Aug. 2, 2017, 2:32 p.m. UTC | #1 
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.
Gaëtan Rivet Aug. 2, 2017, 3:13 p.m. UTC | #2 
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.
diff mbox

Patch

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;