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

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Tue, Feb 27, 2018 at 04:11:29PM +0100, 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>
> ---
> 
> Hi,
> 
> This patch is the following of the discussion we had in this thread:
> https://dpdk.org/dev/patchwork/patch/32284/
> 
> I did my best to keep the consistency inside the PMDs. The behavior
> of eth_mac_addr_set() is inspired from other fonctions in the same
> PMD, usually eth_mac_addr_add(). For instance:
> - dpaa and dpaa2 return 0 on error.
> - some PMDs (bnxt, mlx5, ...?) do not return a -errno code (-1 or
>   positive values).
> - some PMDs (avf, tap) check if the address is the same and return 0
>   in that case. This could go in generic code?
> 
> I tried to use the following errors when relevant:
> - -EPERM when a VF is not allowed to do a change
> - -ENOTSUP if the function is not supported
> - -EIO if this is an unknown error from lower layer (hw or sdk)

Keep in mind EIO is currently documented in ethdev as somewhat
hot-plug-related, as in "device is unresponsive and likely unplugged". The
reaction of a hot-plug-aware application to such an error code might be to
close the device, possibly for the wrong reason.

I just wanted to point it out, I don't think it's a problem for this patch
but can't speak for all PMDs.

> - -EINVAL for other unknown errors
> 
> Please, PMD maintainers, feel free to comment if you ahve specific
> needs for your driver.

OK with the API change and it's fine for mlx4 and mlx5, with a few comments
regarding the latter, please see below.

<snip>
> diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
> index 19c8a223d..c107794ce 100644
> --- a/drivers/net/mlx4/mlx4.h
> +++ b/drivers/net/mlx4/mlx4.h
> @@ -131,7 +131,7 @@ void mlx4_allmulticast_disable(struct rte_eth_dev *dev);
>  void mlx4_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);
>  int mlx4_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  		      uint32_t index, uint32_t vmdq);
> -void mlx4_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr);
> +int mlx4_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr);
>  int mlx4_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on);
>  int mlx4_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats);
>  void mlx4_stats_reset(struct rte_eth_dev *dev);
> diff --git a/drivers/net/mlx4/mlx4_ethdev.c b/drivers/net/mlx4/mlx4_ethdev.c
> index 3bc692731..2442e16a6 100644
> --- a/drivers/net/mlx4/mlx4_ethdev.c
> +++ b/drivers/net/mlx4/mlx4_ethdev.c
> @@ -701,11 +701,14 @@ mlx4_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on)
>   *   Pointer to Ethernet device structure.
>   * @param mac_addr
>   *   MAC address to register.
> + *
> + * @return
> + *   0 on success, negative errno value otherwise and rte_errno is set.
>   */
> -void
> +int
>  mlx4_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
>  {
> -	mlx4_mac_addr_add(dev, mac_addr, 0, 0);
> +	return mlx4_mac_addr_add(dev, mac_addr, 0, 0);
>  }
>  
>  /**
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index 965c19f21..42e58d7f7 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -241,7 +241,7 @@ int priv_get_mac(struct priv *, uint8_t (*)[ETHER_ADDR_LEN]);
>  void mlx5_mac_addr_remove(struct rte_eth_dev *, uint32_t);
>  int mlx5_mac_addr_add(struct rte_eth_dev *, struct ether_addr *, uint32_t,
>  		      uint32_t);
> -void mlx5_mac_addr_set(struct rte_eth_dev *, struct ether_addr *);
> +int mlx5_mac_addr_set(struct rte_eth_dev *, struct ether_addr *);
>  
>  /* mlx5_rss.c */
>  
> diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
> index e8a8d4594..0dc4bec46 100644
> --- a/drivers/net/mlx5/mlx5_mac.c
> +++ b/drivers/net/mlx5/mlx5_mac.c
> @@ -118,10 +118,13 @@ 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.
>   */
> -void
> +int
>  mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
>  {
>  	DEBUG("%p: setting primary MAC address", (void *)dev);
> -	mlx5_mac_addr_add(dev, mac_addr, 0, 0);
> +	return mlx5_mac_addr_add(dev, mac_addr, 0, 0);
>  }

With Nelio's errno rework for mlx5 [1][2], this change should end up being
similar to mlx4.

[1] http://dpdk.org/ml/archives/dev/2018-February/091668.html
[2] http://dpdk.org/ml/archives/dev/2018-February/091678.html

-- 
Adrien Mazarguil
6WIND