From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id C9278FAC8 for ; Wed, 18 Jan 2017 19:40:42 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 18 Jan 2017 10:39:13 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,250,1477983600"; d="scan'208";a="32295157" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.38]) ([10.237.220.38]) by orsmga002.jf.intel.com with ESMTP; 18 Jan 2017 10:39:12 -0800 To: "Steve Shin (jonshin)" , "dev@dpdk.org" References: <3CEF366F-80D7-49E1-838A-59D489585DC6@cisco.com> From: Ferruh Yigit Message-ID: Date: Wed, 18 Jan 2017 18:39:11 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <3CEF366F-80D7-49E1-838A-59D489585DC6@cisco.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] replay MAC address in rte_eth_dev_config_restore() 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: Wed, 18 Jan 2017 18:40:43 -0000 On 1/18/2017 4:12 AM, Steve Shin (jonshin) wrote: > Hi, > > I have a question on MAC address replay logic in rte_eth_dev_config_restore(): > > lib/librte_ether/rte_ethdev.c: > code snippet of rte_eth_dev_config_restore() > ~~ > /* replay MAC address configuration */ > for (i = 0; i < dev_info.max_mac_addrs; i++) { > addr = dev->data->mac_addrs[i]; > > /* skip zero address */ > if (is_zero_ether_addr(&addr)) > 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 { > RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n", > port_id); > /* exit the loop but not return an error */ > break; > } > } > > dev->data->mac_addrs[0] is filled with a HW MAC address (by default) when a device is initialized. But there’s no flag setting for dev->data->mac_pool_sel[0]. > As the value of “(dev->data->mac_pool_sel[i] & (1ULL << pool)) “ becomes 0 for i=0, the replay logic will be stopped without processing the rest of MAC addresses in dev->data->mac_addrs[] list. > Shouldn’t ‘break’ statement be replaced with ‘continue’ to process other MACs in the list? Hi Steve, Looks like you are right. The commit that pool check added (4bdefaad) says: " Fix bug in rte_eth_dev_config_restore function, which will restore all MAC address to default pool. " So intention looks like not stop adding mac_addr loop if mac is not in pool, but instead not adding a mac address if it is not part of pool. But pool check seems added later, and initial "break" is added for "dev_ops->mac_addr_add" check. Logic can be: if (dev_ops->mac_addr_add) if (mac in pool) dev_ops->mac_addr_add() else continue else break But of course, instead of checking the dev_ops in a loop for same device, it makes more sense to remove it out of loop. Are you planning to send a patch for this issue? It is easier to discuss code based on patches.. > > Any feedback would be appreciated. > > Regards, > Steve >