[dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops
Olivier Matz
olivier.matz at 6wind.com
Thu Jan 25 11:40:05 CET 2018
Hi Wenzhuo,
On Thu, Jan 25, 2018 at 10:46:57AM +0800, Wenzhuo Lu wrote:
> Setting the default MAC address may fail on many NICs.
> But the ops return void. So, even it failed, RTE changes
> the MAC address and APP doesn't know the failure.
>
> It's not real patch, just show the idea to add a return
> value for the ops.
Thank you for taking care of this. I had some plans to work
on it too, as discussed here:
https://dpdk.org/dev/patchwork/patch/32284/
I noticed that some PMDs try to manage the error case by
themselve by overriding the mac address in ethdev->data.
See for instance qede_mac_addr_set().
With your patch, these PMDs should be modified. No PMD
should change the value of eth_dev->data->mac_addrs.
> BTW,
> Seems we should do the same thing for
> rte_eth_dev_mac_addr_remove as it also has chance to fail
> in PMD layer.
Agree.
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu at intel.com>
> ---
> drivers/net/i40e/i40e_ethdev_vf.c | 12 +++++++-----
> lib/librte_ether/rte_ethdev.c | 7 +++++--
> lib/librte_ether/rte_ethdev.h | 1 +
> lib/librte_ether/rte_ethdev_core.h | 2 +-
> 4 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
> index 6ac3f8c..1d3898b 100644
> --- a/drivers/net/i40e/i40e_ethdev_vf.c
> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> @@ -120,8 +120,8 @@ static int i40evf_dev_rss_hash_update(struct rte_eth_dev *dev,
> static int i40evf_dev_rss_hash_conf_get(struct rte_eth_dev *dev,
> struct rte_eth_rss_conf *rss_conf);
> static int i40evf_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
> -static void i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
> - struct ether_addr *mac_addr);
> +static int i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
> + struct ether_addr *mac_addr);
> static int
> i40evf_dev_rx_queue_intr_enable(struct rte_eth_dev *dev, uint16_t queue_id);
> static int
> @@ -2655,7 +2655,7 @@ static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
> return ret;
> }
>
> -static void
> +static int
> i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
> struct ether_addr *mac_addr)
> {
> @@ -2664,15 +2664,17 @@ static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
>
> if (!is_valid_assigned_ether_addr(mac_addr)) {
> PMD_DRV_LOG(ERR, "Tried to set invalid MAC address.");
> - return;
> + return -1;
> }
>
> if (vf->flags & I40E_FLAG_VF_MAC_BY_PF)
> - return;
> + return -1;
>
I wonder if returning -errno wouldn't be better?
> i40evf_del_mac_addr_by_addr(dev, (struct ether_addr *)hw->mac.addr);
>
> i40evf_add_mac_addr(dev, mac_addr, 0, 0);
>
> ether_addr_copy(mac_addr, (struct ether_addr *)hw->mac.addr);
> +
> + return 0;
> }
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index f285ba2..869c960 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2816,6 +2816,7 @@ struct rte_eth_dev *
> rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct ether_addr *addr)
> {
> struct rte_eth_dev *dev;
> + int ret;
>
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>
> @@ -2825,11 +2826,13 @@ struct rte_eth_dev *
> dev = &rte_eth_devices[port_id];
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
>
> + ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
> + if (ret)
> + return -EPERM;
> +
And here, we could return the return code of the PMD ops instead of
-EPERM, I think it can give better idea of the error cause.
> /* Update default address in NIC data structure */
> ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>
> - (*dev->dev_ops->mac_addr_set)(dev, addr);
> -
> return 0;
> }
>
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index ccf4a15..e3355cc 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2616,6 +2616,7 @@ int rte_eth_dev_mac_addr_add(uint16_t port_id, struct ether_addr *mac_addr,
> * - (-ENOTSUP) if hardware doesn't support.
> * - (-ENODEV) if *port* invalid.
> * - (-EINVAL) if MAC address is invalid.
> + * - (-EPERM) if the default MAC address cannot be changed.
Here, I suggest:
- (-errno) for any other error returned by the PMD
Thanks
Olivier
More information about the dev
mailing list