Re: [PATCH] app/testpmd: app/testpmd: add device removal command
From: Gaëtan Rivet <hidden>
Date: 2017-08-23 15:09:21
Hello Raslan, 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" + " Remove a device"
I think it should still be a part of the "port" command set (port attach|detach|stop|close, etc). This would probably be easier to understand for users.
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, + NULL, + }, +}; +
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 hunk ↗ jump to hunk
/* *** 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) { + struct rte_devargs devargs; + + if (device == NULL) { + printf("Invalid parameters are specified\n"); + return; + }
I don't think those checks are necessary. This function should not be called with invalid parameters, and you will be the only one calling it.
+ + rte_eal_devargs_parse(device, &devargs);
If you used a port_id, you would not have to parse the device string. You could do something like this: struct rte_eth_dev *eth_dev; struct rte_bus *bus; eth_dev = &rte_eth_devices[port_id]; bus = rte_bus_find_by_device(eth_dev->device);
+
+ if (rte_eal_hotplug_remove(devargs.bus->name, devargs.name)) {
+ if (rte_eal_hotplug_remove(bus->name, eth_dev->device->name)) {
As a side note, I don't see why we need to use those names in the
function parameters.
We could simply use the rte_device handle available. This handle should
propose all necessary infos.
It might be a future API change.
quoted hunk ↗ jump to hunk
+ 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);
void remove_port(uint8_t port_id);
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