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 120A64C9F for ; Tue, 11 Sep 2018 17:35:21 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Sep 2018 08:35:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,360,1531810800"; d="scan'208";a="70024339" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.56]) ([10.237.221.56]) by fmsmga008.fm.intel.com with ESMTP; 11 Sep 2018 08:35:19 -0700 To: Juhamatti Kuusisaari Cc: dev@dpdk.org References: <20180906165613.25848-1-juhamatti.kuusisaari@coriant.com> <20180910165514.908-1-juhamatti.kuusisaari@coriant.com> From: Ferruh Yigit Openpgp: preference=signencrypt Message-ID: <315802bc-db03-369e-2bb6-6a1cd34b9ffb@intel.com> Date: Tue, 11 Sep 2018 16:35:19 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180910165514.908-1-juhamatti.kuusisaari@coriant.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6] net/pcap: physical interface MAC address support 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: Tue, 11 Sep 2018 15:35:22 -0000 On 9/10/2018 5:55 PM, Juhamatti Kuusisaari wrote: > At the moment, PCAP interfaces use dummy MAC by default. This change > adds support for selecting PCAP physical interface MAC with phy_mac=1 > devarg. This allows to setup packet flows using the physical interface > MAC. > > Signed-off-by: Juhamatti Kuusisaari > > --- > v6: > * Review changes: > * Clarified devarg applicability (applies to iface-only) > * Introduced detailed documentation for the devarg > * More checkpatches-fixes > v5-v3: > * FreeBSD related compilation and checkpatches-fixes > v2: > * FreeBSD support introduced > * Release notes added > v1: > * phy_mac=1 devarg support <...> > +#elif defined(__FreeBSD__) Just to double check did you check/verify below code on FreeBSD? <...> > @@ -955,6 +1034,10 @@ eth_from_pcaps_common(struct rte_vdev_device *vdev, > else > (*internals)->if_index = if_nametoindex(pair->value); > > + if (phy_mac && pair) /* phy_mac arg is applied only to iface */ Having this comment is good, but "iface" is so generic, it may be confusing for beyond this context, what about "only if iface devarg provided" kind of detail? <...> > @@ -989,6 +1073,19 @@ eth_from_pcaps(struct rte_vdev_device *vdev, > return 0; > } > > +static int > +select_phy_mac(const char *key, const char *value, void *extra_args) > +{ > + if (extra_args) { > + const int phy_mac = atoi(value); > + int *enable_phy_mac = extra_args; > + > + if (phy_mac) > + *enable_phy_mac = 1; > + } > + return 0; > +} This is causing build error because of "key" not used, needs __rte_unused marker. <...> > @@ -1031,6 +1129,16 @@ pmd_pcap_probe(struct rte_vdev_device *dev) > * reading / writing > */ > if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) == 1) { > + /* > + * We check first whether we want to use phy MAC of the PCAP > + * interface. > + */ > + if (rte_kvargs_count(kvlist, ETH_PCAP_PHY_MAC_ARG)) { Do you need count check at all? > + ret = rte_kvargs_process(kvlist, ETH_PCAP_PHY_MAC_ARG, > + &select_phy_mac, &phy_mac); > + if (ret < 0) > + goto free_kvlist; > + } I would prefer to see this block below ETH_PCAP_IFACE_ARG check, this block is for "iface", so it makes more sense to me first verify it, later verify phy_mac