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 38F94F921 for ; Mon, 19 Dec 2016 12:04:01 +0100 (CET) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga105.fm.intel.com with ESMTP; 19 Dec 2016 03:04:00 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,373,1477983600"; d="scan'208";a="44646567" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.29]) ([10.237.220.29]) by fmsmga006.fm.intel.com with ESMTP; 19 Dec 2016 03:03:58 -0800 To: Vincent JARDIN , dev@dpdk.org References: <20161216143919.4909-1-ferruh.yigit@intel.com> <20161216190257.6921-1-ferruh.yigit@intel.com> <20161216190257.6921-19-ferruh.yigit@intel.com> <716260c7-4120-9984-3c46-02ad50b11a94@6wind.com> Cc: Jingjing Wu , Helin Zhang , Wenzhuo Lu , Chen Jing D , Bernard Iremonger From: Ferruh Yigit Message-ID: <625f5a53-e51e-7fb0-b477-c452e9217f41@intel.com> Date: Mon, 19 Dec 2016 11:03:58 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <716260c7-4120-9984-3c46-02ad50b11a94@6wind.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v5 18/29] app/testpmd: use VFD APIs on i40e 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: Mon, 19 Dec 2016 11:04:01 -0000 On 12/16/2016 8:30 PM, Vincent JARDIN wrote: > Le 16/12/2016 à 20:02, Ferruh Yigit a écrit : >> +#ifdef RTE_LIBRTE_IXGBE_PMD >> "set all queues drop (port_id) (on|off)\n" >> " Set drop enable bit for all queues.\n\n" >> >> "set vf split drop (port_id) (vf_id) (on|off)\n" >> " Set split drop enable bit for a VF from the PF.\n\n" >> +#endif > > it is not related to i40e. This serie should only be for i40e. Please check the patch a little wider, it doesn't introduce the ifdef, instead patch narrows down the scope of ifdef [1], because now some functions are not just for ixgbe, but used both with i40e and ixgbe, so the ifdef moved into the function itself [2]. [1] -#ifdef RTE_LIBRTE_IXGBE_PMD "set tx loopback (port_id) (on|off)\n" " Enable or disable tx loopback.\n\n" +#ifdef RTE_LIBRTE_IXGBE_PMD "set all queues drop (port_id) (on|off)\n" " Set drop enable bit for all queues.\n\n" "set vf split drop (port_id) (vf_id) (on|off)\n" " Set split drop enable bit for a VF from the PF.\n\n" +#endif "set vf mac antispoof (port_id) (vf_id) (on|off).\n" " Set MAC antispoof for a VF from the PF.\n\n" -#endif [2] @@ -11187,10 +11242,21 @@ cmd_set_tx_loopback_parsed( ... +#ifdef RTE_LIBRTE_IXGBE_PMD + if (strstr(dev_info.driver_name, "ixgbe") != NULL) + ret = rte_pmd_ixgbe_set_tx_loopback(res->port_id, is_on); +#endif +#ifdef RTE_LIBRTE_I40E_PMD + if (strstr(dev_info.driver_name, "i40e") != NULL) + ret = rte_pmd_i40e_set_tx_loopback(res->port_id, is_on); +#endif > > Moreover, it is a strange logic: how will it scale for all PMDs? It won't. That is why they are in ifdef. These are PMD specific APIs, as naming convention shows "rte_pmd_i40e_..." For the PMD features, which are not generic enough, but hardware is capable and user may want to benefit from, PMD specific APIs exists. Instead of these functions bloat the eth_dev layer, and give a fake sense of abstraction, these APIs moved into PMDs. And it is always possible to move these into ethdev layer, when multiple PMDs supports same feature. I agree this is something that needs to keep an eye on it, and be sure if an API is generic, move it into eth_dev layer. The application that use the PMD specific API, needs to be conditionally compiled because that API may not always exits. Thanks, ferruh