From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 1A2622C08 for ; Wed, 19 Oct 2016 08:10:15 +0200 (CEST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga105.fm.intel.com with ESMTP; 18 Oct 2016 23:10:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,513,1473145200"; d="scan'208";a="21536631" Received: from jguo15x-mobl3.ccr.corp.intel.com (HELO [10.239.205.35]) ([10.239.205.35]) by orsmga004.jf.intel.com with ESMTP; 18 Oct 2016 23:10:13 -0700 To: Ferruh Yigit , "Ananyev, Konstantin" , "Zhang, Helin" , "Wu, Jingjing" References: <1474887098-115474-1-git-send-email-jia.guo@intel.com> <1476582005-110811-1-git-send-email-jia.guo@intel.com> <2601191342CEEE43887BDE71AB9772583F0C1FF9@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772583F0C23DD@irsmsx105.ger.corp.intel.com> Cc: "dev@dpdk.org" From: "Guo, Jia" Message-ID: Date: Wed, 19 Oct 2016 14:10:12 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 1/2] drivers/i40e: fix X722 macro absence result in compile X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Oct 2016 06:10:16 -0000 hi,yigit Because of remove "#ifdef x722" will related with some prior patch and need related folks to make agreement on that, so we need to make a discuss later to get a conclusion. we definitely confidence that we will get the better balance base on the code maintenance effective. so just ignore this patch and i will rework new patch later. Thanks your great review and also appricate konstanitin's suggestion! On 10/19/2016 12:22 AM, Ferruh Yigit wrote: > On 10/17/2016 10:54 AM, Ananyev, Konstantin wrote: >> Hi Jeff, >> >>> hi, Konstantin >>> Thanks your constructive suggestion. I don't think your question is >>> silly and we also think about the code style simply and effective, but >>> may be i would interpret the reason why we do that. >>> >>> 1) Sure, user definitely can choose to define the macro or not when >>> building dpdk i40e PMD, but i don't think it is >>> necessary to invoke a ret_config option to let up layer user freedom use >>> it, because only the older version i40e driver does not support X722, >>> the newer version i40e driver will always support X722, so the macro >>> will be default hard code in the makefile. and we will use mac.type to >>> distinguish the difference register configure in run time. So we may >>> consider the macro just like a flag that highlight the difference of the >>> shared code between X710 and X722, that would benify the X710/X722 pmd >>> development but hardly no use to exposure to the up layer user. >>> >>> 2) i think the answer also could find from above. But i think if we >>> develop go to a certain stage in the future, mute the macro or use >>> script to remove them like the way from hw driver, for support all >>> device types maybe not a bad idea, right? >> Sorry, but I still didn't get it. >> If i40e driver will always support X722 then why do we need that macro at all? >> Why just not to remove it completely then? >> Same about run-time vs build-time choice: >> If let say i40e_get_rss_key() has to behave in a different way, why not to create >> i40e_get_rss_key_x722() and use it when hw mactype is x7222? >> Or at least inside i40e_get_rss_key() do something like: >> if (hw->mac.type == I40E_MAC_X722) {...} else {...} >> ? >> Why instead you have to pollute whole i40e code with all these #ifdef x7222/#else ...? >> Obviously that looks pretty ugly and hard to maintain. > It is not possible to remove "#ifdef x7222" from shared code, but what > about removing it from DPDK piece of the code, and code as it is always > defined? > > If this is OK, this patch is not more required. > And the removing #ifdef work can be done in another patch later. > >> Konstantin >