diff mbox

[dpdk-dev] net/bonding: fix bond 8023ad mode enable using wrong index

Message ID 40280F65B1B0B44E8089ED31C01616EBA357ADA1@dggeml509-mbx.china.huawei.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers

Checks

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

Commit Message

Lilijun (Jerry) Nov. 20, 2017, 7:15 a.m. UTC 
  Hi all,

In the function bond_mode_8023ad_enable(), the var i is to used to as the second parameter to pass the slave dev's dpdk port id to the function bond_mode_8023ad_activate_slave(). 
I think this variable is only a index for array internals->active_slaves. So its need to be fixed and change i to internals->active_slaves[i].


[Patch] net/bonding: fix bond 8023ad mode enable using wrong index.
Signed-off-by: Lilijun <jerry.lilijun@huawei.com>

Comments

Kyle Larose Nov. 23, 2017, 4:53 p.m. UTC | #1 
Hello,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lilijun (Jerry)
> Sent: Monday, November 20, 2017 2:16 AM
> To: dev@dpdk.org; tomaszx.kulasek@intel.com
> Cc: Zhang, Jerry; Wanghanlin
> Subject: [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable
> using wrong index
> 

...

> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1159,7 +1159,7 @@
>         uint8_t i;
> 
>         for (i = 0; i < internals->active_slave_count; i++)
> -               bond_mode_8023ad_activate_slave(bond_dev, i);
> +               bond_mode_8023ad_activate_slave(bond_dev, internals-
> >active_slaves[i]);
> 
>         return 0;
> }
> 


I don't think active_slaves is correct here. The slave is not yet active, so it may not be in that array yet.
Should we instead use internals->slaves[i].port_id?
Kyle Larose Nov. 23, 2017, 10:56 p.m. UTC | #2 
Sorry -- ignore my previous email. I was looking at it from the wrong
perspective. I was thinking of what happens in active activate_slave, which
calls bond_mode_8023ad_activate_slave *before* adding to the
active_slaves[] array.

In this case, the slaves are already active, since we're iterating over the
active slave list.

On Thu, Nov 23, 2017 at 11:53 AM, Kyle Larose <klarose@sandvine.com> wrote:

> Hello,
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lilijun (Jerry)
> > Sent: Monday, November 20, 2017 2:16 AM
> > To: dev@dpdk.org; tomaszx.kulasek@intel.com
> > Cc: Zhang, Jerry; Wanghanlin
> > Subject: [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable
> > using wrong index
> >
>
> ...
>
> > --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> > +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> > @@ -1159,7 +1159,7 @@
> >         uint8_t i;
> >
> >         for (i = 0; i < internals->active_slave_count; i++)
> > -               bond_mode_8023ad_activate_slave(bond_dev, i);
> > +               bond_mode_8023ad_activate_slave(bond_dev, internals-
> > >active_slaves[i]);
> >
> >         return 0;
> > }
> >
>
>
> I don't think active_slaves is correct here. The slave is not yet active,
> so it may not be in that array yet.
> Should we instead use internals->slaves[i].port_id?
>
>
>
>
Kyle Larose Nov. 23, 2017, 10:57 p.m. UTC | #3 
Hello,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lilijun (Jerry)
> Sent: Monday, November 20, 2017 2:16 AM
> To: dev@dpdk.org; tomaszx.kulasek@intel.com
> Cc: Zhang, Jerry; Wanghanlin
> Subject: [dpdk-dev] [PATCH] net/bonding: fix bond 8023ad mode enable
> using wrong index
> 
> Hi all,
> 
> In the function bond_mode_8023ad_enable(), the var i is to used to
> as the second parameter to pass the slave dev's dpdk port id to the
> function bond_mode_8023ad_activate_slave().
> I think this variable is only a index for array internals-
> >active_slaves. So its need to be fixed and change i to internals-
> >active_slaves[i].
> 
> 
> [Patch] net/bonding: fix bond 8023ad mode enable using wrong index.
> Signed-off-by: Lilijun <jerry.lilijun@huawei.com>
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c
> b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index a2313b3..20a08dc 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1159,7 +1159,7 @@
>         uint8_t i;
> 
>         for (i = 0; i < internals->active_slave_count; i++)
> -               bond_mode_8023ad_activate_slave(bond_dev, i);
> +               bond_mode_8023ad_activate_slave(bond_dev, internals-
> >active_slaves[i]);
> 
>         return 0;
> }
> 

I have tested this, and it alleviates some of the issues I was reporting earlier with link aggregator selection.

Tested-by: Kyle Larose <klarose@sandvine.com>

Thanks,

Kyle
Doherty, Declan Dec. 12, 2017, 3:58 p.m. UTC | #4 
On 20/11/2017 7:15 AM, Lilijun (Jerry) wrote:
> Hi all,
> 
> In the function bond_mode_8023ad_enable(), the var i is to used to as the second parameter to pass the slave dev's dpdk port id to the function bond_mode_8023ad_activate_slave().
> I think this variable is only a index for array internals->active_slaves. So its need to be fixed and change i to internals->active_slaves[i].
> 
> 
> [Patch] net/bonding: fix bond 8023ad mode enable using wrong index.
> Signed-off-by: Lilijun <jerry.lilijun@huawei.com>
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index a2313b3..20a08dc 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1159,7 +1159,7 @@
>          uint8_t i;
> 
>          for (i = 0; i < internals->active_slave_count; i++)
> -               bond_mode_8023ad_activate_slave(bond_dev, i);
> +               bond_mode_8023ad_activate_slave(bond_dev, internals->active_slaves[i]);
> 
>          return 0;
> }
> 
> 

Acked-by: Declan Doherty <declan.doherty@intel.com>
Ferruh Yigit Dec. 15, 2017, 8:48 p.m. UTC | #5 
On 12/12/2017 7:58 AM, Doherty, Declan wrote:
> On 20/11/2017 7:15 AM, Lilijun (Jerry) wrote:
>> Hi all,
>>
>> In the function bond_mode_8023ad_enable(), the var i is to used to as the second parameter to pass the slave dev's dpdk port id to the function bond_mode_8023ad_activate_slave().
>> I think this variable is only a index for array internals->active_slaves. So its need to be fixed and change i to internals->active_slaves[i].

    net/bonding: fix activated slave in 8023ad mode

    In the function bond_mode_8023ad_enable(), the var i is used as slave
    port id to the function bond_mode_8023ad_activate_slave().

    This variable is only a index for array internals->active_slaves.
    So its need to be fixed and change i to internals->active_slaves[i].

    Fixes: 46fb43683679 ("bond: add mode 4")
    Cc: stable@dpdk.org

>>
>>
>> [Patch] net/bonding: fix bond 8023ad mode enable using wrong index.
>> Signed-off-by: Lilijun <jerry.lilijun@huawei.com>

Tested-by: Kyle Larose <klarose@sandvine.com>

> Acked-by: Declan Doherty <declan.doherty@intel.com>

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

Patch

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index a2313b3..20a08dc 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -1159,7 +1159,7 @@ 
        uint8_t i;

        for (i = 0; i < internals->active_slave_count; i++)
-               bond_mode_8023ad_activate_slave(bond_dev, i);
+               bond_mode_8023ad_activate_slave(bond_dev, internals->active_slaves[i]);

        return 0;
}