[PATCH net 0/3] bnxt_en: 2 bug fixes

[PATCH net 0/3] bnxt_en: 2 bug fixes

[Michael Chan]

The first patch is a bug fix for taking netdev_lock twice in the
AER code path if the RoCE driver is loaded.  The second patch is a
refactor patch needed by patch 3.  Patch 3 fixes a packet drop
issue if queue restart is done on a ring belonging to a non-default
RSS context.

Michael Chan (1):
  bnxt_en: Fix netdev locking in ULP IRQ functions

Pavan Chebbi (2):
  bnxt_en: Add a helper function to configure MRU and RSS
  bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 59 +++++++++++++++----
 drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c |  9 +--
 include/net/netdev_lock.h                     |  3 +
 3 files changed, 52 insertions(+), 19 deletions(-)

-- 
2.30.1

[PATCH net 1/3] bnxt_en: Fix netdev locking in ULP IRQ functions

[Michael Chan]

netdev_lock is already held when calling bnxt_ulp_irq_stop() and
bnxt_ulp_irq_restart().  When converting rtnl_lock to netdev_lock,
the original code was rtnl_dereference() to indicate that rtnl_lock
was already held.  rcu_dereference_protected() is the correct
conversion after replacing rtnl_lock with netdev_lock.

Add a new helper netdev_lock_dereference() similar to
rtnl_dereference().

Fixes: 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
Cc: Stanislav Fomichev <sdf@fomichev.me>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 9 +++------
 include/net/netdev_lock.h                     | 3 +++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
index a8e930d5dbb0..7564705d6478 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
@@ -20,6 +20,7 @@
 #include <asm/byteorder.h>
 #include <linux/bitmap.h>
 #include <linux/auxiliary_bus.h>
+#include <net/netdev_lock.h>
 
 #include "bnxt_hsi.h"
 #include "bnxt.h"
@@ -309,14 +310,12 @@ void bnxt_ulp_irq_stop(struct bnxt *bp)
 		if (!ulp->msix_requested)
 			return;
 
-		netdev_lock(bp->dev);
-		ops = rcu_dereference(ulp->ulp_ops);
+		ops = netdev_lock_dereference(ulp->ulp_ops, bp->dev);
 		if (!ops || !ops->ulp_irq_stop)
 			return;
 		if (test_bit(BNXT_STATE_FW_RESET_DET, &bp->state))
 			reset = true;
 		ops->ulp_irq_stop(ulp->handle, reset);
-		netdev_unlock(bp->dev);
 	}
 }
 
@@ -335,8 +334,7 @@ void bnxt_ulp_irq_restart(struct bnxt *bp, int err)
 		if (!ulp->msix_requested)
 			return;
 
-		netdev_lock(bp->dev);
-		ops = rcu_dereference(ulp->ulp_ops);
+		ops = netdev_lock_dereference(ulp->ulp_ops, bp->dev);
 		if (!ops || !ops->ulp_irq_restart)
 			return;
 
@@ -348,7 +346,6 @@ void bnxt_ulp_irq_restart(struct bnxt *bp, int err)
 			bnxt_fill_msix_vecs(bp, ent);
 		}
 		ops->ulp_irq_restart(ulp->handle, ent);
-		netdev_unlock(bp->dev);
 		kfree(ent);
 	}
 }
diff --git a/include/net/netdev_lock.h b/include/net/netdev_lock.h
index c316b551df8d..0ee5bc766810 100644
--- a/include/net/netdev_lock.h
+++ b/include/net/netdev_lock.h
@@ -98,6 +98,9 @@ static inline int netdev_lock_cmp_fn(const struct lockdep_map *a,
 				  &qdisc_xmit_lock_key);	\
 }
 
+#define netdev_lock_dereference(p, dev)				\
+	rcu_dereference_protected(p, lockdep_is_held(&(dev)->lock))
+
 int netdev_debug_event(struct notifier_block *nb, unsigned long event,
 		       void *ptr);
 
-- 
2.30.1

[PATCH net 2/3] bnxt_en: Add a helper function to configure MRU and RSS

[Michael Chan]

From: Pavan Chebbi <pavan.chebbi@broadcom.com>

Add a new helper function that will configure MRU and RSS table
of a VNIC. This will be useful when we configure both on a VNIC
when resetting an RX ring.  This function will be used again in
the next bug fix patch where we have to reconfigure VNICs for RSS
contexts.

Suggested-by: Michael Chan <michael.chan@broadcom.com>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
Cc: David Wei <dw@davidwei.uk>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 36 ++++++++++++++++-------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 6afc2ab6fad2..a45c5ce81111 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -10738,6 +10738,26 @@ void bnxt_del_one_rss_ctx(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx,
 	bp->num_rss_ctx--;
 }
 
+static int bnxt_set_vnic_mru_p5(struct bnxt *bp, struct bnxt_vnic_info *vnic,
+				u16 mru)
+{
+	int rc;
+
+	if (mru) {
+		rc = bnxt_hwrm_vnic_set_rss_p5(bp, vnic, true);
+		if (rc) {
+			netdev_err(bp->dev, "hwrm vnic %d set rss failure rc: %d\n",
+				   vnic->vnic_id, rc);
+			return rc;
+		}
+	}
+	vnic->mru = mru;
+	bnxt_hwrm_vnic_update(bp, vnic,
+			      VNIC_UPDATE_REQ_ENABLES_MRU_VALID);
+
+	return 0;
+}
+
 static void bnxt_hwrm_realloc_rss_ctx_vnic(struct bnxt *bp)
 {
 	bool set_tpa = !!(bp->flags & BNXT_FLAG_TPA);
@@ -15936,15 +15956,10 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
 	for (i = 0; i < bp->nr_vnics; i++) {
 		vnic = &bp->vnic_info[i];
 
-		rc = bnxt_hwrm_vnic_set_rss_p5(bp, vnic, true);
-		if (rc) {
-			netdev_err(bp->dev, "hwrm vnic %d set rss failure rc: %d\n",
-				   vnic->vnic_id, rc);
+		rc = bnxt_set_vnic_mru_p5(bp, vnic,
+					  bp->dev->mtu + ETH_HLEN + VLAN_HLEN);
+		if (rc)
 			return rc;
-		}
-		vnic->mru = bp->dev->mtu + ETH_HLEN + VLAN_HLEN;
-		bnxt_hwrm_vnic_update(bp, vnic,
-				      VNIC_UPDATE_REQ_ENABLES_MRU_VALID);
 	}
 
 	return 0;
@@ -15969,9 +15984,8 @@ static int bnxt_queue_stop(struct net_device *dev, void *qmem, int idx)
 
 	for (i = 0; i < bp->nr_vnics; i++) {
 		vnic = &bp->vnic_info[i];
-		vnic->mru = 0;
-		bnxt_hwrm_vnic_update(bp, vnic,
-				      VNIC_UPDATE_REQ_ENABLES_MRU_VALID);
+
+		bnxt_set_vnic_mru_p5(bp, vnic, 0);
 	}
 	/* Make sure NAPI sees that the VNIC is disabled */
 	synchronize_net();
-- 
2.30.1

[PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Michael Chan]

From: Pavan Chebbi <pavan.chebbi@broadcom.com>

The commit under the Fixes tag below which updates the VNICs' RSS
and MRU during .ndo_queue_start(), needs to be extended to cover any
non-default RSS contexts which have their own VNICs.  Without this
step, packets that are destined to a non-default RSS context may be
dropped after .ndo_queue_start().

Fixes: 5ac066b7b062 ("bnxt_en: Fix queue start to update vnic RSS table")
Reported-by: David Wei <dw@davidwei.uk>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
Cc: David Wei <dw@davidwei.uk>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 27 +++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index a45c5ce81111..71e428710a26 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -10758,6 +10758,24 @@ static int bnxt_set_vnic_mru_p5(struct bnxt *bp, struct bnxt_vnic_info *vnic,
 	return 0;
 }
 
+static int bnxt_set_rss_ctx_vnic_mru(struct bnxt *bp, u16 mru)
+{
+	struct ethtool_rxfh_context *ctx;
+	unsigned long context;
+	int rc;
+
+	xa_for_each(&bp->dev->ethtool->rss_ctx, context, ctx) {
+		struct bnxt_rss_ctx *rss_ctx = ethtool_rxfh_context_priv(ctx);
+		struct bnxt_vnic_info *vnic = &rss_ctx->vnic;
+
+		rc = bnxt_set_vnic_mru_p5(bp, vnic, mru);
+		if (rc)
+			return rc;
+	}
+
+	return 0;
+}
+
 static void bnxt_hwrm_realloc_rss_ctx_vnic(struct bnxt *bp)
 {
 	bool set_tpa = !!(bp->flags & BNXT_FLAG_TPA);
@@ -15904,6 +15922,7 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
 	struct bnxt_vnic_info *vnic;
 	struct bnxt_napi *bnapi;
 	int i, rc;
+	u16 mru;
 
 	rxr = &bp->rx_ring[idx];
 	clone = qmem;
@@ -15953,16 +15972,15 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
 	napi_enable_locked(&bnapi->napi);
 	bnxt_db_nq_arm(bp, &cpr->cp_db, cpr->cp_raw_cons);
 
+	mru = bp->dev->mtu + ETH_HLEN + VLAN_HLEN;
 	for (i = 0; i < bp->nr_vnics; i++) {
 		vnic = &bp->vnic_info[i];
 
-		rc = bnxt_set_vnic_mru_p5(bp, vnic,
-					  bp->dev->mtu + ETH_HLEN + VLAN_HLEN);
+		rc = bnxt_set_vnic_mru_p5(bp, vnic, mru);
 		if (rc)
 			return rc;
 	}
-
-	return 0;
+	return bnxt_set_rss_ctx_vnic_mru(bp, mru);
 
 err_reset:
 	netdev_err(bp->dev, "Unexpected HWRM error during queue start rc: %d\n",
@@ -15987,6 +16005,7 @@ static int bnxt_queue_stop(struct net_device *dev, void *qmem, int idx)
 
 		bnxt_set_vnic_mru_p5(bp, vnic, 0);
 	}
+	bnxt_set_rss_ctx_vnic_mru(bp, 0);
 	/* Make sure NAPI sees that the VNIC is disabled */
 	synchronize_net();
 	rxr = &bp->rx_ring[idx];
-- 
2.30.1

Re: [PATCH net 1/3] bnxt_en: Fix netdev locking in ULP IRQ functions

[Simon Horman]

On Mon, May 19, 2025 at 01:41:28PM -0700, Michael Chan wrote:
> netdev_lock is already held when calling bnxt_ulp_irq_stop() and
> bnxt_ulp_irq_restart().  When converting rtnl_lock to netdev_lock,
> the original code was rtnl_dereference() to indicate that rtnl_lock
> was already held.  rcu_dereference_protected() is the correct
> conversion after replacing rtnl_lock with netdev_lock.
> 
> Add a new helper netdev_lock_dereference() similar to
> rtnl_dereference().
> 
> Fixes: 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 2/3] bnxt_en: Add a helper function to configure MRU and RSS

[Simon Horman]

On Mon, May 19, 2025 at 01:41:29PM -0700, Michael Chan wrote:
> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> 
> Add a new helper function that will configure MRU and RSS table
> of a VNIC. This will be useful when we configure both on a VNIC
> when resetting an RX ring.  This function will be used again in
> the next bug fix patch where we have to reconfigure VNICs for RSS
> contexts.
> 
> Suggested-by: Michael Chan <michael.chan@broadcom.com>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Simon Horman]

On Mon, May 19, 2025 at 01:41:30PM -0700, Michael Chan wrote:
> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> 
> The commit under the Fixes tag below which updates the VNICs' RSS
> and MRU during .ndo_queue_start(), needs to be extended to cover any
> non-default RSS contexts which have their own VNICs.  Without this
> step, packets that are destined to a non-default RSS context may be
> dropped after .ndo_queue_start().
> 
> Fixes: 5ac066b7b062 ("bnxt_en: Fix queue start to update vnic RSS table")
> Reported-by: David Wei <dw@davidwei.uk>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 2/3] bnxt_en: Add a helper function to configure MRU and RSS

[David Wei]

On 5/19/25 13:41, Michael Chan wrote:
> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> 
> Add a new helper function that will configure MRU and RSS table
> of a VNIC. This will be useful when we configure both on a VNIC
> when resetting an RX ring.  This function will be used again in
> the next bug fix patch where we have to reconfigure VNICs for RSS
> contexts.
> 
> Suggested-by: Michael Chan <michael.chan@broadcom.com>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
> Cc: David Wei <dw@davidwei.uk>
> ---
>   drivers/net/ethernet/broadcom/bnxt/bnxt.c | 36 ++++++++++++++++-------
>   1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 6afc2ab6fad2..a45c5ce81111 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -10738,6 +10738,26 @@ void bnxt_del_one_rss_ctx(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx,
>   	bp->num_rss_ctx--;
>   }
>   
> +static int bnxt_set_vnic_mru_p5(struct bnxt *bp, struct bnxt_vnic_info *vnic,
> +				u16 mru)
> +{
> +	int rc;
> +
> +	if (mru) {
> +		rc = bnxt_hwrm_vnic_set_rss_p5(bp, vnic, true);
> +		if (rc) {
> +			netdev_err(bp->dev, "hwrm vnic %d set rss failure rc: %d\n",
> +				   vnic->vnic_id, rc);
> +			return rc;
> +		}
> +	}
> +	vnic->mru = mru;
> +	bnxt_hwrm_vnic_update(bp, vnic,
> +			      VNIC_UPDATE_REQ_ENABLES_MRU_VALID);
> +
> +	return 0;
> +}
> +
>   static void bnxt_hwrm_realloc_rss_ctx_vnic(struct bnxt *bp)
>   {
>   	bool set_tpa = !!(bp->flags & BNXT_FLAG_TPA);
> @@ -15936,15 +15956,10 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
>   	for (i = 0; i < bp->nr_vnics; i++) {
>   		vnic = &bp->vnic_info[i];
>   
> -		rc = bnxt_hwrm_vnic_set_rss_p5(bp, vnic, true);
> -		if (rc) {
> -			netdev_err(bp->dev, "hwrm vnic %d set rss failure rc: %d\n",
> -				   vnic->vnic_id, rc);
> +		rc = bnxt_set_vnic_mru_p5(bp, vnic,
> +					  bp->dev->mtu + ETH_HLEN + VLAN_HLEN);

Pure mechanical change, LGTM.

Only nit is to calculate this mru once outside the loop, like patch 2.

Reviewed-by: David Wei <dw@davidwei.uk>

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[David Wei]

On 5/19/25 13:41, Michael Chan wrote:
> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> 
> The commit under the Fixes tag below which updates the VNICs' RSS
> and MRU during .ndo_queue_start(), needs to be extended to cover any
> non-default RSS contexts which have their own VNICs.  Without this
> step, packets that are destined to a non-default RSS context may be
> dropped after .ndo_queue_start().
> 
> Fixes: 5ac066b7b062 ("bnxt_en: Fix queue start to update vnic RSS table")
> Reported-by: David Wei <dw@davidwei.uk>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
> Cc: David Wei <dw@davidwei.uk>
> ---
>   drivers/net/ethernet/broadcom/bnxt/bnxt.c | 27 +++++++++++++++++++----
>   1 file changed, 23 insertions(+), 4 deletions(-)
> 

Thanks for the fix.

Reviewed-by: David Wei <dw@davidwei.uk>

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Jakub Kicinski]

On Mon, 19 May 2025 13:41:30 -0700 Michael Chan wrote:
> @@ -15987,6 +16005,7 @@ static int bnxt_queue_stop(struct net_device *dev, void *qmem, int idx)
>  
>  		bnxt_set_vnic_mru_p5(bp, vnic, 0);
>  	}
> +	bnxt_set_rss_ctx_vnic_mru(bp, 0);
>  	/* Make sure NAPI sees that the VNIC is disabled */
>  	synchronize_net();
>  	rxr = &bp->rx_ring[idx];

What does setting MRU to zero do? All traffic will be dropped?
Traffic will no longer be filtered based on MRU?  Or.. ?

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Michael Chan]

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

On Tue, May 20, 2025 at 6:28 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 19 May 2025 13:41:30 -0700 Michael Chan wrote:
> > @@ -15987,6 +16005,7 @@ static int bnxt_queue_stop(struct net_device *dev, void *qmem, int idx)
> >
> >               bnxt_set_vnic_mru_p5(bp, vnic, 0);
> >       }
> > +     bnxt_set_rss_ctx_vnic_mru(bp, 0);
> >       /* Make sure NAPI sees that the VNIC is disabled */
> >       synchronize_net();
> >       rxr = &bp->rx_ring[idx];
>
> What does setting MRU to zero do? All traffic will be dropped?
> Traffic will no longer be filtered based on MRU?  Or.. ?

That VNIC with MRU set to zero will not receive any more traffic.
This step was recommended by the FW team when we first started working
with David to implement the queue_mgmt_ops.

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

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Jakub Kicinski]

On Tue, 20 May 2025 18:38:45 -0700 Michael Chan wrote:
> On Tue, May 20, 2025 at 6:28 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Mon, 19 May 2025 13:41:30 -0700 Michael Chan wrote:  
> > > @@ -15987,6 +16005,7 @@ static int bnxt_queue_stop(struct net_device *dev, void *qmem, int idx)
> > >
> > >               bnxt_set_vnic_mru_p5(bp, vnic, 0);
> > >       }
> > > +     bnxt_set_rss_ctx_vnic_mru(bp, 0);
> > >       /* Make sure NAPI sees that the VNIC is disabled */
> > >       synchronize_net();
> > >       rxr = &bp->rx_ring[idx];  
> >
> > What does setting MRU to zero do? All traffic will be dropped?
> > Traffic will no longer be filtered based on MRU?  Or.. ?  
> 
> That VNIC with MRU set to zero will not receive any more traffic.
> This step was recommended by the FW team when we first started working
> with David to implement the queue_mgmt_ops.

Shutting down traffic to ZC queues is one thing, but now you
seem to be walking all RSS contexts and shutting them all down.
The whole point of the queue API is to avoid shutting down 
the entire device.
-- 
pw-bot: cr

Re: [PATCH net 1/3] bnxt_en: Fix netdev locking in ULP IRQ functions

[Jakub Kicinski]

On Mon, 19 May 2025 13:41:28 -0700 Michael Chan wrote:
> netdev_lock is already held when calling bnxt_ulp_irq_stop() and
> bnxt_ulp_irq_restart().  When converting rtnl_lock to netdev_lock,
> the original code was rtnl_dereference() to indicate that rtnl_lock
> was already held.  rcu_dereference_protected() is the correct
> conversion after replacing rtnl_lock with netdev_lock.
> 
> Add a new helper netdev_lock_dereference() similar to
> rtnl_dereference().

I'll apply this one as is since its for the current release

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Michael Chan]

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

On Tue, May 20, 2025 at 6:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 20 May 2025 18:38:45 -0700 Michael Chan wrote:
> > On Tue, May 20, 2025 at 6:28 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Mon, 19 May 2025 13:41:30 -0700 Michael Chan wrote:
> > > > @@ -15987,6 +16005,7 @@ static int bnxt_queue_stop(struct net_device *dev, void *qmem, int idx)
> > > >
> > > >               bnxt_set_vnic_mru_p5(bp, vnic, 0);
> > > >       }
> > > > +     bnxt_set_rss_ctx_vnic_mru(bp, 0);
> > > >       /* Make sure NAPI sees that the VNIC is disabled */
> > > >       synchronize_net();
> > > >       rxr = &bp->rx_ring[idx];
> > >
> > > What does setting MRU to zero do? All traffic will be dropped?
> > > Traffic will no longer be filtered based on MRU?  Or.. ?
> >
> > That VNIC with MRU set to zero will not receive any more traffic.
> > This step was recommended by the FW team when we first started working
> > with David to implement the queue_mgmt_ops.
>
> Shutting down traffic to ZC queues is one thing, but now you
> seem to be walking all RSS contexts and shutting them all down.
> The whole point of the queue API is to avoid shutting down
> the entire device.

The existing code has been setting the MRU to 0 for the default RSS
context's VNIC.  They found that this sequence was reliable.

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

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Jakub Kicinski]

On Tue, 20 May 2025 19:10:37 -0700 Michael Chan wrote:
> > Shutting down traffic to ZC queues is one thing, but now you
> > seem to be walking all RSS contexts and shutting them all down.
> > The whole point of the queue API is to avoid shutting down
> > the entire device.  
> 
> The existing code has been setting the MRU to 0 for the default RSS
> context's VNIC. 

:/ I must have misunderstood. I wouldn't have merged this if I knew.
You can't be shutting down system queues because some application
decided to bind a ZC queue.

> They found that this sequence was reliable.

"reliable" is a bit of a big word that some people would reserve
for code which is production tested or at the very least very
heavily validated.

Re: [PATCH net 0/3] bnxt_en: 2 bug fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 19 May 2025 13:41:27 -0700 you wrote:
> The first patch is a bug fix for taking netdev_lock twice in the
> AER code path if the RoCE driver is loaded.  The second patch is a
> refactor patch needed by patch 3.  Patch 3 fixes a packet drop
> issue if queue restart is done on a ring belonging to a non-default
> RSS context.
> 
> Michael Chan (1):
>   bnxt_en: Fix netdev locking in ULP IRQ functions
> 
> [...]

Here is the summary with links:
  - [net,1/3] bnxt_en: Fix netdev locking in ULP IRQ functions
    https://git.kernel.org/netdev/net/c/aed031da7e8c
  - [net,2/3] bnxt_en: Add a helper function to configure MRU and RSS
    (no matching commit)
  - [net,3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset
    (no matching commit)

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

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Michael Chan]

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

On Tue, May 20, 2025 at 7:17 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 20 May 2025 19:10:37 -0700 Michael Chan wrote:
> > They found that this sequence was reliable.
>
> "reliable" is a bit of a big word that some people would reserve
> for code which is production tested or at the very least very
> heavily validated.

FWIW, queue_mgmt_ops was heavily tested by Somnath under heavy traffic
conditions.  Obviously RSS contexts were not included during testing
and this problem was missed.

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

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[David Wei]

On 5/20/25 19:29, Michael Chan wrote:
> On Tue, May 20, 2025 at 7:17 PM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Tue, 20 May 2025 19:10:37 -0700 Michael Chan wrote:
>>> They found that this sequence was reliable.
>>
>> "reliable" is a bit of a big word that some people would reserve
>> for code which is production tested or at the very least very
>> heavily validated.
> 
> FWIW, queue_mgmt_ops was heavily tested by Somnath under heavy traffic
> conditions.  Obviously RSS contexts were not included during testing
> and this problem was missed.

IIRC from the initial testing w/ Somnath even though the VNICs are reset
the traffic on unrelated queues are unaffected. If we ensure that is the
cse with this patchset, would that resolve your concerns Jakub?

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Jakub Kicinski]

On Thu, 22 May 2025 12:01:34 +0100 David Wei wrote:
> On 5/20/25 19:29, Michael Chan wrote:
> > On Tue, May 20, 2025 at 7:17 PM Jakub Kicinski <kuba@kernel.org> wrote:  
> >> "reliable" is a bit of a big word that some people would reserve
> >> for code which is production tested or at the very least very
> >> heavily validated.  
> > 
> > FWIW, queue_mgmt_ops was heavily tested by Somnath under heavy traffic
> > conditions.  Obviously RSS contexts were not included during testing
> > and this problem was missed.  
> 
> IIRC from the initial testing w/ Somnath even though the VNICs are reset
> the traffic on unrelated queues are unaffected.

How did you check that? IIUC the device does not currently report
packet loss due to MRU clamp (!?!)

> If we ensure that is the cse with this patchset, would that resolve
> your concerns Jakub?

For ZC we expect the queues to be taken out of the main context.
IIUC it'd be a significant improvement over the status quo if
we could check which contexts the queue is in (incl. context 0)
and only clamp MRU on those.

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[David Wei]

On 2025-05-22 16:26, Jakub Kicinski wrote:
> On Thu, 22 May 2025 12:01:34 +0100 David Wei wrote:
>> On 5/20/25 19:29, Michael Chan wrote:
>>> On Tue, May 20, 2025 at 7:17 PM Jakub Kicinski <kuba@kernel.org> wrote:
>>>> "reliable" is a bit of a big word that some people would reserve
>>>> for code which is production tested or at the very least very
>>>> heavily validated.
>>>
>>> FWIW, queue_mgmt_ops was heavily tested by Somnath under heavy traffic
>>> conditions.  Obviously RSS contexts were not included during testing
>>> and this problem was missed.
>>
>> IIRC from the initial testing w/ Somnath even though the VNICs are reset
>> the traffic on unrelated queues are unaffected.
> 
> How did you check that? IIUC the device does not currently report
> packet loss due to MRU clamp (!?!)

Only from iperf3. On the server side while it is running, resetting
queues do not affect it. Didn't check for packet drops, though...

> 
>> If we ensure that is the cse with this patchset, would that resolve
>> your concerns Jakub?
> 
> For ZC we expect the queues to be taken out of the main context.
> IIUC it'd be a significant improvement over the status quo if
> we could check which contexts the queue is in (incl. context 0)
> and only clamp MRU on those.

Got it, thanks. Michael, is that something the FW is able to handle
without affecting the queue reset behaviour?

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Michael Chan]

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

On Tue, May 27, 2025 at 8:37 AM David Wei <dw@davidwei.uk> wrote:
>
> On 2025-05-22 16:26, Jakub Kicinski wrote:
> > For ZC we expect the queues to be taken out of the main context.
> > IIUC it'd be a significant improvement over the status quo if
> > we could check which contexts the queue is in (incl. context 0)
> > and only clamp MRU on those.
>
> Got it, thanks. Michael, is that something the FW is able to handle
> without affecting the queue reset behaviour?

We are looking into ways to limit the reset to the VNIC containing the
ring being reset.  If it works, it should address Jakub's concern.

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

[PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Michael Chan]

From: Pavan Chebbi <pavan.chebbi@broadcom.com>

The commit under the Fixes tag below which updates the VNICs' RSS
and MRU during .ndo_queue_start(), needs to be extended to cover any
non-default RSS contexts which have their own VNICs.  Without this
step, packets that are destined to a non-default RSS context may be
dropped after .ndo_queue_start().

We further optimize this scheme by updating the VNIC only if the
RX ring being restarted is in the RSS table of the VNIC.  Updating
the VNIC (in particular setting the MRU to 0) will momentarily stop
all traffic to all rings in the RSS table.  Any VNIC that has the
RX ring excluded from the RSS table can skip this step and avoid the
traffic disruption.

Note that this scheme is just an improvement.  A VNIC with multiple
rings in the RSS table will still see traffic disruptions to all rings
in the RSS table when one of the rings is being restarted.  We are
working on a FW scheme that will improve upon this further.

Fixes: 5ac066b7b062 ("bnxt_en: Fix queue start to update vnic RSS table")
Reported-by: David Wei <dw@davidwei.uk>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
Cc: David Wei <dw@davidwei.uk>

v2: Reduce scope of traffic disruptions.

v1: https://lore.kernel.org/netdev/20250519204130.3097027-4-michael.chan@broadcom.com/
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 56 +++++++++++++++++++++--
 1 file changed, 51 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index dfd2366d4c8c..2cb3185c442c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -10780,11 +10780,39 @@ void bnxt_del_one_rss_ctx(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx,
 	bp->num_rss_ctx--;
 }
 
+static bool bnxt_vnic_has_rx_ring(struct bnxt *bp, struct bnxt_vnic_info *vnic,
+				  int rxr_id)
+{
+	u16 tbl_size = bnxt_get_rxfh_indir_size(bp->dev);
+	int i, vnic_rx;
+
+	/* Ntuple VNIC always has all the rx rings. Any change of ring id
+	 * must be updated because a future filter may use it.
+	 */
+	if (vnic->flags & BNXT_VNIC_NTUPLE_FLAG)
+		return true;
+
+	for (i = 0; i < tbl_size; i++) {
+		if (vnic->flags & BNXT_VNIC_RSSCTX_FLAG)
+			vnic_rx = ethtool_rxfh_context_indir(vnic->rss_ctx)[i];
+		else
+			vnic_rx = bp->rss_indir_tbl[i];
+
+		if (rxr_id == vnic_rx)
+			return true;
+	}
+
+	return false;
+}
+
 static int bnxt_set_vnic_mru_p5(struct bnxt *bp, struct bnxt_vnic_info *vnic,
-				u16 mru)
+				u16 mru, int rxr_id)
 {
 	int rc;
 
+	if (!bnxt_vnic_has_rx_ring(bp, vnic, rxr_id))
+		return 0;
+
 	if (mru) {
 		rc = bnxt_hwrm_vnic_set_rss_p5(bp, vnic, true);
 		if (rc) {
@@ -10800,6 +10828,24 @@ static int bnxt_set_vnic_mru_p5(struct bnxt *bp, struct bnxt_vnic_info *vnic,
 	return 0;
 }
 
+static int bnxt_set_rss_ctx_vnic_mru(struct bnxt *bp, u16 mru, int rxr_id)
+{
+	struct ethtool_rxfh_context *ctx;
+	unsigned long context;
+	int rc;
+
+	xa_for_each(&bp->dev->ethtool->rss_ctx, context, ctx) {
+		struct bnxt_rss_ctx *rss_ctx = ethtool_rxfh_context_priv(ctx);
+		struct bnxt_vnic_info *vnic = &rss_ctx->vnic;
+
+		rc = bnxt_set_vnic_mru_p5(bp, vnic, mru, rxr_id);
+		if (rc)
+			return rc;
+	}
+
+	return 0;
+}
+
 static void bnxt_hwrm_realloc_rss_ctx_vnic(struct bnxt *bp)
 {
 	bool set_tpa = !!(bp->flags & BNXT_FLAG_TPA);
@@ -16002,12 +16048,11 @@ static int bnxt_queue_start(struct net_device *dev, void *qmem, int idx)
 	for (i = 0; i < bp->nr_vnics; i++) {
 		vnic = &bp->vnic_info[i];
 
-		rc = bnxt_set_vnic_mru_p5(bp, vnic, mru);
+		rc = bnxt_set_vnic_mru_p5(bp, vnic, mru, idx);
 		if (rc)
 			return rc;
 	}
-
-	return 0;
+	return bnxt_set_rss_ctx_vnic_mru(bp, mru, idx);
 
 err_reset:
 	netdev_err(bp->dev, "Unexpected HWRM error during queue start rc: %d\n",
@@ -16030,8 +16075,9 @@ static int bnxt_queue_stop(struct net_device *dev, void *qmem, int idx)
 	for (i = 0; i < bp->nr_vnics; i++) {
 		vnic = &bp->vnic_info[i];
 
-		bnxt_set_vnic_mru_p5(bp, vnic, 0);
+		bnxt_set_vnic_mru_p5(bp, vnic, 0, idx);
 	}
+	bnxt_set_rss_ctx_vnic_mru(bp, 0, idx);
 	/* Make sure NAPI sees that the VNIC is disabled */
 	synchronize_net();
 	rxr = &bp->rx_ring[idx];
-- 
2.30.1

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Simon Horman]

On Fri, Jun 13, 2025 at 04:18:41PM -0700, Michael Chan wrote:
> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> 
> The commit under the Fixes tag below which updates the VNICs' RSS
> and MRU during .ndo_queue_start(), needs to be extended to cover any
> non-default RSS contexts which have their own VNICs.  Without this
> step, packets that are destined to a non-default RSS context may be
> dropped after .ndo_queue_start().

Hi,

This patch seems to be doing two things:
1. Addressing the bug described above
2. Implementing the optimisation below

As such I think it would be best split into two patches.
And I'd lean towards targeting the optimisation at net-next
since "this scheme is just an improvement".

> 
> We further optimize this scheme by updating the VNIC only if the
> RX ring being restarted is in the RSS table of the VNIC.  Updating
> the VNIC (in particular setting the MRU to 0) will momentarily stop
> all traffic to all rings in the RSS table.  Any VNIC that has the
> RX ring excluded from the RSS table can skip this step and avoid the
> traffic disruption.
> 
> Note that this scheme is just an improvement.  A VNIC with multiple
> rings in the RSS table will still see traffic disruptions to all rings
> in the RSS table when one of the rings is being restarted.  We are
> working on a FW scheme that will improve upon this further.
> 
> Fixes: 5ac066b7b062 ("bnxt_en: Fix queue start to update vnic RSS table")
> Reported-by: David Wei <dw@davidwei.uk>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

...

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Michael Chan]

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

On Mon, Jun 16, 2025 at 6:39 AM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Jun 13, 2025 at 04:18:41PM -0700, Michael Chan wrote:
> > From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> >
> > The commit under the Fixes tag below which updates the VNICs' RSS
> > and MRU during .ndo_queue_start(), needs to be extended to cover any
> > non-default RSS contexts which have their own VNICs.  Without this
> > step, packets that are destined to a non-default RSS context may be
> > dropped after .ndo_queue_start().
>
> Hi,
>
> This patch seems to be doing two things:
> 1. Addressing the bug described above
> 2. Implementing the optimisation below
>
> As such I think it would be best split into two patches.
> And I'd lean towards targeting the optimisation at net-next
> since "this scheme is just an improvement".

The original fix (without the improvement) was rejected by Jakub and
that's why we are improving it here.

Jakub, what do you think?

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

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Jakub Kicinski]

On Mon, 16 Jun 2025 10:40:27 -0700 Michael Chan wrote:
> On Mon, Jun 16, 2025 at 6:39 AM Simon Horman <horms@kernel.org> wrote:
> > On Fri, Jun 13, 2025 at 04:18:41PM -0700, Michael Chan wrote:  
> > > From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> > >
> > > The commit under the Fixes tag below which updates the VNICs' RSS
> > > and MRU during .ndo_queue_start(), needs to be extended to cover any
> > > non-default RSS contexts which have their own VNICs.  Without this
> > > step, packets that are destined to a non-default RSS context may be
> > > dropped after .ndo_queue_start().  
> >
> > This patch seems to be doing two things:
> > 1. Addressing the bug described above
> > 2. Implementing the optimisation below
> >
> > As such I think it would be best split into two patches.
> > And I'd lean towards targeting the optimisation at net-next
> > since "this scheme is just an improvement".  
> 
> The original fix (without the improvement) was rejected by Jakub and
> that's why we are improving it here.
> 
> Jakub, what do you think?

I think the phrasing of the commit message could be better, but the fix
is correct as is. We were shutting down just the main vNIC, now we shut
down all the vNICs to which the queue belongs.

It's not an "optimization" in the sense of an improvement to status quo,
IIUC Pavan means that shutting down the vNIC is still not 100% correct
for single queue reset, but best we can do with current FW. If we were
to split this into 2 changes, I don't think those changes would form a
logical progression: reset vNIC 0 (current) -> reset all (net) -> reset
the correct set of vNICs (net-next).. ?

I'd chalk this up to people writing sassy / opinion-tainted commit
messages after reviewers disagree with their initial implementation. 
I tend not to fight it :)

Re: [PATCH net 3/3] bnxt_en: Update MRU and RSS table of RSS contexts on queue reset

[Michael Chan]

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

On Mon, Jun 16, 2025 at 1:48 PM Jakub Kicinski <kuba@kernel.org> wrote:

> I think the phrasing of the commit message could be better, but the fix
> is correct as is. We were shutting down just the main vNIC, now we shut
> down all the vNICs to which the queue belongs.
>
> It's not an "optimization" in the sense of an improvement to status quo,
> IIUC Pavan means that shutting down the vNIC is still not 100% correct
> for single queue reset, but best we can do with current FW.

Correct.  This is the best we can do at the moment to limit the scope
of the reset.  In the future with new FW, we should be able to limit
the traffic disruption to only the queue being reset for any vNIC.

> If we were
> to split this into 2 changes, I don't think those changes would form a
> logical progression: reset vNIC 0 (current) -> reset all (net) -> reset
> the correct set of vNICs (net-next).. ?
>

Agreed.  This progression is not ideal.

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