[dpdk-dev,v4] ethdev: fix MAC address replay
Checks
Commit Message
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(-)
Comments
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>
>
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>>
>
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>>
> >
>
>
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.
@@ -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);
}
}