diff mbox series

[v3,09/10] net/bnxt: release port upon close

Message ID 20200114051435.46093-10-ajit.khaparde@broadcom.com (mailing list archive)
State Superseded, archived
Delegated to: Ajit Khaparde
Headers
Series bnxt patchset with bug fixes |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Ajit Khaparde Jan. 14, 2020, 5:14 a.m. UTC 
  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

Ferruh Yigit Jan. 14, 2020, 12:56 p.m. UTC | #1 
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
Somnath Kotur Jan. 14, 2020, 2:49 p.m. UTC | #2 
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
>
Ferruh Yigit Jan. 16, 2020, 11:20 a.m. UTC | #3 
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
>
Somnath Kotur Jan. 16, 2020, 11:30 a.m. UTC | #4 
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
> >
>
>
diff mbox series

Patch

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;
+
 	PMD_DRV_LOG(INFO,
 		    DRV_MODULE_NAME "found at mem %" PRIX64 ", node addr %pM\n",
 		    pci_dev->mem_resource[0].phys_addr,