On Mon, Aug 28, 2017 at 11:30:47AM +0100, Ferruh Yigit wrote:

On 8/28/2017 10:55 AM, Gaëtan Rivet wrote:

quoted

Hi Raslan,

Redoing the review with the remarks from Thomas in mind.

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>
---
 app/test-pmd/cmdline.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.c | 18 ++++++++++++++++++
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 63 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index cd8c358..b32a368 100644
--- 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"

I think (device) might be hard to understand for a user.
Maybe (device name)?

I am suspicious on adding new root level command to testpmd, it is
getting bigger, each command looks OK on its own context, but as a whole
app getting more confusing [1].

Since dealing with device is kind of new, it can be OK to create new
command tree, but there are already hotplug commands per port:
"port attach #PCI|#VDEV_NAME"
"port detach #P"

Those two commands deal with the etherdev hotplug API.
The new command should test the rte_dev one.

As Thomas pointed out, the etherdev one deals only with eth ports, while
hotplug could be generalized to other devices, such as cryptodev.

perhaps it can be good to keep "attach", "detach" keywords for device to
be consistent?

"device attach #name"
"device detach #name"

I made a point of naming the hotplug operations in rte_bus plug/unplug
to avoid the confusion with the etherdev API. hotplug_add /
hotplug_remove also marks the distinction.

I don't know if it would be helpful for a developer writing a PMD,
searching for a way to test a functionality to have an API name
mismatch.

Also a show equivalent can be added to work in device level:
"show device info"

I think it would be useful.

[1]
http://dpdk.org/ml/archives/dev/2017-August/072764.html

-- 
Gaëtan Rivet
6WIND