[dpdk-dev] app/testpmd: fix ixgbe private API calling

  Some ixgbe private APIs are added to expose ixgbe
specific functions.
When they're used by testpmd, there's no check for
if the NICs are ixgbe. Other NICs also have chance
to  call these APIs.
This patch add the check and the feedback print.

Fixes: 425781ff5afe ("app/testpmd: add ixgbe VF management")

CC: stable@dpdk.org
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 app/test-pmd/cmdline.c | 101 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 81 insertions(+), 20 deletions(-)
  

Hi Wenzhuo,


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> Sent: Wednesday, January 11, 2017 2:48 AM
> To: dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API calling
> 
> Some ixgbe private APIs are added to expose ixgbe specific functions.
> When they're used by testpmd, there's no check for if the NICs are ixgbe.
> Other NICs also have chance to  call these APIs.
> This patch add the check and the feedback print.

I am not sure that testpmd is the right place to do this.
The rte_pmd_ixgbe_* functions are public API's which can be called by other applications.
The checks should be in the rte_pmd_ixgbe_* API's

 
> Fixes: 425781ff5afe ("app/testpmd: add ixgbe VF management")
> 
> CC: stable@dpdk.org
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
>  app/test-pmd/cmdline.c | 101
> +++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 81 insertions(+), 20 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> f768b6b..172861a 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -10883,11 +10883,16 @@ struct cmd_vf_vlan_anti_spoof_result {
>  	__attribute__((unused)) void *data)
>  {
>  	struct cmd_vf_vlan_anti_spoof_result *res = parsed_result;
> -	int ret = 0;
> +	int ret = -ENOTSUP;
>  	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
> +	struct rte_eth_dev_info dev_info;
> 
> -	ret = rte_pmd_ixgbe_set_vf_vlan_anti_spoof(res->port_id, res-
> >vf_id,
> -			is_on);
> +	rte_eth_dev_info_get(res->port_id, &dev_info);
> +
> +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> +		ret = rte_pmd_ixgbe_set_vf_vlan_anti_spoof(res->port_id,
> +							   res->vf_id,
> +							   is_on);
>  	switch (ret) {
>  	case 0:
>  		break;
> @@ -10897,6 +10902,9 @@ struct cmd_vf_vlan_anti_spoof_result {
>  	case -ENODEV:
>  		printf("invalid port_id %d\n", res->port_id);
>  		break;
> +	case -ENOTSUP:
> +		printf("function not implemented\n");
> +		break;
>  	default:
>  		printf("programming error: (%s)\n", strerror(-ret));
>  	}
> @@ -10968,11 +10976,16 @@ struct cmd_vf_mac_anti_spoof_result {
>  	__attribute__((unused)) void *data)
>  {
>  	struct cmd_vf_mac_anti_spoof_result *res = parsed_result;
> -	int ret;
> +	int ret = -ENOTSUP;
>  	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
> +	struct rte_eth_dev_info dev_info;
> 
> -	ret = rte_pmd_ixgbe_set_vf_mac_anti_spoof(res->port_id, res-
> >vf_id,
> -			is_on);
> +	rte_eth_dev_info_get(res->port_id, &dev_info);
> +
> +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> +		ret = rte_pmd_ixgbe_set_vf_mac_anti_spoof(res->port_id,
> +							  res->vf_id,
> +							  is_on);
>  	switch (ret) {
>  	case 0:
>  		break;
> @@ -10982,6 +10995,9 @@ struct cmd_vf_mac_anti_spoof_result {
>  	case -ENODEV:
>  		printf("invalid port_id %d\n", res->port_id);
>  		break;
> +	case -ENOTSUP:
> +		printf("function not implemented\n");
> +		break;
>  	default:
>  		printf("programming error: (%s)\n", strerror(-ret));
>  	}
> @@ -11053,10 +11069,15 @@ struct cmd_vf_vlan_stripq_result {
>  	__attribute__((unused)) void *data)
>  {
>  	struct cmd_vf_vlan_stripq_result *res = parsed_result;
> -	int ret = 0;
> +	int ret = -ENOTSUP;
>  	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
> +	struct rte_eth_dev_info dev_info;
> 
> -	ret = rte_pmd_ixgbe_set_vf_vlan_stripq(res->port_id, res->vf_id,
> is_on);
> +	rte_eth_dev_info_get(res->port_id, &dev_info);
> +
> +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> +		ret = rte_pmd_ixgbe_set_vf_vlan_stripq(res->port_id, res-
> >vf_id,
> +						       is_on);
>  	switch (ret) {
>  	case 0:
>  		break;
> @@ -11066,6 +11087,9 @@ struct cmd_vf_vlan_stripq_result {
>  	case -ENODEV:
>  		printf("invalid port_id %d\n", res->port_id);
>  		break;
> +	case -ENOTSUP:
> +		printf("function not implemented\n");
> +		break;
>  	default:
>  		printf("programming error: (%s)\n", strerror(-ret));
>  	}
> @@ -11137,9 +11161,14 @@ struct cmd_vf_vlan_insert_result {
>  	__attribute__((unused)) void *data)
>  {
>  	struct cmd_vf_vlan_insert_result *res = parsed_result;
> -	int ret;
> +	int ret = -ENOTSUP;
> +	struct rte_eth_dev_info dev_info;
> 
> -	ret = rte_pmd_ixgbe_set_vf_vlan_insert(res->port_id, res->vf_id,
> res->vlan_id);
> +	rte_eth_dev_info_get(res->port_id, &dev_info);
> +
> +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> +		ret = rte_pmd_ixgbe_set_vf_vlan_insert(res->port_id, res-
> >vf_id,
> +						       res->vlan_id);
>  	switch (ret) {
>  	case 0:
>  		break;
> @@ -11149,6 +11178,9 @@ struct cmd_vf_vlan_insert_result {
>  	case -ENODEV:
>  		printf("invalid port_id %d\n", res->port_id);
>  		break;
> +	case -ENOTSUP:
> +		printf("function not implemented\n");
> +		break;
>  	default:
>  		printf("programming error: (%s)\n", strerror(-ret));
>  	}
> @@ -11210,10 +11242,14 @@ struct cmd_tx_loopback_result {
>  	__attribute__((unused)) void *data)
>  {
>  	struct cmd_tx_loopback_result *res = parsed_result;
> -	int ret;
> +	int ret = -ENOTSUP;
>  	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
> +	struct rte_eth_dev_info dev_info;
> 
> -	ret = rte_pmd_ixgbe_set_tx_loopback(res->port_id, is_on);
> +	rte_eth_dev_info_get(res->port_id, &dev_info);
> +
> +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> +		ret = rte_pmd_ixgbe_set_tx_loopback(res->port_id, is_on);
>  	switch (ret) {
>  	case 0:
>  		break;
> @@ -11223,6 +11259,9 @@ struct cmd_tx_loopback_result {
>  	case -ENODEV:
>  		printf("invalid port_id %d\n", res->port_id);
>  		break;
> +	case -ENOTSUP:
> +		printf("function not implemented\n");
> +		break;
>  	default:
>  		printf("programming error: (%s)\n", strerror(-ret));
>  	}
> @@ -11287,10 +11326,14 @@ struct cmd_all_queues_drop_en_result {
>  	__attribute__((unused)) void *data)
>  {
>  	struct cmd_all_queues_drop_en_result *res = parsed_result;
> -	int ret = 0;
> +	int ret = -ENOTSUP;
>  	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
> +	struct rte_eth_dev_info dev_info;
> 
> -	ret = rte_pmd_ixgbe_set_all_queues_drop_en(res->port_id, is_on);
> +	rte_eth_dev_info_get(res->port_id, &dev_info);
> +
> +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> +		ret = rte_pmd_ixgbe_set_all_queues_drop_en(res-
> >port_id, is_on);
>  	switch (ret) {
>  	case 0:
>  		break;
> @@ -11300,6 +11343,9 @@ struct cmd_all_queues_drop_en_result {
>  	case -ENODEV:
>  		printf("invalid port_id %d\n", res->port_id);
>  		break;
> +	case -ENOTSUP:
> +		printf("function not implemented\n");
> +		break;
>  	default:
>  		printf("programming error: (%s)\n", strerror(-ret));
>  	}
> @@ -11370,11 +11416,16 @@ struct cmd_vf_split_drop_en_result {
>  	__attribute__((unused)) void *data)
>  {
>  	struct cmd_vf_split_drop_en_result *res = parsed_result;
> -	int ret;
> +	int ret = -ENOTSUP;
>  	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
> +	struct rte_eth_dev_info dev_info;
> 
> -	ret = rte_pmd_ixgbe_set_vf_split_drop_en(res->port_id, res-
> >vf_id,
> -			is_on);
> +	rte_eth_dev_info_get(res->port_id, &dev_info);
> +
> +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> +		ret = rte_pmd_ixgbe_set_vf_split_drop_en(res->port_id,
> +							 res->vf_id,
> +							 is_on);
>  	switch (ret) {
>  	case 0:
>  		break;
> @@ -11384,6 +11435,9 @@ struct cmd_vf_split_drop_en_result {
>  	case -ENODEV:
>  		printf("invalid port_id %d\n", res->port_id);
>  		break;
> +	case -ENOTSUP:
> +		printf("function not implemented\n");
> +		break;
>  	default:
>  		printf("programming error: (%s)\n", strerror(-ret));
>  	}
> @@ -11455,10 +11509,14 @@ struct cmd_set_vf_mac_addr_result {
>  	__attribute__((unused)) void *data)
>  {
>  	struct cmd_set_vf_mac_addr_result *res = parsed_result;
> -	int ret;
> +	int ret = -ENOTSUP;
> +	struct rte_eth_dev_info dev_info;
> 
> -	ret = rte_pmd_ixgbe_set_vf_mac_addr(res->port_id, res->vf_id,
> -			&res->mac_addr);
> +	rte_eth_dev_info_get(res->port_id, &dev_info);
> +
> +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> +		ret = rte_pmd_ixgbe_set_vf_mac_addr(res->port_id, res-
> >vf_id,
> +						    &res->mac_addr);
>  	switch (ret) {
>  	case 0:
>  		break;
> @@ -11468,6 +11526,9 @@ struct cmd_set_vf_mac_addr_result {
>  	case -ENODEV:
>  		printf("invalid port_id %d\n", res->port_id);
>  		break;
> +	case -ENOTSUP:
> +		printf("function not implemented\n");
> +		break;
>  	default:
>  		printf("programming error: (%s)\n", strerror(-ret));
>  	}
> --
> 1.9.3

Regards,

Bernard.
  

Hi Wenzhuo,

<snip>
> > Subject: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API calling
> >
> > Some ixgbe private APIs are added to expose ixgbe specific functions.
> > When they're used by testpmd, there's no check for if the NICs are ixgbe.
> > Other NICs also have chance to  call these APIs.
> > This patch add the check and the feedback print.
> 
> I am not sure that testpmd is the right place to do this.
> The rte_pmd_ixgbe_* functions are public API's which can be called by other
> applications.
> The checks should be in the rte_pmd_ixgbe_* API's

It is useful to handle the return code -ENOTSUP in testpmd.  
 
 
> > Fixes: 425781ff5afe ("app/testpmd: add ixgbe VF management")
> >
> > CC: stable@dpdk.org
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > ---
> >  app/test-pmd/cmdline.c | 101
> > +++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 81 insertions(+), 20 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > f768b6b..172861a 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -10883,11 +10883,16 @@ struct cmd_vf_vlan_anti_spoof_result {
> >  	__attribute__((unused)) void *data)
> >  {
> >  	struct cmd_vf_vlan_anti_spoof_result *res = parsed_result;
> > -	int ret = 0;
> > +	int ret = -ENOTSUP;
> >  	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
> > +	struct rte_eth_dev_info dev_info;
> >
> > -	ret = rte_pmd_ixgbe_set_vf_vlan_anti_spoof(res->port_id, res-
> > >vf_id,
> > -			is_on);
> > +	rte_eth_dev_info_get(res->port_id, &dev_info);
> > +
> > +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> > +		ret = rte_pmd_ixgbe_set_vf_vlan_anti_spoof(res->port_id,
> > +							   res->vf_id,
> > +							   is_on);
> >  	switch (ret) {
> >  	case 0:
> >  		break;
> > @@ -10897,6 +10902,9 @@ struct cmd_vf_vlan_anti_spoof_result {
> >  	case -ENODEV:
> >  		printf("invalid port_id %d\n", res->port_id);
> >  		break;
> > +	case -ENOTSUP:
> > +		printf("function not implemented\n");
> > +		break;
> >  	default:
> >  		printf("programming error: (%s)\n", strerror(-ret));
> >  	}
> > @@ -10968,11 +10976,16 @@ struct cmd_vf_mac_anti_spoof_result {
> >  	__attribute__((unused)) void *data)
> >  {
> >  	struct cmd_vf_mac_anti_spoof_result *res = parsed_result;
> > -	int ret;
> > +	int ret = -ENOTSUP;
> >  	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
> > +	struct rte_eth_dev_info dev_info;
> >
> > -	ret = rte_pmd_ixgbe_set_vf_mac_anti_spoof(res->port_id, res-
> > >vf_id,
> > -			is_on);
> > +	rte_eth_dev_info_get(res->port_id, &dev_info);
> > +
> > +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> > +		ret = rte_pmd_ixgbe_set_vf_mac_anti_spoof(res->port_id,
> > +							  res->vf_id,
> > +							  is_on);
> >  	switch (ret) {
> >  	case 0:
> >  		break;
> > @@ -10982,6 +10995,9 @@ struct cmd_vf_mac_anti_spoof_result {
> >  	case -ENODEV:
> >  		printf("invalid port_id %d\n", res->port_id);
> >  		break;
> > +	case -ENOTSUP:
> > +		printf("function not implemented\n");
> > +		break;
> >  	default:
> >  		printf("programming error: (%s)\n", strerror(-ret));
> >  	}
> > @@ -11053,10 +11069,15 @@ struct cmd_vf_vlan_stripq_result {
> >  	__attribute__((unused)) void *data)
> >  {
> >  	struct cmd_vf_vlan_stripq_result *res = parsed_result;
> > -	int ret = 0;
> > +	int ret = -ENOTSUP;
> >  	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
> > +	struct rte_eth_dev_info dev_info;
> >
> > -	ret = rte_pmd_ixgbe_set_vf_vlan_stripq(res->port_id, res->vf_id,
> > is_on);
> > +	rte_eth_dev_info_get(res->port_id, &dev_info);
> > +
> > +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> > +		ret = rte_pmd_ixgbe_set_vf_vlan_stripq(res->port_id, res-
> > >vf_id,
> > +						       is_on);
> >  	switch (ret) {
> >  	case 0:
> >  		break;
> > @@ -11066,6 +11087,9 @@ struct cmd_vf_vlan_stripq_result {
> >  	case -ENODEV:
> >  		printf("invalid port_id %d\n", res->port_id);
> >  		break;
> > +	case -ENOTSUP:
> > +		printf("function not implemented\n");
> > +		break;
> >  	default:
> >  		printf("programming error: (%s)\n", strerror(-ret));
> >  	}
> > @@ -11137,9 +11161,14 @@ struct cmd_vf_vlan_insert_result {
> >  	__attribute__((unused)) void *data)
> >  {
> >  	struct cmd_vf_vlan_insert_result *res = parsed_result;
> > -	int ret;
> > +	int ret = -ENOTSUP;
> > +	struct rte_eth_dev_info dev_info;
> >
> > -	ret = rte_pmd_ixgbe_set_vf_vlan_insert(res->port_id, res->vf_id,
> > res->vlan_id);
> > +	rte_eth_dev_info_get(res->port_id, &dev_info);
> > +
> > +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> > +		ret = rte_pmd_ixgbe_set_vf_vlan_insert(res->port_id, res-
> > >vf_id,
> > +						       res->vlan_id);
> >  	switch (ret) {
> >  	case 0:
> >  		break;
> > @@ -11149,6 +11178,9 @@ struct cmd_vf_vlan_insert_result {
> >  	case -ENODEV:
> >  		printf("invalid port_id %d\n", res->port_id);
> >  		break;
> > +	case -ENOTSUP:
> > +		printf("function not implemented\n");
> > +		break;
> >  	default:
> >  		printf("programming error: (%s)\n", strerror(-ret));
> >  	}
> > @@ -11210,10 +11242,14 @@ struct cmd_tx_loopback_result {
> >  	__attribute__((unused)) void *data)
> >  {
> >  	struct cmd_tx_loopback_result *res = parsed_result;
> > -	int ret;
> > +	int ret = -ENOTSUP;
> >  	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
> > +	struct rte_eth_dev_info dev_info;
> >
> > -	ret = rte_pmd_ixgbe_set_tx_loopback(res->port_id, is_on);
> > +	rte_eth_dev_info_get(res->port_id, &dev_info);
> > +
> > +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> > +		ret = rte_pmd_ixgbe_set_tx_loopback(res->port_id, is_on);
> >  	switch (ret) {
> >  	case 0:
> >  		break;
> > @@ -11223,6 +11259,9 @@ struct cmd_tx_loopback_result {
> >  	case -ENODEV:
> >  		printf("invalid port_id %d\n", res->port_id);
> >  		break;
> > +	case -ENOTSUP:
> > +		printf("function not implemented\n");
> > +		break;
> >  	default:
> >  		printf("programming error: (%s)\n", strerror(-ret));
> >  	}
> > @@ -11287,10 +11326,14 @@ struct cmd_all_queues_drop_en_result {
> >  	__attribute__((unused)) void *data)
> >  {
> >  	struct cmd_all_queues_drop_en_result *res = parsed_result;
> > -	int ret = 0;
> > +	int ret = -ENOTSUP;
> >  	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
> > +	struct rte_eth_dev_info dev_info;
> >
> > -	ret = rte_pmd_ixgbe_set_all_queues_drop_en(res->port_id, is_on);
> > +	rte_eth_dev_info_get(res->port_id, &dev_info);
> > +
> > +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> > +		ret = rte_pmd_ixgbe_set_all_queues_drop_en(res-
> > >port_id, is_on);
> >  	switch (ret) {
> >  	case 0:
> >  		break;
> > @@ -11300,6 +11343,9 @@ struct cmd_all_queues_drop_en_result {
> >  	case -ENODEV:
> >  		printf("invalid port_id %d\n", res->port_id);
> >  		break;
> > +	case -ENOTSUP:
> > +		printf("function not implemented\n");
> > +		break;
> >  	default:
> >  		printf("programming error: (%s)\n", strerror(-ret));
> >  	}
> > @@ -11370,11 +11416,16 @@ struct cmd_vf_split_drop_en_result {
> >  	__attribute__((unused)) void *data)
> >  {
> >  	struct cmd_vf_split_drop_en_result *res = parsed_result;
> > -	int ret;
> > +	int ret = -ENOTSUP;
> >  	int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
> > +	struct rte_eth_dev_info dev_info;
> >
> > -	ret = rte_pmd_ixgbe_set_vf_split_drop_en(res->port_id, res-
> > >vf_id,
> > -			is_on);
> > +	rte_eth_dev_info_get(res->port_id, &dev_info);
> > +
> > +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> > +		ret = rte_pmd_ixgbe_set_vf_split_drop_en(res->port_id,
> > +							 res->vf_id,
> > +							 is_on);
> >  	switch (ret) {
> >  	case 0:
> >  		break;
> > @@ -11384,6 +11435,9 @@ struct cmd_vf_split_drop_en_result {
> >  	case -ENODEV:
> >  		printf("invalid port_id %d\n", res->port_id);
> >  		break;
> > +	case -ENOTSUP:
> > +		printf("function not implemented\n");
> > +		break;
> >  	default:
> >  		printf("programming error: (%s)\n", strerror(-ret));
> >  	}
> > @@ -11455,10 +11509,14 @@ struct cmd_set_vf_mac_addr_result {
> >  	__attribute__((unused)) void *data)
> >  {
> >  	struct cmd_set_vf_mac_addr_result *res = parsed_result;
> > -	int ret;
> > +	int ret = -ENOTSUP;
> > +	struct rte_eth_dev_info dev_info;
> >
> > -	ret = rte_pmd_ixgbe_set_vf_mac_addr(res->port_id, res->vf_id,
> > -			&res->mac_addr);
> > +	rte_eth_dev_info_get(res->port_id, &dev_info);
> > +
> > +	if (strstr(dev_info.driver_name, "ixgbe") != NULL)
> > +		ret = rte_pmd_ixgbe_set_vf_mac_addr(res->port_id, res-
> > >vf_id,
> > +						    &res->mac_addr);
> >  	switch (ret) {
> >  	case 0:
> >  		break;
> > @@ -11468,6 +11526,9 @@ struct cmd_set_vf_mac_addr_result {
> >  	case -ENODEV:
> >  		printf("invalid port_id %d\n", res->port_id);
> >  		break;
> > +	case -ENOTSUP:
> > +		printf("function not implemented\n");
> > +		break;
> >  	default:
> >  		printf("programming error: (%s)\n", strerror(-ret));
> >  	}
> > --
> > 1.9.3
 
 Regards,
 
 Bernard.
  

On 1/11/2017 3:20 PM, Iremonger, Bernard wrote:
> Hi Wenzhuo,
> 
> <snip>
>>> Subject: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API calling
>>>
>>> Some ixgbe private APIs are added to expose ixgbe specific functions.
>>> When they're used by testpmd, there's no check for if the NICs are ixgbe.
>>> Other NICs also have chance to  call these APIs.
>>> This patch add the check and the feedback print.
>>
>> I am not sure that testpmd is the right place to do this.
>> The rte_pmd_ixgbe_* functions are public API's which can be called by other
>> applications.
>> The checks should be in the rte_pmd_ixgbe_* API's
> 
> It is useful to handle the return code -ENOTSUP in testpmd.  
>  

Makes sense, and I think it is good idea to add them in your patch,
since it introduces returning -ENOTSUP, would you mind sending a new
version of your patch with this update?
So we can drop this patch completely.

Thanks,
ferruh
  

Hi Ferruh,

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, January 11, 2017 3:27 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix ixgbe
> private API calling
> 
> On 1/11/2017 3:20 PM, Iremonger, Bernard wrote:
> > Hi Wenzhuo,
> >
> > <snip>
> >>> Subject: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API
> >>> calling
> >>>
> >>> Some ixgbe private APIs are added to expose ixgbe specific functions.
> >>> When they're used by testpmd, there's no check for if the NICs are
> ixgbe.
> >>> Other NICs also have chance to  call these APIs.
> >>> This patch add the check and the feedback print.
> >>
> >> I am not sure that testpmd is the right place to do this.
> >> The rte_pmd_ixgbe_* functions are public API's which can be called by
> >> other applications.
> >> The checks should be in the rte_pmd_ixgbe_* API's
> >
> > It is useful to handle the return code -ENOTSUP in testpmd.
> >
> 
> Makes sense, and I think it is good idea to add them in your patch, since it
> introduces returning -ENOTSUP, would you mind sending a new version of
> your patch with this update?
> So we can drop this patch completely.
> 
> Thanks,
> ferruh
>
I don't think this patch should be dropped.
Testpmd is already handling -EINVAL and -ENODEV.
It makes sense for it to handle -ENOTSUP for the cases in the patch.

Regards,

Bernard.
  

On 1/11/2017 3:47 PM, Iremonger, Bernard wrote:
> Hi Ferruh,
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, January 11, 2017 3:27 PM
>> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>; dev@dpdk.org
>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
>> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix ixgbe
>> private API calling
>>
>> On 1/11/2017 3:20 PM, Iremonger, Bernard wrote:
>>> Hi Wenzhuo,
>>>
>>> <snip>
>>>>> Subject: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API
>>>>> calling
>>>>>
>>>>> Some ixgbe private APIs are added to expose ixgbe specific functions.
>>>>> When they're used by testpmd, there's no check for if the NICs are
>> ixgbe.
>>>>> Other NICs also have chance to  call these APIs.
>>>>> This patch add the check and the feedback print.
>>>>
>>>> I am not sure that testpmd is the right place to do this.
>>>> The rte_pmd_ixgbe_* functions are public API's which can be called by
>>>> other applications.
>>>> The checks should be in the rte_pmd_ixgbe_* API's
>>>
>>> It is useful to handle the return code -ENOTSUP in testpmd.
>>>
>>
>> Makes sense, and I think it is good idea to add them in your patch, since it
>> introduces returning -ENOTSUP, would you mind sending a new version of
>> your patch with this update?
>> So we can drop this patch completely.
>>
>> Thanks,
>> ferruh
>>
> I don't think this patch should be dropped.
> Testpmd is already handling -EINVAL and -ENODEV.
> It makes sense for it to handle -ENOTSUP for the cases in the patch.

This patch adds driver check [1] before ixgbe APIs, since now that check
is done within ixgbe APIs by your patch. Why do we need this patch at all?

[1]
if (strstr(dev_info.driver_name, "ixgbe") != NULL)

> 
> Regards,
> 
> Bernard.
>
  

Hi Ferruh,

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, January 11, 2017 4:19 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix ixgbe
> private API calling
> 
> On 1/11/2017 3:47 PM, Iremonger, Bernard wrote:
> > Hi Ferruh,
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Wednesday, January 11, 2017 3:27 PM
> >> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Lu, Wenzhuo
> >> <wenzhuo.lu@intel.com>; dev@dpdk.org
> >> Cc: Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
> >> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix ixgbe
> >> private API calling
> >>
> >> On 1/11/2017 3:20 PM, Iremonger, Bernard wrote:
> >>> Hi Wenzhuo,
> >>>
> >>> <snip>
> >>>>> Subject: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API
> >>>>> calling
> >>>>>
> >>>>> Some ixgbe private APIs are added to expose ixgbe specific functions.
> >>>>> When they're used by testpmd, there's no check for if the NICs are
> >> ixgbe.
> >>>>> Other NICs also have chance to  call these APIs.
> >>>>> This patch add the check and the feedback print.
> >>>>
> >>>> I am not sure that testpmd is the right place to do this.
> >>>> The rte_pmd_ixgbe_* functions are public API's which can be called
> >>>> by other applications.
> >>>> The checks should be in the rte_pmd_ixgbe_* API's
> >>>
> >>> It is useful to handle the return code -ENOTSUP in testpmd.
> >>>
> >>
> >> Makes sense, and I think it is good idea to add them in your patch,
> >> since it introduces returning -ENOTSUP, would you mind sending a new
> >> version of your patch with this update?
> >> So we can drop this patch completely.
> >>
> >> Thanks,
> >> ferruh
> >>
> > I don't think this patch should be dropped.
> > Testpmd is already handling -EINVAL and -ENODEV.
> > It makes sense for it to handle -ENOTSUP for the cases in the patch.
> 
> This patch adds driver check [1] before ixgbe APIs, since now that check is
> done within ixgbe APIs by your patch. Why do we need this patch at all?
> 
> [1]
> if (strstr(dev_info.driver_name, "ixgbe") != NULL)
 
I think our lines have got slightly crossed.
This patch is doing two things, the check [1] above which is not needed now (after my ixgbe patch).
Secondly it is handling the ret code of the rte_pmd_ixgbe_* API's, I think this is useful.

Regards,

Bernard.
  

On 1/11/2017 4:29 PM, Iremonger, Bernard wrote:
> Hi Ferruh,
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, January 11, 2017 4:19 PM
>> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>; dev@dpdk.org
>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
>> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix ixgbe
>> private API calling
>>
>> On 1/11/2017 3:47 PM, Iremonger, Bernard wrote:
>>> Hi Ferruh,
>>>
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Wednesday, January 11, 2017 3:27 PM
>>>> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Lu, Wenzhuo
>>>> <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
>>>> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix ixgbe
>>>> private API calling
>>>>
>>>> On 1/11/2017 3:20 PM, Iremonger, Bernard wrote:
>>>>> Hi Wenzhuo,
>>>>>
>>>>> <snip>
>>>>>>> Subject: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API
>>>>>>> calling
>>>>>>>
>>>>>>> Some ixgbe private APIs are added to expose ixgbe specific functions.
>>>>>>> When they're used by testpmd, there's no check for if the NICs are
>>>> ixgbe.
>>>>>>> Other NICs also have chance to  call these APIs.
>>>>>>> This patch add the check and the feedback print.
>>>>>>
>>>>>> I am not sure that testpmd is the right place to do this.
>>>>>> The rte_pmd_ixgbe_* functions are public API's which can be called
>>>>>> by other applications.
>>>>>> The checks should be in the rte_pmd_ixgbe_* API's
>>>>>
>>>>> It is useful to handle the return code -ENOTSUP in testpmd.
>>>>>
>>>>
>>>> Makes sense, and I think it is good idea to add them in your patch,
>>>> since it introduces returning -ENOTSUP, would you mind sending a new
>>>> version of your patch with this update?
>>>> So we can drop this patch completely.
>>>>
>>>> Thanks,
>>>> ferruh
>>>>
>>> I don't think this patch should be dropped.
>>> Testpmd is already handling -EINVAL and -ENODEV.
>>> It makes sense for it to handle -ENOTSUP for the cases in the patch.
>>
>> This patch adds driver check [1] before ixgbe APIs, since now that check is
>> done within ixgbe APIs by your patch. Why do we need this patch at all?
>>
>> [1]
>> if (strstr(dev_info.driver_name, "ixgbe") != NULL)
>  
> I think our lines have got slightly crossed.
> This patch is doing two things, the check [1] above which is not needed now (after my ixgbe patch).
> Secondly it is handling the ret code of the rte_pmd_ixgbe_* API's, I think this is useful.

I was thinking returning -ENOTSUP introduced for your patch, but it
seems this is not true for all functions, some is already returning
-ENOTSUP, so testpmd should cover them.

I believe intention of this patch is the first item you have mentioned,
but can be converted to second one too.

Wenzhuo,

When do you have time, can you convert this patch to one that covers
"-ENOTSUP" error messages for ixgbe APIs please? If you don't have time,
please let me know I can do the patch?

Thanks,
ferruh

> 
> Regards,
> 
> Bernard.
> 
>   
>
  

Hi Bernard,

> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Wednesday, January 11, 2017 6:27 PM
> To: Lu, Wenzhuo; dev@dpdk.org
> Cc: Wu, Jingjing; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API calling
> 
> Hi Wenzhuo,
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> > Sent: Wednesday, January 11, 2017 2:48 AM
> > To: dev@dpdk.org
> > Cc: Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>; stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API calling
> >
> > Some ixgbe private APIs are added to expose ixgbe specific functions.
> > When they're used by testpmd, there's no check for if the NICs are ixgbe.
> > Other NICs also have chance to  call these APIs.
> > This patch add the check and the feedback print.
> 
> I am not sure that testpmd is the right place to do this.
> The rte_pmd_ixgbe_* functions are public API's which can be called by other
> applications.
> The checks should be in the rte_pmd_ixgbe_* API's
To be safer, it's better to add a check in the APIs. 
But the APIs is so called private API, not really public.  Considering if we have the same function on different NICs, for example we have rte_pmd_ixgbe_a and rte_pmd_i40e_a.
APP still need to call them one by one, like
ret = rte_pmd_ixgbe_a;
ret = rte_pmd_i40e_a;

then, why not add the check, like
If (NIC is ixgbe)
	ret = rte_pmd_ixgbe_a;
if (NIC is i40e)
	ret = rte_pmd_i40e_a;

testpmd is an example to let the users to know how to use the APIs. They should follow the example.
How about your opinion?
  

Hi Wenzhuo,

> -----Original Message-----
> From: Lu, Wenzhuo
> Sent: Thursday, January 12, 2017 1:09 AM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API calling
> 
> Hi Bernard,
> 
> > -----Original Message-----
> > From: Iremonger, Bernard
> > Sent: Wednesday, January 11, 2017 6:27 PM
> > To: Lu, Wenzhuo; dev@dpdk.org
> > Cc: Wu, Jingjing; stable@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API
> > calling
> >
> > Hi Wenzhuo,
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> > > Sent: Wednesday, January 11, 2017 2:48 AM
> > > To: dev@dpdk.org
> > > Cc: Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > <wenzhuo.lu@intel.com>; stable@dpdk.org
> > > Subject: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API
> > > calling
> > >
> > > Some ixgbe private APIs are added to expose ixgbe specific functions.
> > > When they're used by testpmd, there's no check for if the NICs are ixgbe.
> > > Other NICs also have chance to  call these APIs.
> > > This patch add the check and the feedback print.
> >
> > I am not sure that testpmd is the right place to do this.
> > The rte_pmd_ixgbe_* functions are public API's which can be called by
> > other applications.
> > The checks should be in the rte_pmd_ixgbe_* API's
> To be safer, it's better to add a check in the APIs.

I have already sent a patch for ixgbe which Ferruh has reviewed and applied to dpdk-next-net
http://dpdk.org/dev/patchwork/patch/19151/

We should consider doing something similar for the i40e.

> But the APIs is so called private API, not really public.  Considering if we have
> the same function on different NICs, for example we have rte_pmd_ixgbe_a
> and rte_pmd_i40e_a.
> APP still need to call them one by one, like ret = rte_pmd_ixgbe_a; ret =
> rte_pmd_i40e_a;
> 
> then, why not add the check, like
> If (NIC is ixgbe)
> 	ret = rte_pmd_ixgbe_a;
> if (NIC is i40e)
> 	ret = rte_pmd_i40e_a;

There is already code like the above in testpmd in cases where there is a choice of rte_pmd_* functions to call.
 
> testpmd is an example to let the users to know how to use the APIs. They
> should follow the example.
> How about your opinion?

In the case where there is no choice of function to make. There is no need to check the NIC in testpmd (as it is now done in the rte_pmd_ixgbe_* API's).

Regards,

Bernard.