Re: [dpdk-dev] [PATCH] ethdev: return diagnostic when setting MAC address

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

On Fri, Apr 06, 2018 at 05:21:48PM +0200, Olivier Matz wrote:
> Change the prototype and the behavior of dev_ops->eth_mac_addr_set(): a
> return code is added to notify the caller (librte_ether) if an error
> occurred in the PMD.
> 
> The new default MAC address is now copied in dev->data->mac_addrs[0]
> only if the operation is successful.
> 
> The patch also updates all the PMDs accordingly.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
> 
> v3:
> * mlx5: remove empty line as suggested by Adrien
> * mvpp2: remove wrong changes in mrvl_mac_addr_remove() as suggested
>   by Ferruh
> * dpaa/dpaa2: fix return value as suggested by Shreyansh
> 
> v2:
> * add same change for net/cxgbe
> * mrvl was renamed as mvpp2
> * mvpp2: return success if no ppio as suggested by Tomasz
> * mlx5: update comment as suggested by Adrien
> * sfc: replace by Ivan's patch
> 
> 
>  doc/guides/rel_notes/deprecation.rst    |  8 --------
>  drivers/net/ark/ark_ethdev.c            |  9 ++++++---
>  drivers/net/avf/avf_ethdev.c            | 12 +++++++----
>  drivers/net/bnxt/bnxt_ethdev.c          | 10 ++++++----
>  drivers/net/bonding/rte_eth_bond_pmd.c  |  8 ++++++--
>  drivers/net/cxgbe/cxgbe_ethdev.c        |  5 +++--
>  drivers/net/cxgbe/cxgbe_pfvf.h          |  2 +-
>  drivers/net/dpaa/dpaa_ethdev.c          |  4 +++-
>  drivers/net/dpaa2/dpaa2_ethdev.c        |  6 ++++--
>  drivers/net/e1000/igb_ethdev.c          | 12 ++++++-----
>  drivers/net/failsafe/failsafe_ops.c     | 17 +++++++++++++---
>  drivers/net/i40e/i40e_ethdev.c          | 24 +++++++++++++---------
>  drivers/net/i40e/i40e_ethdev_vf.c       | 12 ++++++-----
>  drivers/net/ixgbe/ixgbe_ethdev.c        | 13 +++++++-----
>  drivers/net/mlx4/mlx4.h                 |  2 +-
>  drivers/net/mlx4/mlx4_ethdev.c          |  7 +++++--
>  drivers/net/mlx5/mlx5.h                 |  2 +-
>  drivers/net/mlx5/mlx5_mac.c             |  6 +++++-
>  drivers/net/mvpp2/mrvl_ethdev.c         | 11 ++++++++---
>  drivers/net/null/rte_eth_null.c         |  3 ++-
>  drivers/net/octeontx/octeontx_ethdev.c  |  4 +++-
>  drivers/net/qede/qede_ethdev.c          |  7 +++----
>  drivers/net/sfc/sfc_ethdev.c            | 35 ++++++++++++++++++++++-----------
>  drivers/net/szedata2/rte_eth_szedata2.c |  3 ++-
>  drivers/net/tap/rte_eth_tap.c           | 34 +++++++++++++++++++++-----------
>  drivers/net/virtio/virtio_ethdev.c      | 15 +++++++++-----
>  drivers/net/vmxnet3/vmxnet3_ethdev.c    |  5 +++--
>  lib/librte_ether/rte_ethdev.c           |  7 +++++--
>  lib/librte_ether/rte_ethdev_core.h      |  2 +-
>  test/test/virtual_pmd.c                 |  3 ++-
>  30 files changed, 184 insertions(+), 104 deletions(-)
> 
</snip>
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index faacfd9d6..738709854 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -195,7 +195,7 @@ int mlx5_get_mac(struct rte_eth_dev *dev, uint8_t (*mac)[ETHER_ADDR_LEN]);
>  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);
> -void mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr);
> +int mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr);
>  
>  /* mlx5_rss.c */
>  
> diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
> index 01c7ba17a..6ca1cc4a1 100644
> --- a/drivers/net/mlx5/mlx5_mac.c
> +++ b/drivers/net/mlx5/mlx5_mac.c
> @@ -124,8 +124,11 @@ mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
>   *   Pointer to Ethernet device structure.
>   * @param mac_addr
>   *   MAC address to register.
> + *
> + * @return
> + *   0 on success, a negative errno value otherwise and rte_errno is set.
>   */
> -void
> +int
>  mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
>  {
>  	int ret;
> @@ -137,4 +140,5 @@ mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
>  	if (ret)
>  		DRV_LOG(ERR, "port %u cannot set mac address: %s",
>  			dev->data->port_id, strerror(rte_errno));
> +	return ret;
>  }

You should also remove the DRV_LOG which become useless, it has been
added as the function did not return anything, now has it returns an
error, the application can fully handle it.

Unless that, for MLX5

Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

-- 
Nélio Laranjeiro
6WIND