[PATCH] drivers: fix vmxnet3 return wrong error code in initializing

[PATCH] drivers: fix vmxnet3 return wrong error code in initializing

[root]

From: Kaijun Zeng <corezeng@gmail.com>

In vmxnet3_dev_rxtx_init(), a wrong error code may be thrown after it invokes
vmxnet3_post_rx_bufs() because it negates the error code before returning it.
It causes rte_eth_dev_start() to give a positive number to the invoker, but it
should be a negative number, as described in the comments.

Bugzilla ID: 1239

Signed-off-by: Kaijun Zeng <corezeng@gmail.com>
---
 drivers/net/vmxnet3/vmxnet3_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
index a875ffec07..73ec1e4727 100644
--- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
+++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
@@ -1315,7 +1315,7 @@ vmxnet3_dev_rxtx_init(struct rte_eth_dev *dev)
 				PMD_INIT_LOG(ERR,
 					     "ERROR: Posting Rxq: %d buffers ring: %d",
 					     i, j);
-				return -ret;
+				return ret;
 			}
 			/*
 			 * Updating device with the index:next2fill to fill the
-- 
2.30.2

Re: [PATCH] drivers: fix vmxnet3 return wrong error code in initializing

[Ferruh Yigit]

On 5/28/2023 3:37 PM, root wrote:
> From: Kaijun Zeng <corezeng@gmail.com>
> 
> In vmxnet3_dev_rxtx_init(), a wrong error code may be thrown after it invokes
> vmxnet3_post_rx_bufs() because it negates the error code before returning it.
> It causes rte_eth_dev_start() to give a positive number to the invoker, but it
> should be a negative number, as described in the comments.
> 
> Bugzilla ID: 1239
> 
> Signed-off-by: Kaijun Zeng <corezeng@gmail.com>
> ---
>  drivers/net/vmxnet3/vmxnet3_rxtx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> index a875ffec07..73ec1e4727 100644
> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> @@ -1315,7 +1315,7 @@ vmxnet3_dev_rxtx_init(struct rte_eth_dev *dev)
>  				PMD_INIT_LOG(ERR,
>  					     "ERROR: Posting Rxq: %d buffers ring: %d",
>  					     i, j);
> -				return -ret;
> +				return ret;


Hi Kaijun,

Thanks for the fix, it looks valid.

But 'ret' being 0 also seems problematic, mentioned code is as following:
```
ret = vmxnet3_post_rx_bufs(rxq, j);
if (ret <= 0) {
	PMD_INIT_LOG(ERR,
		"ERROR: Posting Rxq: %d buffers ring: %d", i, j);
	return ret;
}
```

'vmxnet3_dev_rxtx_init()' can return 0 and failure, but caller will take
it as success.

Perhaps better to send an explicit error:

```
if (ret <= 0) {
	...
	return -EXXX
}
```




btw, for the next version of the patch, please use 'net/vmxnet3: '
prefix for patch title, like:
"net/vmxnet3: fix return code in initializing"

Also please include following Fixes tag in the commit log:
```
Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver
implementation")
Cc: stable@dpdk.org
```

For more details you can check contribution guide:
https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-subject-line

[PATCH v2] net/vmxnet3: fix return code in initializing

[Kaijun Zeng]

Improve error handling

Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
Cc: stable@dpdk.org

Signed-off-by: Kaijun Zeng <corezeng@gmail.com>
---
 drivers/net/vmxnet3/vmxnet3_rxtx.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
index 73ec1e4727..e615d40d09 100644
--- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
+++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
@@ -1311,7 +1311,17 @@ vmxnet3_dev_rxtx_init(struct rte_eth_dev *dev)
 		for (j = 0; j < VMXNET3_RX_CMDRING_SIZE; j++) {
 			/* Passing 0 as alloc_num will allocate full ring */
 			ret = vmxnet3_post_rx_bufs(rxq, j);
-			if (ret <= 0) {
+
+			/* Zero number of descriptors in the configuration of the RX queue */
+			if (ret == 0) {
+				PMD_INIT_LOG(ERR,
+					"ERROR: Zero descriptor requirement in Rx queue: %d,"
+					"buffers ring: %d\n",
+					i, j);
+				return -EINVAL;
+			}
+			/* Return the errno */
+			if (ret < 0) {
 				PMD_INIT_LOG(ERR,
 					     "ERROR: Posting Rxq: %d buffers ring: %d",
 					     i, j);
-- 
2.30.2

Re: [PATCH v2] net/vmxnet3: fix return code in initializing

[Ferruh Yigit]

On 6/2/2023 5:44 PM, Kaijun Zeng wrote:
> Improve error handling
> 
> Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kaijun Zeng <corezeng@gmail.com>
> ---
>  drivers/net/vmxnet3/vmxnet3_rxtx.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> index 73ec1e4727..e615d40d09 100644
> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> @@ -1311,7 +1311,17 @@ vmxnet3_dev_rxtx_init(struct rte_eth_dev *dev)
>  		for (j = 0; j < VMXNET3_RX_CMDRING_SIZE; j++) {
>  			/* Passing 0 as alloc_num will allocate full ring */
>  			ret = vmxnet3_post_rx_bufs(rxq, j);
> -			if (ret <= 0) {
> +
> +			/* Zero number of descriptors in the configuration of the RX queue */
> +			if (ret == 0) {
> +				PMD_INIT_LOG(ERR,
> +					"ERROR: Zero descriptor requirement in Rx queue: %d,"
> +					"buffers ring: %d\n",
> +					i, j);
> +				return -EINVAL;
> +			}
> +			/* Return the errno */
> +			if (ret < 0) {
>  				PMD_INIT_LOG(ERR,
>  					     "ERROR: Posting Rxq: %d buffers ring: %d",
>  					     i, j);


Looks good to me, @Jochen any objection?

Re: [PATCH v2] net/vmxnet3: fix return code in initializing

[Stephen Hemminger]

On Fri,  2 Jun 2023 12:44:38 -0400
Kaijun Zeng <corezeng@gmail.com> wrote:

> +				PMD_INIT_LOG(ERR,
> +					"ERROR: Zero descriptor requirement in Rx queue: %d,"
> +					"buffers ring: %d\n",
> +					i, j);

Please don't split messages across source lines, it makes harder for developers
to use tools to scan source for message.

Often, when message is long, it means that there is redundant information or poor wording.
For example, in your message "ERROR:" is redundant.

Re: [PATCH v2] net/vmxnet3: fix return code in initializing

[Ferruh Yigit]

On 6/6/2023 4:36 PM, Stephen Hemminger wrote:
> On Fri,  2 Jun 2023 12:44:38 -0400
> Kaijun Zeng <corezeng@gmail.com> wrote:
> 
>> +				PMD_INIT_LOG(ERR,
>> +					"ERROR: Zero descriptor requirement in Rx queue: %d,"
>> +					"buffers ring: %d\n",
>> +					i, j);
> 
> Please don't split messages across source lines, it makes harder for developers
> to use tools to scan source for message.
> 
> Often, when message is long, it means that there is redundant information or poor wording.
> For example, in your message "ERROR:" is redundant.
>

Agree that 'ERROR:' is redundant, and +1 to not split log message.

Kaijun,

Would you mind sending a new version with above changes?

Thanks,
ferruh

[PATCH] net/vmxnet3: fix return code in initializing

[Kaijun Zeng]

Improve error handling and code style

Bugzilla ID: 1239

Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
Cc: stable@dpdk.org

Signed-off-by: Kaijun Zeng <corezeng@gmail.com>

---
 drivers/net/vmxnet3/vmxnet3_rxtx.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
index a875ffec07..e6e36dca93 100644
--- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
+++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
@@ -1311,11 +1311,18 @@ vmxnet3_dev_rxtx_init(struct rte_eth_dev *dev)
 		for (j = 0; j < VMXNET3_RX_CMDRING_SIZE; j++) {
 			/* Passing 0 as alloc_num will allocate full ring */
 			ret = vmxnet3_post_rx_bufs(rxq, j);
-			if (ret <= 0) {
+
+			/* Zero number of descriptors in the configuration of the RX queue */
+			if (ret == 0) {
 				PMD_INIT_LOG(ERR,
-					     "ERROR: Posting Rxq: %d buffers ring: %d",
-					     i, j);
-				return -ret;
+					"Invalid configuration in Rx queue: %d, buffers ring: %d\n",
+					i, j);
+				return -EINVAL;
+			}
+			/* Return the error number */
+			if (ret < 0) {
+				PMD_INIT_LOG(ERR, "Posting Rxq: %d buffers ring: %d", i, j);
+				return ret;
 			}
 			/*
 			 * Updating device with the index:next2fill to fill the
-- 
2.30.2

[PATCH v3] net/vmxnet3: fix return code in initializing

[Kaijun Zeng]

Improve error handling

Bugzilla ID: 1239
Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
Cc: stable@dpdk.org

Signed-off-by: Kaijun Zeng <corezeng@gmail.com>
---
v3:
* Improve coding style

v2:
* Improve error handling
---
 drivers/net/vmxnet3/vmxnet3_rxtx.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
index a875ffec07..e6e36dca93 100644
--- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
+++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
@@ -1311,11 +1311,18 @@ vmxnet3_dev_rxtx_init(struct rte_eth_dev *dev)
 		for (j = 0; j < VMXNET3_RX_CMDRING_SIZE; j++) {
 			/* Passing 0 as alloc_num will allocate full ring */
 			ret = vmxnet3_post_rx_bufs(rxq, j);
-			if (ret <= 0) {
+
+			/* Zero number of descriptors in the configuration of the RX queue */
+			if (ret == 0) {
 				PMD_INIT_LOG(ERR,
-					     "ERROR: Posting Rxq: %d buffers ring: %d",
-					     i, j);
-				return -ret;
+					"Invalid configuration in Rx queue: %d, buffers ring: %d\n",
+					i, j);
+				return -EINVAL;
+			}
+			/* Return the error number */
+			if (ret < 0) {
+				PMD_INIT_LOG(ERR, "Posting Rxq: %d buffers ring: %d", i, j);
+				return ret;
 			}
 			/*
 			 * Updating device with the index:next2fill to fill the
-- 
2.30.2

Re: [PATCH v3] net/vmxnet3: fix return code in initializing

[Ferruh Yigit]

On 6/7/2023 6:57 PM, Kaijun Zeng wrote:
> Improve error handling
> 
> Bugzilla ID: 1239
> Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kaijun Zeng <corezeng@gmail.com>
>

Reviewed-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.