diff mbox series

[v1] app/testpmd: fix CRC strip config error

Message ID 22fa727a2ce09834133530c40cb02666e0be10ba.1570882485.git.ting.xu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v1] app/testpmd: fix CRC strip config error |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-compilation success Compile Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Xu, Ting Oct. 12, 2019, 12:18 p.m. UTC 
  This patch fixed the bug that an error appears when config rx_offload
crc_strip using command "port config all crc-strip on|off". The reason
is that this command was removed previously. However, the current command
does not enable "crc_strip" option properly, so that testpmd returns
error when config crc_strip.

In this patch, an additional operation is added to recognize "crc_strip"
option, since "crc_strip" and "keep_crc" are using the same flag
"DEV_RX_OFFLOAD_KEEP_CRC". The current command is "port config
<port_id> rx_offload crc_strip on|off".

Fixes: e5db17a1e54e ("app/testpmd: remove duplicated Rx offload commands")
Cc: stable@dpdk.org

Signed-off-by: Ting Xu <ting.xu@intel.com>
---
 app/test-pmd/cmdline.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Iremonger, Bernard Oct. 14, 2019, 11:25 a.m. UTC | #1 
Hi Ting,

> -----Original Message-----
> From: Xu, Ting
> Sent: Saturday, October 12, 2019 1:19 PM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; stable@dpdk.org
> Subject: [PATCH v1] app/testpmd: fix CRC strip config error
> 
> This patch fixed the bug that an error appears when config rx_offload
> crc_strip using command "port config all crc-strip on|off". The reason is that
> this command was removed previously. However, the current command
> does not enable "crc_strip" option properly, so that testpmd returns error
> when config crc_strip.
> 
> In this patch, an additional operation is added to recognize "crc_strip"
> option, since "crc_strip" and "keep_crc" are using the same flag
> "DEV_RX_OFFLOAD_KEEP_CRC". The current command is "port config
> <port_id> rx_offload crc_strip on|off".
> 
> Fixes: e5db17a1e54e ("app/testpmd: remove duplicated Rx offload
> commands")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ting Xu <ting.xu@intel.com>
> ---
>  app/test-pmd/cmdline.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> def471d97..31dbe4a07 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -18084,6 +18084,9 @@ search_rx_offload(const char *name)
>  	int found = 0;
>  	unsigned int bit;
> 
> +	if (!strcmp(name, "crc_strip"))
> +		name = "keep_crc";
> +
>  	single_offload = 1;
>  	for (bit = 0; bit < sizeof(single_offload) * CHAR_BIT; bit++) {
>  		single_name =
> rte_eth_dev_rx_offload_name(single_offload);
> @@ -18113,6 +18116,7 @@ cmd_config_per_port_rx_offload_parsed(void
> *parsed_result,
>  	uint16_t nb_rx_queues;
>  	int q;
>  	int ret;
> +	int res_on_off = 1;
> 
>  	if (port->port_status != RTE_PORT_STOPPED) {
>  		printf("Error: Can't config offload when Port %d "
> @@ -18131,7 +18135,11 @@ cmd_config_per_port_rx_offload_parsed(void
> *parsed_result,
>  		return;
> 
>  	nb_rx_queues = dev_info.nb_rx_queues;
> -	if (!strcmp(res->on_off, "on")) {
> +	res_on_off = strcmp(res->on_off, "on");
> +
> +	if (!strcmp(res->offload, "crc_strip"))
> +		res_on_off = ~res_on_off;

It looks as if res_on_off is intended to have a value of 0 or 1.
The above line gives it a value of -1, is this intended ? 

> +	if (!res_on_off) {
>  		port->dev_conf.rxmode.offloads |= single_offload;
>  		for (q = 0; q < nb_rx_queues; q++)
>  			port->rx_conf[q].offloads |= single_offload;
> --
> 2.17.1

Regards,

Bernard.
Iremonger, Bernard Oct. 14, 2019, 11:41 a.m. UTC | #2 
Hi Ting,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Iremonger, Bernard
> Sent: Monday, October 14, 2019 12:26 PM
> To: Xu, Ting <ting.xu@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1] app/testpmd: fix CRC strip config error
> 
> Hi Ting,
> 
> > -----Original Message-----
> > From: Xu, Ting
> > Sent: Saturday, October 12, 2019 1:19 PM
> > To: dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Iremonger, Bernard
> > <bernard.iremonger@intel.com>; stable@dpdk.org
> > Subject: [PATCH v1] app/testpmd: fix CRC strip config error
> >
> > This patch fixed the bug that an error appears when config rx_offload
> > crc_strip using command "port config all crc-strip on|off". The reason
> > is that this command was removed previously. However, the current
> > command does not enable "crc_strip" option properly, so that testpmd
> > returns error when config crc_strip.
> >
> > In this patch, an additional operation is added to recognize "crc_strip"
> > option, since "crc_strip" and "keep_crc" are using the same flag
> > "DEV_RX_OFFLOAD_KEEP_CRC". The current command is "port config
> > <port_id> rx_offload crc_strip on|off".
> >
> > Fixes: e5db17a1e54e ("app/testpmd: remove duplicated Rx offload
> > commands")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ting Xu <ting.xu@intel.com>
> > ---
> >  app/test-pmd/cmdline.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > def471d97..31dbe4a07 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -18084,6 +18084,9 @@ search_rx_offload(const char *name)
> >  	int found = 0;
> >  	unsigned int bit;
> >
> > +	if (!strcmp(name, "crc_strip"))
> > +		name = "keep_crc";
> > +


I don't understand why the name is being changed from "crc_strip" to "keep_crc" here?

> >  	single_offload = 1;
> >  	for (bit = 0; bit < sizeof(single_offload) * CHAR_BIT; bit++) {
> >  		single_name =
> > rte_eth_dev_rx_offload_name(single_offload);
> > @@ -18113,6 +18116,7 @@ cmd_config_per_port_rx_offload_parsed(void
> > *parsed_result,
> >  	uint16_t nb_rx_queues;
> >  	int q;
> >  	int ret;
> > +	int res_on_off = 1;
> >
> >  	if (port->port_status != RTE_PORT_STOPPED) {
> >  		printf("Error: Can't config offload when Port %d "
> > @@ -18131,7 +18135,11 @@
> cmd_config_per_port_rx_offload_parsed(void
> > *parsed_result,
> >  		return;
> >
> >  	nb_rx_queues = dev_info.nb_rx_queues;
> > -	if (!strcmp(res->on_off, "on")) {
> > +	res_on_off = strcmp(res->on_off, "on");
> > +
> > +	if (!strcmp(res->offload, "crc_strip"))
> > +		res_on_off = ~res_on_off;
> 
> It looks as if res_on_off is intended to have a value of 0 or 1.
> The above line gives it a value of -1, is this intended ?
> 
> > +	if (!res_on_off) {
> >  		port->dev_conf.rxmode.offloads |= single_offload;
> >  		for (q = 0; q < nb_rx_queues; q++)
> >  			port->rx_conf[q].offloads |= single_offload;
> > --
> > 2.17.1
> 
> Regards,
> 
> Bernard.

Regards,

Bernard.
Ferruh Yigit Oct. 14, 2019, 12:28 p.m. UTC | #3 
On 10/12/2019 1:18 PM, Ting Xu wrote:
> This patch fixed the bug that an error appears when config rx_offload
> crc_strip using command "port config all crc-strip on|off". The reason
> is that this command was removed previously. However, the current command
> does not enable "crc_strip" option properly, so that testpmd returns
> error when config crc_strip.

"port config all crc-strip ..." is already removed,
Commit e5db17a1e54e ("app/testpmd: remove duplicated Rx offload commands")

And 'crc_strip' seems left over for the "port config <id> rx_offload ..."
command, instead of fixing it should be removed, and 'keep_crc' should be used,
"port config <id> rx_offload keep_crc on|off"

Did you observe any problem on 'keep_crc' ?

> 
> In this patch, an additional operation is added to recognize "crc_strip"
> option, since "crc_strip" and "keep_crc" are using the same flag
> "DEV_RX_OFFLOAD_KEEP_CRC". The current command is "port config
> <port_id> rx_offload crc_strip on|off".
> 
> Fixes: e5db17a1e54e ("app/testpmd: remove duplicated Rx offload commands")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ting Xu <ting.xu@intel.com>

<...>
Xu, Ting Oct. 15, 2019, 12:49 a.m. UTC | #4 
Hi, Ferruh,

From the log in Commit e5db17a1e54e, I thought crc_strip was still available in command "port config <id> rx_offload ...", so I did such modification.
Since it was removed, I will rework on it.
There is no problem on 'keep_crc'.

Thanks!

-----Original Message-----
From: Yigit, Ferruh 
Sent: Monday, October 14, 2019 8:29 PM
To: Xu, Ting <ting.xu@intel.com>; dev@dpdk.org
Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>; stable@dpdk.org
Subject: Re: [dpdk-stable] [PATCH v1] app/testpmd: fix CRC strip config error

On 10/12/2019 1:18 PM, Ting Xu wrote:
> This patch fixed the bug that an error appears when config rx_offload 
> crc_strip using command "port config all crc-strip on|off". The reason 
> is that this command was removed previously. However, the current 
> command does not enable "crc_strip" option properly, so that testpmd 
> returns error when config crc_strip.

"port config all crc-strip ..." is already removed, Commit e5db17a1e54e ("app/testpmd: remove duplicated Rx offload commands")

And 'crc_strip' seems left over for the "port config <id> rx_offload ..."
command, instead of fixing it should be removed, and 'keep_crc' should be used, "port config <id> rx_offload keep_crc on|off"

Did you observe any problem on 'keep_crc' ?

> 
> In this patch, an additional operation is added to recognize "crc_strip"
> option, since "crc_strip" and "keep_crc" are using the same flag 
> "DEV_RX_OFFLOAD_KEEP_CRC". The current command is "port config 
> <port_id> rx_offload crc_strip on|off".
> 
> Fixes: e5db17a1e54e ("app/testpmd: remove duplicated Rx offload 
> commands")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ting Xu <ting.xu@intel.com>

<...>
Xu, Ting Oct. 15, 2019, 1:03 a.m. UTC | #5 
Hi, Bernard,

In the previous version, the command 'crc_strip' and 'keep_crc' used the same macro  DEV_RX_OFFLOAD_KEEP_CRC. After Commit e5db17a1e54e, "port config all crc-strip on|off" is removed and the new command does not enable 'crc_strip'. So I wanted to change the name to 'keep_crc' to use DEV_RX_OFFLOAD_KEEP_CRC for 'crc_strip'.

However, since Ferruh said that the command 'crc_strip' is removed, I will rework the code totally. Thanks!

-----Original Message-----
From: Iremonger, Bernard 
Sent: Monday, October 14, 2019 7:42 PM
To: Iremonger, Bernard <bernard.iremonger@intel.com>; Xu, Ting <ting.xu@intel.com>; dev@dpdk.org
Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
Subject: RE: [PATCH v1] app/testpmd: fix CRC strip config error

Hi Ting,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Iremonger, 
> Bernard
> Sent: Monday, October 14, 2019 12:26 PM
> To: Xu, Ting <ting.xu@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing 
> <jingjing.wu@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1] app/testpmd: fix CRC strip config 
> error
> 
> Hi Ting,
> 
> > -----Original Message-----
> > From: Xu, Ting
> > Sent: Saturday, October 12, 2019 1:19 PM
> > To: dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing 
> > <jingjing.wu@intel.com>; Iremonger, Bernard 
> > <bernard.iremonger@intel.com>; stable@dpdk.org
> > Subject: [PATCH v1] app/testpmd: fix CRC strip config error
> >
> > This patch fixed the bug that an error appears when config 
> > rx_offload crc_strip using command "port config all crc-strip 
> > on|off". The reason is that this command was removed previously. 
> > However, the current command does not enable "crc_strip" option 
> > properly, so that testpmd returns error when config crc_strip.
> >
> > In this patch, an additional operation is added to recognize "crc_strip"
> > option, since "crc_strip" and "keep_crc" are using the same flag 
> > "DEV_RX_OFFLOAD_KEEP_CRC". The current command is "port config 
> > <port_id> rx_offload crc_strip on|off".
> >
> > Fixes: e5db17a1e54e ("app/testpmd: remove duplicated Rx offload
> > commands")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ting Xu <ting.xu@intel.com>
> > ---
> >  app/test-pmd/cmdline.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > def471d97..31dbe4a07 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -18084,6 +18084,9 @@ search_rx_offload(const char *name)
> >  	int found = 0;
> >  	unsigned int bit;
> >
> > +	if (!strcmp(name, "crc_strip"))
> > +		name = "keep_crc";
> > +


I don't understand why the name is being changed from "crc_strip" to "keep_crc" here?

> >  	single_offload = 1;
> >  	for (bit = 0; bit < sizeof(single_offload) * CHAR_BIT; bit++) {
> >  		single_name =
> > rte_eth_dev_rx_offload_name(single_offload);
> > @@ -18113,6 +18116,7 @@ cmd_config_per_port_rx_offload_parsed(void
> > *parsed_result,
> >  	uint16_t nb_rx_queues;
> >  	int q;
> >  	int ret;
> > +	int res_on_off = 1;
> >
> >  	if (port->port_status != RTE_PORT_STOPPED) {
> >  		printf("Error: Can't config offload when Port %d "
> > @@ -18131,7 +18135,11 @@
> cmd_config_per_port_rx_offload_parsed(void
> > *parsed_result,
> >  		return;
> >
> >  	nb_rx_queues = dev_info.nb_rx_queues;
> > -	if (!strcmp(res->on_off, "on")) {
> > +	res_on_off = strcmp(res->on_off, "on");
> > +
> > +	if (!strcmp(res->offload, "crc_strip"))
> > +		res_on_off = ~res_on_off;
> 
> It looks as if res_on_off is intended to have a value of 0 or 1.
> The above line gives it a value of -1, is this intended ?
> 
> > +	if (!res_on_off) {
> >  		port->dev_conf.rxmode.offloads |= single_offload;
> >  		for (q = 0; q < nb_rx_queues; q++)
> >  			port->rx_conf[q].offloads |= single_offload;
> > --
> > 2.17.1
> 
> Regards,
> 
> Bernard.

Regards,

Bernard.
diff mbox series

Patch

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index def471d97..31dbe4a07 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -18084,6 +18084,9 @@  search_rx_offload(const char *name)
 	int found = 0;
 	unsigned int bit;
 
+	if (!strcmp(name, "crc_strip"))
+		name = "keep_crc";
+
 	single_offload = 1;
 	for (bit = 0; bit < sizeof(single_offload) * CHAR_BIT; bit++) {
 		single_name = rte_eth_dev_rx_offload_name(single_offload);
@@ -18113,6 +18116,7 @@  cmd_config_per_port_rx_offload_parsed(void *parsed_result,
 	uint16_t nb_rx_queues;
 	int q;
 	int ret;
+	int res_on_off = 1;
 
 	if (port->port_status != RTE_PORT_STOPPED) {
 		printf("Error: Can't config offload when Port %d "
@@ -18131,7 +18135,11 @@  cmd_config_per_port_rx_offload_parsed(void *parsed_result,
 		return;
 
 	nb_rx_queues = dev_info.nb_rx_queues;
-	if (!strcmp(res->on_off, "on")) {
+	res_on_off = strcmp(res->on_off, "on");
+
+	if (!strcmp(res->offload, "crc_strip"))
+		res_on_off = ~res_on_off;
+	if (!res_on_off) {
 		port->dev_conf.rxmode.offloads |= single_offload;
 		for (q = 0; q < nb_rx_queues; q++)
 			port->rx_conf[q].offloads |= single_offload;