From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk0-f53.google.com (mail-vk0-f53.google.com [209.85.213.53]) by dpdk.org (Postfix) with ESMTP id A8047DE5 for ; Fri, 20 Jan 2017 13:17:58 +0100 (CET) Received: by mail-vk0-f53.google.com with SMTP id x75so50198895vke.2 for ; Fri, 20 Jan 2017 04:17:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nfware-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=NwfiV333UB1eBgTpGyEjYcAz0tpDxl6CF59Kfk717DA=; b=WrKUw5RmaiJrzyhzP4MqEVIj/NImR6Niu5BasAkautBM9jrt1hCaG7iDL7ZvcZiuZi 5cqka0pY0H+WE74Z8PPbqCNlg0fkp6w4OzKjMLLgGWsSWdtu6YY6dzs94rKTURAc5jzB fW63tv5oZ4rgxsvCQ3IDIkFpSGpdJ6pcPqtdDxrDKZPtF6iq4lWwW84fkYF7sPnzA/gu M/6EcHh0LYzb6P8EYNGnXtK/9KYiJSj47bHmQ+2dvoqomci/o0z7gkUnwMmrcNmKRt+n ECrQx03jjKEEfysXh+JFdc2mcZ02sN/Y2o4/ADUQWXjBqCbvRkkD7653ygojyRJ360QY TQuw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=NwfiV333UB1eBgTpGyEjYcAz0tpDxl6CF59Kfk717DA=; b=bZsepIM00ygH9n9F7sHgrsgiBanaKGegOcjBg9urbDOa9273ElrJlHDTbTxKIDa752 cBZSExiKpu6ksE2boA/TYmMaR+NRYc3M55duwKb5GsrUAPQA2dR+2Ixtl4EIIU3QOH6I EK2sOlzpttmC1filFHeazCYrq2yMC6P18SYuUYW1m49y2HMtQlSIFmn9iu0lZHKKLRNB JQKZpC/WkDpUwXE4sg5SH3kyPN6MEFwLr1SZPUUgAmjekTQh2yyWFr9bnNeuL57UqPCY 1zxTyzJcfkk/9Iy3KrNm/QeFiG+rrE8kXkeFv/clih1FsuGT9wZ5ONY5j4doCCs1DL81 1GwQ== X-Gm-Message-State: AIkVDXLcuAK9AA92PWVwm2ikVYvNRhZErk7c3cftcrGaw2EFdeYlDa7fU3Uj4fjCa6UkOW/dpstgGZDEYrGQtQ== X-Received: by 10.31.7.145 with SMTP id 139mr7063145vkh.132.1484914678122; Fri, 20 Jan 2017 04:17:58 -0800 (PST) MIME-Version: 1.0 Received: by 10.159.35.80 with HTTP; Fri, 20 Jan 2017 04:17:57 -0800 (PST) X-Originating-IP: [95.182.74.2] In-Reply-To: References: <20170119184721.22348-1-jonshin@cisco.com> From: Igor Ryzhov Date: Fri, 20 Jan 2017 15:17:57 +0300 Message-ID: To: "Steve Shin (jonshin)" Cc: "dev@dpdk.org" , "ferruh.yigit@intel.com" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC address replay 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, 20 Jan 2017 12:17:59 -0000 Hello Steve, I think it's not the right solution, because if we want to restore default MAC address, it should be done using dev->dev_ops->mac_addr_set(), not using dev->dev_ops->mac_addr_add(). I think that right solution may be to store default MAC address in separate variable dev->data->default_mac_addr, not in dev->data->mac_addrs array. And, accordingly, restore default_mac_addr using mac_addr_set(), and restore mac_addrs array using mac_addr_add(). Best regards, Igor On Fri, Jan 20, 2017 at 5:30 AM, Steve Shin (jonshin) wrote: > Thanks Igor for your comments! > > > > A good point on the first item. > > For the second question, I walked through device initialization routines > to see any other potential issue. > > I found one case where index 0 is still required to replay MAC address; > ex) MAC address change on the PHY port =E2=80=93 index 0 > > No code path was found to restore a changed MAC value with index 0. > Therefore, we may have to use the existing rte_eth_dev_config_restore() > function to replay 0 indexed MAC. > > Currently rte_eth_dev_default_mac_addr_set() is called to program PHY MAC > without setting mac_pool_sel for 0 index. > > So the following code is also required inside rte_eth_dev_default_mac_add= r_set() > routine as a complete solution: > > @@ -2237,6 +2234,9 @@ struct rte_eth_dev * > > > > (*dev->dev_ops->mac_addr_set)(dev, addr); > > > > + /* Update pool bitmap in NIC data structure */ > > + dev->data->mac_pool_sel[0] =3D 1; > > + > > return 0; > > > > Any thoughts would be appreciated. > > > > Regards, > > Steve > > > > *From: *Igor Ryzhov > *Date: *Thursday, January 19, 2017 at 2:39 PM > *To: *Steve Shin > *Cc: *"dev@dpdk.org" , "ferruh.yigit@intel.com" < > ferruh.yigit@intel.com> > *Subject: *Re: [dpdk-dev] [PATCH] lib/librte_ether: error handling on MAC > address replay > > > > Hello Steve, > > > > Thank you for the patch. > > > > I think a couple of improvements can be done: > > 1. Function existence check =E2=80=93 if (*dev->dev_ops->mac_addr_add) = =E2=80=93 can be > taken out of the loop. We don't need to check it on each iteration. > > 2. I'm not completely sure, but I think loop can be started from 1, not > from 0, because mac_pool_sel[0] is always zero. Am I right? > > > > Best regards, > > Igor > > > > On Thu, Jan 19, 2017 at 10:35 PM, Steve Shin (jonshin) > wrote: > > Dear maintainer, > > Sorry that I forgot to add =E2=80=9CFixes:=E2=80=9D line as follows: > > Fixes: 4bdefaade6d1 ("ethdev: VMDQ enhancements") > > Can you please add the above line as part of comment? > > Thanks, > Steve > > > On 1/19/17, 10:47 AM, "Steve Shin (jonshin)" wrote: > > This patch fixes a bug in replaying MAC address to the hardware > in rte_eth_dev_config_restore() routine. > > Signed-off-by: Steve Shin > --- > lib/librte_ether/rte_ethdev.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/lib/librte_ether/rte_ethdev.c > b/lib/librte_ether/rte_ethdev.c > index 4790faf..7e01f10 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -951,10 +951,12 @@ rte_eth_dev_config_restore(uint8_t port_id) > continue; > > /* add address to the hardware */ > - if (*dev->dev_ops->mac_addr_add && > - (dev->data->mac_pool_sel[i] & (1ULL << pool))) > - (*dev->dev_ops->mac_addr_add)(dev, &addr, i, > pool); > - else { > + if (*dev->dev_ops->mac_addr_add) { > + if (dev->data->mac_pool_sel[i] & (1ULL << pool)) > + (*dev->dev_ops->mac_addr_add)(dev, &addr, > i, pool); > + else > + continue; > + } else { > RTE_PMD_DEBUG_TRACE("port %d: MAC address array > not supported\n", > port_id); > /* exit the loop but not return an error */ > -- > 2.9.3 > > > >