Re: [PATCH v2] examples/ethtool: update rxmode to increase functionality

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 11 Aug 2022 17:58:40 +0500
Muhammad Jawad Hussain <jawad.hussain@emumba.com> wrote:

> @@ -142,7 +183,6 @@ pcmd_quit_callback(__rte_unused void *ptr_params,
>  	cmdline_quit(ctx);
>  }
>  
> -
>  static void
>  pcmd_drvinfo_callback(__rte_unused void *ptr_params,
>  	__rte_unused struct cmdline *ctx,

Please don't do unrelated whitespace changes.

>  
> -
>  static void
> -pcmd_rxmode_callback(void *ptr_params,
> -	__rte_unused struct cmdline *ctx,
> -	__rte_unused void *ptr_data)
> +pcmd_rxmode_callback(void *parsed_result,
> +		       __rte_unused struct cmdline *cl,
> +		       __rte_unused void *data)
>  {
> -	struct pcmd_intstr_params *params = ptr_params;
> -	int stat;
> +	int ret = -ENOTSUP;
> +	uint16_t vf_rxmode = 0;
> +	struct cmd_set_vf_rxmode *res = parsed_result;
> +
> +	int is_on = (strcmp(res->on, "on") == 0) ? 1 : 0;

Simpler as:
	bool is_on = !strcmp(res->on, "on");

Also what if use gives bogus value for on/off?

> +	if (!strcmp(res->what, "rxmode")) {
> +		if (!strcmp(res->mode, "AUPE"))
> +			vf_rxmode |= RTE_ETH_VMDQ_ACCEPT_UNTAG;
> +		else if (!strcmp(res->mode, "ROPE"))
> +			vf_rxmode |= RTE_ETH_VMDQ_ACCEPT_HASH_UC;
> +		else if (!strcmp(res->mode, "BAM"))
> +			vf_rxmode |= RTE_ETH_VMDQ_ACCEPT_BROADCAST;
> +		else if (!strncmp(res->mode, "MPE", 3))
> +			vf_rxmode |= RTE_ETH_VMDQ_ACCEPT_MULTICAST;
> +	}

You need to handle the "none of the above" case.

>  
> -	if (!rte_eth_dev_is_valid_port(params->port)) {
> -		printf("Error: Invalid port number %i\n", params->port);
> -		return;
> +	RTE_SET_USED(is_on);
> +	RTE_SET_USED(vf_rxmode);
> +
> +#ifdef RTE_NET_IXGBE
> +	if (ret == -ENOTSUP) {
> +		ret = rte_pmd_ixgbe_set_vf_rxmode(res->port_id, res->vf_id,
> +						  vf_rxmode, (uint8_t)is_on);
> +		if (ret == -ENOTSUP)
> +			printf("ixgbe not supported\n");
>  	}
> -	stat = rte_ethtool_net_set_rx_mode(params->port);
> -	if (stat == 0)
> -		return;
> -	else if (stat == -ENOTSUP)
> -		printf("Port %i: Operation not supported\n", params->port);
> -	else
> -		printf("Port %i: Error setting rx mode\n", params->port);
> +#endif
> +#ifdef RTE_NET_BNXT
> +	if (ret == -ENOTSUP) {
> +		ret = rte_pmd_bnxt_set_vf_rxmode(res->port_id, res->vf_id,
> +						 vf_rxmode, (uint8_t)is_on);
> +		if (ret == -ENOTSUP)
> +			printf("bnxt not supported\n");
> +	}
> +#endif


This is a mess. And it shows missing ethdev functionality.
Calling a driver with some unrelated device may break it.

Not sure if anyone really cares about ethtool?
But if they do please fixup and resubmit