From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 829805A3E for ; Thu, 23 Feb 2017 18:45:33 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga105.fm.intel.com with ESMTP; 23 Feb 2017 09:45:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,198,1484035200"; d="scan'208";a="827719581" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.137]) ([10.237.220.137]) by FMSMGA003.fm.intel.com with ESMTP; 23 Feb 2017 09:45:31 -0800 To: "Choi, Sy Jong" , "spp@dpdk.org" References: <697F8B1B48670548A5BAB03E8283550F36A2215F@PGSMSX108.gar.corp.intel.com> From: Ferruh Yigit Message-ID: <3566ecfd-8ab7-57f7-65a8-21d95383b048@intel.com> Date: Thu, 23 Feb 2017 17:45:31 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <697F8B1B48670548A5BAB03E8283550F36A2215F@PGSMSX108.gar.corp.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [spp] SPP PCIe hotplug changes for SPP X-BeenThere: spp@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Soft Patch Panel List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Feb 2017 17:45:34 -0000 On 2/23/2017 1:21 PM, Choi, Sy Jong wrote: > Hi Everyone, > > First patch to enable spp & primary to support port start/stop and attach/detach. > > From e58c90661f702276dfeae2fc1f3e43750fe2ae2c Mon Sep 17 00:00:00 2001 > From: "Choi, Sy Jong" > Date: Thu, 23 Feb 2017 12:54:50 +0800 > Subject: [PATCH] Signed-off-by: Choi, Sy Jong Would it be right if patch subject changed to: "spp: add PCIe hotplug support" > > spp: adding port start/stop detach/attach > primary: adding port start/stop detach/attach Can you please document new added commands. 1- There is document for spp commands, spp_commands.md, new commands can be documented there 2- Usage can be described briefly in setup_guide.md Sign off should be here, with "name surname " format: Signed-off-by: Sy Jong Choi > --- > src/primary/init.c | 2 +- > src/primary/init.h | 2 ++ > src/primary/main.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++--- > src/spp.py | 46 ++++++++++++++++++++++++--- > 4 files changed, 131 insertions(+), 10 deletions(-) > > diff --git a/src/primary/init.c b/src/primary/init.c > index 5ea21f9..e5809e3 100644 > --- a/src/primary/init.c > +++ b/src/primary/init.c > @@ -47,7 +47,7 @@ > struct client *clients; > > /* The mbuf pool for packet rx */ > -static struct rte_mempool *pktmbuf_pool; > +struct rte_mempool *pktmbuf_pool; > > /* the port details */ > struct port_info *ports; > diff --git a/src/primary/init.h b/src/primary/init.h > index 915552c..82602a2 100644 > --- a/src/primary/init.h > +++ b/src/primary/init.h > @@ -56,6 +56,8 @@ struct client { > }; > > extern struct client *clients; > +/* The mbuf pool for packet rx */ > +extern struct rte_mempool *pktmbuf_pool; > > /* the shared port information: port numbers, rx and tx stats etc. */ > extern struct port_info *ports; > diff --git a/src/primary/main.c b/src/primary/main.c > index 15fa53a..94cda18 100644 > --- a/src/primary/main.c > +++ b/src/primary/main.c > @@ -185,9 +185,11 @@ static int > parse_command(char *str) > { > char *token_list[MAX_PARAMETER] = {NULL}; > + char name[RTE_ETH_NAME_MAX_LEN]; > int ret = 0; > int i = 0; > - > + uint8_t port_id = 255; //invalid port id Please use /* style comments > + Patch adds trailing whitespace, please get rid of them. > /* tokenize the user commands from controller */ > token_list[i] = strtok(str, " "); > while (token_list[i] != NULL) { > @@ -205,6 +207,88 @@ parse_command(char *str) > else > sprintf(str, "Server Idling\n"); > > + } else if (!strcmp(token_list[0], "port")) { > + //RTE_LOG(INFO, APP, "port\n"); Please remove dead code. Or uncomment it. > + if (i <= 1) { > + //RTE_LOG(INFO, APP, "i lesser 1\n"); And if uncommented, please describe what "i" is. > + return -1; > + } > + if (strcmp(token_list[1], "stop") == 0) { > + if (i <= 2) { > + //RTE_LOG(INFO, APP, "i lesser 2\n"); > + return -1; > + } > + > + port_id = atoi(token_list[2]); > + if (port_id == 255) { > + //RTE_LOG(INFO, APP, "port lesser 0\n"); > + return -1; > + } > + else { > + rte_eth_dev_stop(port_id); > + sprintf(str, "Port %d stopped\n", port_id); > + } > + } else if (strcmp(token_list[1], "start") == 0) { > + if (i <= 2) { > + return -1; > + } > + > + port_id = atoi(token_list[2]); > + if (port_id == 255) { > + return -1; > + } > + else { > + if (init_port(port_id, pktmbuf_pool) == 0) > + sprintf(str, "Port %d started\n", port_id); > + } > + } else if (strcmp(token_list[1], "close") == 0) { > + if (i <= 2) { > + return -1; > + } > + > + port_id = atoi(token_list[2]); > + if (port_id == 255) { > + return -1; > + } > + else { > + rte_eth_dev_close(port_id); > + sprintf(str, "Port %d closed\n", port_id); > + } > + } else if (strcmp(token_list[1], "detach") == 0) { > + if (i <= 2) { > + return -1; > + } > + > + port_id = atoi(token_list[2]); > + if (port_id == 255) { > + return -1; > + } > + else { > + if (rte_eth_dev_detach(port_id, name) == 0) > + sprintf(str, "Port %d %s detached\n", port_id, name); > + } > + } else if (strcmp(token_list[1], "attach") == 0) { > + if (i <= 2) { > + return -1; > + } > + > + if (token_list[2] == NULL) { > + return -1; > + } > + else { > + if (rte_eth_dev_attach(token_list[2], &port_id) == 0) > + { > + sprintf(str, "Port %d %s attached\n", port_id, name); > + } > + else > + { > + sprintf(str, "PCIe %s attached failed\n", name); > + } > + } > + } else { > + //RTE_LOG(INFO, APP, "else port\n"); > + ret = -1; > + } > } else if (!strcmp(token_list[0], "exit")) { > RTE_LOG(DEBUG, APP, "exit\n"); > RTE_LOG(DEBUG, APP, "stop\n"); > @@ -322,15 +406,14 @@ main(int argc, char *argv[]) > sleep(1); > continue; > } > - > ret = do_receive(&connected, &sock, str); > if (ret < 0) > continue; > - > + > RTE_LOG(DEBUG, APP, "Received string: %s\n", str); > > ret = parse_command(str); > - if (ret < 0) > + if (ret < 0) There are trailing whitespace in newly added code, please clean them. > break; > > /* Send the message back to client */ > diff --git a/src/spp.py b/src/spp.py > index b937b5a..3da74fa 100755 > --- a/src/spp.py > +++ b/src/spp.py > @@ -229,13 +229,44 @@ def close_all_secondary(): > command_secondary(i, 'exit') > SECONDARY_COUNT = 0 > > +def check_pri_cmds(cmds): > + """Validate primary commands before sending""" > + level1 = ['status', 'exit', 'clear'] > + level2 = ['add', 'del', 'port'] > + add_del_args = ['ring', 'vhost'] > + port_args = ['stop', 'detach', 'start', 'attach', 'close'] > + valid = 0 > + cmdlist = cmds.split(' ') > + > + length = len(cmdlist) > + if length == 1: > + if cmdlist[0] in level1: > + valid = 1 > + elif length == 3: > + if cmdlist[0] in level2: > + if cmdlist[0] == 'add' or cmdlist[0] == 'del': > + if cmdlist[1] in add_del_args: > + if str.isdigit(cmdlist[2]): > + valid = 1 > + elif cmdlist[0] == 'port': > + if cmdlist[1] in port_args: > + if cmdlist[1] != 'attach': > + if str.isdigit(cmdlist[2]): > + valid = 1 > + else: > + if cmdlist[2]: > + valid = 1 > + > + return valid > + > def check_sec_cmds(cmds): > """Validate secondary commands before sending""" > > - level1 = ['status', 'exit', 'forward', 'stop'] > - level2 = ['add', 'patch', 'del'] > + level1 = ['status', 'exit', 'forward', 'stop', 'start'] > + level2 = ['add', 'patch', 'del', 'port'] > patch_args = ['reset'] > add_del_args = ['ring', 'vhost'] > + port_args = ['stop', 'detach', 'start', 'attach', 'close'] > cmdlist = cmds.split(' ') > valid = 0 > > @@ -256,7 +287,10 @@ def check_sec_cmds(cmds): > elif cmdlist[0] == 'patch': > if str.isdigit(cmdlist[1]) and str.isdigit(cmdlist[2]): > valid = 1 > - > + elif cmdlist[0] == 'port': > + if cmdlist[1] in port_args: > + if str.isdigit(cmdlist[2]): > + valid = 1 > return valid > > class Shell(cmd.Cmd): > @@ -267,7 +301,8 @@ class Shell(cmd.Cmd): > recorded_file = None > > COMMANDS = ['status', 'add', 'patch', 'ring', 'vhost', > - 'reset', 'exit', 'forward', 'stop', 'clear'] > + 'reset', 'exit', 'forward', 'stop', 'clear', > + 'pause', 'start', 'port', 'detach'] > > def complete_pri(self, text, line, begidx, endidx): > """Completion for primary process commands""" > @@ -301,7 +336,8 @@ class Shell(cmd.Cmd): > def do_pri(self, command): > """Send command to primary process""" > > - if command and command in self.COMMANDS: > +# if command and command in self.COMMANDS: Please remove dead code. > + if check_pri_cmds(command): > command_primary(command) > else: > print ("primary invalid command") >