From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by dpdk.org (Postfix) with ESMTP id 897E31C00 for ; Fri, 8 Dec 2017 11:14:12 +0100 (CET) Received: by mail-wm0-f66.google.com with SMTP id f140so2387600wmd.2 for ; Fri, 08 Dec 2017 02:14:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-transfer-encoding:content-language; bh=pJlQ2hI7M8PzYtCeoqmq35CX9gPLBaOOjPmqiV8vR/Q=; b=OrD+/onB43PG8IhwclRXhsvHSxE8vFye+NYsobTl7kONrPOU62fRt1EWmOsB89jNLJ VxQ5SxQuCLjliAp37klPpwOnPiaQOimTsm4y5mhKefN6G5KR5hJ7nDloj0oy2gQryRaY UrPHJKD3qXFdr7uoqs2x70zffkfv8NygVx6lqxJjWwubHWYBxMoX35hAMGxib+JXRjPu NpYJLQjOCEqAlEJb/xlr3bpFjWfSAG0wz7aOlNGQAtPD20jzVP+MTGjdfTbPzXcbdvSD Tzoz+Di+/ambvgde5oYir7W9egJxUtCRGenJOx/c9P1lio0AgsH0sUeTuSX1Mn6iF9KS sSmQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=pJlQ2hI7M8PzYtCeoqmq35CX9gPLBaOOjPmqiV8vR/Q=; b=oQRjJWLWwe4wb+zlP0iTr1GkNQfVSAzCa0cYF1YKsEeWQTlg6NNq/U8qkYdhDHQqg9 REz4sixUgkEKU0u+2Rk/6IUinSZs1y0vxOpSPbTPhFbevP02ICzygyK/m4ZxomaVMuWD klp0Jn/IF0LQXt2S18/gCHM7sECxcp2f1gIIKQrMEOl8/gSQzyLqP5YqY+QJT64LH92L u1A8yVd6+oQEbposenH3l8SiFG/tU8SDKNVjarMbAvzPhNZGwy2uzLFkSS+gNMx2bqTd BVT56/cmegGRCvABmPljneI+wVTRPrOPR+HFMKcisj1YeKfMpAEKzDh6WrvZNLlgH6KD SPQg== X-Gm-Message-State: AKGB3mILzlmI8R5t00MRYhvPGT2zQfjnorv4FcBeFz712/hRoRK8p5OP aYfvaPNkv+xZLv+5021FwVWDfKjkVic= X-Google-Smtp-Source: AGs4zMaSgc3hqk/7gfONZZH9Wrkur2aksXMwhjO5q7abGhxoCLehqG36Bpk/jUMwC5UZQQApxtNFCQ== X-Received: by 10.28.234.80 with SMTP id i77mr3447327wmh.76.1512728051879; Fri, 08 Dec 2017 02:14:11 -0800 (PST) Received: from [192.168.1.79] (82.107.69.91.rev.sfr.net. [91.69.107.82]) by smtp.gmail.com with ESMTPSA id n14sm1217356wmh.37.2017.12.08.02.14.10 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 08 Dec 2017 02:14:11 -0800 (PST) To: dev@dpdk.org References: <1512071396-10653-1-git-send-email-vipin.varghese@intel.com> From: Pascal Mazon Message-ID: <6c4d799a-7b0d-2860-cf76-fb7b400c7825@6wind.com> Date: Fri, 8 Dec 2017 11:14:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US 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: Fri, 08 Dec 2017 10:14:12 -0000 Hi, Can you also not use a global value for user_mac, but instead change the last argument for eth_dev_tap_create(): Use directly a char mac[ETHER_ADDR_LEN], automatic variable from rte_pmd_tap_probe(). In set_mac_type(), you can check either for "fixed" or a correct custom mac address. Then eth_dev_tap_create() can check if the provided mac is empty (!fixed and !custom_mac), to generate a random one. /* desired format aa:bb:cc:dd:ee:ff:11 */      The :11 goes beyond standard MAC addresses ;-) The commit log has few mistakes: - missing "to" after "applications" - "argumentinfrastructure" I otherwise concur with Ferruh's remarks. Best regards, Pascal On 07/12/2017 21:11, Ferruh Yigit wrote: > 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; >> } >> >>