[dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC address replay

[dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC address replay

[Steve Shin]

This patch fixes a bug in replaying MAC address to the hardware
in rte_eth_dev_config_restore() routine.

Signed-off-by: Steve Shin <jonshin@cisco.com>
---
 lib/librte_ether/rte_ethdev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 4790faf..7e01f10 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -951,10 +951,12 @@ rte_eth_dev_config_restore(uint8_t port_id)
 			continue;
 
 		/* add address to the hardware */
-		if  (*dev->dev_ops->mac_addr_add &&
-			(dev->data->mac_pool_sel[i] & (1ULL << pool)))
-			(*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
-		else {
+		if  (*dev->dev_ops->mac_addr_add) {
+			if (dev->data->mac_pool_sel[i] & (1ULL << pool))
+				(*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
+			else
+				continue;
+		} else {
 			RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n",
 					port_id);
 			/* exit the loop but not return an error */
-- 
2.9.3

Re: [dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC address replay

[Steve Shin (jonshin)]

Dear maintainer, 

Sorry that I forgot to add “Fixes:” line as follows:

       Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")

Can you please add the above line as part of comment?

Thanks,
Steve

On 1/19/17, 10:47 AM, "Steve Shin (jonshin)" <jonshin@cisco.com> wrote:

    This patch fixes a bug in replaying MAC address to the hardware
    in rte_eth_dev_config_restore() routine.
    
    Signed-off-by: Steve Shin <jonshin@cisco.com>
    ---
     lib/librte_ether/rte_ethdev.c | 10 ++++++----
     1 file changed, 6 insertions(+), 4 deletions(-)
    
    diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
    index 4790faf..7e01f10 100644
    --- a/lib/librte_ether/rte_ethdev.c
    +++ b/lib/librte_ether/rte_ethdev.c
    @@ -951,10 +951,12 @@ rte_eth_dev_config_restore(uint8_t port_id)
     			continue;
     
     		/* add address to the hardware */
    -		if  (*dev->dev_ops->mac_addr_add &&
    -			(dev->data->mac_pool_sel[i] & (1ULL << pool)))
    -			(*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
    -		else {
    +		if  (*dev->dev_ops->mac_addr_add) {
    +			if (dev->data->mac_pool_sel[i] & (1ULL << pool))
    +				(*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
    +			else
    +				continue;
    +		} else {
     			RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n",
     					port_id);
     			/* exit the loop but not return an error */
    -- 
    2.9.3
    
    

Re: [dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC address replay

[Igor Ryzhov]

Hello Steve,

Thank you for the patch.

I think a couple of improvements can be done:
1. Function existence check – if (*dev->dev_ops->mac_addr_add) – can be
taken out of the loop. We don't need to check it on each iteration.
2. I'm not completely sure, but I think loop can be started from 1, not
from 0, because mac_pool_sel[0] is always zero. Am I right?

Best regards,
Igor

On Thu, Jan 19, 2017 at 10:35 PM, Steve Shin (jonshin) <jonshin@cisco.com>
wrote:

> Dear maintainer,
>
> Sorry that I forgot to add “Fixes:” line as follows:
>
>        Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")
>
> Can you please add the above line as part of comment?
>
> Thanks,
> Steve
>
> On 1/19/17, 10:47 AM, "Steve Shin (jonshin)" <jonshin@cisco.com> wrote:
>
>     This patch fixes a bug in replaying MAC address to the hardware
>     in rte_eth_dev_config_restore() routine.
>
>     Signed-off-by: Steve Shin <jonshin@cisco.com>
>     ---
>      lib/librte_ether/rte_ethdev.c | 10 ++++++----
>      1 file changed, 6 insertions(+), 4 deletions(-)
>
>     diff --git a/lib/librte_ether/rte_ethdev.c
> b/lib/librte_ether/rte_ethdev.c
>     index 4790faf..7e01f10 100644
>     --- a/lib/librte_ether/rte_ethdev.c
>     +++ b/lib/librte_ether/rte_ethdev.c
>     @@ -951,10 +951,12 @@ rte_eth_dev_config_restore(uint8_t port_id)
>                         continue;
>
>                 /* add address to the hardware */
>     -           if  (*dev->dev_ops->mac_addr_add &&
>     -                   (dev->data->mac_pool_sel[i] & (1ULL << pool)))
>     -                   (*dev->dev_ops->mac_addr_add)(dev, &addr, i,
> pool);
>     -           else {
>     +           if  (*dev->dev_ops->mac_addr_add) {
>     +                   if (dev->data->mac_pool_sel[i] & (1ULL << pool))
>     +                           (*dev->dev_ops->mac_addr_add)(dev, &addr,
> i, pool);
>     +                   else
>     +                           continue;
>     +           } else {
>                         RTE_PMD_DEBUG_TRACE("port %d: MAC address array
> not supported\n",
>                                         port_id);
>                         /* exit the loop but not return an error */
>     --
>     2.9.3
>
>
>
>

Re: [dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC address replay

[Steve Shin (jonshin)]

Thanks Igor for your comments!

A good point on the first item.
For the second question, I walked through device initialization routines to see any other potential issue.
I found one case where index 0 is still required to replay MAC address; ex) MAC address change on the PHY port – index 0
No code path was found to restore a changed MAC value with index 0. Therefore, we may have to use the existing rte_eth_dev_config_restore() function to replay 0 indexed MAC.
Currently rte_eth_dev_default_mac_addr_set() is called to program PHY MAC without setting mac_pool_sel for 0 index.
So the following code is also required inside rte_eth_dev_default_mac_addr_set() routine as a complete solution:
@@ -2237,6 +2234,9 @@ struct rte_eth_dev *

        (*dev->dev_ops->mac_addr_set)(dev, addr);

+       /* Update pool bitmap in NIC data structure */
+       dev->data->mac_pool_sel[0] = 1;
+
        return 0;

Any thoughts would be appreciated.

Regards,
Steve

From: Igor Ryzhov <iryzhov@nfware.com>
Date: Thursday, January 19, 2017 at 2:39 PM
To: Steve Shin <jonshin@cisco.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "ferruh.yigit@intel.com" <ferruh.yigit@intel.com>
Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC address replay

Hello Steve,

Thank you for the patch.

I think a couple of improvements can be done:
1. Function existence check – if (*dev->dev_ops->mac_addr_add) – can be taken out of the loop. We don't need to check it on each iteration.
2. I'm not completely sure, but I think loop can be started from 1, not from 0, because mac_pool_sel[0] is always zero. Am I right?

Best regards,
Igor

On Thu, Jan 19, 2017 at 10:35 PM, Steve Shin (jonshin) <jonshin@cisco.com<mailto:jonshin@cisco.com>> wrote:
Dear maintainer,

Sorry that I forgot to add “Fixes:” line as follows:

       Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")

Can you please add the above line as part of comment?

Thanks,
Steve

On 1/19/17, 10:47 AM, "Steve Shin (jonshin)" <jonshin@cisco.com<mailto:jonshin@cisco.com>> wrote:

    This patch fixes a bug in replaying MAC address to the hardware
    in rte_eth_dev_config_restore() routine.

    Signed-off-by: Steve Shin <jonshin@cisco.com<mailto:jonshin@cisco.com>>
    ---
     lib/librte_ether/rte_ethdev.c | 10 ++++++----
     1 file changed, 6 insertions(+), 4 deletions(-)

    diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
    index 4790faf..7e01f10 100644
    --- a/lib/librte_ether/rte_ethdev.c
    +++ b/lib/librte_ether/rte_ethdev.c
    @@ -951,10 +951,12 @@ rte_eth_dev_config_restore(uint8_t port_id)
                        continue;

                /* add address to the hardware */
    -           if  (*dev->dev_ops->mac_addr_add &&
    -                   (dev->data->mac_pool_sel[i] & (1ULL << pool)))
    -                   (*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
    -           else {
    +           if  (*dev->dev_ops->mac_addr_add) {
    +                   if (dev->data->mac_pool_sel[i] & (1ULL << pool))
    +                           (*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
    +                   else
    +                           continue;
    +           } else {
                        RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n",
                                        port_id);
                        /* exit the loop but not return an error */
    --
    2.9.3

Re: [dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC address replay

[Igor Ryzhov]

Hello Steve,

I think it's not the right solution, because if we want to restore default
MAC address, it should be done using dev->dev_ops->mac_addr_set(), not
using dev->dev_ops->mac_addr_add().

I think that right solution may be to store default MAC address in separate
variable dev->data->default_mac_addr, not in dev->data->mac_addrs array.
And, accordingly, restore default_mac_addr using mac_addr_set(), and
restore mac_addrs array using mac_addr_add().

Best regards,
Igor

On Fri, Jan 20, 2017 at 5:30 AM, Steve Shin (jonshin) <jonshin@cisco.com>
wrote:

> Thanks Igor for your comments!
>
>
>
> A good point on the first item.
>
> For the second question, I walked through device initialization routines
> to see any other potential issue.
>
> I found one case where index 0 is still required to replay MAC address;
> ex) MAC address change on the PHY port – index 0
>
> No code path was found to restore a changed MAC value with index 0.
> Therefore, we may have to use the existing rte_eth_dev_config_restore()
> function to replay 0 indexed MAC.
>
> Currently rte_eth_dev_default_mac_addr_set() is called to program PHY MAC
> without setting mac_pool_sel for 0 index.
>
> So the following code is also required inside rte_eth_dev_default_mac_addr_set()
> routine as a complete solution:
>
> @@ -2237,6 +2234,9 @@ struct rte_eth_dev *
>
>
>
>         (*dev->dev_ops->mac_addr_set)(dev, addr);
>
>
>
> +       /* Update pool bitmap in NIC data structure */
>
> +       dev->data->mac_pool_sel[0] = 1;
>
> +
>
>         return 0;
>
>
>
> Any thoughts would be appreciated.
>
>
>
> Regards,
>
> Steve
>
>
>
> *From: *Igor Ryzhov <iryzhov@nfware.com>
> *Date: *Thursday, January 19, 2017 at 2:39 PM
> *To: *Steve Shin <jonshin@cisco.com>
> *Cc: *"dev@dpdk.org" <dev@dpdk.org>, "ferruh.yigit@intel.com" <
> ferruh.yigit@intel.com>
> *Subject: *Re: [dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC
> address replay
>
>
>
> Hello Steve,
>
>
>
> Thank you for the patch.
>
>
>
> I think a couple of improvements can be done:
>
> 1. Function existence check – if (*dev->dev_ops->mac_addr_add) – can be
> taken out of the loop. We don't need to check it on each iteration.
>
> 2. I'm not completely sure, but I think loop can be started from 1, not
> from 0, because mac_pool_sel[0] is always zero. Am I right?
>
>
>
> Best regards,
>
> Igor
>
>
>
> On Thu, Jan 19, 2017 at 10:35 PM, Steve Shin (jonshin) <jonshin@cisco.com>
> wrote:
>
> Dear maintainer,
>
> Sorry that I forgot to add “Fixes:” line as follows:
>
>        Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")
>
> Can you please add the above line as part of comment?
>
> Thanks,
> Steve
>
>
> On 1/19/17, 10:47 AM, "Steve Shin (jonshin)" <jonshin@cisco.com> wrote:
>
>     This patch fixes a bug in replaying MAC address to the hardware
>     in rte_eth_dev_config_restore() routine.
>
>     Signed-off-by: Steve Shin <jonshin@cisco.com>
>     ---
>      lib/librte_ether/rte_ethdev.c | 10 ++++++----
>      1 file changed, 6 insertions(+), 4 deletions(-)
>
>     diff --git a/lib/librte_ether/rte_ethdev.c
> b/lib/librte_ether/rte_ethdev.c
>     index 4790faf..7e01f10 100644
>     --- a/lib/librte_ether/rte_ethdev.c
>     +++ b/lib/librte_ether/rte_ethdev.c
>     @@ -951,10 +951,12 @@ rte_eth_dev_config_restore(uint8_t port_id)
>                         continue;
>
>                 /* add address to the hardware */
>     -           if  (*dev->dev_ops->mac_addr_add &&
>     -                   (dev->data->mac_pool_sel[i] & (1ULL << pool)))
>     -                   (*dev->dev_ops->mac_addr_add)(dev, &addr, i,
> pool);
>     -           else {
>     +           if  (*dev->dev_ops->mac_addr_add) {
>     +                   if (dev->data->mac_pool_sel[i] & (1ULL << pool))
>     +                           (*dev->dev_ops->mac_addr_add)(dev, &addr,
> i, pool);
>     +                   else
>     +                           continue;
>     +           } else {
>                         RTE_PMD_DEBUG_TRACE("port %d: MAC address array
> not supported\n",
>                                         port_id);
>                         /* exit the loop but not return an error */
>     --
>     2.9.3
>
>
>
>

Re: [dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC address replay

[Steve Shin (jonshin)]

Dear Igor,

That makes sense to use mac_addr_set() for default MAC address replay.

As default MAC is kept in dev->data->mac_addrs[0], we may not need to create a separate variable for default one.
Will run some unit test with new change and upload a new diff for your review.

FYI: Ferruh recommended to change tag in patch subject title. So next patch will have a different tile as “ethdev: fix MAC address replay”.

Thanks & Regards,
Steve

From: Igor Ryzhov <iryzhov@nfware.com>
Date: Friday, January 20, 2017 at 4:17 AM
To: Steve Shin <jonshin@cisco.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "ferruh.yigit@intel.com" <ferruh.yigit@intel.com>
Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC address replay

Hello Steve,

I think it's not the right solution, because if we want to restore default MAC address, it should be done using dev->dev_ops->mac_addr_set(), not using dev->dev_ops->mac_addr_add().

I think that right solution may be to store default MAC address in separate variable dev->data->default_mac_addr, not in dev->data->mac_addrs array.
And, accordingly, restore default_mac_addr using mac_addr_set(), and restore mac_addrs array using mac_addr_add().

Best regards,
Igor

On Fri, Jan 20, 2017 at 5:30 AM, Steve Shin (jonshin) <jonshin@cisco.com<mailto:jonshin@cisco.com>> wrote:
Thanks Igor for your comments!

A good point on the first item.
For the second question, I walked through device initialization routines to see any other potential issue.
I found one case where index 0 is still required to replay MAC address; ex) MAC address change on the PHY port – index 0
No code path was found to restore a changed MAC value with index 0. Therefore, we may have to use the existing rte_eth_dev_config_restore() function to replay 0 indexed MAC.
Currently rte_eth_dev_default_mac_addr_set() is called to program PHY MAC without setting mac_pool_sel for 0 index.
So the following code is also required inside rte_eth_dev_default_mac_addr_set() routine as a complete solution:
@@ -2237,6 +2234,9 @@ struct rte_eth_dev *

        (*dev->dev_ops->mac_addr_set)(dev, addr);

+       /* Update pool bitmap in NIC data structure */
+       dev->data->mac_pool_sel[0] = 1;
+
        return 0;

Any thoughts would be appreciated.

Regards,
Steve

From: Igor Ryzhov <iryzhov@nfware.com<mailto:iryzhov@nfware.com>>
Date: Thursday, January 19, 2017 at 2:39 PM
To: Steve Shin <jonshin@cisco.com<mailto:jonshin@cisco.com>>
Cc: "dev@dpdk.org<mailto:dev@dpdk.org>" <dev@dpdk.org<mailto:dev@dpdk.org>>, "ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>" <ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>>
Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC address replay

Hello Steve,

Thank you for the patch.

I think a couple of improvements can be done:
1. Function existence check – if (*dev->dev_ops->mac_addr_add) – can be taken out of the loop. We don't need to check it on each iteration.
2. I'm not completely sure, but I think loop can be started from 1, not from 0, because mac_pool_sel[0] is always zero. Am I right?

Best regards,
Igor

On Thu, Jan 19, 2017 at 10:35 PM, Steve Shin (jonshin) <jonshin@cisco.com<mailto:jonshin@cisco.com>> wrote:
Dear maintainer,

Sorry that I forgot to add “Fixes:” line as follows:

       Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")

Can you please add the above line as part of comment?

Thanks,
Steve

On 1/19/17, 10:47 AM, "Steve Shin (jonshin)" <jonshin@cisco.com<mailto:jonshin@cisco.com>> wrote:

    This patch fixes a bug in replaying MAC address to the hardware
    in rte_eth_dev_config_restore() routine.

    Signed-off-by: Steve Shin <jonshin@cisco.com<mailto:jonshin@cisco.com>>
    ---
     lib/librte_ether/rte_ethdev.c | 10 ++++++----
     1 file changed, 6 insertions(+), 4 deletions(-)

    diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
    index 4790faf..7e01f10 100644
    --- a/lib/librte_ether/rte_ethdev.c
    +++ b/lib/librte_ether/rte_ethdev.c
    @@ -951,10 +951,12 @@ rte_eth_dev_config_restore(uint8_t port_id)
                        continue;

                /* add address to the hardware */
    -           if  (*dev->dev_ops->mac_addr_add &&
    -                   (dev->data->mac_pool_sel[i] & (1ULL << pool)))
    -                   (*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
    -           else {
    +           if  (*dev->dev_ops->mac_addr_add) {
    +                   if (dev->data->mac_pool_sel[i] & (1ULL << pool))
    +                           (*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
    +                   else
    +                           continue;
    +           } else {
                        RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n",
                                        port_id);
                        /* exit the loop but not return an error */
    --
    2.9.3

[dpdk-dev] [PATCH v2] ethdev: fix MAC address replay

[Steve Shin]

This patch fixes a bug in replaying MAC address to the hardware
in rte_eth_dev_config_restore() routine. Added default MAC replay as well.

Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")

---
v2: Added default MAC replay & Code optimization

Signed-off-by: Steve Shin <jonshin@cisco.com>
---
 lib/librte_ether/rte_ethdev.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 4790faf..150f350 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -931,7 +931,7 @@ rte_eth_dev_config_restore(uint8_t port_id)
 {
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
-	struct ether_addr addr;
+	struct ether_addr *addr;
 	uint16_t i;
 	uint32_t pool = 0;
 
@@ -942,23 +942,23 @@ rte_eth_dev_config_restore(uint8_t port_id)
 	if (RTE_ETH_DEV_SRIOV(dev).active)
 		pool = RTE_ETH_DEV_SRIOV(dev).def_vmdq_idx;
 
-	/* replay MAC address configuration */
-	for (i = 0; i < dev_info.max_mac_addrs; i++) {
-		addr = dev->data->mac_addrs[i];
+	/* replay MAC address configuration including default MAC */
+	if (*dev->dev_ops->mac_addr_set != NULL) {
+		addr = &dev->data->mac_addrs[0];
+		(*dev->dev_ops->mac_addr_set)(dev, addr);
+	}
 
-		/* skip zero address */
-		if (is_zero_ether_addr(&addr))
-			continue;
+	if (*dev->dev_ops->mac_addr_add != NULL) {
+		for (i = 1; i < dev_info.max_mac_addrs; i++) {
+			addr = &dev->data->mac_addrs[i];
 
-		/* add address to the hardware */
-		if  (*dev->dev_ops->mac_addr_add &&
-			(dev->data->mac_pool_sel[i] & (1ULL << pool)))
-			(*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
-		else {
-			RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n",
-					port_id);
-			/* exit the loop but not return an error */
-			break;
+			/* skip zero address */
+			if (is_zero_ether_addr(addr))
+				continue;
+
+			/* add address to the hardware */
+			if (dev->data->mac_pool_sel[i] & (1ULL << pool))
+				(*dev->dev_ops->mac_addr_add)(dev, addr, i, pool);
 		}
 	}
 
-- 
2.9.3

Re: [dpdk-dev] [PATCH v2] ethdev: fix MAC address replay

[Igor Ryzhov]

Hello Steve,

Thank you for all the fixes.
But I think I noticed another one issue in MAC replay process.

Pool number is extracted only once:

if (RTE_ETH_DEV_SRIOV(dev).active)
pool = RTE_ETH_DEV_SRIOV(dev).def_vmdq_idx;

But when MAC address is added using rte_eth_dev_mac_addr_add several
different pool numbers can be used.
Shouldn't we extract pool number for each MAC address separately from
mac_pool_sel array during restoration process?

Best regards,
Igor

On Sat, Jan 21, 2017 at 1:23 AM, Steve Shin <jonshin@cisco.com> wrote:

> This patch fixes a bug in replaying MAC address to the hardware
> in rte_eth_dev_config_restore() routine. Added default MAC replay as well.
>
> Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")
>
> ---
> v2: Added default MAC replay & Code optimization
>
> Signed-off-by: Steve Shin <jonshin@cisco.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 4790faf..150f350 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -931,7 +931,7 @@ rte_eth_dev_config_restore(uint8_t port_id)
>  {
>         struct rte_eth_dev *dev;
>         struct rte_eth_dev_info dev_info;
> -       struct ether_addr addr;
> +       struct ether_addr *addr;
>         uint16_t i;
>         uint32_t pool = 0;
>
> @@ -942,23 +942,23 @@ rte_eth_dev_config_restore(uint8_t port_id)
>         if (RTE_ETH_DEV_SRIOV(dev).active)
>                 pool = RTE_ETH_DEV_SRIOV(dev).def_vmdq_idx;
>
> -       /* replay MAC address configuration */
> -       for (i = 0; i < dev_info.max_mac_addrs; i++) {
> -               addr = dev->data->mac_addrs[i];
> +       /* replay MAC address configuration including default MAC */
> +       if (*dev->dev_ops->mac_addr_set != NULL) {
> +               addr = &dev->data->mac_addrs[0];
> +               (*dev->dev_ops->mac_addr_set)(dev, addr);
> +       }
>
> -               /* skip zero address */
> -               if (is_zero_ether_addr(&addr))
> -                       continue;
> +       if (*dev->dev_ops->mac_addr_add != NULL) {
> +               for (i = 1; i < dev_info.max_mac_addrs; i++) {
> +                       addr = &dev->data->mac_addrs[i];
>
> -               /* add address to the hardware */
> -               if  (*dev->dev_ops->mac_addr_add &&
> -                       (dev->data->mac_pool_sel[i] & (1ULL << pool)))
> -                       (*dev->dev_ops->mac_addr_add)(dev, &addr, i,
> pool);
> -               else {
> -                       RTE_PMD_DEBUG_TRACE("port %d: MAC address array
> not supported\n",
> -                                       port_id);
> -                       /* exit the loop but not return an error */
> -                       break;
> +                       /* skip zero address */
> +                       if (is_zero_ether_addr(addr))
> +                               continue;
> +
> +                       /* add address to the hardware */
> +                       if (dev->data->mac_pool_sel[i] & (1ULL << pool))
> +                               (*dev->dev_ops->mac_addr_add)(dev, addr,
> i, pool);
>                 }
>         }
>
> --
> 2.9.3
>
>

Re: [dpdk-dev] [PATCH v2] ethdev: fix MAC address replay

[Steve Shin (jonshin)]

Dear Igor,

Yes, you’re right. We need to handle a case (ex, SR-IOV) where multiple pools exist in the mac_pool_sel array.
A new diff file will be uploaded with PATCH v3.

Thanks & Regards,
Steve

From: Igor Ryzhov <iryzhov@nfware.com>
Date: Monday, January 23, 2017 at 12:50 AM
To: Steve Shin <jonshin@cisco.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Ferruh Yigit <ferruh.yigit@intel.com>
Subject: Re: [PATCH v2] ethdev: fix MAC address replay

Hello Steve,

Thank you for all the fixes.
But I think I noticed another one issue in MAC replay process.

Pool number is extracted only once:

if (RTE_ETH_DEV_SRIOV(dev).active)
pool = RTE_ETH_DEV_SRIOV(dev).def_vmdq_idx;

But when MAC address is added using rte_eth_dev_mac_addr_add several different pool numbers can be used.
Shouldn't we extract pool number for each MAC address separately from mac_pool_sel array during restoration process?

Best regards,
Igor

On Sat, Jan 21, 2017 at 1:23 AM, Steve Shin <jonshin@cisco.com<mailto:jonshin@cisco.com>> wrote:
This patch fixes a bug in replaying MAC address to the hardware
in rte_eth_dev_config_restore() routine. Added default MAC replay as well.

Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")

---
v2: Added default MAC replay & Code optimization

Signed-off-by: Steve Shin <jonshin@cisco.com<mailto:jonshin@cisco.com>>
---
 lib/librte_ether/rte_ethdev.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 4790faf..150f350 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -931,7 +931,7 @@ rte_eth_dev_config_restore(uint8_t port_id)
 {
        struct rte_eth_dev *dev;
        struct rte_eth_dev_info dev_info;
-       struct ether_addr addr;
+       struct ether_addr *addr;
        uint16_t i;
        uint32_t pool = 0;

@@ -942,23 +942,23 @@ rte_eth_dev_config_restore(uint8_t port_id)
        if (RTE_ETH_DEV_SRIOV(dev).active)
                pool = RTE_ETH_DEV_SRIOV(dev).def_vmdq_idx;

-       /* replay MAC address configuration */
-       for (i = 0; i < dev_info.max_mac_addrs; i++) {
-               addr = dev->data->mac_addrs[i];
+       /* replay MAC address configuration including default MAC */
+       if (*dev->dev_ops->mac_addr_set != NULL) {
+               addr = &dev->data->mac_addrs[0];
+               (*dev->dev_ops->mac_addr_set)(dev, addr);
+       }

-               /* skip zero address */
-               if (is_zero_ether_addr(&addr))
-                       continue;
+       if (*dev->dev_ops->mac_addr_add != NULL) {
+               for (i = 1; i < dev_info.max_mac_addrs; i++) {
+                       addr = &dev->data->mac_addrs[i];

-               /* add address to the hardware */
-               if  (*dev->dev_ops->mac_addr_add &&
-                       (dev->data->mac_pool_sel[i] & (1ULL << pool)))
-                       (*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
-               else {
-                       RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n",
-                                       port_id);
-                       /* exit the loop but not return an error */
-                       break;
+                       /* skip zero address */
+                       if (is_zero_ether_addr(addr))
+                               continue;
+
+                       /* add address to the hardware */
+                       if (dev->data->mac_pool_sel[i] & (1ULL << pool))
+                               (*dev->dev_ops->mac_addr_add)(dev, addr, i, pool);
                }
        }

--
2.9.3

[dpdk-dev] [PATCH v3] ethdev: fix MAC address replay

[Steve Shin]

    This patch fixes a bug in replaying MAC address to the hardware
    in rte_eth_dev_config_restore() routine. Added default MAC replay as well.

    Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")

    ---
    v2: Added default MAC replay & Code optimization
    V3: Covered a case (ex, SR-IOV) where multiple pools
        exist in the mac_pool_sel array.

Signed-off-by: Steve Shin <jonshin@cisco.com>
---
 lib/librte_ether/rte_ethdev.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 4790faf..fbc0765 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -931,34 +931,38 @@ rte_eth_dev_config_restore(uint8_t port_id)
 {
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
-	struct ether_addr addr;
+	struct ether_addr *addr;
 	uint16_t i;
-	uint32_t pool = 0;
+	uint32_t pool;
+	uint64_t pool_mask;
 
 	dev = &rte_eth_devices[port_id];
 
 	rte_eth_dev_info_get(port_id, &dev_info);
 
-	if (RTE_ETH_DEV_SRIOV(dev).active)
-		pool = RTE_ETH_DEV_SRIOV(dev).def_vmdq_idx;
+	/* replay MAC address configuration including default MAC */
+	if (*dev->dev_ops->mac_addr_set != NULL) {
+		addr = &dev->data->mac_addrs[0];
+		(*dev->dev_ops->mac_addr_set)(dev, addr);
+	}
 
-	/* replay MAC address configuration */
-	for (i = 0; i < dev_info.max_mac_addrs; i++) {
-		addr = dev->data->mac_addrs[i];
+	if (*dev->dev_ops->mac_addr_add != NULL) {
+		for (i = 1; i < dev_info.max_mac_addrs; i++) {
+			addr = &dev->data->mac_addrs[i];
 
-		/* skip zero address */
-		if (is_zero_ether_addr(&addr))
-			continue;
+			/* skip zero address */
+			if (is_zero_ether_addr(addr))
+				continue;
 
-		/* add address to the hardware */
-		if  (*dev->dev_ops->mac_addr_add &&
-			(dev->data->mac_pool_sel[i] & (1ULL << pool)))
-			(*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
-		else {
-			RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n",
-					port_id);
-			/* exit the loop but not return an error */
-			break;
+			pool = 0;
+			pool_mask = dev->data->mac_pool_sel[i];
+
+			do {
+				if (pool_mask & 1ULL)
+					(*dev->dev_ops->mac_addr_add)(dev, addr, i, pool);
+				pool_mask >>= 1;
+				pool++;
+			} while (pool_mask);
 		}
 	}
 
-- 
2.9.3

[dpdk-dev] [PATCH v4] ethdev: fix MAC address replay

[Steve Shin]

This patch fixes a bug in replaying MAC address to the hardware
in rte_eth_dev_config_restore() routine. Added default MAC replay as well.

Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")

---
v2: Added default MAC replay & Code optimization
v3: Covered a case (ex, SR-IOV) where multiple pools
       exist in the mac_pool_sel array.
v4: removed a coding style warning

Signed-off-by: Steve Shin <jonshin@cisco.com>
---
 lib/librte_ether/rte_ethdev.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 4790faf..166ffa6 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -931,34 +931,39 @@ rte_eth_dev_config_restore(uint8_t port_id)
 {
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
-	struct ether_addr addr;
+	struct ether_addr *addr;
 	uint16_t i;
-	uint32_t pool = 0;
+	uint32_t pool;
+	uint64_t pool_mask;
 
 	dev = &rte_eth_devices[port_id];
 
 	rte_eth_dev_info_get(port_id, &dev_info);
 
-	if (RTE_ETH_DEV_SRIOV(dev).active)
-		pool = RTE_ETH_DEV_SRIOV(dev).def_vmdq_idx;
+	/* replay MAC address configuration including default MAC */
+	if (*dev->dev_ops->mac_addr_set != NULL) {
+		addr = &dev->data->mac_addrs[0];
+		(*dev->dev_ops->mac_addr_set)(dev, addr);
+	}
 
-	/* replay MAC address configuration */
-	for (i = 0; i < dev_info.max_mac_addrs; i++) {
-		addr = dev->data->mac_addrs[i];
+	if (*dev->dev_ops->mac_addr_add != NULL) {
+		for (i = 1; i < dev_info.max_mac_addrs; i++) {
+			addr = &dev->data->mac_addrs[i];
 
-		/* skip zero address */
-		if (is_zero_ether_addr(&addr))
-			continue;
+			/* skip zero address */
+			if (is_zero_ether_addr(addr))
+				continue;
 
-		/* add address to the hardware */
-		if  (*dev->dev_ops->mac_addr_add &&
-			(dev->data->mac_pool_sel[i] & (1ULL << pool)))
-			(*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
-		else {
-			RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n",
-					port_id);
-			/* exit the loop but not return an error */
-			break;
+			pool = 0;
+			pool_mask = dev->data->mac_pool_sel[i];
+
+			do {
+				if (pool_mask & 1ULL)
+					(*dev->dev_ops->mac_addr_add)(dev,
+						 addr, i, pool);
+				pool_mask >>= 1;
+				pool++;
+			} while (pool_mask);
 		}
 	}
 
-- 
2.9.3

Re: [dpdk-dev] [PATCH v4] ethdev: fix MAC address replay

[Igor Ryzhov]

Thank you Steve.

I never did it before and I don't know if I have rights for that, but:

Acked-by: Igor Ryzhov <iryzhov@nfware.com>

On Tue, Jan 24, 2017 at 5:21 AM, Steve Shin <jonshin@cisco.com> wrote:

> This patch fixes a bug in replaying MAC address to the hardware
> in rte_eth_dev_config_restore() routine. Added default MAC replay as well.
>
> Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")
>
> ---
> v2: Added default MAC replay & Code optimization
> v3: Covered a case (ex, SR-IOV) where multiple pools
>        exist in the mac_pool_sel array.
> v4: removed a coding style warning
>
> Signed-off-by: Steve Shin <jonshin@cisco.com>
>

Re: [dpdk-dev] [PATCH v4] ethdev: fix MAC address replay

[Ferruh Yigit]

On 1/24/2017 10:09 AM, Igor Ryzhov wrote:
> Thank you Steve.
> 

> I never did it before and I don't know if I have rights for that, but:
> 
> Acked-by: Igor Ryzhov <iryzhov@nfware.com <mailto:iryzhov@nfware.com>>

Unrelated to the patch itself, but since it has been mentioned, let me
share what I know, I believe Thomas or others will correct me if I am wrong:

- Everyone can Ack.
  And this is useful information for maintainers, so it is something
good when more people review and ack. Please do.

- Multiple ack or review is better.

- But each Ack does not have same weight, maintainer decides on this
weight, based on contribution of the person who ack'ed.

- There is slight difference between Acked-by and Reviewed-by:

-- Acked-by: Kind of asking for patch to be applied, saying this patch
is good and please get it.

-- Reviewed-by: Saying I have done the review at my best and patch looks
good to me.

Acked-by has slightly more responsibility than Reviewed-by.

If you are not maintainer of that field, and not have strong opinion
about that patch to be merged, it is possible to prefer Reviewed-by
against Acked-by.

But overall both are good, and definitely better than not saying
anything at all.

Thanks,
ferruh

> 
> On Tue, Jan 24, 2017 at 5:21 AM, Steve Shin <jonshin@cisco.com
> <mailto:jonshin@cisco.com>> wrote:
> 
>     This patch fixes a bug in replaying MAC address to the hardware
>     in rte_eth_dev_config_restore() routine. Added default MAC replay as
>     well.
> 
>     Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")
> 
>     ---
>     v2: Added default MAC replay & Code optimization
>     v3: Covered a case (ex, SR-IOV) where multiple pools
>            exist in the mac_pool_sel array.
>     v4: removed a coding style warning
> 
>     Signed-off-by: Steve Shin <jonshin@cisco.com <mailto:jonshin@cisco.com>>
> 

Re: [dpdk-dev] [PATCH v4] ethdev: fix MAC address replay

[Igor Ryzhov]

Hello Ferruh,

Thanks for the explanation.

I tried to find something like that in "Contribution Guidelines" and found
that both "Acked-by" and "Reviewed-by" are just mentioned but not explained.
Meaning of these sentences can be different in different projects so it can
be good to explain it in DPDK development guidelines.

Best regards,
Igor

On Tue, Jan 24, 2017 at 4:21 PM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 1/24/2017 10:09 AM, Igor Ryzhov wrote:
> > Thank you Steve.
> >
>
> > I never did it before and I don't know if I have rights for that, but:
> >
> > Acked-by: Igor Ryzhov <iryzhov@nfware.com <mailto:iryzhov@nfware.com>>
>
> Unrelated to the patch itself, but since it has been mentioned, let me
> share what I know, I believe Thomas or others will correct me if I am
> wrong:
>
> - Everyone can Ack.
>   And this is useful information for maintainers, so it is something
> good when more people review and ack. Please do.
>
> - Multiple ack or review is better.
>
> - But each Ack does not have same weight, maintainer decides on this
> weight, based on contribution of the person who ack'ed.
>
> - There is slight difference between Acked-by and Reviewed-by:
>
> -- Acked-by: Kind of asking for patch to be applied, saying this patch
> is good and please get it.
>
> -- Reviewed-by: Saying I have done the review at my best and patch looks
> good to me.
>
> Acked-by has slightly more responsibility than Reviewed-by.
>
> If you are not maintainer of that field, and not have strong opinion
> about that patch to be merged, it is possible to prefer Reviewed-by
> against Acked-by.
>
> But overall both are good, and definitely better than not saying
> anything at all.
>
> Thanks,
> ferruh
>
> >
> > On Tue, Jan 24, 2017 at 5:21 AM, Steve Shin <jonshin@cisco.com
> > <mailto:jonshin@cisco.com>> wrote:
> >
> >     This patch fixes a bug in replaying MAC address to the hardware
> >     in rte_eth_dev_config_restore() routine. Added default MAC replay as
> >     well.
> >
> >     Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")
> >
> >     ---
> >     v2: Added default MAC replay & Code optimization
> >     v3: Covered a case (ex, SR-IOV) where multiple pools
> >            exist in the mac_pool_sel array.
> >     v4: removed a coding style warning
> >
> >     Signed-off-by: Steve Shin <jonshin@cisco.com <mailto:
> jonshin@cisco.com>>
> >
>
>

Re: [dpdk-dev] [PATCH v4] ethdev: fix MAC address replay

[Thomas Monjalon]

2017-01-24 13:21, Ferruh Yigit:
> On 1/24/2017 10:09 AM, Igor Ryzhov wrote:
> > Thank you Steve.
> > 
> 
> > I never did it before and I don't know if I have rights for that, but:
> > 
> > Acked-by: Igor Ryzhov <iryzhov@nfware.com <mailto:iryzhov@nfware.com>>
> 
> Unrelated to the patch itself, but since it has been mentioned, let me
> share what I know, I believe Thomas or others will correct me if I am wrong:
> 
> - Everyone can Ack.
>   And this is useful information for maintainers, so it is something
> good when more people review and ack. Please do.
> 
> - Multiple ack or review is better.
> 
> - But each Ack does not have same weight, maintainer decides on this
> weight, based on contribution of the person who ack'ed.
> 
> - There is slight difference between Acked-by and Reviewed-by:
> 
> -- Acked-by: Kind of asking for patch to be applied, saying this patch
> is good and please get it.
> 
> -- Reviewed-by: Saying I have done the review at my best and patch looks
> good to me.
> 
> Acked-by has slightly more responsibility than Reviewed-by.
> 
> If you are not maintainer of that field, and not have strong opinion
> about that patch to be merged, it is possible to prefer Reviewed-by
> against Acked-by.
> 
> But overall both are good, and definitely better than not saying
> anything at all.

We should definitely better document these tags.

My view is that Reviewed-by is stronger because it says you really checked
the patch.
Acked-by means you agree with the intent and you trust the author.
Any of these tags will be stronger if it is delivered by a maintainer.

As conclusion, here you should stress you took the review job with a
Reviewed-by tag.
A maintainer is more inclined to use the Acked-by tag, even if he does
a review.

As the maintainer of ethdev, I thank you to take the review job so I won't
have to wonder which kind of regression could be in the patch.
I will just check the intent and will rely on your Reviewed-by.

[dpdk-dev] [PATCH v5] ethdev: fix MAC address replay

[Steve Shin]

This patch fixes a bug in replaying MAC address to the hardware
in rte_eth_dev_config_restore() routine. Added default MAC replay as well.

Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")

---
v2: Added default MAC replay & Code optimization.
v3: Covered a case (ex, SR-IOV) where multiple pools
    exist in the mac_pool_sel array.
v4: removed a coding style warning.
v5: Added default MAC replay with dev_ops->mac_addr_add.

Signed-off-by: Steve Shin <jonshin@cisco.com>
---
 lib/librte_ether/rte_ethdev.c | 48 ++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 4790faf..15746a7 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -931,34 +931,40 @@ rte_eth_dev_config_restore(uint8_t port_id)
 {
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
-	struct ether_addr addr;
+	struct ether_addr *addr;
 	uint16_t i;
 	uint32_t pool = 0;
+	uint64_t pool_mask;
 
 	dev = &rte_eth_devices[port_id];
 
 	rte_eth_dev_info_get(port_id, &dev_info);
 
-	if (RTE_ETH_DEV_SRIOV(dev).active)
-		pool = RTE_ETH_DEV_SRIOV(dev).def_vmdq_idx;
-
-	/* replay MAC address configuration */
-	for (i = 0; i < dev_info.max_mac_addrs; i++) {
-		addr = dev->data->mac_addrs[i];
-
-		/* skip zero address */
-		if (is_zero_ether_addr(&addr))
-			continue;
-
-		/* add address to the hardware */
-		if  (*dev->dev_ops->mac_addr_add &&
-			(dev->data->mac_pool_sel[i] & (1ULL << pool)))
-			(*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
-		else {
-			RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n",
-					port_id);
-			/* exit the loop but not return an error */
-			break;
+	/* replay MAC address configuration including default MAC */
+	addr = &dev->data->mac_addrs[0];
+	if (*dev->dev_ops->mac_addr_set != NULL)
+		(*dev->dev_ops->mac_addr_set)(dev, addr);
+	else if (*dev->dev_ops->mac_addr_add != NULL)
+		(*dev->dev_ops->mac_addr_add)(dev, addr, 0, pool);
+
+	if (*dev->dev_ops->mac_addr_add != NULL) {
+		for (i = 1; i < dev_info.max_mac_addrs; i++) {
+			addr = &dev->data->mac_addrs[i];
+
+			/* skip zero address */
+			if (is_zero_ether_addr(addr))
+				continue;
+
+			pool = 0;
+			pool_mask = dev->data->mac_pool_sel[i];
+
+			do {
+				if (pool_mask & 1ULL)
+					(*dev->dev_ops->mac_addr_add)(dev,
+						addr, i, pool);
+				pool_mask >>= 1;
+				pool++;
+			} while (pool_mask);
 		}
 	}
 
-- 
2.9.3

Re: [dpdk-dev] [PATCH v5] ethdev: fix MAC address replay

[Thomas Monjalon]

2017-01-27 09:57, Steve Shin:
> This patch fixes a bug in replaying MAC address to the hardware
> in rte_eth_dev_config_restore() routine. Added default MAC replay as well.
> 
> Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements")
> 
> ---
> v2: Added default MAC replay & Code optimization.
> v3: Covered a case (ex, SR-IOV) where multiple pools
>     exist in the mac_pool_sel array.
> v4: removed a coding style warning.
> v5: Added default MAC replay with dev_ops->mac_addr_add.

The changelog should be after the SoB,
because everything after --- is removed when applying.

> Signed-off-by: Steve Shin <jonshin@cisco.com>

I've added 
Reviewed-by: Igor Ryzhov <iryzhov@nfware.com>

Applied, thanks