From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ferruh Yigit <ferruh.yigit@intel.com>
Subject: Re: [PATCH v5 18/29] app/testpmd: use VFD APIs on i40e
Date: Mon, 19 Dec 2016 11:03:58 +0000
Message-ID: <625f5a53-e51e-7fb0-b477-c452e9217f41@intel.com>
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>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit
Cc: Jingjing Wu <jingjing.wu@intel.com>, Helin Zhang <helin.zhang@intel.com>,
 Wenzhuo Lu <wenzhuo.lu@intel.com>, Chen Jing D <jing.d.chen@intel.com>,
 Bernard Iremonger <bernard.iremonger@intel.com>
To: Vincent JARDIN <vincent.jardin@6wind.com>, dev@dpdk.org
Return-path: <dev-bounces@dpdk.org>
Received: from mga05.intel.com (mga05.intel.com [192.55.52.43])
 by dpdk.org (Postfix) with ESMTP id 38F94F921
 for <dev@dpdk.org>; Mon, 19 Dec 2016 12:04:01 +0100 (CET)
In-Reply-To: <716260c7-4120-9984-3c46-02ad50b11a94@6wind.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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