port: do not unplug static ports

[david-marchand]

If some port is available since grout startup, don't try to remove the underlying device. This is to accomodate with buses that do not properly implement device hotplug.

[david-marchand]

@vjardin @maxime-leroy could you have a try with this patch?
It is not perfect.. but it should be a more generic workaround for buses that do not implement proper device hotplug.

[vjardin]

It is a very good idea to add a hotplug capability but see the comment, it is not enough.

Moreover, can you display this capability on the show interface ?
See david-marchand#1

[vjardin]

It is nok, you need to prevent the call to rte_eth_dev_close() too.

[david-marchand]

Can you describe why the rte_eth_dev_close() is problematic? The life of a rte_device should be separated from the rte_eth_dev object.

[rjarry]

I don't see great value in including the "hot-plugged" information in the API. It is more for debugging purposes than anything.

[vjardin]

I don't see great value in including the "hot-plugged" information in the API. It is more for debugging purposes than anything.

@rjarry > what would be the proper mean to get the internals of the grout structures if they are not reported thru the API ? Just logs ?

[rjarry]

@rjarry > what would be the proper mean to get the internals of the grout structures if they are not reported thru the API ? Just logs ?

Probably gdb in this case :)

[vjardin]

Probably gdb in this case :)

I am in a world where gdb is mostly not an option since source code is not available along with the runtime (and gdbserver cannot get a connection).

OK, let's focus on the hotplug topic, and let's forget this gdb/show option. It should be another thread but not this pull request.

[rjarry]

I don't mind adding DEBUG logs for such cases by the way. Or if you really think that hotplug information is useful in the long run for end users, we could add it in the API.

[rjarry]

@vjardin where are we for this patch? Did you get more info about why rte_eth_dev_stop() cannot be called for your net_dpaa2 ports?

[vjardin]

@vjardin where are we for this patch? Did you get more info about why rte_eth_dev_stop() cannot be called for your net_dpaa2 ports?

nope, I could not analyze it yet ; I am on another topic and I won't be able to be back on it next week neither.

[david-marchand]

@vjardin any update?

[maxime-leroy]

With vincent workaround:

grcli add interface port dpni.1 devargs fslmc:dpni.1
grcli del interface dpni.1
grcli add interface port dpni.1 devargs fslmc:dpni.1

ok good

Without vincent failed as expected:

# grcli add interface port dpni.1 devargs fslmc:dpni.1
NOTICE: GROUT: port_mac_add: dpni.1: multicast: Operation not supported
NOTICE: GROUT: port_mac_add: dpni.1: enabling allmulti
Created interface 257
# grcli del interface dpni.1
WARN: GROUT: port_mac_del: dpni.1: multicast: Operation not supported
# grcli add interface port dpni.1 devargs fslmc:dpni.1
error: command failed: No such file or directory

With david fixes:

# grcli add interface port dpni.1 devargs fslmc:dpni.1
NOTICE: GROUT: port_mac_add: dpni.1: multicast: Operation not supported
NOTICE: GROUT: port_mac_add: dpni.1: enabling allmulti
Created interface 257
# grcli del interface dpni.1
WARN: GROUT: port_mac_del: dpni.1: multicast: Operation not supported
# grcli add interface port dpni.1 devargs fslmc:dpni.1
error: command failed: No such file or directory

[maxime-leroy]

Minimal changes on David patches::

-       if ((ret = rte_eth_dev_close(port->port_id)) < 0)
-               LOG(ERR, "rte_eth_dev_close: %s", rte_strerror(-ret));
-       if (port->hotplugged && info.device != NULL && (ret = rte_dev_remove(info.device)) < 0)
-               LOG(ERR, "rte_dev_remove: %s", rte_strerror(-ret));
+       if (port->hotplugged) {
+               if ((ret = rte_eth_dev_close(port->port_id)) < 0)
+                       LOG(ERR, "rte_eth_dev_close: %s", rte_strerror(-ret));
+               if (info.device != NULL && (ret = rte_dev_remove(info.device)) < 0)
+                       LOG(ERR, "rte_dev_remove: %s", rte_strerror(-ret));
+       }

rte_eth_dev_close is not working properly on dpaa2 driver.

[maxime-leroy]

static int
fslmc_bus_unplug(struct rte_device dev __rte_unused)
{
/ No operation is performed while unplugging the device */
return 0;
}

It means that:

rte_dpaa2_remove is never called.
so rte_eth_dev_release_port is never called to liberate the port.

So in iface_port_init:

if (!rte_eth_dev_is_valid_port(port_id)) { -> this never called, because port is still used.
	LOG(INFO, "%s: port %u calling rte_dev_probe %s !!!!!", __func__, port_id, api->devargs);

	if ((ret = rte_dev_probe(api->devargs)) < 0)
		return errno_set(-ret);
	RTE_ETH_FOREACH_MATCHING_DEV(port_id, api->devargs, &iterator) {
		rte_eth_iterator_cleanup(&iterator);
		hotplugged = true;
		break;
	}
}

But rte_dev_probe will probably do nothing as fslmc_bus_plug do nothing to !