[PATCH] app/testpmd: fix forward port ids setting

[PATCH] app/testpmd: fix forward port ids setting

[Matan Azrad]

The corrupted code didn't check the port availability when
it was trying to set the forward port IDs array.
However, when it was counting the number of ports, the availability
was checked by RTE_ETH_FOREACH_DEV iterator.

Hence, even when ETH devices ports were not in ATTACHED state,
the testpmd tried to forward traffic by them and got segmentation
fault at queue access time.

For example:
When EAL command line parameters include two devices, the first
is failsafe with two sub devices and the second is any device,
testpmd gets two devices by the iterator and sets for forwarding
both, the failsafe device and the failsafe first sub device
(instead of the second sub device).
After the first failsafe sub device state was changed to DEFERRED,
testpmd tries to forward traffic through the deferred device
because it didn't check the port availability in setting time.

The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward
port IDs default setting.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 app/test-pmd/testpmd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Hi All
I would like to bring up a discussion to complete this bug
fix.

When user wants to set the list of forwarding ports
by "set portlist" (testpmd command line), the testpmd
application checks the availability of the ports by 
rte_eth_dev_is_valid_port API.
By this way, it gets the DEFERRED port as valid port and 
will try to recieve\send packets via this port.

This scenario will cause the same error as this patch fixes.

Should testpmd allow user to run traffic by DEFERRED port
directly?

If any application wants to check a port availability for device
usage (conf\rxtx), Which API should be used?

According to the patch cb894d99eceb ("ethdev: add deferred intermediate device state"),
DEFERRED ports should be invisible to application,
So maybe the rte_eth_dev_is_valid_port API should be internal
and a new ethdev API should be created for applications.

What do you think?  

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 7d40139..f9bdbf8 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -463,9 +463,10 @@ static void
 set_default_fwd_ports_config(void)
 {
 	portid_t pt_id;
+	int i = 0;
 
-	for (pt_id = 0; pt_id < nb_ports; pt_id++)
-		fwd_ports_ids[pt_id] = pt_id;
+	RTE_ETH_FOREACH_DEV(pt_id)
+		fwd_ports_ids[i++] = pt_id;
 
 	nb_cfg_ports = nb_ports;
 	nb_fwd_ports = nb_ports;
-- 
2.7.4

Re: [PATCH] app/testpmd: fix forward port ids setting

[Gaëtan Rivet]

Hi Matan,

On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> The corrupted code didn't check the port availability when
> it was trying to set the forward port IDs array.
> However, when it was counting the number of ports, the availability
> was checked by RTE_ETH_FOREACH_DEV iterator.
> 
> Hence, even when ETH devices ports were not in ATTACHED state,
> the testpmd tried to forward traffic by them and got segmentation
> fault at queue access time.
> 
> For example:
> When EAL command line parameters include two devices, the first
> is failsafe with two sub devices and the second is any device,
> testpmd gets two devices by the iterator and sets for forwarding
> both, the failsafe device and the failsafe first sub device
> (instead of the second sub device).
> After the first failsafe sub device state was changed to DEFERRED,
> testpmd tries to forward traffic through the deferred device
> because it didn't check the port availability in setting time.
> 
> The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward
> port IDs default setting.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  app/test-pmd/testpmd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Hi All
> I would like to bring up a discussion to complete this bug
> fix.
> 
> When user wants to set the list of forwarding ports
> by "set portlist" (testpmd command line), the testpmd
> application checks the availability of the ports by 
> rte_eth_dev_is_valid_port API.
> By this way, it gets the DEFERRED port as valid port and 
> will try to recieve\send packets via this port.
> 
> This scenario will cause the same error as this patch fixes.
> 
> Should testpmd allow user to run traffic by DEFERRED port
> directly?
> 
> If any application wants to check a port availability for device
> usage (conf\rxtx), Which API should be used?
> 
> According to the patch cb894d99eceb ("ethdev: add deferred intermediate device state"),
> DEFERRED ports should be invisible to application,
> So maybe the rte_eth_dev_is_valid_port API should be internal
> and a new ethdev API should be created for applications.
> 
> What do you think?  
> 

I think that regardless of the semantic of the DEFERRED state or any
other port handling, the correct assumption is to consider any port
iterated over by RTE_ETH_FOREACH_DEV to be the exact set of devices that
are supposed to be usable. In the event of an API evolution regarding
port states, this macro should remain correct.

So I think your fix is correct, and that the implication of
RTE_ETH_FOREACH_DEV avoiding DEFERRED devices is correct.

> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 7d40139..f9bdbf8 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -463,9 +463,10 @@ static void
>  set_default_fwd_ports_config(void)
>  {
>  	portid_t pt_id;
> +	int i = 0;
>  
> -	for (pt_id = 0; pt_id < nb_ports; pt_id++)
> -		fwd_ports_ids[pt_id] = pt_id;
> +	RTE_ETH_FOREACH_DEV(pt_id)
> +		fwd_ports_ids[i++] = pt_id;
>  
>  	nb_cfg_ports = nb_ports;
>  	nb_fwd_ports = nb_ports;
> -- 
> 2.7.4
> 

-- 
Gaëtan Rivet
6WIND

Re: [PATCH] app/testpmd: fix forward port ids setting

[Matan Azrad]

Hi

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Monday, September 4, 2017 11:46 AM
> To: Matan Azrad <matan@mellanox.com>
> Cc: Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; dev@dpdk.org; Ori Kam <orika@mellanox.com>;
> stable@dpdk.org
> Subject: Re: [PATCH] app/testpmd: fix forward port ids setting
> 
> Hi Matan,
> 
> On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> > The corrupted code didn't check the port availability when it was
> > trying to set the forward port IDs array.
> > However, when it was counting the number of ports, the availability
> > was checked by RTE_ETH_FOREACH_DEV iterator.
> >
> > Hence, even when ETH devices ports were not in ATTACHED state, the
> > testpmd tried to forward traffic by them and got segmentation fault at
> > queue access time.
> >
> > For example:
> > When EAL command line parameters include two devices, the first is
> > failsafe with two sub devices and the second is any device, testpmd
> > gets two devices by the iterator and sets for forwarding both, the
> > failsafe device and the failsafe first sub device (instead of the
> > second sub device).
> > After the first failsafe sub device state was changed to DEFERRED,
> > testpmd tries to forward traffic through the deferred device because
> > it didn't check the port availability in setting time.
> >
> > The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward port IDs
> > default setting.
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> >  app/test-pmd/testpmd.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > Hi All
> > I would like to bring up a discussion to complete this bug fix.
> >
> > When user wants to set the list of forwarding ports by "set portlist"
> > (testpmd command line), the testpmd application checks the
> > availability of the ports by rte_eth_dev_is_valid_port API.
> > By this way, it gets the DEFERRED port as valid port and will try to
> > recieve\send packets via this port.
> >
> > This scenario will cause the same error as this patch fixes.
> >
> > Should testpmd allow user to run traffic by DEFERRED port directly?
> >
> > If any application wants to check a port availability for device usage
> > (conf\rxtx), Which API should be used?
> >
> > According to the patch cb894d99eceb ("ethdev: add deferred
> > intermediate device state"), DEFERRED ports should be invisible to
> > application, So maybe the rte_eth_dev_is_valid_port API should be
> > internal and a new ethdev API should be created for applications.
> >
> > What do you think?
> >
> 
> I think that regardless of the semantic of the DEFERRED state or any other
> port handling, the correct assumption is to consider any port iterated over by
> RTE_ETH_FOREACH_DEV to be the exact set of devices that are supposed to
> be usable. In the event of an API evolution regarding port states, this macro
> should remain correct.
> 
> So I think your fix is correct, and that the implication of
> RTE_ETH_FOREACH_DEV avoiding DEFERRED devices is correct.
> 

This patch fixes the default forward ports setting (actually when user don't use --portmask param or "set portlist"),
But it don't fix the port validation which PMD does for "set portlist" command.
So, the discussion is how to fix this port validation.
In current code, testpmd uses  rte_eth_dev_is_valid_port which return the DEFERRED device too for forwarding.
Should it use the RTE_ETH_FOREACH_DEV  iterator for one port validation? 
Don't you think we need new API for one port?

> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 7d40139..f9bdbf8 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -463,9 +463,10 @@ static void
> >  set_default_fwd_ports_config(void)
> >  {
> >  	portid_t pt_id;
> > +	int i = 0;
> >
> > -	for (pt_id = 0; pt_id < nb_ports; pt_id++)
> > -		fwd_ports_ids[pt_id] = pt_id;
> > +	RTE_ETH_FOREACH_DEV(pt_id)
> > +		fwd_ports_ids[i++] = pt_id;
> >
> >  	nb_cfg_ports = nb_ports;
> >  	nb_fwd_ports = nb_ports;
> > --
> > 2.7.4
> >
> 
> --
> Gaëtan Rivet
> 6WIND

Re: [PATCH] app/testpmd: fix forward port ids setting

[Gaëtan Rivet]

On Mon, Sep 04, 2017 at 09:25:04AM +0000, Matan Azrad wrote:
> Hi
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Monday, September 4, 2017 11:46 AM
> > To: Matan Azrad <matan@mellanox.com>
> > Cc: Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon
> > <thomas@monjalon.net>; dev@dpdk.org; Ori Kam <orika@mellanox.com>;
> > stable@dpdk.org
> > Subject: Re: [PATCH] app/testpmd: fix forward port ids setting
> > 
> > Hi Matan,
> > 
> > On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> > > The corrupted code didn't check the port availability when it was
> > > trying to set the forward port IDs array.
> > > However, when it was counting the number of ports, the availability
> > > was checked by RTE_ETH_FOREACH_DEV iterator.
> > >
> > > Hence, even when ETH devices ports were not in ATTACHED state, the
> > > testpmd tried to forward traffic by them and got segmentation fault at
> > > queue access time.
> > >
> > > For example:
> > > When EAL command line parameters include two devices, the first is
> > > failsafe with two sub devices and the second is any device, testpmd
> > > gets two devices by the iterator and sets for forwarding both, the
> > > failsafe device and the failsafe first sub device (instead of the
> > > second sub device).
> > > After the first failsafe sub device state was changed to DEFERRED,
> > > testpmd tries to forward traffic through the deferred device because
> > > it didn't check the port availability in setting time.
> > >
> > > The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward port IDs
> > > default setting.
> > >
> > > Fixes: af75078fece3 ("first public release")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > ---
> > >  app/test-pmd/testpmd.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > Hi All
> > > I would like to bring up a discussion to complete this bug fix.
> > >
> > > When user wants to set the list of forwarding ports by "set portlist"
> > > (testpmd command line), the testpmd application checks the
> > > availability of the ports by rte_eth_dev_is_valid_port API.
> > > By this way, it gets the DEFERRED port as valid port and will try to
> > > recieve\send packets via this port.
> > >
> > > This scenario will cause the same error as this patch fixes.
> > >
> > > Should testpmd allow user to run traffic by DEFERRED port directly?
> > >
> > > If any application wants to check a port availability for device usage
> > > (conf\rxtx), Which API should be used?
> > >
> > > According to the patch cb894d99eceb ("ethdev: add deferred
> > > intermediate device state"), DEFERRED ports should be invisible to
> > > application, So maybe the rte_eth_dev_is_valid_port API should be
> > > internal and a new ethdev API should be created for applications.
> > >
> > > What do you think?
> > >
> > 
> > I think that regardless of the semantic of the DEFERRED state or any other
> > port handling, the correct assumption is to consider any port iterated over by
> > RTE_ETH_FOREACH_DEV to be the exact set of devices that are supposed to
> > be usable. In the event of an API evolution regarding port states, this macro
> > should remain correct.
> > 
> > So I think your fix is correct, and that the implication of
> > RTE_ETH_FOREACH_DEV avoiding DEFERRED devices is correct.
> > 
> 
> This patch fixes the default forward ports setting (actually when user don't use --portmask param or "set portlist"),
> But it don't fix the port validation which PMD does for "set portlist" command.
> So, the discussion is how to fix this port validation.

You could make a static rte_eth_dev_is_valid_port with a different name,
declare both RTE_ETH_VALID_PORT* macros within rte_ethdev.c
and introduce a new rte_eth_dev_is_valid_port which would restrict
devices to those ATTACHED.

I'm not sure this would be interesting for applications. Currently this
check is performed internally by the ether layer, I guess most
applications rely on it. Making the "extended" version (ATTACHED +
DEFERRED) private with the strict one public would probably not change
behaviors, thus it would probably have little to no effect.

So my opinion is "why not, but the risk is adding dead code for no real
benefit".

> In current code, testpmd uses  rte_eth_dev_is_valid_port which return the DEFERRED device too for forwarding.
> Should it use the RTE_ETH_FOREACH_DEV  iterator for one port validation? 
> Don't you think we need new API for one port?
> 
> > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > > 7d40139..f9bdbf8 100644
> > > --- a/app/test-pmd/testpmd.c
> > > +++ b/app/test-pmd/testpmd.c
> > > @@ -463,9 +463,10 @@ static void
> > >  set_default_fwd_ports_config(void)
> > >  {
> > >  	portid_t pt_id;
> > > +	int i = 0;
> > >
> > > -	for (pt_id = 0; pt_id < nb_ports; pt_id++)
> > > -		fwd_ports_ids[pt_id] = pt_id;
> > > +	RTE_ETH_FOREACH_DEV(pt_id)
> > > +		fwd_ports_ids[i++] = pt_id;
> > >
> > >  	nb_cfg_ports = nb_ports;
> > >  	nb_fwd_ports = nb_ports;
> > > --
> > > 2.7.4
> > >
> > 
> > --
> > Gaëtan Rivet
> > 6WIND

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-stable] [PATCH] app/testpmd: fix forward port ids setting

[Thomas Monjalon]

04/09/2017 11:52, Gaëtan Rivet:
> On Mon, Sep 04, 2017 at 09:25:04AM +0000, Matan Azrad wrote:
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> > > > Hi All
> > > > I would like to bring up a discussion to complete this bug fix.
> > > >
> > > > When user wants to set the list of forwarding ports by "set portlist"
> > > > (testpmd command line), the testpmd application checks the
> > > > availability of the ports by rte_eth_dev_is_valid_port API.
> > > > By this way, it gets the DEFERRED port as valid port and will try to
> > > > recieve\send packets via this port.
> > > >
> > > > This scenario will cause the same error as this patch fixes.
> > > >
> > > > Should testpmd allow user to run traffic by DEFERRED port directly?
> > > >
> > > > If any application wants to check a port availability for device usage
> > > > (conf\rxtx), Which API should be used?
> > > >
> > > > According to the patch cb894d99eceb ("ethdev: add deferred
> > > > intermediate device state"), DEFERRED ports should be invisible to
> > > > application, So maybe the rte_eth_dev_is_valid_port API should be
> > > > internal and a new ethdev API should be created for applications.
> > > >
> > > > What do you think?
> > > >
> > > 
> > > I think that regardless of the semantic of the DEFERRED state or any other
> > > port handling, the correct assumption is to consider any port iterated over by
> > > RTE_ETH_FOREACH_DEV to be the exact set of devices that are supposed to
> > > be usable. In the event of an API evolution regarding port states, this macro
> > > should remain correct.
> > > 
> > > So I think your fix is correct, and that the implication of
> > > RTE_ETH_FOREACH_DEV avoiding DEFERRED devices is correct.
> > > 
> > 
> > This patch fixes the default forward ports setting (actually when user don't use --portmask param or "set portlist"),
> > But it don't fix the port validation which PMD does for "set portlist" command.
> > So, the discussion is how to fix this port validation.
> 
> You could make a static rte_eth_dev_is_valid_port with a different name,
> declare both RTE_ETH_VALID_PORT* macros within rte_ethdev.c
> and introduce a new rte_eth_dev_is_valid_port which would restrict
> devices to those ATTACHED.
> 
> I'm not sure this would be interesting for applications. Currently this
> check is performed internally by the ether layer, I guess most
> applications rely on it. Making the "extended" version (ATTACHED +
> DEFERRED) private with the strict one public would probably not change
> behaviors, thus it would probably have little to no effect.
> 
> So my opinion is "why not, but the risk is adding dead code for no real
> benefit".
> 
> > In current code, testpmd uses  rte_eth_dev_is_valid_port which return the DEFERRED device too for forwarding.
> > Should it use the RTE_ETH_FOREACH_DEV  iterator for one port validation? 
> > Don't you think we need new API for one port?

Please, let's continue this ethdev discussion in a separate thread.
I've started a new one:
	http://dpdk.org/ml/archives/dev/2017-September/074656.html

Re: [dpdk-stable] [PATCH] app/testpmd: fix forward port ids setting

[Matan Azrad]

Hi Jinging,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, September 6, 2017 1:21 PM
> To: Gaëtan Rivet <gaetan.rivet@6wind.com>; Matan Azrad
> <matan@mellanox.com>
> Cc: Jingjing Wu <jingjing.wu@intel.com>; dev@dpdk.org; Ori Kam
> <orika@mellanox.com>
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix forward port ids setting
> 
> 04/09/2017 11:52, Gaëtan Rivet:
> > On Mon, Sep 04, 2017 at 09:25:04AM +0000, Matan Azrad wrote:
> > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > > On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> > > > > Hi All
> > > > > I would like to bring up a discussion to complete this bug fix.
> > > > >
> > > > > When user wants to set the list of forwarding ports by "set portlist"
> > > > > (testpmd command line), the testpmd application checks the
> > > > > availability of the ports by rte_eth_dev_is_valid_port API.
> > > > > By this way, it gets the DEFERRED port as valid port and will
> > > > > try to recieve\send packets via this port.
> > > > >
> > > > > This scenario will cause the same error as this patch fixes.
> > > > >
> > > > > Should testpmd allow user to run traffic by DEFERRED port directly?
> > > > >
> > > > > If any application wants to check a port availability for device
> > > > > usage (conf\rxtx), Which API should be used?
> > > > >
> > > > > According to the patch cb894d99eceb ("ethdev: add deferred
> > > > > intermediate device state"), DEFERRED ports should be invisible
> > > > > to application, So maybe the rte_eth_dev_is_valid_port API
> > > > > should be internal and a new ethdev API should be created for
> applications.
> > > > >
> > > > > What do you think?
> > > > >
> > > >
> > > > I think that regardless of the semantic of the DEFERRED state or
> > > > any other port handling, the correct assumption is to consider any
> > > > port iterated over by RTE_ETH_FOREACH_DEV to be the exact set of
> > > > devices that are supposed to be usable. In the event of an API
> > > > evolution regarding port states, this macro should remain correct.
> > > >
> > > > So I think your fix is correct, and that the implication of
> > > > RTE_ETH_FOREACH_DEV avoiding DEFERRED devices is correct.
> > > >
> > >
> > > This patch fixes the default forward ports setting (actually when
> > > user don't use --portmask param or "set portlist"), But it don't fix the port
> validation which PMD does for "set portlist" command.
> > > So, the discussion is how to fix this port validation.
> >
> > You could make a static rte_eth_dev_is_valid_port with a different
> > name, declare both RTE_ETH_VALID_PORT* macros within rte_ethdev.c
> and
> > introduce a new rte_eth_dev_is_valid_port which would restrict devices
> > to those ATTACHED.
> >
> > I'm not sure this would be interesting for applications. Currently
> > this check is performed internally by the ether layer, I guess most
> > applications rely on it. Making the "extended" version (ATTACHED +
> > DEFERRED) private with the strict one public would probably not change
> > behaviors, thus it would probably have little to no effect.
> >
> > So my opinion is "why not, but the risk is adding dead code for no
> > real benefit".
> >
> > > In current code, testpmd uses  rte_eth_dev_is_valid_port which return
> the DEFERRED device too for forwarding.
> > > Should it use the RTE_ETH_FOREACH_DEV  iterator for one port
> validation?
> > > Don't you think we need new API for one port?
> 
> Please, let's continue this ethdev discussion in a separate thread.
> I've started a new one:
> 	https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F
> %2Fdpdk.org%2Fml%2Farchives%2Fdev%2F2017-
> September%2F074656.html&data=02%7C01%7Corika%40mellanox.com%7C5
> 9017f577e004c8be80c08d4f51104ec%7Ca652971c7d2e4d9ba6a4d149256f461b
> %7C0%7C0%7C636402900840946032&sdata=lPFh1ro1cJTyiYYC7KQtRm7CQ8M
> rkct7i6%2BUBW1HEsM%3D&reserved=0

I think you can acknowledge this fix for the default forward port IDs setting (this patch fixes it).
I will send fix in a separated patch to the  "set portlist" port validation after ethdev discussion will be done.

Regards,
Matan Azrad

Re: [PATCH] app/testpmd: fix forward port ids setting

[Wu, Jingjing]

> -----Original Message-----
> From: Matan Azrad [mailto:matan@mellanox.com]
> Sent: Sunday, September 3, 2017 9:19 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Gaetan Rivet <gaetan.rivet@6wind.com>;
> Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; stable@dpdk.org
> Subject: [PATCH] app/testpmd: fix forward port ids setting
> 
> The corrupted code didn't check the port availability when
> it was trying to set the forward port IDs array.
> However, when it was counting the number of ports, the availability
> was checked by RTE_ETH_FOREACH_DEV iterator.
> 
> Hence, even when ETH devices ports were not in ATTACHED state,
> the testpmd tried to forward traffic by them and got segmentation
> fault at queue access time.
> 
> For example:
> When EAL command line parameters include two devices, the first
> is failsafe with two sub devices and the second is any device,
> testpmd gets two devices by the iterator and sets for forwarding
> both, the failsafe device and the failsafe first sub device
> (instead of the second sub device).
> After the first failsafe sub device state was changed to DEFERRED,
> testpmd tries to forward traffic through the deferred device
> because it didn't check the port availability in setting time.
> 
> The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward
> port IDs default setting.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>

>From the view of testpmd, the fix is good.

Acked-by: Jingjing Wu <jingjing.wu@intel.com>

Thanks
Jingjing

Re: [dpdk-stable] [PATCH] app/testpmd: fix forward port ids setting

[Ferruh Yigit]

On 9/7/2017 8:44 AM, Wu, Jingjing wrote:
> 
> 
>> -----Original Message-----
>> From: Matan Azrad [mailto:matan@mellanox.com]
>> Sent: Sunday, September 3, 2017 9:19 PM
>> To: Wu, Jingjing <jingjing.wu@intel.com>; Gaetan Rivet <gaetan.rivet@6wind.com>;
>> Thomas Monjalon <thomas@monjalon.net>
>> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; stable@dpdk.org
>> Subject: [PATCH] app/testpmd: fix forward port ids setting
>>
>> The corrupted code didn't check the port availability when
>> it was trying to set the forward port IDs array.
>> However, when it was counting the number of ports, the availability
>> was checked by RTE_ETH_FOREACH_DEV iterator.
>>
>> Hence, even when ETH devices ports were not in ATTACHED state,
>> the testpmd tried to forward traffic by them and got segmentation
>> fault at queue access time.
>>
>> For example:
>> When EAL command line parameters include two devices, the first
>> is failsafe with two sub devices and the second is any device,
>> testpmd gets two devices by the iterator and sets for forwarding
>> both, the failsafe device and the failsafe first sub device
>> (instead of the second sub device).
>> After the first failsafe sub device state was changed to DEFERRED,
>> testpmd tries to forward traffic through the deferred device
>> because it didn't check the port availability in setting time.
>>
>> The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward
>> port IDs default setting.
>>
>> Fixes: af75078fece3 ("first public release")

I think this has been broken by introducing DEFERRED state without
fixing iterators, it wasn't broken previously, so updating fixes line as:

Fixes: cb894d99eceb ("ethdev: add deferred intermediate device state")

>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Matan Azrad <matan@mellanox.com>

> Acked-by: Jingjing Wu <jingjing.wu@intel.com>

Applied to dpdk-next-net/master, thanks.