Re: [PATCH] app/testpmd: app/testpmd: add device removal command
From: Gaëtan Rivet <hidden>
Date: 2017-08-28 09:55:33
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 hunk ↗ jump to hunk
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)?
+ " Remove a device"
Here it should be: + " Remove a device.\n\n"
quoted hunk ↗ jump to hunk
); }@@ -1125,6 +1128,46 @@ cmdline_parse_inst_t cmd_operate_detach_port = { }, }; +/* *** Remove a specified device *** */ +struct cmd_operate_device_remove_result { + cmdline_fixed_string_t device; + cmdline_fixed_string_t keyword; + cmdline_fixed_string_t identifier; +}; + +static void cmd_operate_device_remove_parsed(void *parsed_result, + __attribute__((unused)) struct cmdline *cl, + __attribute__((unused)) void *data) +{ + struct cmd_operate_device_remove_result *res = parsed_result; + if (!strcmp(res->keyword, "remove")) + device_remove((char *) res->identifier); + else + printf("Unknown parameter\n"); +} + +cmdline_parse_token_string_t cmd_operate_device_remove_device = + TOKEN_STRING_INITIALIZER(struct cmd_operate_device_remove_result, + device, "device"); +cmdline_parse_token_string_t cmd_operate_device_remove_keyword = + TOKEN_STRING_INITIALIZER(struct cmd_operate_device_remove_result, + keyword, "remove"); +cmdline_parse_token_string_t cmd_operate_device_remove_identifier = + TOKEN_STRING_INITIALIZER(struct cmd_operate_device_remove_result, + identifier, NULL); + +cmdline_parse_inst_t cmd_operate_device_remove = { + .f = cmd_operate_device_remove_parsed, + .data = NULL, + .help_str = "device remove <device>: (device)", + .tokens = { + (void *)&cmd_operate_device_remove_device, + (void *)&cmd_operate_device_remove_keyword, + (void *)&cmd_operate_device_remove_identifier,
I know you have to follow the conventions set for this file and these names are correct. But I have to say: they are horrible.
quoted hunk ↗ jump to hunk
+ NULL, + }, +}; + /* *** configure speed for all ports *** */ struct cmd_config_speed_all { cmdline_fixed_string_t port;@@ -14276,6 +14319,7 @@ cmdline_parse_ctx_t main_ctx[] = { (cmdline_parse_inst_t *)&cmd_operate_specific_port, (cmdline_parse_inst_t *)&cmd_operate_attach_port, (cmdline_parse_inst_t *)&cmd_operate_detach_port, + (cmdline_parse_inst_t *)&cmd_operate_device_remove, (cmdline_parse_inst_t *)&cmd_config_speed_all, (cmdline_parse_inst_t *)&cmd_config_speed_specific, (cmdline_parse_inst_t *)&cmd_config_rx_tx,diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index 7d40139..a2e8526 100644 --- a/app/test-pmd/testpmd.c +++ b/app/test-pmd/testpmd.c@@ -1742,6 +1742,24 @@ detach_port(uint8_t port_id) } void +device_remove(char *device) {
device is not descriptive enough. I think "devname" would be better.
+ struct rte_devargs devargs;
+
+ if (device == NULL) {
+ printf("Invalid parameters are specified\n");
+ return;
+ }No need to check for this. If device is NULL, something went very wrong. Having a hard segfault in this case will stop anyone in their tracks and force the bug to be investigated. And it has less LOCs.
+ + rte_eal_devargs_parse(device, &devargs); +
After some thinking, I think using the devargs_parse utility is the simplest and will allow the command to follow future evolutions without having to change anything here.
quoted hunk ↗ jump to hunk
+ if (rte_eal_hotplug_remove(devargs.bus->name, devargs.name)) { + printf("Fail to remove device\n"); + return; + } + printf("Device removed successfully\n"); +} + +void pmd_test_exit(void) { portid_t pt_id;diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index c9d7739..0780f55 100644 --- a/app/test-pmd/testpmd.h +++ b/app/test-pmd/testpmd.h@@ -612,6 +612,7 @@ void stop_port(portid_t pid); void close_port(portid_t pid); void attach_port(char *identifier); void detach_port(uint8_t port_id); +void device_remove(char *device); int all_ports_stopped(void); int port_is_started(portid_t port_id); void pmd_test_exit(void);-- 2.7.4
-- Gaëtan Rivet 6WIND