Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add commands to test new Rx offload API

["Wu, Jingjing" <jingjing.wu@intel.com>]

<...>

> +static int
> +config_rx_offload(const char *name, uint64_t *offload, int on)
> +{
> +	uint64_t local = *offload;
> +
> +	if (!strcmp(name, "vlan_strip")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_VLAN_STRIP;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_VLAN_STRIP;
Would it decrease the lines if move the "on" checking to the end of this function, just set the
BIT which you want to set or mask here?
<...>

> +static void
> +cmd_config_per_port_rx_offload_parsed(void *parsed_result,
> +				__attribute__((unused)) struct cmdline *cl,
> +				__attribute__((unused)) void *data)
> +{
> +	struct cmd_config_per_port_rx_offload_result *res = parsed_result;
> +	portid_t port_id = res->port_id;
> +	struct rte_port *port = &ports[port_id];
> +	int on;
> +
> +	if (port->port_status != RTE_PORT_STOPPED) {
> +		printf("Error: Can't config offload when Port %d "
> +		       "is not stopped\n", port_id);
> +		return;
> +	}
> +
> +	if (!strcmp(res->en_dis, "enable"))
> +		on = 1;
> +	else if (!strcmp(res->en_dis, "disable"))
> +		on = 0;
> +	else {
> +		printf("Unknown parameter: %s\n", res->en_dis);
> +		return;
The en_dis is already defined as "enable#disable", so the else is useless here.

<...>

> +static void
> +cmd_config_per_queue_rx_offload_parsed(void *parsed_result,
> +				__attribute__((unused)) struct cmdline *cl,
> +				__attribute__((unused)) void *data)
> +{
> +	struct cmd_config_per_queue_rx_offload_result *res = parsed_result;
> +	struct rte_eth_dev_info dev_info;
> +	portid_t port_id = res->port_id;
> +	uint16_t queue_id = res->queue_id;
> +	struct rte_port *port = &ports[port_id];
> +	int on;
> +
> +	if (port->port_status != RTE_PORT_STOPPED) {
> +		printf("Error: Can't config offload when Port %d "
> +		       "is not stopped\n", port_id);
> +		return;
> +	}
> +
> +	if (!strcmp(res->en_dis, "enable"))
> +		on = 1;
> +	else if (!strcmp(res->en_dis, "disable"))
> +		on = 0;
> +	else {
> +		printf("Unknown parameter: %s\n", res->en_dis);
> +		return;
Same comments as above.
<...>

> @@ -707,6 +708,17 @@ init_config(void)
>  			}
>  		}
> 
> +		port->rx_offloads = rte_zmalloc("testpmd: port->rx_offloads",
> +						sizeof(port->rx_offloads[0]) *
> +						port->dev_info.max_rx_queues,
> +						sizeof(port->rx_offloads[0]));

When will you free it?

> +		if (port->rx_offloads == NULL)
> +			rte_exit(EXIT_FAILURE,
> +				 "rte_zmalloc(%d port->rx_offload[0]) "
> +				 "failed\n", port->dev_info.max_rx_queues);
> +		for (k = 0; k < port->dev_info.max_rx_queues; k++)
> +			port->rx_offloads[k] = port->dev_conf.rxmode.offloads;
> +
In the get configure command, the port->rx_offloads is printed.
Here you init it to be port configuration, when will you update it?