From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from proxy.6wind.com (host.76.145.23.62.rev.coltfrance.com [62.23.145.76]) by dpdk.org (Postfix) with ESMTP id B771C727A for ; Thu, 25 Jan 2018 11:40:17 +0100 (CET) Received: from core.dev.6wind.com (unknown [10.0.0.1]) by proxy.6wind.com (Postfix) with ESMTPS id 96A4D127036; Thu, 25 Jan 2018 11:38:24 +0100 (CET) Received: from [10.16.0.195] (helo=6wind.com) by core.dev.6wind.com with smtp (Exim 4.84_2) (envelope-from ) id 1eeewv-00061k-Jm; Thu, 25 Jan 2018 11:40:06 +0100 Received: by 6wind.com (sSMTP sendmail emulation); Thu, 25 Jan 2018 11:40:05 +0100 Date: Thu, 25 Jan 2018 11:40:05 +0100 From: Olivier Matz To: Wenzhuo Lu Cc: dev@dpdk.org Message-ID: <20180125104005.upmo7l4imeg2oyj7@glumotte.dev.6wind.com> References: <1516848417-77912-1-git-send-email-wenzhuo.lu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1516848417-77912-1-git-send-email-wenzhuo.lu@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Jan 2018 10:40:17 -0000 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 > --- > 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