diff mbox

[dpdk-dev,v6,2/2] app/testpmd: fix port stop

Message ID 1485514200-25230-3-git-send-email-bernard.iremonger@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Iremonger, Bernard Jan. 27, 2017, 10:50 a.m. UTC 
  The rte_eth_dev_stop function is not called if the port_status is
not RTE_PORT_STARTED. This can happen if the rte_eth_dev_start
function is called directly, ie not through the start_port function.

Make sure rte_eth_dev_stop is always called in stop_port function.

Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")

CC: stable@dpdk.org
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 app/test-pmd/testpmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jingjing Wu Feb. 3, 2017, 8:21 a.m. UTC | #1 
> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Friday, January 27, 2017 6:50 PM
> To: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: Iremonger, Bernard <bernard.iremonger@intel.com>; stable@dpdk.org
> Subject: [PATCH v6 2/2] app/testpmd: fix port stop
> 
> The rte_eth_dev_stop function is not called if the port_status is not
> RTE_PORT_STARTED. This can happen if the rte_eth_dev_start function is
> called directly, ie not through the start_port function.
> 
> Make sure rte_eth_dev_stop is always called in stop_port function.
> 
> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
> 
> CC: stable@dpdk.org
> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> ---
>  app/test-pmd/testpmd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 3d25436..0d7a4d4 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1490,13 +1490,13 @@ stop_port(portid_t pid)
>  			continue;
>  		}
> 
> +		rte_eth_dev_stop(pi);
> +
>  		port = &ports[pi];
>  		if (rte_atomic16_cmpset(&(port->port_status),
> RTE_PORT_STARTED,
>  						RTE_PORT_HANDLING) == 0)
>  			continue;
> 
> -		rte_eth_dev_stop(pi);
> -

I don't think this fix is correct to move rte_eth_dev_stop above.

We need to make sure rte_eth_dev_start is called in start_port. For vmdq configuration,
You just need to change the configuration when port is stopped.
Iremonger, Bernard Feb. 3, 2017, 10:37 a.m. UTC | #2 
Hi Jingjing

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Friday, February 3, 2017 8:22 AM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v6 2/2] app/testpmd: fix port stop
> 
> 
> 
> > -----Original Message-----
> > From: Iremonger, Bernard
> > Sent: Friday, January 27, 2017 6:50 PM
> > To: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>
> > Cc: Iremonger, Bernard <bernard.iremonger@intel.com>;
> stable@dpdk.org
> > Subject: [PATCH v6 2/2] app/testpmd: fix port stop
> >
> > The rte_eth_dev_stop function is not called if the port_status is not
> > RTE_PORT_STARTED. This can happen if the rte_eth_dev_start function is
> > called directly, ie not through the start_port function.
> >
> > Make sure rte_eth_dev_stop is always called in stop_port function.
> >
> > Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
> >
> > CC: stable@dpdk.org
> > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> > ---
> >  app/test-pmd/testpmd.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 3d25436..0d7a4d4 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -1490,13 +1490,13 @@ stop_port(portid_t pid)
> >  			continue;
> >  		}
> >
> > +		rte_eth_dev_stop(pi);
> > +
> >  		port = &ports[pi];
> >  		if (rte_atomic16_cmpset(&(port->port_status),
> > RTE_PORT_STARTED,
> >  						RTE_PORT_HANDLING) == 0)
> >  			continue;
> >
> > -		rte_eth_dev_stop(pi);
> > -
> 
> I don't think this fix is correct to move rte_eth_dev_stop above.
> 
> We need to make sure rte_eth_dev_start is called in start_port. For vmdq
> configuration, You just need to change the configuration when port is
> stopped.

I think the stop_port() function should always stop the port even if the port_status is not correct for any reason.
At present stop_port() returns without stopping the port if the port_status is not RTE_PORT_STARTED.

The VMDq configuration is done whet the port is stopped, however to the complete the VMDq configuration the port must be started.

Regards,

Bernard.
Jingjing Wu Feb. 12, 2017, 4:34 a.m. UTC | #3 
> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Friday, February 3, 2017 6:38 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v6 2/2] app/testpmd: fix port stop
> 
> Hi Jingjing
> 
> > -----Original Message-----
> > From: Wu, Jingjing
> > Sent: Friday, February 3, 2017 8:22 AM
> > To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH v6 2/2] app/testpmd: fix port stop
> >
> >
> >
> > > -----Original Message-----
> > > From: Iremonger, Bernard
> > > Sent: Friday, January 27, 2017 6:50 PM
> > > To: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>
> > > Cc: Iremonger, Bernard <bernard.iremonger@intel.com>;
> > stable@dpdk.org
> > > Subject: [PATCH v6 2/2] app/testpmd: fix port stop
> > >
> > > The rte_eth_dev_stop function is not called if the port_status is
> > > not RTE_PORT_STARTED. This can happen if the rte_eth_dev_start
> > > function is called directly, ie not through the start_port function.
> > >
> > > Make sure rte_eth_dev_stop is always called in stop_port function.
> > >
> > > Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
> > >
> > > CC: stable@dpdk.org
> > > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> > > ---
> > >  app/test-pmd/testpmd.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > > 3d25436..0d7a4d4 100644
> > > --- a/app/test-pmd/testpmd.c
> > > +++ b/app/test-pmd/testpmd.c
> > > @@ -1490,13 +1490,13 @@ stop_port(portid_t pid)
> > >  			continue;
> > >  		}
> > >
> > > +		rte_eth_dev_stop(pi);
> > > +
> > >  		port = &ports[pi];
> > >  		if (rte_atomic16_cmpset(&(port->port_status),
> > > RTE_PORT_STARTED,
> > >  						RTE_PORT_HANDLING) == 0)
> > >  			continue;
> > >
> > > -		rte_eth_dev_stop(pi);
> > > -
> >
> > I don't think this fix is correct to move rte_eth_dev_stop above.
> >
> > We need to make sure rte_eth_dev_start is called in start_port. For
> > vmdq configuration, You just need to change the configuration when
> > port is stopped.
> 
> I think the stop_port() function should always stop the port even if the
> port_status is not correct for any reason.
> At present stop_port() returns without stopping the port if the port_status is not
> RTE_PORT_STARTED.
>
This is testpmd's design. If you think it is an issue, maybe you need to prepare patch to optimize it. But for VMDQ configuration, I'd like to make it independent but not mixed.
 
> The VMDq configuration is done whet the port is stopped, however to the
> complete the VMDq configuration the port must be started.
> 

To change minor, we can stop port, then configure VMDQ and then start port.

You make port started in VMDQ config, the Symmetry of stop/start command is broken and it is not easy to maintain.

> Regards,
> 
> Bernard.
Thomas Monjalon March 10, 2017, 1:17 p.m. UTC | #4 
2017-02-12 04:34, Wu, Jingjing:
> > > > --- a/app/test-pmd/testpmd.c
> > > > +++ b/app/test-pmd/testpmd.c
> > > > @@ -1490,13 +1490,13 @@ stop_port(portid_t pid)
> > > >  			continue;
> > > >  		}
> > > >
> > > > +		rte_eth_dev_stop(pi);
> > > > +
> > > >  		port = &ports[pi];
> > > >  		if (rte_atomic16_cmpset(&(port->port_status),
> > > > RTE_PORT_STARTED,
> > > >  						RTE_PORT_HANDLING) == 0)
> > > >  			continue;
> > > >
> > > > -		rte_eth_dev_stop(pi);
> > > > -
> > >
> > > I don't think this fix is correct to move rte_eth_dev_stop above.
> > >
> > > We need to make sure rte_eth_dev_start is called in start_port. For
> > > vmdq configuration, You just need to change the configuration when
> > > port is stopped.
> > 
> > I think the stop_port() function should always stop the port even if the
> > port_status is not correct for any reason.
> > At present stop_port() returns without stopping the port if the port_status is not
> > RTE_PORT_STARTED.
> >
> This is testpmd's design. If you think it is an issue, maybe you need to prepare patch to optimize it. But for VMDQ configuration, I'd like to make it independent but not mixed.
>  
> > The VMDq configuration is done whet the port is stopped, however to the
> > complete the VMDq configuration the port must be started.
> > 
> 
> To change minor, we can stop port, then configure VMDQ and then start port.
> 
> You make port started in VMDQ config, the Symmetry of stop/start command is broken and it is not easy to maintain.

Should we close this patch in patchwork?
Jingjing Wu March 10, 2017, 4:56 p.m. UTC | #5 
> > To change minor, we can stop port, then configure VMDQ and then start port.
> >
> > You make port started in VMDQ config, the Symmetry of stop/start command
> is broken and it is not easy to maintain.
> 
> Should we close this patch in patchwork?

Yes, I think so.

Thanks
Jingjing
Iremonger, Bernard March 10, 2017, 4:59 p.m. UTC | #6 
> -----Original Message-----
> From: Wu, Jingjing
> Sent: Friday, March 10, 2017 4:56 PM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v6 2/2] app/testpmd: fix port stop
> 
> > > To change minor, we can stop port, then configure VMDQ and then start
> port.
> > >
> > > You make port started in VMDQ config, the Symmetry of stop/start
> command
> > is broken and it is not easy to maintain.
> >
> > Should we close this patch in patchwork?
> 
> Yes, I think so.
> 
> Thanks
> Jingjing

Agreed.

Regards,

Bernard.
diff mbox

Patch

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 3d25436..0d7a4d4 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1490,13 +1490,13 @@  stop_port(portid_t pid)
 			continue;
 		}
 
+		rte_eth_dev_stop(pi);
+
 		port = &ports[pi];
 		if (rte_atomic16_cmpset(&(port->port_status), RTE_PORT_STARTED,
 						RTE_PORT_HANDLING) == 0)
 			continue;
 
-		rte_eth_dev_stop(pi);
-
 		if (rte_atomic16_cmpset(&(port->port_status),
 			RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
 			printf("Port %d can not be set into stopped\n", pi);