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