[v3,09/10] net/bnxt: release port upon close
Checks
Commit Message
From: Somnath Kotur <somnath.kotur@broadcom.com>
Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private resources
for the port can be freed by rte_eth_dev_close().
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
drivers/net/bnxt/bnxt_ethdev.c | 5 +++++
1 file changed, 5 insertions(+)
Comments
On 1/14/2020 5:14 AM, Ajit Khaparde wrote:
> From: Somnath Kotur <somnath.kotur@broadcom.com>
>
> Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private resources
> for the port can be freed by rte_eth_dev_close().
>
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Reviewed-by: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
> drivers/net/bnxt/bnxt_ethdev.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index ba3f0a7d9..8acfade5f 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -4856,6 +4856,11 @@ bnxt_dev_init(struct rte_eth_dev *eth_dev)
> if (rc)
> goto error_free;
>
> + /* Pass the information to the rte_eth_dev_close() that it should also
> + * release the private port resources.
> + */
> + eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
> +
When you give this flag, 'rte_eth_dev_close()' will release port resources, it
will memset the 'eth_dev->data'.
In case user first closed the port, later removed it, remove path should be safe
for it. I can see 'bnxt_dev_uninit()' operates on the values of 'eth_dev->data'
which should crash in this case.
Can you please double check the return path, all following should be safe:
- close the port
- remove the port
- close and remove the port
Ferruh,
Will recheck and revert on
Thanks
Som
On Tue, 14 Jan 2020, 18:26 Ferruh Yigit, <ferruh.yigit@intel.com> wrote:
> On 1/14/2020 5:14 AM, Ajit Khaparde wrote:
> > From: Somnath Kotur <somnath.kotur@broadcom.com>
> >
> > Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private resources
> > for the port can be freed by rte_eth_dev_close().
> >
> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > Reviewed-by: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@broadcom.com
> >
> > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > ---
> > drivers/net/bnxt/bnxt_ethdev.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/net/bnxt/bnxt_ethdev.c
> b/drivers/net/bnxt/bnxt_ethdev.c
> > index ba3f0a7d9..8acfade5f 100644
> > --- a/drivers/net/bnxt/bnxt_ethdev.c
> > +++ b/drivers/net/bnxt/bnxt_ethdev.c
> > @@ -4856,6 +4856,11 @@ bnxt_dev_init(struct rte_eth_dev *eth_dev)
> > if (rc)
> > goto error_free;
> >
> > + /* Pass the information to the rte_eth_dev_close() that it should
> also
> > + * release the private port resources.
> > + */
> > + eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
> > +
>
> When you give this flag, 'rte_eth_dev_close()' will release port
> resources, it
> will memset the 'eth_dev->data'.
>
> In case user first closed the port, later removed it, remove path should
> be safe
> for it. I can see 'bnxt_dev_uninit()' operates on the values of
> 'eth_dev->data'
> which should crash in this case.
>
> Can you please double check the return path, all following should be safe:
> - close the port
> - remove the port
> - close and remove the port
>
On 1/14/2020 2:49 PM, Somnath Kotur wrote:
> Ferruh,
> Will recheck and revert on
Hi Ajit,
this is blocking the next-net-brcm tree to be pulled. Would you want to drop
this patch from your tree and continue with merge and deal with this patch later?
Or if this patch is the critical for the series I can keep waiting.
Thanks,
ferruh
>
> Thanks
> Som
>
> On Tue, 14 Jan 2020, 18:26 Ferruh Yigit, <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
>
> On 1/14/2020 5:14 AM, Ajit Khaparde wrote:
> > From: Somnath Kotur <somnath.kotur@broadcom.com
> <mailto:somnath.kotur@broadcom.com>>
> >
> > Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private resources
> > for the port can be freed by rte_eth_dev_close().
> >
> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com
> <mailto:somnath.kotur@broadcom.com>>
> > Reviewed-by: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@broadcom.com
> <mailto:kalesh-anakkur.purayil@broadcom.com>>
> > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com
> <mailto:ajit.khaparde@broadcom.com>>
> > ---
> > drivers/net/bnxt/bnxt_ethdev.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> > index ba3f0a7d9..8acfade5f 100644
> > --- a/drivers/net/bnxt/bnxt_ethdev.c
> > +++ b/drivers/net/bnxt/bnxt_ethdev.c
> > @@ -4856,6 +4856,11 @@ bnxt_dev_init(struct rte_eth_dev *eth_dev)
> > if (rc)
> > goto error_free;
> >
> > + /* Pass the information to the rte_eth_dev_close() that it should also
> > + * release the private port resources.
> > + */
> > + eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
> > +
>
> When you give this flag, 'rte_eth_dev_close()' will release port resources, it
> will memset the 'eth_dev->data'.
>
> In case user first closed the port, later removed it, remove path should be safe
> for it. I can see 'bnxt_dev_uninit()' operates on the values of 'eth_dev->data'
> which should crash in this case.
>
> Can you please double check the return path, all following should be safe:
> - close the port
> - remove the port
> - close and remove the port
>
Ferruh,
On Thu, 16 Jan 2020, 16:50 Ferruh Yigit, <ferruh.yigit@intel.com> wrote:
> On 1/14/2020 2:49 PM, Somnath Kotur wrote:
> > Ferruh,
> > Will recheck and revert on
>
> Hi Ajit,
>
> this is blocking the next-net-brcm tree to be pulled. Would you want to
> drop
> this patch from your tree and continue with merge and deal with this patch
> later?
> Or if this patch is the critical for the series I can keep waiting.
>
Kalesh will be sending v4 of the this patch set addressing this patch as
well shortly ..few hours full now
Thanks
Som
>
> Thanks,
> ferruh
>
> >
> > Thanks
> > Som
> >
> > On Tue, 14 Jan 2020, 18:26 Ferruh Yigit, <ferruh.yigit@intel.com
> > <mailto:ferruh.yigit@intel.com>> wrote:
> >
> > On 1/14/2020 5:14 AM, Ajit Khaparde wrote:
> > > From: Somnath Kotur <somnath.kotur@broadcom.com
> > <mailto:somnath.kotur@broadcom.com>>
> > >
> > > Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private
> resources
> > > for the port can be freed by rte_eth_dev_close().
> > >
> > > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com
> > <mailto:somnath.kotur@broadcom.com>>
> > > Reviewed-by: Kalesh Anakkur Purayil <
> kalesh-anakkur.purayil@broadcom.com
> > <mailto:kalesh-anakkur.purayil@broadcom.com>>
> > > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com
> > <mailto:ajit.khaparde@broadcom.com>>
> > > ---
> > > drivers/net/bnxt/bnxt_ethdev.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/net/bnxt/bnxt_ethdev.c
> b/drivers/net/bnxt/bnxt_ethdev.c
> > > index ba3f0a7d9..8acfade5f 100644
> > > --- a/drivers/net/bnxt/bnxt_ethdev.c
> > > +++ b/drivers/net/bnxt/bnxt_ethdev.c
> > > @@ -4856,6 +4856,11 @@ bnxt_dev_init(struct rte_eth_dev *eth_dev)
> > > if (rc)
> > > goto error_free;
> > >
> > > + /* Pass the information to the rte_eth_dev_close() that it
> should also
> > > + * release the private port resources.
> > > + */
> > > + eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
> > > +
> >
> > When you give this flag, 'rte_eth_dev_close()' will release port
> resources, it
> > will memset the 'eth_dev->data'.
> >
> > In case user first closed the port, later removed it, remove path
> should be safe
> > for it. I can see 'bnxt_dev_uninit()' operates on the values of
> 'eth_dev->data'
> > which should crash in this case.
> >
> > Can you please double check the return path, all following should be
> safe:
> > - close the port
> > - remove the port
> > - close and remove the port
> >
>
>
@@ -4856,6 +4856,11 @@ bnxt_dev_init(struct rte_eth_dev *eth_dev)
if (rc)
goto error_free;
+ /* Pass the information to the rte_eth_dev_close() that it should also
+ * release the private port resources.
+ */
+ eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
+
PMD_DRV_LOG(INFO,
DRV_MODULE_NAME "found at mem %" PRIX64 ", node addr %pM\n",
pci_dev->mem_resource[0].phys_addr,