From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 61BF11B61A for ; Fri, 22 Dec 2017 22:55:54 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Dec 2017 13:55:53 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,442,1508828400"; d="scan'208";a="13983877" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.224.109]) ([10.241.224.109]) by FMSMGA003.fm.intel.com with ESMTP; 22 Dec 2017 13:55:52 -0800 To: Hemant Agrawal , dev@dpdk.org References: <1512042367-6361-1-git-send-email-hemant.agrawal@nxp.com> From: Ferruh Yigit Message-ID: <1146be98-43a9-91f5-8ad2-53ae31b48b4c@intel.com> Date: Fri, 22 Dec 2017 13:55:52 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <1512042367-6361-1-git-send-email-hemant.agrawal@nxp.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 1/3] kni: support for MAC addr change 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, 22 Dec 2017 21:55:55 -0000 On 11/30/2017 3:46 AM, Hemant Agrawal wrote: > This patch adds following: > 1. Option to configure the mac address during create. Generate random > address only if the user has not provided any valid address. > 2. Inform usespace, if mac address is being changed in linux. > 3. Implement default handling of mac address change in the corresponding > ethernet device.> > Signed-off-by: Hemant Agrawal Overall lgtm, there are a few issues commented below. Thanks, ferruh <...> > @@ -269,11 +275,15 @@ The code for allocating the kernel NIC interfaces for a specific port is as foll > conf.addr = dev_info.pci_dev->addr; > conf.id = dev_info.pci_dev->id; > > + /* Get the interface default mac address */ > + rte_eth_macaddr_get(port_id, struct ether_addr *)&conf.mac_addr); a parentheses is missing, good to fix although this is document :) <...> > @@ -587,3 +603,26 @@ Currently, setting a new MTU and configuring the network interface (up/ down) ar > RTE_LOG(ERR, APP, "Failed to start port %d\n", port_id); > return ret; > } > + > + /* Callback for request of configuring device mac address */ > + > + static int > + kni_config_mac_address(uint16_t port_id, uint8_t mac_addr[]) > + { > + int ret = 0; > + > + if (port_id >= rte_eth_dev_count() || port_id >= RTE_MAX_ETHPORTS) { > + RTE_LOG(ERR, KNI, "Invalid port id %d\n", port_id); > + return -EINVAL; > + } > + > + RTE_LOG(INFO, KNI, "Configure mac address of %d", port_id); > + /* Configure network interface mac address */ > + ret = rte_eth_dev_default_mac_addr_set(port_id, > + (struct ether_addr *)mac_addr); > + if (ret < 0) > + RTE_LOG(ERR, KNI, "Failed to config mac_addr for port %d\n", > + port_id); > + > + return ret; > + } It is hard to maintain code in doc, I am aware other related code is already in document but what do you think keeping this minimal, like: static int kni_config_mac_address(uint16_t port_id, uint8_t mac_addr[]) { .... } <...> > @@ -559,6 +583,14 @@ rte_kni_handle_request(struct rte_kni *kni) > req->result = kni->ops.config_network_if(\ > kni->ops.port_id, req->if_up); > break; > + case RTE_KNI_REQ_CHANGE_MAC_ADDR: /* Change MAC Address */ > + if (kni->ops.config_mac_address) > + req->result = kni->ops.config_mac_address( > + kni->ops.port_id, req->mac_addr); > + else > + req->result = kni_config_mac_address( > + kni->ops.port_id, req->mac_addr); ops.port_id can be unset if there is no physically backing device the kni interface. And I guess for that case port_id will be 0 and it will corrupt other interface's data. There needs to find a way to handle the port_id not set case. Since kni sample always creates a KNI interface backed by pyhsical device, this is not an issue for kni sample app but please think about kni pmd case. <...> > @@ -87,6 +91,7 @@ struct rte_kni_conf { > unsigned mbuf_size; /* mbuf size */ > struct rte_pci_addr addr; > struct rte_pci_id id; > + char mac_addr[ETHER_ADDR_LEN]; /* MAC address assigned to KNI */ > > __extension__ > uint8_t force_bind : 1; /* Flag to bind kernel thread */ "struct rte_kni_conf" is a public struct. Adding a variable into the middle of the struct will break the ABI. But I think it is OK to add to the end, unless struct is not used as array. <...>