[dpdk-dev] [PATCH 0/3] bugfix for kni

[dpdk-dev] [PATCH 0/3] bugfix for kni

[Min Hu (Connor)]

This patchset contains three bugfixes for kni.

Min Hu (Connor) (3):
  kni: fix wrong comments
  net/kni: fix rewritten return value
  kni: fix unchecked return value

 app/test/test_kni.c           | 8 ++++++--
 drivers/net/kni/rte_eth_kni.c | 5 +++++
 2 files changed, 11 insertions(+), 2 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/3] kni: fix wrong comments

[Min Hu (Connor)]

This patch changed 'subsytem' to 'subsystem'.

Fixes: 0c6bc8ef70ba ("kni: memzone pool for alloc and release")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 app/test/test_kni.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/test_kni.c b/app/test/test_kni.c
index f53a53e..3470005 100644
--- a/app/test/test_kni.c
+++ b/app/test/test_kni.c
@@ -562,7 +562,7 @@ test_kni(void)
 	}
 	closedir(dir);
 
-	/* Initialize KNI subsytem */
+	/* Initialize KNI subsystem */
 	rte_kni_init(KNI_TEST_MAX_PORTS);
 
 	if (test_kni_allocate_lcores() < 0) {
-- 
2.7.4

Re: [dpdk-dev] [PATCH 1/3] kni: fix wrong comments

[Ferruh Yigit]

On 4/22/2021 4:56 AM, Min Hu (Connor) wrote:
> This patch changed 'subsytem' to 'subsystem'.
> 
> Fixes: 0c6bc8ef70ba ("kni: memzone pool for alloc and release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

This is not on the "kni:", which implies the kni library, but the kni unit test,
will fix title while merging.

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

[dpdk-dev] [PATCH 2/3] net/kni: fix rewritten return value

[Min Hu (Connor)]

Return value of function 'eth_kni_dev_stop' passed to 'ret' is
rewritten later, and this is unreasonable.

This patch fixes it.

Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/kni/rte_eth_kni.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/kni/rte_eth_kni.c b/drivers/net/kni/rte_eth_kni.c
index 9ce74e5..067584c 100644
--- a/drivers/net/kni/rte_eth_kni.c
+++ b/drivers/net/kni/rte_eth_kni.c
@@ -211,6 +211,11 @@ eth_kni_close(struct rte_eth_dev *eth_dev)
 		return 0;
 
 	ret = eth_kni_dev_stop(eth_dev);
+	if (ret) {
+		PMD_LOG(WARNING, "Not able to stop kni for %s",
+			eth_dev->data->name);
+		return ret;
+	}
 
 	/* mac_addrs must not be freed alone because part of dev_private */
 	eth_dev->data->mac_addrs = NULL;
-- 
2.7.4

Re: [dpdk-dev] [PATCH 2/3] net/kni: fix rewritten return value

[Ferruh Yigit]

On 4/22/2021 4:56 AM, Min Hu (Connor) wrote:
> Return value of function 'eth_kni_dev_stop' passed to 'ret' is
> rewritten later, and this is unreasonable.
> 
> This patch fixes it.
> 
> Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  drivers/net/kni/rte_eth_kni.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/kni/rte_eth_kni.c b/drivers/net/kni/rte_eth_kni.c
> index 9ce74e5..067584c 100644
> --- a/drivers/net/kni/rte_eth_kni.c
> +++ b/drivers/net/kni/rte_eth_kni.c
> @@ -211,6 +211,11 @@ eth_kni_close(struct rte_eth_dev *eth_dev)
>  		return 0;
>  
>  	ret = eth_kni_dev_stop(eth_dev);
> +	if (ret) {
> +		PMD_LOG(WARNING, "Not able to stop kni for %s",
> +			eth_dev->data->name);
> +		return ret;
> +	}
>  

'eth_kni_close()' is called by 'eth_kni_remove()', and returning here without
setting 'eth_dev->data->mac_addrs' to NULL, 'eth_kni_remove()' may crash.

And the close ops is to not use the device anymore, there is not much to do if
stop() fails here. So what do you think to log but not return on the
'eth_kni_dev_stop()' failure?


>  	/* mac_addrs must not be freed alone because part of dev_private */
>  	eth_dev->data->mac_addrs = NULL;
> 

Re: [dpdk-dev] [PATCH 2/3] net/kni: fix rewritten return value

[Min Hu (Connor)]

在 2021/4/26 21:18, Ferruh Yigit 写道:
> On 4/22/2021 4:56 AM, Min Hu (Connor) wrote:
>> Return value of function 'eth_kni_dev_stop' passed to 'ret' is
>> rewritten later, and this is unreasonable.
>>
>> This patch fixes it.
>>
>> Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   drivers/net/kni/rte_eth_kni.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/kni/rte_eth_kni.c b/drivers/net/kni/rte_eth_kni.c
>> index 9ce74e5..067584c 100644
>> --- a/drivers/net/kni/rte_eth_kni.c
>> +++ b/drivers/net/kni/rte_eth_kni.c
>> @@ -211,6 +211,11 @@ eth_kni_close(struct rte_eth_dev *eth_dev)
>>   		return 0;
>>   
>>   	ret = eth_kni_dev_stop(eth_dev);
>> +	if (ret) {
>> +		PMD_LOG(WARNING, "Not able to stop kni for %s",
>> +			eth_dev->data->name);
>> +		return ret;
>> +	}
>>   
> 
> 'eth_kni_close()' is called by 'eth_kni_remove()', and returning here without
> setting 'eth_dev->data->mac_addrs' to NULL, 'eth_kni_remove()' may crash.
> 
> And the close ops is to not use the device anymore, there is not much to do if
> stop() fails here. So what do you think to log but not return on the
> 'eth_kni_dev_stop()' failure?
> 
Agreed, fixed in v2, thanks.
> 
>>   	/* mac_addrs must not be freed alone because part of dev_private */
>>   	eth_dev->data->mac_addrs = NULL;
>>
> 
> .
> 

[dpdk-dev] [PATCH v2] net/kni: fix rewritten return value

[Min Hu (Connor)]

Return value of function 'eth_kni_dev_stop' passed to 'ret' is
rewritten later, and this is unreasonable.

This patch fixes it.

Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v2:
* just log but not return.
---
 drivers/net/kni/rte_eth_kni.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/kni/rte_eth_kni.c b/drivers/net/kni/rte_eth_kni.c
index 9ce74e5..00f9e1b 100644
--- a/drivers/net/kni/rte_eth_kni.c
+++ b/drivers/net/kni/rte_eth_kni.c
@@ -211,6 +211,9 @@ eth_kni_close(struct rte_eth_dev *eth_dev)
 		return 0;
 
 	ret = eth_kni_dev_stop(eth_dev);
+	if (ret)
+		PMD_LOG(WARNING, "Not able to stop kni for %s",
+			eth_dev->data->name);
 
 	/* mac_addrs must not be freed alone because part of dev_private */
 	eth_dev->data->mac_addrs = NULL;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] net/kni: fix rewritten return value

[Ferruh Yigit]

On 4/27/2021 3:08 AM, Min Hu (Connor) wrote:
> Return value of function 'eth_kni_dev_stop' passed to 'ret' is
> rewritten later, and this is unreasonable.
> 
> This patch fixes it.
> 
> Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

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

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

[dpdk-dev] [PATCH 3/3] kni: fix unchecked return value

[Min Hu (Connor)]

Return value 'rte_kni_init' of a function is not checked. If
it fails, error handling (logging and return) should be done.

This patch fixed it.

Fixes: 0c6bc8ef70ba ("kni: memzone pool for alloc and release")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 app/test/test_kni.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/test/test_kni.c b/app/test/test_kni.c
index 3470005..9673355 100644
--- a/app/test/test_kni.c
+++ b/app/test/test_kni.c
@@ -563,7 +563,11 @@ test_kni(void)
 	closedir(dir);
 
 	/* Initialize KNI subsystem */
-	rte_kni_init(KNI_TEST_MAX_PORTS);
+	ret = rte_kni_init(KNI_TEST_MAX_PORTS);
+	if (ret < 0) {
+		printf("fail to initialize KNI subsystem\n");
+		return -1;
+	}
 
 	if (test_kni_allocate_lcores() < 0) {
 		printf("No enough lcores for kni processing\n");
-- 
2.7.4

Re: [dpdk-dev] [PATCH 3/3] kni: fix unchecked return value

[Ferruh Yigit]

On 4/22/2021 4:56 AM, Min Hu (Connor) wrote:
> Return value 'rte_kni_init' of a function is not checked. If
> it fails, error handling (logging and return) should be done.
> 
> This patch fixed it.
> 
> Fixes: 0c6bc8ef70ba ("kni: memzone pool for alloc and release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

With the patch title update,

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

Re: [dpdk-dev] [PATCH 0/3] bugfix for kni

[Ferruh Yigit]

On 4/22/2021 4:56 AM, Min Hu (Connor) wrote:
> This patchset contains three bugfixes for kni.
> 
> Min Hu (Connor) (3):
>   kni: fix wrong comments
>   net/kni: fix rewritten return value
>   kni: fix unchecked return value
> 

Except from 2/3,
Series applied to dpdk-next-net/main, thanks.