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.m...@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
