All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] lib/librte_ether: error handling on MAC address replay
@ 2017-01-19 18:47 Steve Shin
  2017-01-19 19:35 ` Steve Shin (jonshin)
  2017-01-20 22:23 ` [PATCH v2] ethdev: fix " Steve Shin
  0 siblings, 2 replies; 17+ messages in thread
From: Steve Shin @ 2017-01-19 18:47 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] lib/librte_ether: error handling on MAC address replay
  2017-01-19 18:47 [PATCH] lib/librte_ether: error handling on MAC address replay Steve Shin
@ 2017-01-19 19:35 ` Steve Shin (jonshin)
  2017-01-19 22:39   ` Igor Ryzhov
  2017-01-20 22:23 ` [PATCH v2] ethdev: fix " Steve Shin
  1 sibling, 1 reply; 17+ messages in thread
From: Steve Shin (jonshin) @ 2017-01-19 19:35 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit

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
    
    


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] lib/librte_ether: error handling on MAC address replay
  2017-01-19 19:35 ` Steve Shin (jonshin)
@ 2017-01-19 22:39   ` Igor Ryzhov
  2017-01-20  2:30     ` Steve Shin (jonshin)
  0 siblings, 1 reply; 17+ messages in thread
From: Igor Ryzhov @ 2017-01-19 22:39 UTC (permalink / raw)
  To: Steve Shin (jonshin); +Cc: dev, ferruh.yigit

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
>
>
>
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] lib/librte_ether: error handling on MAC address replay
  2017-01-19 22:39   ` Igor Ryzhov
@ 2017-01-20  2:30     ` Steve Shin (jonshin)
  2017-01-20 12:17       ` Igor Ryzhov
  0 siblings, 1 reply; 17+ messages in thread
From: Steve Shin (jonshin) @ 2017-01-20  2:30 UTC (permalink / raw)
  To: Igor Ryzhov; +Cc: dev, ferruh.yigit

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




^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] lib/librte_ether: error handling on MAC address replay
  2017-01-20  2:30     ` Steve Shin (jonshin)
@ 2017-01-20 12:17       ` Igor Ryzhov
  2017-01-20 19:12         ` Steve Shin (jonshin)
  0 siblings, 1 reply; 17+ messages in thread
From: Igor Ryzhov @ 2017-01-20 12:17 UTC (permalink / raw)
  To: Steve Shin (jonshin); +Cc: dev, ferruh.yigit

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
>
>
>
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] lib/librte_ether: error handling on MAC address replay
  2017-01-20 12:17       ` Igor Ryzhov
@ 2017-01-20 19:12         ` Steve Shin (jonshin)
  0 siblings, 0 replies; 17+ messages in thread
From: Steve Shin (jonshin) @ 2017-01-20 19:12 UTC (permalink / raw)
  To: Igor Ryzhov; +Cc: dev, ferruh.yigit

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




^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] ethdev: fix MAC address replay
  2017-01-19 18:47 [PATCH] lib/librte_ether: error handling on MAC address replay Steve Shin
  2017-01-19 19:35 ` Steve Shin (jonshin)
@ 2017-01-20 22:23 ` Steve Shin
  2017-01-23  8:50   ` Igor Ryzhov
  2017-01-23 23:50   ` [PATCH v3] " Steve Shin
  1 sibling, 2 replies; 17+ messages in thread
From: Steve Shin @ 2017-01-20 22:23 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, iryzhov, 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] ethdev: fix MAC address replay
  2017-01-20 22:23 ` [PATCH v2] ethdev: fix " Steve Shin
@ 2017-01-23  8:50   ` Igor Ryzhov
  2017-01-23 23:19     ` Steve Shin (jonshin)
  2017-01-23 23:50   ` [PATCH v3] " Steve Shin
  1 sibling, 1 reply; 17+ messages in thread
From: Igor Ryzhov @ 2017-01-23  8:50 UTC (permalink / raw)
  To: Steve Shin; +Cc: dev, Ferruh Yigit

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
>
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] ethdev: fix MAC address replay
  2017-01-23  8:50   ` Igor Ryzhov
@ 2017-01-23 23:19     ` Steve Shin (jonshin)
  0 siblings, 0 replies; 17+ messages in thread
From: Steve Shin (jonshin) @ 2017-01-23 23:19 UTC (permalink / raw)
  To: Igor Ryzhov; +Cc: dev, Ferruh Yigit

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3]     ethdev: fix MAC address replay
  2017-01-20 22:23 ` [PATCH v2] ethdev: fix " Steve Shin
  2017-01-23  8:50   ` Igor Ryzhov
@ 2017-01-23 23:50   ` Steve Shin
  2017-01-24  2:21     ` [PATCH v4] " Steve Shin
  1 sibling, 1 reply; 17+ messages in thread
From: Steve Shin @ 2017-01-23 23:50 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, iryzhov, 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4] ethdev: fix MAC address replay
  2017-01-23 23:50   ` [PATCH v3] " Steve Shin
@ 2017-01-24  2:21     ` Steve Shin
  2017-01-24 10:09       ` Igor Ryzhov
  2017-01-27 17:57       ` [PATCH v5] " Steve Shin
  0 siblings, 2 replies; 17+ messages in thread
From: Steve Shin @ 2017-01-24  2:21 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, iryzhov, 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4] ethdev: fix MAC address replay
  2017-01-24  2:21     ` [PATCH v4] " Steve Shin
@ 2017-01-24 10:09       ` Igor Ryzhov
  2017-01-24 13:21         ` Ferruh Yigit
  2017-01-27 17:57       ` [PATCH v5] " Steve Shin
  1 sibling, 1 reply; 17+ messages in thread
From: Igor Ryzhov @ 2017-01-24 10:09 UTC (permalink / raw)
  To: Steve Shin; +Cc: dev, Ferruh Yigit

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>
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4] ethdev: fix MAC address replay
  2017-01-24 10:09       ` Igor Ryzhov
@ 2017-01-24 13:21         ` Ferruh Yigit
  2017-01-24 14:00           ` Igor Ryzhov
  2017-01-25 10:25           ` Thomas Monjalon
  0 siblings, 2 replies; 17+ messages in thread
From: Ferruh Yigit @ 2017-01-24 13:21 UTC (permalink / raw)
  To: Igor Ryzhov, Steve Shin; +Cc: dev, Thomas Monjalon

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>>
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4] ethdev: fix MAC address replay
  2017-01-24 13:21         ` Ferruh Yigit
@ 2017-01-24 14:00           ` Igor Ryzhov
  2017-01-25 10:25           ` Thomas Monjalon
  1 sibling, 0 replies; 17+ messages in thread
From: Igor Ryzhov @ 2017-01-24 14:00 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Steve Shin, dev, Thomas Monjalon

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>>
> >
>
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4] ethdev: fix MAC address replay
  2017-01-24 13:21         ` Ferruh Yigit
  2017-01-24 14:00           ` Igor Ryzhov
@ 2017-01-25 10:25           ` Thomas Monjalon
  1 sibling, 0 replies; 17+ messages in thread
From: Thomas Monjalon @ 2017-01-25 10:25 UTC (permalink / raw)
  To: Ferruh Yigit, Igor Ryzhov; +Cc: Steve Shin, dev

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v5] ethdev: fix MAC address replay
  2017-01-24  2:21     ` [PATCH v4] " Steve Shin
  2017-01-24 10:09       ` Igor Ryzhov
@ 2017-01-27 17:57       ` Steve Shin
  2017-01-30  9:21         ` Thomas Monjalon
  1 sibling, 1 reply; 17+ messages in thread
From: Steve Shin @ 2017-01-27 17:57 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, iryzhov, 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5] ethdev: fix MAC address replay
  2017-01-27 17:57       ` [PATCH v5] " Steve Shin
@ 2017-01-30  9:21         ` Thomas Monjalon
  0 siblings, 0 replies; 17+ messages in thread
From: Thomas Monjalon @ 2017-01-30  9:21 UTC (permalink / raw)
  To: Steve Shin; +Cc: dev, ferruh.yigit, iryzhov

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2017-01-30  9:21 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-19 18:47 [PATCH] lib/librte_ether: error handling on MAC address replay Steve Shin
2017-01-19 19:35 ` Steve Shin (jonshin)
2017-01-19 22:39   ` Igor Ryzhov
2017-01-20  2:30     ` Steve Shin (jonshin)
2017-01-20 12:17       ` Igor Ryzhov
2017-01-20 19:12         ` Steve Shin (jonshin)
2017-01-20 22:23 ` [PATCH v2] ethdev: fix " Steve Shin
2017-01-23  8:50   ` Igor Ryzhov
2017-01-23 23:19     ` Steve Shin (jonshin)
2017-01-23 23:50   ` [PATCH v3] " Steve Shin
2017-01-24  2:21     ` [PATCH v4] " Steve Shin
2017-01-24 10:09       ` Igor Ryzhov
2017-01-24 13:21         ` Ferruh Yigit
2017-01-24 14:00           ` Igor Ryzhov
2017-01-25 10:25           ` Thomas Monjalon
2017-01-27 17:57       ` [PATCH v5] " Steve Shin
2017-01-30  9:21         ` Thomas Monjalon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.