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
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
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
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
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; >
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>
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>
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.
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
在 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; >> > > . >
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.