[dpdk-dev,v2,3/3] net/mlx5: implement multicast add list devop

Message ID 870d4840cdb872d363d103808f82eb3645fd36b0.1524059312.git.nelio.laranjeiro@6wind.com (mailing list archive)
State Superseded, archived
Delegated to: Shahaf Shuler
Headers

Checks

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

Commit Message

Nélio Laranjeiro April 18, 2018, 1:50 p.m. UTC
  Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5.c      |  2 ++
 drivers/net/mlx5/mlx5.h      |  2 ++
 drivers/net/mlx5/mlx5_defs.h |  5 ++++-
 drivers/net/mlx5/mlx5_mac.c  | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 41 insertions(+), 1 deletion(-)
  

Comments

Shahaf Shuler April 23, 2018, 5:52 a.m. UTC | #1
Hi Nelio,

Small comments before I merge.

Wednesday, April 18, 2018 4:50 PM, Nelio Laranjeiro:
> Subject: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list
> devop
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> ---
>  drivers/net/mlx5/mlx5.c      |  2 ++
>  drivers/net/mlx5/mlx5.h      |  2 ++
>  drivers/net/mlx5/mlx5_defs.h |  5 ++++-  drivers/net/mlx5/mlx5_mac.c  | 33
> +++++++++++++++++++++++++++++++++
>  4 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> 68783c3ac..887924d07 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -277,6 +277,7 @@ const struct eth_dev_ops mlx5_dev_ops = {
>  	.mac_addr_remove = mlx5_mac_addr_remove,
>  	.mac_addr_add = mlx5_mac_addr_add,
>  	.mac_addr_set = mlx5_mac_addr_set,
> +	.set_mc_addr_list = mlx5_set_mc_addr_list,
>  	.mtu_set = mlx5_dev_set_mtu,
>  	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
>  	.vlan_offload_set = mlx5_vlan_offload_set, @@ -329,6 +330,7 @@
> const struct eth_dev_ops mlx5_dev_ops_isolate = {
>  	.mac_addr_remove = mlx5_mac_addr_remove,
>  	.mac_addr_add = mlx5_mac_addr_add,
>  	.mac_addr_set = mlx5_mac_addr_set,
> +	.set_mc_addr_list = mlx5_set_mc_addr_list,
>  	.mtu_set = mlx5_dev_set_mtu,
>  	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
>  	.vlan_offload_set = mlx5_vlan_offload_set, diff --git
> a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> 6ad41390a..64f025d22 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -203,6 +203,8 @@ void mlx5_mac_addr_remove(struct rte_eth_dev
> *dev, uint32_t index);  int mlx5_mac_addr_add(struct rte_eth_dev *dev,
> struct ether_addr *mac,
>  		      uint32_t index, uint32_t vmdq);  int
> mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr
> *mac_addr);
> +int mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> +			  struct ether_addr *mc_addr_set, uint32_t
> nb_mc_addr);
> 
>  /* mlx5_rss.c */
> 
> diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
> index e3aa03bef..5973742a7 100644
> --- a/drivers/net/mlx5/mlx5_defs.h
> +++ b/drivers/net/mlx5/mlx5_defs.h
> @@ -15,8 +15,11 @@
> 
>  /* Maximum number of simultaneous unicast MAC addresses. */  #define
> MLX5_MAX_UC_MAC_ADDRESSES 128
> +/* Maximum number of simultaneous Multicast MAC addresses. */ #define
> +MLX5_MAX_MC_MAC_ADDRESSES 128
>  /* Maximum number of simultaneous MAC addresses. */ -#define
> MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES
> +#define MLX5_MAX_MAC_ADDRESSES \
> +	(MLX5_MAX_UC_MAC_ADDRESSES +
> MLX5_MAX_MC_MAC_ADDRESSES)
> 
>  /* Maximum number of simultaneous VLAN filters. */  #define
> MLX5_MAX_VLAN_IDS 128 diff --git a/drivers/net/mlx5/mlx5_mac.c
> b/drivers/net/mlx5/mlx5_mac.c index e9334ea4a..32560d55e 100644
> --- a/drivers/net/mlx5/mlx5_mac.c
> +++ b/drivers/net/mlx5/mlx5_mac.c
> @@ -201,3 +201,36 @@ mlx5_mac_addr_set(struct rte_eth_dev *dev, struct
> ether_addr *mac_addr)
>  		dev->data->port_id);
>  	return mlx5_mac_addr_add(dev, mac_addr, 0, 0);  }
> +
> +/**
> + * DPDK callback to set multicast addresses list.
> + *
> + * @param dev
> + *   Pointer to Ethernet device structure.
> + * @param mac_addr_set
> + *   Multicast MAC address pointer array.
> + * @param nb_mac_addr
> + *   Number of entries in the array.
> + *
> + * @return
> + *   0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +int
> +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> +		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
> +	uint32_t i;
> +	int ret;
> +

We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES before we start operate. 

> +	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> MLX5_MAX_MAC_ADDRESSES; ++i)
> +		mlx5_internal_mac_addr_remove(dev, i);
> +	i = MLX5_MAX_UC_MAC_ADDRESSES;
> +	while (nb_mc_addr--) {

Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip + warn if it is not. 

> +		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,
> i++);
> +		if (ret)
> +			return ret;
> +	}
> +	if (!dev->data->promiscuous)
> +		return mlx5_traffic_restart(dev);
> +	return 0;
> +}
> --
> 2.17.0
  
Nélio Laranjeiro April 23, 2018, 7:33 a.m. UTC | #2
On Mon, Apr 23, 2018 at 05:52:08AM +0000, Shahaf Shuler wrote:
> Hi Nelio,
> 
> Small comments before I merge.
> 
> Wednesday, April 18, 2018 4:50 PM, Nelio Laranjeiro:
> > Subject: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list
> > devop
> > 
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > ---
> >  drivers/net/mlx5/mlx5.c      |  2 ++
> >  drivers/net/mlx5/mlx5.h      |  2 ++
> >  drivers/net/mlx5/mlx5_defs.h |  5 ++++-  drivers/net/mlx5/mlx5_mac.c  | 33
> > +++++++++++++++++++++++++++++++++
> >  4 files changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > 68783c3ac..887924d07 100644
> > --- a/drivers/net/mlx5/mlx5.c
> > +++ b/drivers/net/mlx5/mlx5.c
> > @@ -277,6 +277,7 @@ const struct eth_dev_ops mlx5_dev_ops = {
> >  	.mac_addr_remove = mlx5_mac_addr_remove,
> >  	.mac_addr_add = mlx5_mac_addr_add,
> >  	.mac_addr_set = mlx5_mac_addr_set,
> > +	.set_mc_addr_list = mlx5_set_mc_addr_list,
> >  	.mtu_set = mlx5_dev_set_mtu,
> >  	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
> >  	.vlan_offload_set = mlx5_vlan_offload_set, @@ -329,6 +330,7 @@
> > const struct eth_dev_ops mlx5_dev_ops_isolate = {
> >  	.mac_addr_remove = mlx5_mac_addr_remove,
> >  	.mac_addr_add = mlx5_mac_addr_add,
> >  	.mac_addr_set = mlx5_mac_addr_set,
> > +	.set_mc_addr_list = mlx5_set_mc_addr_list,
> >  	.mtu_set = mlx5_dev_set_mtu,
> >  	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
> >  	.vlan_offload_set = mlx5_vlan_offload_set, diff --git
> > a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > 6ad41390a..64f025d22 100644
> > --- a/drivers/net/mlx5/mlx5.h
> > +++ b/drivers/net/mlx5/mlx5.h
> > @@ -203,6 +203,8 @@ void mlx5_mac_addr_remove(struct rte_eth_dev
> > *dev, uint32_t index);  int mlx5_mac_addr_add(struct rte_eth_dev *dev,
> > struct ether_addr *mac,
> >  		      uint32_t index, uint32_t vmdq);  int
> > mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr
> > *mac_addr);
> > +int mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > +			  struct ether_addr *mc_addr_set, uint32_t
> > nb_mc_addr);
> > 
> >  /* mlx5_rss.c */
> > 
> > diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
> > index e3aa03bef..5973742a7 100644
> > --- a/drivers/net/mlx5/mlx5_defs.h
> > +++ b/drivers/net/mlx5/mlx5_defs.h
> > @@ -15,8 +15,11 @@
> > 
> >  /* Maximum number of simultaneous unicast MAC addresses. */  #define
> > MLX5_MAX_UC_MAC_ADDRESSES 128
> > +/* Maximum number of simultaneous Multicast MAC addresses. */ #define
> > +MLX5_MAX_MC_MAC_ADDRESSES 128
> >  /* Maximum number of simultaneous MAC addresses. */ -#define
> > MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES
> > +#define MLX5_MAX_MAC_ADDRESSES \
> > +	(MLX5_MAX_UC_MAC_ADDRESSES +
> > MLX5_MAX_MC_MAC_ADDRESSES)
> > 
> >  /* Maximum number of simultaneous VLAN filters. */  #define
> > MLX5_MAX_VLAN_IDS 128 diff --git a/drivers/net/mlx5/mlx5_mac.c
> > b/drivers/net/mlx5/mlx5_mac.c index e9334ea4a..32560d55e 100644
> > --- a/drivers/net/mlx5/mlx5_mac.c
> > +++ b/drivers/net/mlx5/mlx5_mac.c
> > @@ -201,3 +201,36 @@ mlx5_mac_addr_set(struct rte_eth_dev *dev, struct
> > ether_addr *mac_addr)
> >  		dev->data->port_id);
> >  	return mlx5_mac_addr_add(dev, mac_addr, 0, 0);  }
> > +
> > +/**
> > + * DPDK callback to set multicast addresses list.
> > + *
> > + * @param dev
> > + *   Pointer to Ethernet device structure.
> > + * @param mac_addr_set
> > + *   Multicast MAC address pointer array.
> > + * @param nb_mac_addr
> > + *   Number of entries in the array.
> > + *
> > + * @return
> > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +int
> > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > +		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
> > +	uint32_t i;
> > +	int ret;
> > +
> 
> We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES before we start
> operate. 

This verification is done in the sub function.

Considering an application calling such API wants to remove/replace the
old list with new entries.
That this new one can be just an addition or totally different list or
even empty.
This new list can be larger than the amount of MAC addresses the PMD can
support.

There are two possibilities:

1. The list is too large:  the application will enable the all multicast
mode to receive the extra mac addresses it needs.
2. The list fits (or empty): no issues.

At the end the application can also call this API with an empty list to
clear it before/after enabling the "all multicast" mode.
The final result being the same, does it worse to add a duplicated 
verification?

Note: if an error happens the new list is not committed yet i.e. the
traffic remains untouched.

> > +	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> > MLX5_MAX_MAC_ADDRESSES; ++i)
> > +		mlx5_internal_mac_addr_remove(dev, i);
> > +	i = MLX5_MAX_UC_MAC_ADDRESSES;
> > +	while (nb_mc_addr--) {
> 
> Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
> + warn if it is not. 

Such verification should be done in the public API i.e. ethdev.

> > +		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,
> > i++);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	if (!dev->data->promiscuous)
> > +		return mlx5_traffic_restart(dev);
> > +	return 0;
> > +}
> > --
> > 2.17.0

Regards,
  
Shahaf Shuler April 23, 2018, 7:57 a.m. UTC | #3
Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro:

[...]

> > > +/**
> > > + * DPDK callback to set multicast addresses list.
> > > + *
> > > + * @param dev
> > > + *   Pointer to Ethernet device structure.
> > > + * @param mac_addr_set
> > > + *   Multicast MAC address pointer array.
> > > + * @param nb_mac_addr
> > > + *   Number of entries in the array.
> > > + *
> > > + * @return
> > > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > > + */
> > > +int
> > > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > > +		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
> > > +	uint32_t i;
> > > +	int ret;
> > > +
> >
> > We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES
> before we start
> > operate.
> 
> This verification is done in the sub function.

I see only assert. Did I missed anything? 

> 
> Considering an application calling such API wants to remove/replace the old
> list with new entries.
> That this new one can be just an addition or totally different list or even
> empty.
> This new list can be larger than the amount of MAC addresses the PMD can
> support.
> 
> There are two possibilities:
> 
> 1. The list is too large:  the application will enable the all multicast mode to
> receive the extra mac addresses it needs.

How can application know the size of the MC list?
only the UC size is being reported:
info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES

> 2. The list fits (or empty): no issues.
> 
> At the end the application can also call this API with an empty list to clear it
> before/after enabling the "all multicast" mode.
> The final result being the same, does it worse to add a duplicated
> verification?

At the current code if the list is too large and the PMD was compiled w/o debug mode it will results in seg fault. 

> 
> Note: if an error happens the new list is not committed yet i.e. the traffic
> remains untouched.
> 
> > > +	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> > > MLX5_MAX_MAC_ADDRESSES; ++i)
> > > +		mlx5_internal_mac_addr_remove(dev, i);
> > > +	i = MLX5_MAX_UC_MAC_ADDRESSES;
> > > +	while (nb_mc_addr--) {
> >
> > Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
> > + warn if it is not.
> 
> Such verification should be done in the public API i.e. ethdev.

I don't understand. 
In the first patch of the series you add extra verification to check the mac address validity. But for the MC you claim it should be done on ethdev layer. 

It should be consistant. Either ethdev verify the MAC address or the PMD. If the first one, then there is no need to add the is_zero_ether_addr check on the first patch. 


> 
> > > +		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,
> > > i++);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +	if (!dev->data->promiscuous)
> > > +		return mlx5_traffic_restart(dev);
> > > +	return 0;
> > > +}
> > > --
> > > 2.17.0
> 
> Regards,
> 
> --
> Nélio Laranjeiro
> 6WIND
  
Nélio Laranjeiro April 23, 2018, 9:34 a.m. UTC | #4
On Mon, Apr 23, 2018 at 07:57:36AM +0000, Shahaf Shuler wrote:
> Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro:
> [...]
> > > > +/**
> > > > + * DPDK callback to set multicast addresses list.
> > > > + *
> > > > + * @param dev
> > > > + *   Pointer to Ethernet device structure.
> > > > + * @param mac_addr_set
> > > > + *   Multicast MAC address pointer array.
> > > > + * @param nb_mac_addr
> > > > + *   Number of entries in the array.
> > > > + *
> > > > + * @return
> > > > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > > > + */
> > > > +int
> > > > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > > > +		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
> > > > +	uint32_t i;
> > > > +	int ret;
> > > > +
> > >
> > > We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES
> > before we start
> > > operate.
> > 
> > This verification is done in the sub function.
> 
> I see only assert. Did I missed anything? 

No.

> > Considering an application calling such API wants to remove/replace the old
> > list with new entries.
> > That this new one can be just an addition or totally different list or even
> > empty.
> > This new list can be larger than the amount of MAC addresses the PMD can
> > support.
> > 
> > There are two possibilities:
> > 
> > 1. The list is too large:  the application will enable the all multicast mode to
> > receive the extra mac addresses it needs.
> 
> How can application know the size of the MC list?
> only the UC size is being reported:
> info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES

Such information is not reported at all.  The application has to guess.

> > 2. The list fits (or empty): no issues.
> > 
> > At the end the application can also call this API with an empty list to clear it
> > before/after enabling the "all multicast" mode.
> > The final result being the same, does it worse to add a duplicated
> > verification?
> 
> At the current code if the list is too large and the PMD was compiled
> w/o debug mode it will results in seg fault. 

Right it needs a verification.

> > Note: if an error happens the new list is not committed yet i.e. the traffic
> > remains untouched.
> > 
> > > > +	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> > > > MLX5_MAX_MAC_ADDRESSES; ++i)
> > > > +		mlx5_internal_mac_addr_remove(dev, i);
> > > > +	i = MLX5_MAX_UC_MAC_ADDRESSES;
> > > > +	while (nb_mc_addr--) {
> > >
> > > Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
> > > + warn if it is not.
> > 
> > Such verification should be done in the public API i.e. ethdev.
> 
> I don't understand. 
> In the first patch of the series you add extra verification to check
> the mac address validity.

It only verify the MAC address is not zero, it does not verify the MAC
address is valid in the function context (e.g. unicast in
mlx5_mac_addr_add()).

> But for the MC you claim it should be done on ethdev layer. 

Dito.

> It should be consistant. Either ethdev verify the MAC address or the
> PMD. If the first one, then there is no need to add the
> is_zero_ether_addr check on the first patch. 

It is consistent, the PMD only verify the MAC address is not zero and
this in both API.

> > > > +		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,
> > > > i++);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +	if (!dev->data->promiscuous)
> > > > +		return mlx5_traffic_restart(dev);
> > > > +	return 0;
> > > > +}
> > > > --
> > > > 2.17.0

Regards,
  
Shahaf Shuler April 23, 2018, 9:58 a.m. UTC | #5
Monday, April 23, 2018 12:34 PM, Nélio Laranjeiro:
> Subject: Re: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add
> list devop
> 
> On Mon, Apr 23, 2018 at 07:57:36AM +0000, Shahaf Shuler wrote:
> > Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro:
> > [...]
> > > > > +/**
> > > > > + * DPDK callback to set multicast addresses list.
> > > > > + *
> > > > > + * @param dev
> > > > > + *   Pointer to Ethernet device structure.
> > > > > + * @param mac_addr_set
> > > > > + *   Multicast MAC address pointer array.
> > > > > + * @param nb_mac_addr
> > > > > + *   Number of entries in the array.
> > > > > + *
> > > > > + * @return
> > > > > + *   0 on success, a negative errno value otherwise and rte_errno is
> set.
> > > > > + */
> > > > > +int
> > > > > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > > > > +		      struct ether_addr *mc_addr_set, uint32_t
> nb_mc_addr) {
> > > > > +	uint32_t i;
> > > > > +	int ret;
> > > > > +
> > > >
> > > > We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES
> > > before we start
> > > > operate.
> > >
> > > This verification is done in the sub function.
> >
> > I see only assert. Did I missed anything?
> 
> No.
> 
> > > Considering an application calling such API wants to remove/replace
> > > the old list with new entries.
> > > That this new one can be just an addition or totally different list
> > > or even empty.
> > > This new list can be larger than the amount of MAC addresses the PMD
> > > can support.
> > >
> > > There are two possibilities:
> > >
> > > 1. The list is too large:  the application will enable the all
> > > multicast mode to receive the extra mac addresses it needs.
> >
> > How can application know the size of the MC list?
> > only the UC size is being reported:
> > info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES
> 
> Such information is not reported at all.  The application has to guess.
> 
> > > 2. The list fits (or empty): no issues.
> > >
> > > At the end the application can also call this API with an empty list
> > > to clear it before/after enabling the "all multicast" mode.
> > > The final result being the same, does it worse to add a duplicated
> > > verification?
> >
> > At the current code if the list is too large and the PMD was compiled
> > w/o debug mode it will results in seg fault.
> 
> Right it needs a verification.
> 
> > > Note: if an error happens the new list is not committed yet i.e. the
> > > traffic remains untouched.
> > >
> > > > > +	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> > > > > MLX5_MAX_MAC_ADDRESSES; ++i)
> > > > > +		mlx5_internal_mac_addr_remove(dev, i);
> > > > > +	i = MLX5_MAX_UC_MAC_ADDRESSES;
> > > > > +	while (nb_mc_addr--) {
> > > >
> > > > Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to
> > > > skip
> > > > + warn if it is not.
> > >
> > > Such verification should be done in the public API i.e. ethdev.
> >
> > I don't understand.
> > In the first patch of the series you add extra verification to check
> > the mac address validity.
> 
> It only verify the MAC address is not zero, it does not verify the MAC address
> is valid in the function context (e.g. unicast in mlx5_mac_addr_add()).
> 
> > But for the MC you claim it should be done on ethdev layer.
> 
> Dito.
> 
> > It should be consistant. Either ethdev verify the MAC address or the
> > PMD. If the first one, then there is no need to add the
> > is_zero_ether_addr check on the first patch.
> 
> It is consistent, the PMD only verify the MAC address is not zero and this in
> both API.

OK,
Then I wait for the next version for merge. 

> 
> > > > > +		ret = mlx5_internal_mac_addr_add(dev,
> mc_addr_set++,
> > > > > i++);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > +	if (!dev->data->promiscuous)
> > > > > +		return mlx5_traffic_restart(dev);
> > > > > +	return 0;
> > > > > +}
> > > > > --
> > > > > 2.17.0
> 
> Regards,
> 
> --
> Nélio Laranjeiro
> 6WIND
  

Patch

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 68783c3ac..887924d07 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -277,6 +277,7 @@  const struct eth_dev_ops mlx5_dev_ops = {
 	.mac_addr_remove = mlx5_mac_addr_remove,
 	.mac_addr_add = mlx5_mac_addr_add,
 	.mac_addr_set = mlx5_mac_addr_set,
+	.set_mc_addr_list = mlx5_set_mc_addr_list,
 	.mtu_set = mlx5_dev_set_mtu,
 	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
 	.vlan_offload_set = mlx5_vlan_offload_set,
@@ -329,6 +330,7 @@  const struct eth_dev_ops mlx5_dev_ops_isolate = {
 	.mac_addr_remove = mlx5_mac_addr_remove,
 	.mac_addr_add = mlx5_mac_addr_add,
 	.mac_addr_set = mlx5_mac_addr_set,
+	.set_mc_addr_list = mlx5_set_mc_addr_list,
 	.mtu_set = mlx5_dev_set_mtu,
 	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
 	.vlan_offload_set = mlx5_vlan_offload_set,
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 6ad41390a..64f025d22 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -203,6 +203,8 @@  void mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);
 int mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
 		      uint32_t index, uint32_t vmdq);
 int mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr);
+int mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+			  struct ether_addr *mc_addr_set, uint32_t nb_mc_addr);
 
 /* mlx5_rss.c */
 
diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index e3aa03bef..5973742a7 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -15,8 +15,11 @@ 
 
 /* Maximum number of simultaneous unicast MAC addresses. */
 #define MLX5_MAX_UC_MAC_ADDRESSES 128
+/* Maximum number of simultaneous Multicast MAC addresses. */
+#define MLX5_MAX_MC_MAC_ADDRESSES 128
 /* Maximum number of simultaneous MAC addresses. */
-#define MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES
+#define MLX5_MAX_MAC_ADDRESSES \
+	(MLX5_MAX_UC_MAC_ADDRESSES + MLX5_MAX_MC_MAC_ADDRESSES)
 
 /* Maximum number of simultaneous VLAN filters. */
 #define MLX5_MAX_VLAN_IDS 128
diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
index e9334ea4a..32560d55e 100644
--- a/drivers/net/mlx5/mlx5_mac.c
+++ b/drivers/net/mlx5/mlx5_mac.c
@@ -201,3 +201,36 @@  mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 		dev->data->port_id);
 	return mlx5_mac_addr_add(dev, mac_addr, 0, 0);
 }
+
+/**
+ * DPDK callback to set multicast addresses list.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param mac_addr_set
+ *   Multicast MAC address pointer array.
+ * @param nb_mac_addr
+ *   Number of entries in the array.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
+ */
+int
+mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr)
+{
+	uint32_t i;
+	int ret;
+
+	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i != MLX5_MAX_MAC_ADDRESSES; ++i)
+		mlx5_internal_mac_addr_remove(dev, i);
+	i = MLX5_MAX_UC_MAC_ADDRESSES;
+	while (nb_mc_addr--) {
+		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++, i++);
+		if (ret)
+			return ret;
+	}
+	if (!dev->data->promiscuous)
+		return mlx5_traffic_restart(dev);
+	return 0;
+}