[PATCH net-next 0/5] bnxt_en: Error recovery fixes.

[PATCH net-next 0/5] bnxt_en: Error recovery fixes.

[Michael Chan]

This series adds some fixes and enhancements to the error recovery
logic.  The health register logic is improved and we also add missing
code to free and re-create VF representors in the firmware after
error recovery.

Michael Chan (2):
  bnxt_en: Treat health register value 0 as valid in
    bnxt_try_reover_fw().
  bnxt_en: Refactor __bnxt_vf_reps_destroy().

Sriharsha Basavapatna (2):
  bnxt_en: Refactor bnxt_vf_reps_create().
  bnxt_en: Free and allocate VF-Reps during error recovery.

Vasundhara Volam (1):
  bnxt_en: Invalidate health register mapping at the end of probe.

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   8 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 122 ++++++++++++++----
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h |  12 ++
 3 files changed, 115 insertions(+), 27 deletions(-)

-- 
2.18.1

[PATCH net-next 1/5] bnxt_en: Treat health register value 0 as valid in bnxt_try_reover_fw().

[Michael Chan]

The retry loop in bnxt_try_recover_fw() should not abort when the
health register value is 0.  It is a valid value that indicates the
firmware is booting up.

Fixes: 861aae786f2f ("bnxt_en: Enhance retry of the first message to the firmware.")
Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 6f13642121c4..3d36f945baf8 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -9532,8 +9532,8 @@ static int bnxt_try_recover_fw(struct bnxt *bp)
 		do {
 			sts = bnxt_fw_health_readl(bp, BNXT_FW_HEALTH_REG);
 			rc = __bnxt_hwrm_ver_get(bp, true);
-			if (!sts || (!BNXT_FW_IS_BOOTING(sts) &&
-				     !BNXT_FW_IS_RECOVERING(sts)))
+			if (!BNXT_FW_IS_BOOTING(sts) &&
+			    !BNXT_FW_IS_RECOVERING(sts))
 				break;
 			retry++;
 		} while (rc == -EBUSY && retry < BNXT_FW_RETRY);
-- 
2.18.1

[PATCH net-next 2/5] bnxt_en: Invalidate health register mapping at the end of probe.

[Michael Chan]

From: Vasundhara Volam <vasundhara-v.volam@broadcom.com>

After probe is successful, interface may not be bought up in all
the cases and health register mapping could be invalid if firmware
undergoes reset. Fix it by invalidating the health register at the
end of probe. It will be remapped during ifup.

Fixes: 43a440c4007b ("bnxt_en: Improve the status_reliable flag in bp->fw_health.")
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 3d36f945baf8..3f5fbdf61257 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -12972,6 +12972,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 				   rc);
 	}
 
+	bnxt_inv_fw_health_reg(bp);
 	bnxt_dl_register(bp);
 
 	rc = register_netdev(dev);
-- 
2.18.1

[PATCH net-next 3/5] bnxt_en: Refactor bnxt_vf_reps_create().

[Michael Chan]

From: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>

Add a new function bnxt_alloc_vf_rep() to allocate a VF representor.
This function will be needed in subsequent patches to recreate the
VF reps after error recovery.

Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 40 ++++++++++---------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index 4b5c8fd76a51..b5d6cd63bea7 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -350,6 +350,26 @@ void bnxt_vf_reps_destroy(struct bnxt *bp)
 	__bnxt_vf_reps_destroy(bp);
 }
 
+static int bnxt_alloc_vf_rep(struct bnxt *bp, struct bnxt_vf_rep *vf_rep,
+			     u16 *cfa_code_map)
+{
+	/* get cfa handles from FW */
+	if (hwrm_cfa_vfr_alloc(bp, vf_rep->vf_idx, &vf_rep->tx_cfa_action,
+			       &vf_rep->rx_cfa_code))
+		return -ENOLINK;
+
+	cfa_code_map[vf_rep->rx_cfa_code] = vf_rep->vf_idx;
+	vf_rep->dst = metadata_dst_alloc(0, METADATA_HW_PORT_MUX, GFP_KERNEL);
+	if (!vf_rep->dst)
+		return -ENOMEM;
+
+	/* only cfa_action is needed to mux a packet while TXing */
+	vf_rep->dst->u.port_info.port_id = vf_rep->tx_cfa_action;
+	vf_rep->dst->u.port_info.lower_dev = bp->dev;
+
+	return 0;
+}
+
 /* Use the OUI of the PF's perm addr and report the same mac addr
  * for the same VF-rep each time
  */
@@ -428,25 +448,9 @@ static int bnxt_vf_reps_create(struct bnxt *bp)
 		vf_rep->vf_idx = i;
 		vf_rep->tx_cfa_action = CFA_HANDLE_INVALID;
 
-		/* get cfa handles from FW */
-		rc = hwrm_cfa_vfr_alloc(bp, vf_rep->vf_idx,
-					&vf_rep->tx_cfa_action,
-					&vf_rep->rx_cfa_code);
-		if (rc) {
-			rc = -ENOLINK;
-			goto err;
-		}
-		cfa_code_map[vf_rep->rx_cfa_code] = vf_rep->vf_idx;
-
-		vf_rep->dst = metadata_dst_alloc(0, METADATA_HW_PORT_MUX,
-						 GFP_KERNEL);
-		if (!vf_rep->dst) {
-			rc = -ENOMEM;
+		rc = bnxt_alloc_vf_rep(bp, vf_rep, cfa_code_map);
+		if (rc)
 			goto err;
-		}
-		/* only cfa_action is needed to mux a packet while TXing */
-		vf_rep->dst->u.port_info.port_id = vf_rep->tx_cfa_action;
-		vf_rep->dst->u.port_info.lower_dev = bp->dev;
 
 		bnxt_vf_rep_netdev_init(bp, vf_rep, dev);
 		rc = register_netdev(dev);
-- 
2.18.1

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

[Michael Chan]

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;
+
+	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

[PATCH net-next 5/5] bnxt_en: Free and allocate VF-Reps during error recovery.

[Michael Chan]

From: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>

During firmware recovery, VF-Rep configuration in the firmware is lost.
Fix it by freeing and (re)allocating VF-Reps in FW at relevant points
during the error recovery process.

Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  3 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 61 ++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h | 12 ++++
 3 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 3f5fbdf61257..e15d454e33f0 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -11081,6 +11081,7 @@ static void bnxt_fw_reset_close(struct bnxt *bp)
 		pci_disable_device(bp->pdev);
 	}
 	__bnxt_close_nic(bp, true, false);
+	bnxt_vf_reps_free(bp);
 	bnxt_clear_int_mode(bp);
 	bnxt_hwrm_func_drv_unrgtr(bp);
 	if (pci_is_enabled(bp->pdev))
@@ -11825,6 +11826,8 @@ static void bnxt_fw_reset_task(struct work_struct *work)
 		bnxt_ulp_start(bp, rc);
 		if (!rc)
 			bnxt_reenable_sriov(bp);
+		bnxt_vf_reps_alloc(bp);
+		bnxt_vf_reps_open(bp);
 		bnxt_dl_health_recovery_done(bp);
 		bnxt_dl_health_status_update(bp, true);
 		rtnl_unlock();
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index a4ac11f5b0e5..dd66302343a2 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -284,8 +284,11 @@ void bnxt_vf_reps_open(struct bnxt *bp)
 	if (bp->eswitch_mode != DEVLINK_ESWITCH_MODE_SWITCHDEV)
 		return;
 
-	for (i = 0; i < pci_num_vf(bp->pdev); i++)
-		bnxt_vf_rep_open(bp->vf_reps[i]->dev);
+	for (i = 0; i < pci_num_vf(bp->pdev); i++) {
+		/* Open the VF-Rep only if it is allocated in the FW */
+		if (bp->vf_reps[i]->tx_cfa_action != CFA_HANDLE_INVALID)
+			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)
@@ -361,6 +364,23 @@ void bnxt_vf_reps_destroy(struct bnxt *bp)
 	__bnxt_vf_reps_destroy(bp);
 }
 
+/* Free the VF-Reps in firmware, during firmware hot-reset processing.
+ * Note that the VF-Rep netdevs are still active (not unregistered) during
+ * this process. As the mode transition from SWITCHDEV to LEGACY happens
+ * under the rtnl_lock() this routine is safe under the rtnl_lock().
+ */
+void bnxt_vf_reps_free(struct bnxt *bp)
+{
+	u16 num_vfs = pci_num_vf(bp->pdev);
+	int i;
+
+	if (bp->eswitch_mode != DEVLINK_ESWITCH_MODE_SWITCHDEV)
+		return;
+
+	for (i = 0; i < num_vfs; i++)
+		__bnxt_free_one_vf_rep(bp, bp->vf_reps[i]);
+}
+
 static int bnxt_alloc_vf_rep(struct bnxt *bp, struct bnxt_vf_rep *vf_rep,
 			     u16 *cfa_code_map)
 {
@@ -381,6 +401,43 @@ static int bnxt_alloc_vf_rep(struct bnxt *bp, struct bnxt_vf_rep *vf_rep,
 	return 0;
 }
 
+/* Allocate the VF-Reps in firmware, during firmware hot-reset processing.
+ * Note that the VF-Rep netdevs are still active (not unregistered) during
+ * this process. As the mode transition from SWITCHDEV to LEGACY happens
+ * under the rtnl_lock() this routine is safe under the rtnl_lock().
+ */
+int bnxt_vf_reps_alloc(struct bnxt *bp)
+{
+	u16 *cfa_code_map = bp->cfa_code_map, num_vfs = pci_num_vf(bp->pdev);
+	struct bnxt_vf_rep *vf_rep;
+	int rc, i;
+
+	if (bp->eswitch_mode != DEVLINK_ESWITCH_MODE_SWITCHDEV)
+		return 0;
+
+	if (!cfa_code_map)
+		return -EINVAL;
+
+	for (i = 0; i < MAX_CFA_CODE; i++)
+		cfa_code_map[i] = VF_IDX_INVALID;
+
+	for (i = 0; i < num_vfs; i++) {
+		vf_rep = bp->vf_reps[i];
+		vf_rep->vf_idx = i;
+
+		rc = bnxt_alloc_vf_rep(bp, vf_rep, cfa_code_map);
+		if (rc)
+			goto err;
+	}
+
+	return 0;
+
+err:
+	netdev_info(bp->dev, "%s error=%d\n", __func__, rc);
+	bnxt_vf_reps_free(bp);
+	return rc;
+}
+
 /* Use the OUI of the PF's perm addr and report the same mac addr
  * for the same VF-rep each time
  */
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h
index d7287651422f..5637a84884d7 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h
@@ -19,6 +19,8 @@ void bnxt_vf_reps_close(struct bnxt *bp);
 void bnxt_vf_reps_open(struct bnxt *bp);
 void bnxt_vf_rep_rx(struct bnxt *bp, struct sk_buff *skb);
 struct net_device *bnxt_get_vf_rep(struct bnxt *bp, u16 cfa_code);
+int bnxt_vf_reps_alloc(struct bnxt *bp);
+void bnxt_vf_reps_free(struct bnxt *bp);
 
 static inline u16 bnxt_vf_rep_get_fid(struct net_device *dev)
 {
@@ -61,5 +63,15 @@ static inline bool bnxt_dev_is_vf_rep(struct net_device *dev)
 {
 	return false;
 }
+
+static inline int bnxt_vf_reps_alloc(struct bnxt *bp)
+{
+	return 0;
+}
+
+static inline void bnxt_vf_reps_free(struct bnxt *bp)
+{
+}
+
 #endif /* CONFIG_BNXT_SRIOV */
 #endif /* BNXT_VFR_H */
-- 
2.18.1

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

[Leon Romanovsky]

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
> 

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

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1629 bytes --]

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.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

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

[Leon Romanovsky]

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

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

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 2244 bytes --]

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.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 0/5] bnxt_en: Error recovery fixes.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Sun, 11 Apr 2021 20:18:10 -0400 you wrote:
> This series adds some fixes and enhancements to the error recovery
> logic.  The health register logic is improved and we also add missing
> code to free and re-create VF representors in the firmware after
> error recovery.
> 
> Michael Chan (2):
>   bnxt_en: Treat health register value 0 as valid in
>     bnxt_try_reover_fw().
>   bnxt_en: Refactor __bnxt_vf_reps_destroy().
> 
> [...]

Here is the summary with links:
  - [net-next,1/5] bnxt_en: Treat health register value 0 as valid in bnxt_try_reover_fw().
    https://git.kernel.org/netdev/net-next/c/17e1be342d46
  - [net-next,2/5] bnxt_en: Invalidate health register mapping at the end of probe.
    https://git.kernel.org/netdev/net-next/c/190eda1a9dbc
  - [net-next,3/5] bnxt_en: Refactor bnxt_vf_reps_create().
    https://git.kernel.org/netdev/net-next/c/ea2d37b2b307
  - [net-next,4/5] bnxt_en: Refactor __bnxt_vf_reps_destroy().
    https://git.kernel.org/netdev/net-next/c/90f4fd029687
  - [net-next,5/5] bnxt_en: Free and allocate VF-Reps during error recovery.
    https://git.kernel.org/netdev/net-next/c/ac797ced1fd0

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Leon Romanovsky]

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