[spp] SPP PCIe hotplug changes for SPP

[Ferruh Yigit]

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" <sy.jong.choi at intel.com>
> Date: Thu, 23 Feb 2017 12:54:50 +0800
> Subject: [PATCH] Signed-off-by: Choi, Sy Jong <sy.jong.choi at intel.com>

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 <mail>" format:
Signed-off-by: Sy Jong Choi <sy.jong.choi at intel.com>

> ---
>  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")
>