From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id A8F251B2A5 for ; Fri, 26 Jan 2018 03:19:16 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jan 2018 18:19:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,414,1511856000"; d="scan'208";a="14373936" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga002.fm.intel.com with ESMTP; 25 Jan 2018 18:19:15 -0800 Received: from fmsmsx102.amr.corp.intel.com (10.18.124.200) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 25 Jan 2018 18:19:14 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX102.amr.corp.intel.com (10.18.124.200) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 25 Jan 2018 18:19:14 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.116]) with mapi id 14.03.0319.002; Fri, 26 Jan 2018 10:19:12 +0800 From: "Lu, Wenzhuo" To: Olivier Matz CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops Thread-Index: AQHTlYZ8trb6VNiRqEK8USuEbkVK4qOD4I+AgAF/p6A= Date: Fri, 26 Jan 2018 02:19:11 +0000 Message-ID: <6A0DE07E22DDAD4C9103DF62FEBC09093B720B5F@shsmsx102.ccr.corp.intel.com> References: <1516848417-77912-1-git-send-email-wenzhuo.lu@intel.com> <20180125104005.upmo7l4imeg2oyj7@glumotte.dev.6wind.com> In-Reply-To: <20180125104005.upmo7l4imeg2oyj7@glumotte.dev.6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 02:19:17 -0000 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 MA= C set > ops >=20 > Hi Wenzhuo, >=20 > 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. >=20 > 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 c= ontinue, right? >=20 > 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(). >=20 > 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 des= ign. >=20 > > 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. >=20 > Agree. >=20 > > 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; > > >=20 > 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 chan= ged. 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 A= PP to know what's the problem. The same below. >=20 >=20 > > 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 =3D &rte_eth_devices[port_id]; > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, - > ENOTSUP); > > > > + ret =3D (*dev->dev_ops->mac_addr_set)(dev, addr); > > + if (ret) > > + return -EPERM; > > + >=20 > 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. >=20 >=20 > > /* 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. >=20 > Here, I suggest: >=20 > - (-errno) for any other error returned by the PMD >=20 >=20 > Thanks > Olivier