From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id DF5E21B345 for ; Fri, 26 Jan 2018 17:54:46 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Jan 2018 08:54:46 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,417,1511856000"; d="scan'208";a="22553355" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.48]) ([10.237.220.48]) by orsmga003.jf.intel.com with ESMTP; 26 Jan 2018 08:54:44 -0800 To: "Lu, Wenzhuo" , Olivier Matz Cc: "dev@dpdk.org" References: <1516848417-77912-1-git-send-email-wenzhuo.lu@intel.com> <20180125104005.upmo7l4imeg2oyj7@glumotte.dev.6wind.com> <6A0DE07E22DDAD4C9103DF62FEBC09093B720B5F@shsmsx102.ccr.corp.intel.com> From: Ferruh Yigit Message-ID: <460287e3-db94-b677-e262-4c45a59fb486@intel.com> Date: Fri, 26 Jan 2018 16:54:43 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC09093B720B5F@shsmsx102.ccr.corp.intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Fri, 26 Jan 2018 16:54:47 -0000 On 1/26/2018 2:19 AM, Lu, Wenzhuo wrote: > Hi Olivier, > > >> -----Original Message----- >> From: Olivier Matz [mailto:olivier.matz@6wind.com] >> Sent: Thursday, January 25, 2018 6:40 PM >> To: Lu, Wenzhuo >> Cc: dev@dpdk.org >> Subject: Re: [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set >> ops >> >> 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/ > Sorry, didn't notice that. Glad to know you're working on that. So you'll continue, right? Plan was send PMD fixes and deprecation notice on this release and send fix next release [1]. I think PMD fixes are merged now, but deprecation notice not send yet. [1] https://dpdk.org/ml/archives/dev/2018-January/085195.html > >> >> 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. > Yes. Every NIC's PMD need to be checked to make sure its behavior is by design. > >> >>> 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? > Agree. > After a quick glance at the PMD code, the code of every NIC need to be changed. I'm not familiar with much of them, so a little scared about figuring out the error number :) But totally agree that the erron is good for the APP to know what's the problem. > The same below. > >> >> >>> 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