[PATCH 0/3] net/i40e: vf port reset

[PATCH 0/3] net/i40e: vf port reset

[Wei Zhao]

The patches mainly finish following functions:
1) get pf reset vf comamand falg from interrupt server function.
2) reset vf port from testpmd app using a command.
3) sore and restore vf related parameters.

zhao wei (3):
  app/testpmd: add port reset command into testpmd
  lib/librte_ether: add support for port reset
  net/i40e: implement device reset on port

 app/test-pmd/cmdline.c                 |  17 ++-
 app/test-pmd/testpmd.c                 |  68 ++++++++++
 app/test-pmd/testpmd.h                 |   1 +
 drivers/net/i40e/i40e_ethdev.c         |   2 +-
 drivers/net/i40e/i40e_ethdev.h         |  16 +++
 drivers/net/i40e/i40e_ethdev_vf.c      | 222 +++++++++++++++++++++++++++++++--
 drivers/net/i40e/i40e_rxtx.c           |  10 ++
 drivers/net/i40e/i40e_rxtx.h           |   4 +
 lib/librte_ether/rte_ethdev.c          |  17 +++
 lib/librte_ether/rte_ethdev.h          |  25 ++++
 lib/librte_ether/rte_ether_version.map |   6 +
 11 files changed, 376 insertions(+), 12 deletions(-)

-- 
2.9.3

[PATCH 1/3] app/testpmd: add port reset command into testpmd

[Wei Zhao]

Add vf port reset command into testpmd project, it is the interface for
user to reset vf port.

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 app/test-pmd/cmdline.c | 17 ++++++++++---
 app/test-pmd/testpmd.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 81 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 43fc636..59db672 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -596,6 +596,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"port close (port_id|all)\n"
 			"    Close all ports or port_id.\n\n"
 
+			"port reset (port_id|all)\n"
+			"    Reset all ports or port_id.\n\n"
+
 			"port attach (ident)\n"
 			"    Attach physical or virtual dev by pci address or virtual device name\n\n"
 
@@ -906,6 +909,8 @@ static void cmd_operate_port_parsed(void *parsed_result,
 		stop_port(RTE_PORT_ALL);
 	else if (!strcmp(res->name, "close"))
 		close_port(RTE_PORT_ALL);
+	else if (!strcmp(res->name, "reset"))
+		reset_port(RTE_PORT_ALL);
 	else
 		printf("Unknown parameter\n");
 }
@@ -915,14 +920,15 @@ cmdline_parse_token_string_t cmd_operate_port_all_cmd =
 								"port");
 cmdline_parse_token_string_t cmd_operate_port_all_port =
 	TOKEN_STRING_INITIALIZER(struct cmd_operate_port_result, name,
-						"start#stop#close");
+						"start#stop#close#reset");
 cmdline_parse_token_string_t cmd_operate_port_all_all =
 	TOKEN_STRING_INITIALIZER(struct cmd_operate_port_result, value, "all");
 
 cmdline_parse_inst_t cmd_operate_port = {
 	.f = cmd_operate_port_parsed,
 	.data = NULL,
-	.help_str = "port start|stop|close all: Start/Stop/Close all ports",
+	.help_str = "port start|stop|close|reset all: Start/Stop/Close/"
+			"Reset all ports",
 	.tokens = {
 		(void *)&cmd_operate_port_all_cmd,
 		(void *)&cmd_operate_port_all_port,
@@ -950,6 +956,8 @@ static void cmd_operate_specific_port_parsed(void *parsed_result,
 		stop_port(res->value);
 	else if (!strcmp(res->name, "close"))
 		close_port(res->value);
+	else if (!strcmp(res->name, "reset"))
+		reset_port(res->value);
 	else
 		printf("Unknown parameter\n");
 }
@@ -959,7 +967,7 @@ cmdline_parse_token_string_t cmd_operate_specific_port_cmd =
 							keyword, "port");
 cmdline_parse_token_string_t cmd_operate_specific_port_port =
 	TOKEN_STRING_INITIALIZER(struct cmd_operate_specific_port_result,
-						name, "start#stop#close");
+						name, "start#stop#close#reset");
 cmdline_parse_token_num_t cmd_operate_specific_port_id =
 	TOKEN_NUM_INITIALIZER(struct cmd_operate_specific_port_result,
 							value, UINT8);
@@ -967,7 +975,8 @@ cmdline_parse_token_num_t cmd_operate_specific_port_id =
 cmdline_parse_inst_t cmd_operate_specific_port = {
 	.f = cmd_operate_specific_port_parsed,
 	.data = NULL,
-	.help_str = "port start|stop|close <port_id>: Start/Stop/Close port_id",
+	.help_str = "port start|stop|close|reset <port_id>: Start/Stop/Close/"
+			"Reset port_id",
 	.tokens = {
 		(void *)&cmd_operate_specific_port_cmd,
 		(void *)&cmd_operate_specific_port_port,
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index bfb2f8e..12b7aaa 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -137,6 +137,7 @@ portid_t  nb_fwd_ports;  /**< Number of forwarding ports. */
 
 unsigned int fwd_lcores_cpuids[RTE_MAX_LCORE]; /**< CPU ids configuration. */
 portid_t fwd_ports_ids[RTE_MAX_ETHPORTS];      /**< Port ids configuration. */
+volatile char reset_ports[RTE_MAX_ETHPORTS] = {0};  /**< Portr reset falg. */
 
 struct fwd_stream **fwd_streams; /**< For each RX queue of each port. */
 streamid_t nb_fwd_streams;       /**< Is equal to (nb_ports * nb_rxq). */
@@ -601,6 +602,7 @@ init_config(void)
 	if (init_fwd_streams() < 0)
 		rte_exit(EXIT_FAILURE, "FAIL from init_fwd_streams()\n");
 
+
 	fwd_config_setup();
 }
 
@@ -1305,6 +1307,17 @@ port_is_closed(portid_t port_id)
 	return 1;
 }
 
+static void
+reset_event_callback(uint8_t port_id, enum rte_eth_event_type type, void *param)
+{
+	RTE_SET_USED(param);
+
+	printf("Event type: %s on port %d\n",
+		type == RTE_ETH_EVENT_INTR_RESET ? "RESET interrupt" :
+		"unknown event", port_id);
+	reset_ports[port_id] = 1;
+}
+
 int
 start_port(portid_t pid)
 {
@@ -1350,6 +1363,10 @@ start_port(portid_t pid)
 				return -1;
 			}
 		}
+
+		/* register reset interrupt callback */
+		rte_eth_dev_callback_register(pi, RTE_ETH_EVENT_INTR_RESET,
+			reset_event_callback, NULL);
 		if (port->need_reconfig_queues > 0) {
 			port->need_reconfig_queues = 0;
 			/* setup tx queues */
@@ -1559,6 +1576,56 @@ close_port(portid_t pid)
 }
 
 void
+reset_port(portid_t pid)
+{
+	portid_t pi;
+	struct rte_port *port;
+
+	if (port_id_is_invalid(pid, ENABLED_WARN))
+		return;
+
+	printf("Closing ports...\n");
+
+	FOREACH_PORT(pi, ports) {
+		if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
+			continue;
+
+		if (port_is_forwarding(pi) != 0 && test_done == 0) {
+			printf("Please remove port %d from forwarding "
+					"configuration.\n", pi);
+			continue;
+		}
+
+		if (port_is_bonding_slave(pi)) {
+			printf("Please remove port %d from "
+					"bonded device.\n", pi);
+			continue;
+		}
+
+		if (!reset_ports[pi]) {
+			printf("vf must get reset port %d info from "
+					"pf before reset.\n", pi);
+			continue;
+		}
+
+		port = &ports[pi];
+		if (rte_atomic16_cmpset(&(port->port_status),
+			RTE_PORT_STARTED, RTE_PORT_HANDLING) == 1) {
+			printf("Port %d is not started\n", pi);
+			continue;
+		}
+
+		reset_ports[pi] = 0;
+
+		if (rte_atomic16_cmpset(&(port->port_status),
+			RTE_PORT_HANDLING, RTE_PORT_STARTED) == 0)
+			printf("Port %d cannot be set to started\n", pi);
+	}
+
+	printf("Done\n");
+}
+
+void
 attach_port(char *identifier)
 {
 	portid_t pi = 0;
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 8cf2860..0c7e44c 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -586,6 +586,7 @@ int init_port_dcb_config(portid_t pid, enum dcb_mode_enable dcb_mode,
 int start_port(portid_t pid);
 void stop_port(portid_t pid);
 void close_port(portid_t pid);
+void reset_port(portid_t pid);
 void attach_port(char *identifier);
 void detach_port(uint8_t port_id);
 int all_ports_stopped(void);
-- 
2.9.3

Re: [PATCH 1/3] app/testpmd: add port reset command into testpmd

[Ferruh Yigit]

On 3/3/2017 4:56 AM, Wei Zhao wrote:
> Add vf port reset command into testpmd project, it is the interface for
> user to reset vf port.

I think it is better to change the order of this patch, first implement
new API in ethdev, later this patch implement new API in testpmd.

> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
>  app/test-pmd/cmdline.c | 17 ++++++++++---
>  app/test-pmd/testpmd.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  app/test-pmd/testpmd.h |  1 +
>  3 files changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 43fc636..59db672 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -596,6 +596,9 @@ static void cmd_help_long_parsed(void *parsed_result,
>  			"port close (port_id|all)\n"
>  			"    Close all ports or port_id.\n\n"
>  
> +			"port reset (port_id|all)\n"
> +			"    Reset all ports or port_id.\n\n"

It is not clear what reset does to the port. This is only for VF right?
Adding reset here hides that it is for VF.

<...>

> @@ -601,6 +602,7 @@ init_config(void)
>  	if (init_fwd_streams() < 0)
>  		rte_exit(EXIT_FAILURE, "FAIL from init_fwd_streams()\n");
>  
> +

This may be unintentional.

<...>

> @@ -1350,6 +1363,10 @@ start_port(portid_t pid)
>  				return -1;
>  			}
>  		}
> +
> +		/* register reset interrupt callback */
> +		rte_eth_dev_callback_register(pi, RTE_ETH_EVENT_INTR_RESET,
> +			reset_event_callback, NULL);

So each port started will register a callback to handle reset events,

1- isn't this overkill for the usecases that does not need this reset?
2- should there be an unregister event?
3- This issue can be fixed in testpmd, but for user application, is this
the suggested way?

>  		if (port->need_reconfig_queues > 0) {
>  			port->need_reconfig_queues = 0;
>  			/* setup tx queues */
> @@ -1559,6 +1576,56 @@ close_port(portid_t pid)
>  }
>  
>  void
> +reset_port(portid_t pid)
> +{
> +	portid_t pi;
> +	struct rte_port *port;
> +
> +	if (port_id_is_invalid(pid, ENABLED_WARN))
> +		return;
> +
> +	printf("Closing ports...\n");
> +
> +	FOREACH_PORT(pi, ports) {

Since we already know the port_id (pid), why iterating through all ports?

> +		if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
> +			continue;
> +
> +		if (port_is_forwarding(pi) != 0 && test_done == 0) {
> +			printf("Please remove port %d from forwarding "
> +					"configuration.\n", pi);
> +			continue;
> +		}
> +
> +		if (port_is_bonding_slave(pi)) {
> +			printf("Please remove port %d from "
> +					"bonded device.\n", pi);
> +			continue;
> +		}
> +
> +		if (!reset_ports[pi]) {
> +			printf("vf must get reset port %d info from "
> +					"pf before reset.\n", pi);
> +			continue;
> +		}

Can there be a timing issue here? Is it possible that reset occurred
already and we are in the middle of the callback function when this
check done?

<...>

Re: [PATCH 1/3] app/testpmd: add port reset command into testpmd

[Zhao1, Wei]

Hi, Ferruh 

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, March 8, 2017 7:07 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 1/3] app/testpmd: add port reset command
> into testpmd
> 
> On 3/3/2017 4:56 AM, Wei Zhao wrote:
> > Add vf port reset command into testpmd project, it is the interface
> > for user to reset vf port.
> 
> I think it is better to change the order of this patch, first implement new API
> in ethdev, later this patch implement new API in testpmd.
> 

You means change the order of patch 0001 and 0002 ?0003 remain the same?
Ok，I will do that in v2 patch
 
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > ---
> >  app/test-pmd/cmdline.c | 17 ++++++++++---  app/test-pmd/testpmd.c |
> > 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  app/test-pmd/testpmd.h |  1 +
> >  3 files changed, 81 insertions(+), 4 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 43fc636..59db672 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -596,6 +596,9 @@ static void cmd_help_long_parsed(void
> *parsed_result,
> >  			"port close (port_id|all)\n"
> >  			"    Close all ports or port_id.\n\n"
> >
> > +			"port reset (port_id|all)\n"
> > +			"    Reset all ports or port_id.\n\n"
> 
> It is not clear what reset does to the port. This is only for VF right?
> Adding reset here hides that it is for VF.

Yes, it is only for VF port reset, maybe I should change cmd line command to "port vf_reset  index"
that mode to avoid ambiguous for user in v2

> 
> <...>
> 
> > @@ -601,6 +602,7 @@ init_config(void)
> >  	if (init_fwd_streams() < 0)
> >  		rte_exit(EXIT_FAILURE, "FAIL from init_fwd_streams()\n");
> >
> > +
> 
> This may be unintentional.

Yes, I will fix it in v2 patch. 

> 
> <...>
> 
> > @@ -1350,6 +1363,10 @@ start_port(portid_t pid)
> >  				return -1;
> >  			}
> >  		}
> > +
> > +		/* register reset interrupt callback */
> > +		rte_eth_dev_callback_register(pi,
> RTE_ETH_EVENT_INTR_RESET,
> > +			reset_event_callback, NULL);
> 
> So each port started will register a callback to handle reset events,
> 
> 1- isn't this overkill for the usecases that does not need this reset?
> 2- should there be an unregister event?
> 3- This issue can be fixed in testpmd, but for user application, is this the
> suggested way?

1. it is hard to know which port may be need to reset in the feature at the beginning stage, so there is  register event for all ports.
2. this is only a demo for user application. I think in user application, there should be a specific thread to check whether reset_ports is set by 
 I40e function _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_RESET, NULL),  so  RTE_ETH_EVENT_INTR_RESET must be registered.
 IF true, then user application should call function rte_eth_dev_reset(pi)
3. . this is only a demo for user application, user application can create a new specific thread to do this work , but testpmd app can not because that may be introduce 
   A lot of unknow problem for existing feature.

> 
> >  		if (port->need_reconfig_queues > 0) {
> >  			port->need_reconfig_queues = 0;
> >  			/* setup tx queues */
> > @@ -1559,6 +1576,56 @@ close_port(portid_t pid)  }
> >
> >  void
> > +reset_port(portid_t pid)
> > +{
> > +	portid_t pi;
> > +	struct rte_port *port;
> > +
> > +	if (port_id_is_invalid(pid, ENABLED_WARN))
> > +		return;
> > +
> > +	printf("Closing ports...\n");
> > +
> > +	FOREACH_PORT(pi, ports) {
> 
> Since we already know the port_id (pid), why iterating through all ports?

This is usefully, because when user command is “port reset all”, the port_id is 0xffffffff
If all of these ports are vf, then it will need to retrieve all port.

> 
> > +		if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
> > +			continue;
> > +
> > +		if (port_is_forwarding(pi) != 0 && test_done == 0) {
> > +			printf("Please remove port %d from forwarding "
> > +					"configuration.\n", pi);
> > +			continue;
> > +		}
> > +
> > +		if (port_is_bonding_slave(pi)) {
> > +			printf("Please remove port %d from "
> > +					"bonded device.\n", pi);
> > +			continue;
> > +		}
> > +
> > +		if (!reset_ports[pi]) {
> > +			printf("vf must get reset port %d info from "
> > +					"pf before reset.\n", pi);
> > +			continue;
> > +		}
> 
> Can there be a timing issue here? Is it possible that reset occurred already
> and we are in the middle of the callback function when this check done?
Process:
1. pf reset, and pass message to vf.
2. registered callback function of RTE_ETH_EVENT_INTR_RESET event set flag of  reset_ports[pi], and give a printf message of port index
3. then user use command of testpmd cli " port reset index" to reset vf, and reset port and set flag to 0.

So, I do not find any timing issue by now.

> 
> <...>

Re: [PATCH 1/3] app/testpmd: add port reset command into testpmd

[Zhao1, Wei]

Hi, Ferruh

> -----Original Message-----
> From: Zhao1, Wei
> Sent: Tuesday, March 14, 2017 3:58 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH 1/3] app/testpmd: add port reset command
> into testpmd
> 
> Hi, Ferruh
> 
> > -----Original Message-----
> > From: Yigit, Ferruh
> > Sent: Wednesday, March 8, 2017 7:07 PM
> > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/3] app/testpmd: add port reset
> > command into testpmd
> >
> > On 3/3/2017 4:56 AM, Wei Zhao wrote:
> > > Add vf port reset command into testpmd project, it is the interface
> > > for user to reset vf port.
> >
> > I think it is better to change the order of this patch, first
> > implement new API in ethdev, later this patch implement new API in
> testpmd.
> >
> 
> You means change the order of patch 0001 and 0002 ?0003 remain the same?
> Ok，I will do that in v2 patch
> 
> > >
> > > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > > ---
> > >  app/test-pmd/cmdline.c | 17 ++++++++++---  app/test-pmd/testpmd.c |
> > > 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  app/test-pmd/testpmd.h |  1 +
> > >  3 files changed, 81 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > 43fc636..59db672 100644
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c
> > > @@ -596,6 +596,9 @@ static void cmd_help_long_parsed(void
> > *parsed_result,
> > >  			"port close (port_id|all)\n"
> > >  			"    Close all ports or port_id.\n\n"
> > >
> > > +			"port reset (port_id|all)\n"
> > > +			"    Reset all ports or port_id.\n\n"
> >
> > It is not clear what reset does to the port. This is only for VF right?
> > Adding reset here hides that it is for VF.
> 
> Yes, it is only for VF port reset, maybe I should change cmd line command to
> "port vf_reset  index"
> that mode to avoid ambiguous for user in v2

I should apologize about this. This rte layer of reset command is used for both vf and pf port reset.
This feature is related to vf, but pf port reset cmd line can also use it. There is mistake when I explain to you last time.   

> 
> >
> > <...>
> >
> > > @@ -601,6 +602,7 @@ init_config(void)
> > >  	if (init_fwd_streams() < 0)
> > >  		rte_exit(EXIT_FAILURE, "FAIL from init_fwd_streams()\n");
> > >
> > > +
> >
> > This may be unintentional.
> 
> Yes, I will fix it in v2 patch.
> 
> >
> > <...>
> >
> > > @@ -1350,6 +1363,10 @@ start_port(portid_t pid)
> > >  				return -1;
> > >  			}
> > >  		}
> > > +
> > > +		/* register reset interrupt callback */
> > > +		rte_eth_dev_callback_register(pi,
> > RTE_ETH_EVENT_INTR_RESET,
> > > +			reset_event_callback, NULL);
> >
> > So each port started will register a callback to handle reset events,
> >
> > 1- isn't this overkill for the usecases that does not need this reset?
> > 2- should there be an unregister event?
> > 3- This issue can be fixed in testpmd, but for user application, is
> > this the suggested way?
> 
> 1. it is hard to know which port may be need to reset in the feature at the
> beginning stage, so there is  register event for all ports.
> 2. this is only a demo for user application. I think in user application, there
> should be a specific thread to check whether reset_ports is set by  I40e
> function _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_RESET,
> NULL),  so  RTE_ETH_EVENT_INTR_RESET must be registered.
>  IF true, then user application should call function rte_eth_dev_reset(pi) 3. .
> this is only a demo for user application, user application can create a new
> specific thread to do this work , but testpmd app can not because that may
> be introduce
>    A lot of unknow problem for existing feature.
> 
> >
> > >  		if (port->need_reconfig_queues > 0) {
> > >  			port->need_reconfig_queues = 0;
> > >  			/* setup tx queues */
> > > @@ -1559,6 +1576,56 @@ close_port(portid_t pid)  }
> > >
> > >  void
> > > +reset_port(portid_t pid)
> > > +{
> > > +	portid_t pi;
> > > +	struct rte_port *port;
> > > +
> > > +	if (port_id_is_invalid(pid, ENABLED_WARN))
> > > +		return;
> > > +
> > > +	printf("Closing ports...\n");
> > > +
> > > +	FOREACH_PORT(pi, ports) {
> >
> > Since we already know the port_id (pid), why iterating through all ports?
> 
> This is usefully, because when user command is “port reset all”, the port_id is
> 0xffffffff If all of these ports are vf, then it will need to retrieve all port.
> 
> >
> > > +		if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
> > > +			continue;
> > > +
> > > +		if (port_is_forwarding(pi) != 0 && test_done == 0) {
> > > +			printf("Please remove port %d from forwarding "
> > > +					"configuration.\n", pi);
> > > +			continue;
> > > +		}
> > > +
> > > +		if (port_is_bonding_slave(pi)) {
> > > +			printf("Please remove port %d from "
> > > +					"bonded device.\n", pi);
> > > +			continue;
> > > +		}
> > > +
> > > +		if (!reset_ports[pi]) {
> > > +			printf("vf must get reset port %d info from "
> > > +					"pf before reset.\n", pi);
> > > +			continue;
> > > +		}
> >
> > Can there be a timing issue here? Is it possible that reset occurred
> > already and we are in the middle of the callback function when this check
> done?
> Process:
> 1. pf reset, and pass message to vf.
> 2. registered callback function of RTE_ETH_EVENT_INTR_RESET event set flag
> of  reset_ports[pi], and give a printf message of port index 3. then user use
> command of testpmd cli " port reset index" to reset vf, and reset port and
> set flag to 0.
> 
> So, I do not find any timing issue by now.
> 
> >
> > <...>

[PATCH 2/3] lib/librte_ether: add support for port reset

[Wei Zhao]

Add support for vf port reset in rte layer.
Add an API to restart the device.
It's for VF device in this scenario, kernel PF + DPDK VF.
When the PF port down->up, APP should call this API to
restart VF port. Most likely, APP should call it in its
management thread and guarantee the thread safe. It means
APP should stop the rx/tx and the device, then restart
the device, then recover the device and rx/tx.
Also, it's APP's responsibilty to release the occupied
memory.

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 app/test-pmd/testpmd.c                 |  1 +
 lib/librte_ether/rte_ethdev.c          | 17 +++++++++++++++++
 lib/librte_ether/rte_ethdev.h          | 25 +++++++++++++++++++++++++
 lib/librte_ether/rte_ether_version.map |  6 ++++++
 4 files changed, 49 insertions(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 12b7aaa..44fcc51 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1615,6 +1615,7 @@ reset_port(portid_t pid)
 			continue;
 		}
 
+		rte_eth_dev_reset(pi);
 		reset_ports[pi] = 0;
 
 		if (rte_atomic16_cmpset(&(port->port_status),
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index eb0a94a..2e06dca 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -3273,3 +3273,20 @@ rte_eth_dev_l2_tunnel_offload_set(uint8_t port_id,
 				-ENOTSUP);
 	return (*dev->dev_ops->l2_tunnel_offload_set)(dev, l2_tunnel, mask, en);
 }
+
+int
+rte_eth_dev_reset(uint8_t port_id)
+{
+	struct rte_eth_dev *dev;
+	int diag;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	dev = &rte_eth_devices[port_id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_reset, -ENOTSUP);
+
+	diag = (*dev->dev_ops->dev_reset)(dev);
+
+	return diag;
+}
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 97f3e2d..77a5b79 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1367,6 +1367,9 @@ typedef int (*eth_l2_tunnel_offload_set_t)
 	 uint8_t en);
 /**< @internal enable/disable the l2 tunnel offload functions */
 
+typedef int  (*eth_dev_reset_t)(struct rte_eth_dev *dev);
+/**< @internal Function used to reset a configured Ethernet device. */
+
 #ifdef RTE_NIC_BYPASS
 
 enum {
@@ -1508,6 +1511,9 @@ struct eth_dev_ops {
 	eth_l2_tunnel_offload_set_t   l2_tunnel_offload_set;
 	/** Enable/disable l2 tunnel offload functions. */
 
+	/** Reset device. */
+	eth_dev_reset_t dev_reset;
+
 	eth_set_queue_rate_limit_t set_queue_rate_limit; /**< Set queue rate limit. */
 
 	rss_hash_update_t          rss_hash_update; /** Configure RSS hash protocols. */
@@ -4407,6 +4413,25 @@ int
 rte_eth_dev_get_name_by_port(uint8_t port_id, char *name);
 
 /**
+ * Reset an ethernet device when it's not working. One scenario is, after PF
+ * port is down and up, the related VF port should be reset.
+ * The API will stop the port, clear the rx/tx queues, re-setup the rx/tx
+ * queues, restart the port.
+ * Before calling this API, APP should stop the rx/tx. When tx is being stopped,
+ * APP can drop the packets and release the buffer instead of sending them.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-ENODEV) if port identifier is invalid.
+ *   - (-ENOTSUP) if hardware doesn't support this function.
+ */
+int
+rte_eth_dev_reset(uint8_t port_id);
+
+/**
  * @internal
  * Wrapper for use by pci drivers as a .probe function to attach to a ethdev
  * interface.
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index c6c9d0d..529b27f 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -154,3 +154,9 @@ DPDK_17.02 {
 	rte_flow_validate;
 
 } DPDK_16.11;
+
+DPDK_17.05 {
+	global:
+
+	rte_eth_dev_reset;
+} DPDK_17.02;
\ No newline at end of file
-- 
2.9.3

[PATCH 3/3] net/i40e: implement device reset on port

[Wei Zhao]

Implement the device reset function on vf port.
This restart function will detach device then
attach device, reconfigure dev, re-setup the Rx/Tx queues.

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c    |   2 +-
 drivers/net/i40e/i40e_ethdev.h    |  16 +++
 drivers/net/i40e/i40e_ethdev_vf.c | 222 ++++++++++++++++++++++++++++++++++++--
 drivers/net/i40e/i40e_rxtx.c      |  10 ++
 drivers/net/i40e/i40e_rxtx.h      |   4 +
 5 files changed, 246 insertions(+), 8 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 508fcc8..bc3ba22 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -6006,7 +6006,7 @@ i40e_find_vlan_filter(struct i40e_vsi *vsi,
 		return 0;
 }
 
-static void
+void
 i40e_store_vlan_filter(struct i40e_vsi *vsi,
 		       uint16_t vlan_id, bool on)
 {
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index f545850..314fad7 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -651,6 +651,16 @@ struct i40e_vf_tx_queues {
 	uint32_t tx_ring_len;
 };
 
+struct i40e_vf_reset_store {
+	struct ether_addr *mac_addrs;  /* Device Ethernet Link address. */
+	bool promisc_unicast_enabled;
+	bool promisc_multicast_enabled;
+	uint16_t vlan_num;    /* Total VLAN number */
+	uint32_t vfta[I40E_VFTA_SIZE];        /* VLAN bitmap */
+	uint16_t mac_num;        /* Total mac number */
+};
+
+
 /*
  * Structure to store private data specific for VF instance.
  */
@@ -708,6 +718,10 @@ struct i40e_adapter {
 	struct rte_timecounter systime_tc;
 	struct rte_timecounter rx_tstamp_tc;
 	struct rte_timecounter tx_tstamp_tc;
+
+	/* For VF reset */
+	volatile uint8_t reset_number;
+	void *reset_store_data;
 };
 
 extern const struct rte_flow_ops i40e_flow_ops;
@@ -748,6 +762,8 @@ int i40e_dev_link_update(struct rte_eth_dev *dev,
 			 __rte_unused int wait_to_complete);
 void i40e_vsi_queues_bind_intr(struct i40e_vsi *vsi);
 void i40e_vsi_queues_unbind_intr(struct i40e_vsi *vsi);
+void i40e_store_vlan_filter(struct i40e_vsi *vsi,
+		       uint16_t vlan_id, bool on);
 int i40e_vsi_vlan_pvid_set(struct i40e_vsi *vsi,
 			   struct i40e_vsi_vlan_pvid_info *info);
 int i40e_vsi_config_vlan_stripping(struct i40e_vsi *vsi, bool on);
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 55fd344..b30f09f 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -161,6 +161,13 @@ i40evf_dev_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t queue_id);
 static void i40evf_handle_pf_event(__rte_unused struct rte_eth_dev *dev,
 				   uint8_t *msg,
 				   uint16_t msglen);
+static int i40evf_dev_uninit(struct rte_eth_dev *eth_dev);
+static int i40evf_dev_init(struct rte_eth_dev *eth_dev);
+static void i40evf_dev_close(struct rte_eth_dev *dev);
+static int i40evf_dev_start(struct rte_eth_dev *dev);
+static int i40evf_dev_configure(struct rte_eth_dev *dev);
+static int i40evf_handle_vf_reset(struct rte_eth_dev *dev);
+
 
 /* Default hash key buffer for RSS */
 static uint32_t rss_key_default[I40E_VFQF_HKEY_MAX_INDEX + 1];
@@ -230,6 +237,7 @@ static const struct eth_dev_ops i40evf_eth_dev_ops = {
 	.rss_hash_conf_get    = i40evf_dev_rss_hash_conf_get,
 	.mtu_set              = i40evf_dev_mtu_set,
 	.mac_addr_set         = i40evf_set_default_mac_addr,
+	.dev_reset            = i40evf_handle_vf_reset,
 };
 
 /*
@@ -885,10 +893,13 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = I40E_AQ_BUF_SZ;
 	err = i40evf_execute_vf_cmd(dev, &args);
-	if (err)
+	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command "
 			    "OP_ADD_ETHER_ADDRESS");
-
+		goto DONE;
+	}
+	vf->vsi.mac_num++;
+DONE:
 	return;
 }
 
@@ -923,9 +934,13 @@ i40evf_del_mac_addr_by_addr(struct rte_eth_dev *dev,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = I40E_AQ_BUF_SZ;
 	err = i40evf_execute_vf_cmd(dev, &args);
-	if (err)
+	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command "
 			    "OP_DEL_ETHER_ADDRESS");
+		goto DONE;
+	}
+	vf->vsi.mac_num--;
+DONE:
 	return;
 }
 
@@ -1047,6 +1062,7 @@ static int
 i40evf_add_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
 {
 	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
+	struct i40e_vsi *vsi = &vf->vsi;
 	struct i40e_virtchnl_vlan_filter_list *vlan_list;
 	uint8_t cmd_buffer[sizeof(struct i40e_virtchnl_vlan_filter_list) +
 							sizeof(uint16_t)];
@@ -1064,9 +1080,13 @@ i40evf_add_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = I40E_AQ_BUF_SZ;
 	err = i40evf_execute_vf_cmd(dev, &args);
-	if (err)
+	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_VLAN");
-
+		goto DONE;
+	}
+	i40e_store_vlan_filter(vsi, vlanid, 1);
+	vsi->vlan_num++;
+DONE:
 	return err;
 }
 
@@ -1074,6 +1094,7 @@ static int
 i40evf_del_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
 {
 	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
+	struct i40e_vsi *vsi = &vf->vsi;
 	struct i40e_virtchnl_vlan_filter_list *vlan_list;
 	uint8_t cmd_buffer[sizeof(struct i40e_virtchnl_vlan_filter_list) +
 							sizeof(uint16_t)];
@@ -1091,9 +1112,13 @@ i40evf_del_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = I40E_AQ_BUF_SZ;
 	err = i40evf_execute_vf_cmd(dev, &args);
-	if (err)
+	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_DEL_VLAN");
-
+		goto DONE;
+	}
+	i40e_store_vlan_filter(vsi, vlanid, 0);
+	vsi->vlan_num--;
+DONE:
 	return err;
 }
 
@@ -2716,3 +2741,186 @@ i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
 
 	i40evf_add_mac_addr(dev, mac_addr, 0, 0);
 }
+
+static void
+i40evf_restore_vlan_filter(struct rte_eth_dev *dev,
+			uint32_t *vfta)
+{
+	uint32_t vid_idx, vid_bit;
+	uint16_t vlan_id;
+
+	for  (vid_idx = 0; vid_idx < I40E_VFTA_SIZE; vid_idx++) {
+		for  (vid_bit = 0; vid_bit < I40E_UINT32_BIT_SIZE; vid_bit++) {
+			if (vfta[vid_idx] & (1 << vid_bit)) {
+				vlan_id = (vid_idx << 5) + vid_bit;
+				i40evf_add_vlan(dev, vlan_id);
+			}
+		}
+	}
+}
+
+static void
+i40evf_restore_macaddr(struct rte_eth_dev *dev,
+		struct ether_addr *mac_addrs)
+{
+	struct ether_addr *addr;
+	uint16_t i;
+
+	/* replay MAC address configuration including default MAC */
+	addr = &mac_addrs[0];
+
+	memcpy(dev->data->mac_addrs, mac_addrs,
+			ETHER_ADDR_LEN * I40E_NUM_MACADDR_MAX);
+
+	i40evf_set_default_mac_addr(dev, addr);
+
+	for (i = 1; i < I40E_NUM_MACADDR_MAX; i++) {
+		addr = &mac_addrs[i];
+
+		/* skip zero address */
+		if (is_zero_ether_addr(addr))
+			continue;
+
+		i40evf_add_mac_addr(dev, addr, 0, 0);
+	}
+}
+
+
+static void
+i40e_vf_queue_reset(struct rte_eth_dev *dev)
+{
+	uint16_t i;
+
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		struct i40e_rx_queue *rxq = dev->data->rx_queues[i];
+
+		if (rxq->q_set) {
+			i40e_dev_rx_queue_setup(dev,
+						rxq->queue_id,
+						rxq->nb_rx_desc,
+						rxq->socket_id,
+						&rxq->rxconf,
+						rxq->mp);
+		}
+	}
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		struct i40e_tx_queue *txq = dev->data->tx_queues[i];
+
+		if (txq->q_set) {
+			i40e_dev_tx_queue_setup(dev,
+						txq->queue_id,
+						txq->nb_tx_desc,
+						txq->socket_id,
+						&txq->txconf);
+		}
+	}
+}
+
+static int
+i40evf_store_before_reset(struct rte_eth_dev *dev)
+{
+	struct i40e_adapter *adapter =
+		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
+	struct i40e_vf_reset_store *store_data;
+	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
+
+	adapter->reset_store_data = rte_zmalloc("i40evf_store_reset",
+					sizeof(struct i40e_vf_reset_store), 0);
+	if (adapter->reset_store_data == NULL) {
+		PMD_INIT_LOG(ERR, "Failed to allocate %ld bytes needed to"
+				" to store data when reset vf",
+				sizeof(struct i40e_vf_reset_store));
+		return -ENOMEM;
+	}
+	store_data =
+		(struct i40e_vf_reset_store *)adapter->reset_store_data;
+	store_data->mac_addrs = rte_zmalloc("i40evf_mac_store_reset",
+			ETHER_ADDR_LEN * I40E_NUM_MACADDR_MAX, 0);
+	if (store_data->mac_addrs == NULL) {
+		PMD_INIT_LOG(ERR, "Failed to allocate %d bytes needed to"
+				" to store MAC addresses when reset vf",
+				ETHER_ADDR_LEN * I40E_NUM_MACADDR_MAX);
+	}
+
+	memcpy(store_data->mac_addrs, dev->data->mac_addrs,
+			ETHER_ADDR_LEN * I40E_NUM_MACADDR_MAX);
+
+	store_data->promisc_unicast_enabled = vf->promisc_unicast_enabled;
+	store_data->promisc_multicast_enabled = vf->promisc_multicast_enabled;
+
+	store_data->vlan_num = vf->vsi.vlan_num;
+	memcpy(store_data->vfta, vf->vsi.vfta,
+			sizeof(uint32_t) * I40E_VFTA_SIZE);
+
+	store_data->mac_num = vf->vsi.mac_num;
+
+	return 0;
+}
+
+static void
+i40evf_restore_after_reset(struct rte_eth_dev *dev)
+{
+	struct i40e_adapter *adapter =
+		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
+	struct i40e_vf_reset_store *store_data =
+		(struct i40e_vf_reset_store *)adapter->reset_store_data;
+
+	if (store_data->promisc_unicast_enabled)
+		i40evf_dev_promiscuous_enable(dev);
+	if (store_data->promisc_multicast_enabled)
+		i40evf_dev_allmulticast_enable(dev);
+
+	if (store_data->vlan_num)
+		i40evf_restore_vlan_filter(dev, store_data->vfta);
+
+	if (store_data->mac_num)
+		i40evf_restore_macaddr(dev, store_data->mac_addrs);
+}
+
+static void
+i40e_vf_reset_dev(struct rte_eth_dev *dev)
+{
+	struct i40e_adapter *adapter =
+		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
+
+	i40evf_store_before_reset(dev);
+
+	i40evf_dev_close(dev);
+	PMD_DRV_LOG(DEBUG, "i40evf dev close complete");
+
+	i40evf_dev_uninit(dev);
+	PMD_DRV_LOG(DEBUG, "i40evf dev detached");
+
+	memset(dev->data->dev_private, 0,
+	       (uint64_t)&adapter->reset_number - (uint64_t)adapter);
+
+	i40evf_dev_init(dev);
+	PMD_DRV_LOG(DEBUG, "i40evf dev attached");
+
+	i40evf_dev_configure(dev);
+
+	i40e_vf_queue_reset(dev);
+	PMD_DRV_LOG(DEBUG, "i40evf queue reset");
+
+	i40evf_restore_after_reset(dev);
+
+	i40evf_dev_start(dev);
+	PMD_DRV_LOG(DEBUG, "i40evf dev restart");
+}
+
+static int
+i40evf_handle_vf_reset(struct rte_eth_dev *dev)
+{
+	struct i40e_adapter *adapter =
+		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
+
+	if (!dev->data->dev_started)
+		return 0;
+
+	adapter->reset_number = 1;
+	i40e_vf_reset_dev(dev);
+	adapter->reset_number = 0;
+
+	return 0;
+}
+
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 65a702f..4d30a79 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1709,6 +1709,7 @@ i40e_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	uint16_t len, i;
 	uint16_t base, bsf, tc_mapping;
 	int use_def_burst_func = 1;
+	struct rte_eth_rxconf conf = *rx_conf;
 
 	if (hw->mac.type == I40E_MAC_VF || hw->mac.type == I40E_MAC_X722_VF) {
 		struct i40e_vf *vf =
@@ -1748,6 +1749,8 @@ i40e_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 	rxq->mp = mp;
 	rxq->nb_rx_desc = nb_desc;
+	rxq->socket_id = socket_id;
+	rxq->rxconf = conf;
 	rxq->rx_free_thresh = rx_conf->rx_free_thresh;
 	rxq->queue_id = queue_idx;
 	if (hw->mac.type == I40E_MAC_VF || hw->mac.type == I40E_MAC_X722_VF)
@@ -1937,6 +1940,7 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev *dev,
 	uint32_t ring_size;
 	uint16_t tx_rs_thresh, tx_free_thresh;
 	uint16_t i, base, bsf, tc_mapping;
+	struct rte_eth_txconf conf = *tx_conf;
 
 	if (hw->mac.type == I40E_MAC_VF || hw->mac.type == I40E_MAC_X722_VF) {
 		struct i40e_vf *vf =
@@ -2059,6 +2063,8 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev *dev,
 	}
 
 	txq->nb_tx_desc = nb_desc;
+	txq->socket_id = socket_id;
+	txq->txconf = conf;
 	txq->tx_rs_thresh = tx_rs_thresh;
 	txq->tx_free_thresh = tx_free_thresh;
 	txq->pthresh = tx_conf->tx_thresh.pthresh;
@@ -2525,8 +2531,12 @@ void
 i40e_dev_free_queues(struct rte_eth_dev *dev)
 {
 	uint16_t i;
+	struct i40e_adapter *adapter =
+		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 
 	PMD_INIT_FUNC_TRACE();
+	if (adapter->reset_number)
+		return;
 
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		if (!dev->data->rx_queues[i])
diff --git a/drivers/net/i40e/i40e_rxtx.h b/drivers/net/i40e/i40e_rxtx.h
index a87bdb0..0e3cc19 100644
--- a/drivers/net/i40e/i40e_rxtx.h
+++ b/drivers/net/i40e/i40e_rxtx.h
@@ -136,6 +136,8 @@ struct i40e_rx_queue {
 	bool rx_deferred_start; /**< don't start this queue in dev start */
 	uint16_t rx_using_sse; /**<flag indicate the usage of vPMD for rx */
 	uint8_t dcb_tc;         /**< Traffic class of rx queue */
+	uint8_t socket_id;
+	struct rte_eth_rxconf rxconf;
 };
 
 struct i40e_tx_entry {
@@ -177,6 +179,8 @@ struct i40e_tx_queue {
 	bool q_set; /**< indicate if tx queue has been configured */
 	bool tx_deferred_start; /**< don't start this queue in dev start */
 	uint8_t dcb_tc;         /**< Traffic class of tx queue */
+	uint8_t socket_id;
+	struct rte_eth_txconf txconf;
 };
 
 /** Offload features */
-- 
2.9.3

Re: [PATCH 3/3] net/i40e: implement device reset on port

[Ferruh Yigit]

On 3/3/2017 4:56 AM, Wei Zhao wrote:
> Implement the device reset function on vf port.
> This restart function will detach device then
> attach device, reconfigure dev, re-setup the Rx/Tx queues.
> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

<...>

> +static int i40evf_dev_uninit(struct rte_eth_dev *eth_dev);
> +static int i40evf_dev_init(struct rte_eth_dev *eth_dev);
> +static void i40evf_dev_close(struct rte_eth_dev *dev);
> +static int i40evf_dev_start(struct rte_eth_dev *dev);
> +static int i40evf_dev_configure(struct rte_eth_dev *dev);
> +static int i40evf_handle_vf_reset(struct rte_eth_dev *dev);

Some of them already seems declared, please avoid unnecessary or
duplicate declarations.

> +
>  
>  /* Default hash key buffer for RSS */
>  static uint32_t rss_key_default[I40E_VFQF_HKEY_MAX_INDEX + 1];
> @@ -230,6 +237,7 @@ static const struct eth_dev_ops i40evf_eth_dev_ops = {
>  	.rss_hash_conf_get    = i40evf_dev_rss_hash_conf_get,
>  	.mtu_set              = i40evf_dev_mtu_set,
>  	.mac_addr_set         = i40evf_set_default_mac_addr,
> +	.dev_reset            = i40evf_handle_vf_reset,
>  };
>  
>  /*
> @@ -885,10 +893,13 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
>  	args.out_buffer = vf->aq_resp;
>  	args.out_size = I40E_AQ_BUF_SZ;
>  	err = i40evf_execute_vf_cmd(dev, &args);
> -	if (err)
> +	if (err) {
>  		PMD_DRV_LOG(ERR, "fail to execute command "
>  			    "OP_ADD_ETHER_ADDRESS");
> -
> +		goto DONE;

Please prefer lowercase labels,
also this is error exit, I would prefer other name than "done"

> +	}
> +	vf->vsi.mac_num++;
> +DONE:
>  	return;
>  }
>  

<...>

> +static int
> +i40evf_handle_vf_reset(struct rte_eth_dev *dev)
> +{
> +	struct i40e_adapter *adapter =
> +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +
> +	if (!dev->data->dev_started)
> +		return 0;
> +
> +	adapter->reset_number = 1;
> +	i40e_vf_reset_dev(dev);

What happens if user called this function for PF ?

> +	adapter->reset_number = 0;
> +
> +	return 0;
> +}
> +

<...>

Re: [PATCH 3/3] net/i40e: implement device reset on port

[Zhao1, Wei]

Hi，Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, March 8, 2017 7:21 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 3/3] net/i40e: implement device reset on
> port
> 
> On 3/3/2017 4:56 AM, Wei Zhao wrote:
> > Implement the device reset function on vf port.
> > This restart function will detach device then attach device,
> > reconfigure dev, re-setup the Rx/Tx queues.
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> 
> <...>
> 
> > +static int i40evf_dev_uninit(struct rte_eth_dev *eth_dev); static int
> > +i40evf_dev_init(struct rte_eth_dev *eth_dev); static void
> > +i40evf_dev_close(struct rte_eth_dev *dev); static int
> > +i40evf_dev_start(struct rte_eth_dev *dev); static int
> > +i40evf_dev_configure(struct rte_eth_dev *dev); static int
> > +i40evf_handle_vf_reset(struct rte_eth_dev *dev);
> 
> Some of them already seems declared, please avoid unnecessary or
> duplicate declarations.

Ok, I will delete useless function declaration in V2 patch.

> 
> > +
> >
> >  /* Default hash key buffer for RSS */  static uint32_t
> > rss_key_default[I40E_VFQF_HKEY_MAX_INDEX + 1]; @@ -230,6 +237,7
> @@
> > static const struct eth_dev_ops i40evf_eth_dev_ops = {
> >  	.rss_hash_conf_get    = i40evf_dev_rss_hash_conf_get,
> >  	.mtu_set              = i40evf_dev_mtu_set,
> >  	.mac_addr_set         = i40evf_set_default_mac_addr,
> > +	.dev_reset            = i40evf_handle_vf_reset,
> >  };
> >
> >  /*
> > @@ -885,10 +893,13 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
> >  	args.out_buffer = vf->aq_resp;
> >  	args.out_size = I40E_AQ_BUF_SZ;
> >  	err = i40evf_execute_vf_cmd(dev, &args);
> > -	if (err)
> > +	if (err) {
> >  		PMD_DRV_LOG(ERR, "fail to execute command "
> >  			    "OP_ADD_ETHER_ADDRESS");
> > -
> > +		goto DONE;
> 
> Please prefer lowercase labels,
> also this is error exit, I would prefer other name than "done"

Ok, I will delete useless function declaration in V2 patch.

> 
> > +	}
> > +	vf->vsi.mac_num++;
> > +DONE:
> >  	return;
> >  }
> >
> 
> <...>
> 
> > +static int
> > +i40evf_handle_vf_reset(struct rte_eth_dev *dev) {
> > +	struct i40e_adapter *adapter =
> > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > +
> > +	if (!dev->data->dev_started)
> > +		return 0;
> > +
> > +	adapter->reset_number = 1;
> > +	i40e_vf_reset_dev(dev);
> 
> What happens if user called this function for PF ?

This is an illegal operation, so I will may be add an check of whether it is vf  port,
if not , a return command to avoid  illegal operation.

> 
> > +	adapter->reset_number = 0;
> > +
> > +	return 0;
> > +}
> > +
> 
> <...>

Re: [PATCH 3/3] net/i40e: implement device reset on port

[Wu, Jingjing]

> >
> > > +static int
> > > +i40evf_handle_vf_reset(struct rte_eth_dev *dev) {
> > > +	struct i40e_adapter *adapter =
> > > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > > +
> > > +	if (!dev->data->dev_started)
> > > +		return 0;
> > > +
> > > +	adapter->reset_number = 1;
> > > +	i40e_vf_reset_dev(dev);
> >
> > What happens if user called this function for PF ?
> 
> This is an illegal operation, so I will may be add an check of whether it is vf
> port, if not , a return command to avoid  illegal operation.
>

VF driver is probed when it is the VF (by device id). So I don't think it
Will be called when it is PF.


Thanks
Jingjing

Re: [PATCH 3/3] net/i40e: implement device reset on port

[Wu, Jingjing]

>  /*
>   * Structure to store private data specific for VF instance.
>   */
> @@ -708,6 +718,10 @@ struct i40e_adapter {
>  	struct rte_timecounter systime_tc;
>  	struct rte_timecounter rx_tstamp_tc;
>  	struct rte_timecounter tx_tstamp_tc;
> +
> +	/* For VF reset */
> +	volatile uint8_t reset_number;
> +	void *reset_store_data;
>  };

How to move it to i40_vf? Or it can be used for PF in future?


> +
> +static int
> +i40evf_store_before_reset(struct rte_eth_dev *dev) {
> +	struct i40e_adapter *adapter =
> +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +	struct i40e_vf_reset_store *store_data;
> +	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-
> >dev_private);
> +
> +	adapter->reset_store_data = rte_zmalloc("i40evf_store_reset",
> +					sizeof(struct i40e_vf_reset_store), 0);
> +	if (adapter->reset_store_data == NULL) {
> +		PMD_INIT_LOG(ERR, "Failed to allocate %ld bytes needed to"
> +				" to store data when reset vf",
> +				sizeof(struct i40e_vf_reset_store));
> +		return -ENOMEM;
> +	}
i40evf_store_before_reset is allocated when reset happens.
Don't forget to free it when reset is done. Otherwise memory leak.

The same as below mac_addrs.
> +	store_data =
> +		(struct i40e_vf_reset_store *)adapter->reset_store_data;
> +	store_data->mac_addrs = rte_zmalloc("i40evf_mac_store_reset",
> +			ETHER_ADDR_LEN * I40E_NUM_MACADDR_MAX, 0);
> +	if (store_data->mac_addrs == NULL) {
> +		PMD_INIT_LOG(ERR, "Failed to allocate %d bytes needed to"
> +				" to store MAC addresses when reset vf",
> +				ETHER_ADDR_LEN *
> I40E_NUM_MACADDR_MAX);
> +	}
> +
> +	memcpy(store_data->mac_addrs, dev->data->mac_addrs,
> +			ETHER_ADDR_LEN * I40E_NUM_MACADDR_MAX);
> +
> +	store_data->promisc_unicast_enabled = vf->promisc_unicast_enabled;
> +	store_data->promisc_multicast_enabled = vf-
> >promisc_multicast_enabled;
> +
> +	store_data->vlan_num = vf->vsi.vlan_num;
> +	memcpy(store_data->vfta, vf->vsi.vfta,
> +			sizeof(uint32_t) * I40E_VFTA_SIZE);
> +
> +	store_data->mac_num = vf->vsi.mac_num;
> +
> +	return 0;
> +}



> +static int
> +i40evf_handle_vf_reset(struct rte_eth_dev *dev) {
> +	struct i40e_adapter *adapter =
> +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +
> +	if (!dev->data->dev_started)
> +		return 0;
> +
I think even the dev is not started, we also need to reset it. Because
the VF need to reconnect with PF.
> +	adapter->reset_number = 1;
> +	i40e_vf_reset_dev(dev);
> +	adapter->reset_number = 0;
> +
How to name the reset_number to resetting?

Re: [PATCH 3/3] net/i40e: implement device reset on port

[Zhao1, Wei]

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Friday, March 24, 2017 5:07 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Zhao1, Wei <wei.zhao1@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH 3/3] net/i40e: implement device reset on
> port
> 
> 
> >  /*
> >   * Structure to store private data specific for VF instance.
> >   */
> > @@ -708,6 +718,10 @@ struct i40e_adapter {
> >  	struct rte_timecounter systime_tc;
> >  	struct rte_timecounter rx_tstamp_tc;
> >  	struct rte_timecounter tx_tstamp_tc;
> > +
> > +	/* For VF reset */
> > +	volatile uint8_t reset_number;
> > +	void *reset_store_data;
> >  };
> 
> How to move it to i40_vf? Or it can be used for PF in future?

This is a legacy from wenzhuo's code, his code is design in this way. If this feature is be used by pf, locus of here is may be  reasonable?
 
> 
> 
> > +
> > +static int
> > +i40evf_store_before_reset(struct rte_eth_dev *dev) {
> > +	struct i40e_adapter *adapter =
> > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > +	struct i40e_vf_reset_store *store_data;
> > +	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-
> > >dev_private);
> > +
> > +	adapter->reset_store_data = rte_zmalloc("i40evf_store_reset",
> > +					sizeof(struct i40e_vf_reset_store), 0);
> > +	if (adapter->reset_store_data == NULL) {
> > +		PMD_INIT_LOG(ERR, "Failed to allocate %ld bytes needed
> to"
> > +				" to store data when reset vf",
> > +				sizeof(struct i40e_vf_reset_store));
> > +		return -ENOMEM;
> > +	}
> i40evf_store_before_reset is allocated when reset happens.
> Don't forget to free it when reset is done. Otherwise memory leak.
> 
> The same as below mac_addrs.

Ok, I will add rte_free for them.

> > +	store_data =
> > +		(struct i40e_vf_reset_store *)adapter->reset_store_data;
> > +	store_data->mac_addrs = rte_zmalloc("i40evf_mac_store_reset",
> > +			ETHER_ADDR_LEN * I40E_NUM_MACADDR_MAX, 0);
> > +	if (store_data->mac_addrs == NULL) {
> > +		PMD_INIT_LOG(ERR, "Failed to allocate %d bytes needed to"
> > +				" to store MAC addresses when reset vf",
> > +				ETHER_ADDR_LEN *
> > I40E_NUM_MACADDR_MAX);
> > +	}
> > +
> > +	memcpy(store_data->mac_addrs, dev->data->mac_addrs,
> > +			ETHER_ADDR_LEN * I40E_NUM_MACADDR_MAX);
> > +
> > +	store_data->promisc_unicast_enabled = vf-
> >promisc_unicast_enabled;
> > +	store_data->promisc_multicast_enabled = vf-
> > >promisc_multicast_enabled;
> > +
> > +	store_data->vlan_num = vf->vsi.vlan_num;
> > +	memcpy(store_data->vfta, vf->vsi.vfta,
> > +			sizeof(uint32_t) * I40E_VFTA_SIZE);
> > +
> > +	store_data->mac_num = vf->vsi.mac_num;
> > +
> > +	return 0;
> > +}
> 
> 
> 
> > +static int
> > +i40evf_handle_vf_reset(struct rte_eth_dev *dev) {
> > +	struct i40e_adapter *adapter =
> > +		I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > +
> > +	if (!dev->data->dev_started)
> > +		return 0;
> > +
> I think even the dev is not started, we also need to reset it. Because the VF
> need to reconnect with PF.

Ok.
 
> > +	adapter->reset_number = 1;
> > +	i40e_vf_reset_dev(dev);
> > +	adapter->reset_number = 0;
> > +
> How to name the reset_number to resetting?
> 

I will change name to "adapter->reset_flag"

Re: [PATCH 0/3] net/i40e: vf port reset

[Wu, Jingjing]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Zhao
> Sent: Friday, March 3, 2017 12:56 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 0/3] net/i40e: vf port reset
> 
> The patches mainly finish following functions:
> 1) get pf reset vf comamand falg from interrupt server function.
> 2) reset vf port from testpmd app using a command.
> 3) sore and restore vf related parameters.
> 

The patch order is wrong. You need to make sure the compile can be 
Successful when apply each patch in order. So I think should like
1. ethdev API
2. i40e change
3. testpmd change.


Another comment is like: 
The reset API can be used for device reset. Please don't limit the scope it
to VF.

Re: [PATCH 0/3] net/i40e: vf port reset

[Zhao1, Wei]

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Friday, March 24, 2017 1:57 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 0/3] net/i40e: vf port reset
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Zhao
> > Sent: Friday, March 3, 2017 12:56 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH 0/3] net/i40e: vf port reset
> >
> > The patches mainly finish following functions:
> > 1) get pf reset vf comamand falg from interrupt server function.
> > 2) reset vf port from testpmd app using a command.
> > 3) sore and restore vf related parameters.
> >
> 
> The patch order is wrong. You need to make sure the compile can be
> Successful when apply each patch in order. So I think should like 1. ethdev
> API 2. i40e change 3. testpmd change.
> 

Ok, I will change the order in v2
> 
> Another comment is like:
> The reset API can be used for device reset. Please don't limit the scope it
> to VF.

Ok.