[v1] net/ice: fix memzone leak when device init failed
Checks
Commit Message
When flow engine initialization or FXP resource reset failed, it needs
to free the memory zone and unregister the interrupt callback.
Bugzilla ID: 752
Fixes: 84dc7a95a2d3 ("net/ice: enable flow director engine")
Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling")
Fixes: 7edc7158d771 ("net/ice: cleanup RSS/FDIR profile on device init")
Cc: stable@dpdk.org
Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
---
drivers/net/ice/ice_ethdev.c | 10 ++++++++--
drivers/net/ice/ice_fdir_filter.c | 2 ++
2 files changed, 10 insertions(+), 2 deletions(-)
Comments
On Fri, Aug 13, 2021 at 8:45 AM Haiyue Wang <haiyue.wang@intel.com> wrote:
>
> When flow engine initialization or FXP resource reset failed, it needs
> to free the memory zone and unregister the interrupt callback.
>
> Bugzilla ID: 752
> Fixes: 84dc7a95a2d3 ("net/ice: enable flow director engine")
> Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling")
> Fixes: 7edc7158d771 ("net/ice: cleanup RSS/FDIR profile on device init")
> Cc: stable@dpdk.org
>
> Reported-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> ---
> drivers/net/ice/ice_ethdev.c | 10 ++++++++--
> drivers/net/ice/ice_fdir_filter.c | 2 ++
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 64ee569525..8d62b84805 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -2139,20 +2139,26 @@ ice_dev_init(struct rte_eth_dev *dev)
> ret = ice_flow_init(ad);
> if (ret) {
> PMD_INIT_LOG(ERR, "Failed to initialize flow");
> - return ret;
> + goto err_flow_init;
Is it safe to call flow engine uninit callbacks when ice_flow_init() fails?
> }
> }
>
> ret = ice_reset_fxp_resource(hw);
> if (ret) {
> PMD_INIT_LOG(ERR, "Failed to reset fxp resource");
> - return ret;
> + goto err_flow_init;
> }
>
> pf->supported_rxdid = ice_get_supported_rxdid(hw);
>
> return 0;
>
> +err_flow_init:
> + ice_flow_uninit(ad);
> + rte_intr_disable(intr_handle);
> + ice_pf_disable_irq0(hw);
> + rte_intr_callback_unregister(intr_handle,
> + ice_interrupt_handler, dev);
> err_pf_setup:
> ice_res_pool_destroy(&pf->msix_pool);
> err_msix_pool_init:
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, August 17, 2021 17:19
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev <dev@dpdk.org>; Zhang, Qi Z <qi.z.zhang@intel.com>; dpdk stable <stable@dpdk.org>; Yang,
> Qiming <qiming.yang@intel.com>; Xiaolong Ye <xiaolong.ye@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Wang, Ying A <ying.a.wang@intel.com>
> Subject: Re: [PATCH v1] net/ice: fix memzone leak when device init failed
>
> On Fri, Aug 13, 2021 at 8:45 AM Haiyue Wang <haiyue.wang@intel.com> wrote:
> >
> > When flow engine initialization or FXP resource reset failed, it needs
> > to free the memory zone and unregister the interrupt callback.
> >
> > Bugzilla ID: 752
> > Fixes: 84dc7a95a2d3 ("net/ice: enable flow director engine")
> > Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling")
> > Fixes: 7edc7158d771 ("net/ice: cleanup RSS/FDIR profile on device init")
> > Cc: stable@dpdk.org
> >
> > Reported-by: David Marchand <david.marchand@redhat.com>
> > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > ---
> > drivers/net/ice/ice_ethdev.c | 10 ++++++++--
> > drivers/net/ice/ice_fdir_filter.c | 2 ++
> > 2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> > index 64ee569525..8d62b84805 100644
> > --- a/drivers/net/ice/ice_ethdev.c
> > +++ b/drivers/net/ice/ice_ethdev.c
> > @@ -2139,20 +2139,26 @@ ice_dev_init(struct rte_eth_dev *dev)
> > ret = ice_flow_init(ad);
> > if (ret) {
> > PMD_INIT_LOG(ERR, "Failed to initialize flow");
> > - return ret;
> > + goto err_flow_init;
>
> Is it safe to call flow engine uninit callbacks when ice_flow_init() fails?
If each engine->init/uninit handles its internal setting correctly, yes,
it's safe, if not, this single engine has BUG, let's fix it. ;-)
int
ice_flow_init(struct ice_adapter *ad)
{
TAILQ_FOREACH_SAFE(engine, &engine_list, node, temp) {
if (engine->init == NULL) {
PMD_INIT_LOG(ERR, "Invalid engine type (%d)",
engine->type);
return -ENOTSUP;
}
ret = engine->init(ad);
if (ret) {
PMD_INIT_LOG(ERR, "Failed to initialize engine %d",
engine->type);
return ret;
}
}
}
void
ice_flow_uninit(struct ice_adapter *ad)
{
struct ice_pf *pf = &ad->pf;
struct ice_flow_engine *engine;
struct rte_flow *p_flow;
struct ice_flow_parser_node *p_parser;
void *temp;
TAILQ_FOREACH_SAFE(engine, &engine_list, node, temp) {
if (engine->uninit)
engine->uninit(ad);
}
}
>
>
> > }
> > }
> >
> > ret = ice_reset_fxp_resource(hw);
> > if (ret) {
> > PMD_INIT_LOG(ERR, "Failed to reset fxp resource");
> > - return ret;
> > + goto err_flow_init;
> > }
> >
> > pf->supported_rxdid = ice_get_supported_rxdid(hw);
> >
> > return 0;
> >
> > +err_flow_init:
> > + ice_flow_uninit(ad);
> > + rte_intr_disable(intr_handle);
> > + ice_pf_disable_irq0(hw);
> > + rte_intr_callback_unregister(intr_handle,
> > + ice_interrupt_handler, dev);
> > err_pf_setup:
> > ice_res_pool_destroy(&pf->msix_pool);
> > err_msix_pool_init:
>
>
> --
> David Marchand
On Wed, Aug 18, 2021 at 2:46 AM Wang, Haiyue <haiyue.wang@intel.com> wrote:
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Tuesday, August 17, 2021 17:19
> > To: Wang, Haiyue <haiyue.wang@intel.com>
> > Cc: dev <dev@dpdk.org>; Zhang, Qi Z <qi.z.zhang@intel.com>; dpdk stable <stable@dpdk.org>; Yang,
> > Qiming <qiming.yang@intel.com>; Xiaolong Ye <xiaolong.ye@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Wang, Ying A <ying.a.wang@intel.com>
> > Subject: Re: [PATCH v1] net/ice: fix memzone leak when device init failed
> >
> > On Fri, Aug 13, 2021 at 8:45 AM Haiyue Wang <haiyue.wang@intel.com> wrote:
> > >
> > > When flow engine initialization or FXP resource reset failed, it needs
> > > to free the memory zone and unregister the interrupt callback.
> > >
> > > Bugzilla ID: 752
> > > Fixes: 84dc7a95a2d3 ("net/ice: enable flow director engine")
> > > Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling")
> > > Fixes: 7edc7158d771 ("net/ice: cleanup RSS/FDIR profile on device init")
> > > Cc: stable@dpdk.org
> > >
> > > Reported-by: David Marchand <david.marchand@redhat.com>
> > > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > > ---
> > > drivers/net/ice/ice_ethdev.c | 10 ++++++++--
> > > drivers/net/ice/ice_fdir_filter.c | 2 ++
> > > 2 files changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> > > index 64ee569525..8d62b84805 100644
> > > --- a/drivers/net/ice/ice_ethdev.c
> > > +++ b/drivers/net/ice/ice_ethdev.c
> > > @@ -2139,20 +2139,26 @@ ice_dev_init(struct rte_eth_dev *dev)
> > > ret = ice_flow_init(ad);
> > > if (ret) {
> > > PMD_INIT_LOG(ERR, "Failed to initialize flow");
> > > - return ret;
> > > + goto err_flow_init;
> >
> > Is it safe to call flow engine uninit callbacks when ice_flow_init() fails?
>
> If each engine->init/uninit handles its internal setting correctly, yes,
> it's safe, if not, this single engine has BUG, let's fix it. ;-)
That was my understanding, but I preferred to ask.
Then, patch lgtm, thanks Haiyue.
> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Sent: Friday, August 13, 2021 2:22 PM
> To: dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>; stable@dpdk.org; David Marchand
> <david.marchand@redhat.com>; Yang, Qiming <qiming.yang@intel.com>;
> Xiaolong Ye <xiaolong.ye@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> Wang, Ying A <ying.a.wang@intel.com>
> Subject: [PATCH v1] net/ice: fix memzone leak when device init failed
>
> When flow engine initialization or FXP resource reset failed, it needs to free the
> memory zone and unregister the interrupt callback.
>
> Bugzilla ID: 752
> Fixes: 84dc7a95a2d3 ("net/ice: enable flow director engine")
> Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling")
> Fixes: 7edc7158d771 ("net/ice: cleanup RSS/FDIR profile on device init")
> Cc: stable@dpdk.org
>
> Reported-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
Acked-by: Qi Zhang <qi.z.zhang@intel.com>
Applied dpdk-next-net-intel.
Thanks
Qi
@@ -2139,20 +2139,26 @@ ice_dev_init(struct rte_eth_dev *dev)
ret = ice_flow_init(ad);
if (ret) {
PMD_INIT_LOG(ERR, "Failed to initialize flow");
- return ret;
+ goto err_flow_init;
}
}
ret = ice_reset_fxp_resource(hw);
if (ret) {
PMD_INIT_LOG(ERR, "Failed to reset fxp resource");
- return ret;
+ goto err_flow_init;
}
pf->supported_rxdid = ice_get_supported_rxdid(hw);
return 0;
+err_flow_init:
+ ice_flow_uninit(ad);
+ rte_intr_disable(intr_handle);
+ ice_pf_disable_irq0(hw);
+ rte_intr_callback_unregister(intr_handle,
+ ice_interrupt_handler, dev);
err_pf_setup:
ice_res_pool_destroy(&pf->msix_pool);
err_msix_pool_init:
@@ -651,8 +651,10 @@ ice_fdir_teardown(struct ice_pf *pf)
ice_tx_queue_release(pf->fdir.txq);
pf->fdir.txq = NULL;
+ rte_eth_dma_zone_free(eth_dev, "fdir_tx_ring", ICE_FDIR_QUEUE_ID);
ice_rx_queue_release(pf->fdir.rxq);
pf->fdir.rxq = NULL;
+ rte_eth_dma_zone_free(eth_dev, "fdir_rx_ring", ICE_FDIR_QUEUE_ID);
ice_fdir_prof_rm_all(pf);
ice_fdir_prof_free(hw);
ice_release_vsi(vsi);