From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 09A1537A2 for ; Thu, 7 Dec 2017 21:11:41 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Dec 2017 12:11:40 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,374,1508828400"; d="scan'208";a="9995072" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.225.165]) ([10.241.225.165]) by FMSMGA003.fm.intel.com with ESMTP; 07 Dec 2017 12:11:40 -0800 To: Vipin Varghese , dev@dpdk.org Cc: david.hunt@intel.com, deepak.k.jain@intel.com References: <1512071396-10653-1-git-send-email-vipin.varghese@intel.com> From: Ferruh Yigit Message-ID: Date: Thu, 7 Dec 2017 12:11:39 -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: <1512071396-10653-1-git-send-email-vipin.varghese@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v1] net/tap: allow user mac to be passed as args 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: Thu, 07 Dec 2017 20:11:42 -0000 On 11/30/2017 11:49 AM, Vipin Varghese wrote: > One of the uses cases from customer site is use TAP PMD to intake > user specific MAC address during probe. This allows applications > make use of interfaces with desired MAC. > > Extending MAC argumentinfrastructure for tap PMD; we pass custom > MAC address in string format (sample - 11:22:33:44:55:66). Overall lgtm, please check a few comments below. > > Signed-off-by: Vipin Varghese > --- > drivers/net/tap/rte_eth_tap.c | 56 +++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 52 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c > index 6b27679..0c53458 100644 > --- a/drivers/net/tap/rte_eth_tap.c > +++ b/drivers/net/tap/rte_eth_tap.c > @@ -81,6 +81,8 @@ > #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0) > #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0) > > +static unsigned char user_mac[ETHER_ADDR_LEN]; > + > static struct rte_vdev_driver pmd_tap_drv; > > static const char *valid_arguments[] = { > @@ -1291,13 +1293,20 @@ enum ioctl_mode { > pmd->txq[i].fd = -1; > } > > - if (fixed_mac_type) { > + if (fixed_mac_type == 1) { Instead of hardcoded type values 1 & 2, can you please use macros? > /* fixed mac = 00:64:74:61:70: */ > static int iface_idx; > char mac[ETHER_ADDR_LEN] = "\0dtap"; > > mac[ETHER_ADDR_LEN - 1] = iface_idx++; > rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN); > + } else if (fixed_mac_type == 2) { > + /* user mac is recieved */ s/recieved/received > + RTE_LOG(INFO, PMD, > + "Using user MAC (%02x:%02x:%02x:%02x:%02x:%02x)\n", > + user_mac[0], user_mac[1], user_mac[2], > + user_mac[3], user_mac[4], user_mac[5]); > + rte_memcpy(&pmd->eth_addr, user_mac, ETHER_ADDR_LEN); > } else { > eth_random_addr((uint8_t *)&pmd->eth_addr); > } > @@ -1471,9 +1480,48 @@ enum ioctl_mode { > const char *value, > void *extra_args) > { > - if (value && > - !strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED))) > - *(int *)extra_args = 1; > + char macTemp[20], *macByte = NULL; > + unsigned int index = 0; > + > + if (value) { > + if (!strncasecmp(ETH_TAP_MAC_FIXED, value, > + strlen(ETH_TAP_MAC_FIXED))) { > + *(int *)extra_args = 1; > + } else { > + RTE_LOG(INFO, PMD, "User shared MAC param (%s)\n", > + value); Is this log needs to be in INFO level, since there is already and info level log when MAC set, what about making this debug? > + > + /* desired format aa:bb:cc:dd:ee:ff:11 */ Can you please update RTE_PMD_REGISTER_PARAM_STRING with new param, put expected format info there as well? > + if (strlen(value) == 17) { > + strncpy(macTemp, value, 18); > + > + macByte = strtok(macTemp, ":"); > + while ((macByte != NULL) && > + (strspn(macByte, "0123456789ABCDEFabcdef")) && > + (strspn((macByte + 1), "0123456789ABCDEFabcdef")) && > + (strlen(macByte) == 2)) { > + user_mac[index] = strtoul(macByte, NULL, 16); > + macByte = strtok(NULL, ":"); > + index += 1; > + } I would extract the string to mac logic into its own function, but up to you. > + > + if (index != 6) { > + RTE_LOG(ERR, PMD, " failure in MAC value (%s) at %u\n", > + macByte, index + 1); > + return -1; And this is not related to this patch, but just as reminder, when a virtual driver probe fails vdev bus stops probing with an error, so all remaining virtual devices are not probed, in case one might want to fix ;) > + } > + > + RTE_LOG(DEBUG, PMD, " User MAC (%s) considered\n", "User defined MAC" ? And can you please add an port identifier, port_id or device name or interface name, for the case there are multiple tap device to identify which one get user defined MAC. > + value); > + *(int *)extra_args = 2; > + } else { > + RTE_LOG(ERR, PMD, " Mismatch on format for (%s)\n", > + value); > + return -1; > + } > + } > + } > + > return 0; > } > >