diff mbox series

[net-next,4/5] bnxt_en: Refactor __bnxt_vf_reps_destroy().

Message ID 1618186695-18823-5-git-send-email-michael.chan@broadcom.com
State New
Headers show
Series bnxt_en: Error recovery fixes. | expand

Commit Message

Michael Chan April 12, 2021, 12:18 a.m. UTC 
Add a new helper function __bnxt_free_one_vf_rep() to free one VF rep.
We also reintialize the VF rep fields to proper initial values so that
the function can be used without freeing the VF rep data structure.  This
will be used in subsequent patches to free and recreate VF reps after
error recovery.

Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>
Reviewed-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 21 ++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

Comments

Leon Romanovsky April 12, 2021, 7:37 a.m. UTC | #1 
On Sun, Apr 11, 2021 at 08:18:14PM -0400, Michael Chan wrote:
> Add a new helper function __bnxt_free_one_vf_rep() to free one VF rep.
> We also reintialize the VF rep fields to proper initial values so that
> the function can be used without freeing the VF rep data structure.  This
> will be used in subsequent patches to free and recreate VF reps after
> error recovery.
> 
> Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>
> Reviewed-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 21 ++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
> index b5d6cd63bea7..a4ac11f5b0e5 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
> @@ -288,6 +288,21 @@ void bnxt_vf_reps_open(struct bnxt *bp)
>  		bnxt_vf_rep_open(bp->vf_reps[i]->dev);
>  }
>  
> +static void __bnxt_free_one_vf_rep(struct bnxt *bp, struct bnxt_vf_rep *vf_rep)
> +{
> +	if (!vf_rep)
> +		return;

How can it be NULL if you check that vf_rep != NULL when called to
__bnxt_free_one_vf_rep() ?

Thanks

> +
> +	if (vf_rep->dst) {
> +		dst_release((struct dst_entry *)vf_rep->dst);
> +		vf_rep->dst = NULL;
> +	}
> +	if (vf_rep->tx_cfa_action != CFA_HANDLE_INVALID) {
> +		hwrm_cfa_vfr_free(bp, vf_rep->vf_idx);
> +		vf_rep->tx_cfa_action = CFA_HANDLE_INVALID;
> +	}
> +}
> +
>  static void __bnxt_vf_reps_destroy(struct bnxt *bp)
>  {
>  	u16 num_vfs = pci_num_vf(bp->pdev);
> @@ -297,11 +312,7 @@ static void __bnxt_vf_reps_destroy(struct bnxt *bp)
>  	for (i = 0; i < num_vfs; i++) {
>  		vf_rep = bp->vf_reps[i];
>  		if (vf_rep) {
> -			dst_release((struct dst_entry *)vf_rep->dst);
> -
> -			if (vf_rep->tx_cfa_action != CFA_HANDLE_INVALID)
> -				hwrm_cfa_vfr_free(bp, vf_rep->vf_idx);
> -
> +			__bnxt_free_one_vf_rep(bp, vf_rep);
>  			if (vf_rep->dev) {
>  				/* if register_netdev failed, then netdev_ops
>  				 * would have been set to NULL
> -- 
> 2.18.1
>
Michael Chan April 12, 2021, 4:31 p.m. UTC | #2 
On Mon, Apr 12, 2021 at 12:37 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Sun, Apr 11, 2021 at 08:18:14PM -0400, Michael Chan wrote:
> > Add a new helper function __bnxt_free_one_vf_rep() to free one VF rep.
> > We also reintialize the VF rep fields to proper initial values so that
> > the function can be used without freeing the VF rep data structure.  This
> > will be used in subsequent patches to free and recreate VF reps after
> > error recovery.
> >
> > Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>
> > Reviewed-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> > ---
> >  drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 21 ++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
> > index b5d6cd63bea7..a4ac11f5b0e5 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
> > @@ -288,6 +288,21 @@ void bnxt_vf_reps_open(struct bnxt *bp)
> >               bnxt_vf_rep_open(bp->vf_reps[i]->dev);
> >  }
> >
> > +static void __bnxt_free_one_vf_rep(struct bnxt *bp, struct bnxt_vf_rep *vf_rep)
> > +{
> > +     if (!vf_rep)
> > +             return;
>
> How can it be NULL if you check that vf_rep != NULL when called to
> __bnxt_free_one_vf_rep() ?
>

For this patch, the if (!vf_rep) check here is redundant.  But it is
needed in the next patch (patch 5) that calls this function from
bnxt_vf_reps_free() in a different context.  Thanks.
Leon Romanovsky April 12, 2021, 5:33 p.m. UTC | #3 
On Mon, Apr 12, 2021 at 09:31:33AM -0700, Michael Chan wrote:
> On Mon, Apr 12, 2021 at 12:37 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Sun, Apr 11, 2021 at 08:18:14PM -0400, Michael Chan wrote:
> > > Add a new helper function __bnxt_free_one_vf_rep() to free one VF rep.
> > > We also reintialize the VF rep fields to proper initial values so that
> > > the function can be used without freeing the VF rep data structure.  This
> > > will be used in subsequent patches to free and recreate VF reps after
> > > error recovery.
> > >
> > > Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>
> > > Reviewed-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> > > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> > > ---
> > >  drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 21 ++++++++++++++-----
> > >  1 file changed, 16 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
> > > index b5d6cd63bea7..a4ac11f5b0e5 100644
> > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
> > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
> > > @@ -288,6 +288,21 @@ void bnxt_vf_reps_open(struct bnxt *bp)
> > >               bnxt_vf_rep_open(bp->vf_reps[i]->dev);
> > >  }
> > >
> > > +static void __bnxt_free_one_vf_rep(struct bnxt *bp, struct bnxt_vf_rep *vf_rep)
> > > +{
> > > +     if (!vf_rep)
> > > +             return;
> >
> > How can it be NULL if you check that vf_rep != NULL when called to
> > __bnxt_free_one_vf_rep() ?
> >
> 
> For this patch, the if (!vf_rep) check here is redundant.  But it is
> needed in the next patch (patch 5) that calls this function from
> bnxt_vf_reps_free() in a different context.  Thanks.

So add it in the patch that needs it.

Thanks
Leon Romanovsky April 13, 2021, 5:25 a.m. UTC | #4 
On Mon, Apr 12, 2021 at 10:51:25AM -0700, Michael Chan wrote:
> On Mon, Apr 12, 2021 at 10:33 AM Leon Romanovsky <leon@kernel.org> wrote:

> >

> > On Mon, Apr 12, 2021 at 09:31:33AM -0700, Michael Chan wrote:

> > > On Mon, Apr 12, 2021 at 12:37 AM Leon Romanovsky <leon@kernel.org> wrote:

> > > >

> > > > On Sun, Apr 11, 2021 at 08:18:14PM -0400, Michael Chan wrote:

> > > > > Add a new helper function __bnxt_free_one_vf_rep() to free one VF rep.

> > > > > We also reintialize the VF rep fields to proper initial values so that

> > > > > the function can be used without freeing the VF rep data structure.  This

> > > > > will be used in subsequent patches to free and recreate VF reps after

> > > > > error recovery.

> > > > >

> > > > > Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>

> > > > > Reviewed-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>

> > > > > Signed-off-by: Michael Chan <michael.chan@broadcom.com>

> > > > > ---

> > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 21 ++++++++++++++-----

> > > > >  1 file changed, 16 insertions(+), 5 deletions(-)

> > > > >

> > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c

> > > > > index b5d6cd63bea7..a4ac11f5b0e5 100644

> > > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c

> > > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c

> > > > > @@ -288,6 +288,21 @@ void bnxt_vf_reps_open(struct bnxt *bp)

> > > > >               bnxt_vf_rep_open(bp->vf_reps[i]->dev);

> > > > >  }

> > > > >

> > > > > +static void __bnxt_free_one_vf_rep(struct bnxt *bp, struct bnxt_vf_rep *vf_rep)

> > > > > +{

> > > > > +     if (!vf_rep)

> > > > > +             return;

> > > >

> > > > How can it be NULL if you check that vf_rep != NULL when called to

> > > > __bnxt_free_one_vf_rep() ?

> > > >

> > >

> > > For this patch, the if (!vf_rep) check here is redundant.  But it is

> > > needed in the next patch (patch 5) that calls this function from

> > > bnxt_vf_reps_free() in a different context.  Thanks.

> >

> > So add it in the patch that needs it.

> >

> 

> As stated in the changelog, we added more code to make this function

> more general and usable from another context.  The check for !vf_rep

> is part of that.  In my opinion, I think it is ok to keep it here

> given that the intent of this patch is to create a more general

> function.  Thanks.


I disagreed, but given the fact that Dave already merged this series, it
doesn't matter anymore.

Thanks
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index b5d6cd63bea7..a4ac11f5b0e5 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -288,6 +288,21 @@  void bnxt_vf_reps_open(struct bnxt *bp)
 		bnxt_vf_rep_open(bp->vf_reps[i]->dev);
 }
 
+static void __bnxt_free_one_vf_rep(struct bnxt *bp, struct bnxt_vf_rep *vf_rep)
+{
+	if (!vf_rep)
+		return;
+
+	if (vf_rep->dst) {
+		dst_release((struct dst_entry *)vf_rep->dst);
+		vf_rep->dst = NULL;
+	}
+	if (vf_rep->tx_cfa_action != CFA_HANDLE_INVALID) {
+		hwrm_cfa_vfr_free(bp, vf_rep->vf_idx);
+		vf_rep->tx_cfa_action = CFA_HANDLE_INVALID;
+	}
+}
+
 static void __bnxt_vf_reps_destroy(struct bnxt *bp)
 {
 	u16 num_vfs = pci_num_vf(bp->pdev);
@@ -297,11 +312,7 @@  static void __bnxt_vf_reps_destroy(struct bnxt *bp)
 	for (i = 0; i < num_vfs; i++) {
 		vf_rep = bp->vf_reps[i];
 		if (vf_rep) {
-			dst_release((struct dst_entry *)vf_rep->dst);
-
-			if (vf_rep->tx_cfa_action != CFA_HANDLE_INVALID)
-				hwrm_cfa_vfr_free(bp, vf_rep->vf_idx);
-
+			__bnxt_free_one_vf_rep(bp, vf_rep);
 			if (vf_rep->dev) {
 				/* if register_netdev failed, then netdev_ops
 				 * would have been set to NULL