[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

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

[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

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

[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 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 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 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 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 --]

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