| Message ID | 1618186695-18823-5-git-send-email-michael.chan@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 90f4fd02968720bdeb38a16deeff96fa770206e4 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bnxt_en: Error recovery fixes. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 33 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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 >
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.
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
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.
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 --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