On Wed, Aug 23, 2017 at 06:18:34PM +0200, Thomas Monjalon wrote:

23/08/2017 17:09, Gaëtan Rivet:

quoted

Hello Raslan,

On Wed, Aug 23, 2017 at 05:37:04PM +0300, Raslan Darawsheh wrote:

quoted

Added hotplug in testpmd, to be able to test hotplug function
in the PMD's.

Signed-off-by: Raslan Darawsheh <redacted>

[...]

quoted

quoted

--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c

@@ -716,6 +716,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"port config (port_id|all) l2-tunnel E-tag"
 			" (enable|disable)\n"
 			"    Enable/disable the E-tag support.\n\n"
+
+			" device remove (device)\n"
+			"    Remove a device"

I think it should still be a part of the "port" command set (port
attach|detach|stop|close, etc).

I tend to disagree.
As far as I know, we use port for ethdev or cryptodev.
Here we want to deal with EAL rte_device.

I see, that makes sense.
I will redo the review with that in mind.

quoted

This would probably be easier to understand for users.

[...]

quoted

Continuing on using the port ...
format, then the port_id should allow to remove it instead of the device
identifier.
Using the device identifier will complexify your implementation.

[...]

quoted

	eth_dev = &rte_eth_devices[port_id];
	bus = rte_bus_find_by_device(eth_dev->device);

Note that we are going to remove eth_dev->device which implies eth_dev
but maybe also more device interfaces for the same HW.
That's why I think we need to distinguish port and device somehow.

-- 
Gaëtan Rivet
6WIND