[PATCH net v2 0/2] bnge fixes

[PATCH net v2 0/2] bnge fixes

[Vikas Gupta]

Hi,
 This series fix two issues.

 Patch-1: 
    Due to wrong HWRM sequence, driver do not get the correct
    information regarding resources and capabilities.
    The patch fixes the initial HWRM sequence.
Patch-2:
    Remove the unsupported backing store type initialization, which is
    not supported in Thor Ultra devices.

Thanks,
Vikas

v1->v2: 
   Include Fixes tags.


Vikas Gupta (2):
  bnge: fix initial HWRM sequence
  bnge: remove unsupported backing store type

 .../net/ethernet/broadcom/bnge/bnge_core.c    | 39 ++++++++++---------
 .../net/ethernet/broadcom/bnge/bnge_rmem.c    | 16 --------
 2 files changed, 21 insertions(+), 34 deletions(-)

-- 
2.47.1

[PATCH net v2 1/2] bnge: fix initial HWRM sequence

[Vikas Gupta]

Firmware may not advertize correct resources if backing store is not
enabled before resource information is queried.
Fix the initial sequence of HWRMs so that driver gets capabilities
and resource information correctly.

Fixes: 3fa9e977a0cd ("bng_en: Initialize default configuration")
Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
Reviewed-by: Rahul Gupta <rahul-rg.gupta@broadcom.com>
---
 .../net/ethernet/broadcom/bnge/bnge_core.c    | 39 ++++++++++---------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_core.c b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
index b4090283df0f..9f6a33b912a6 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_core.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
@@ -73,25 +73,35 @@ static int bnge_func_qcaps(struct bnge_dev *bd)
 		return rc;
 	}
 
+	rc = bnge_alloc_ctx_mem(bd);
+	if (rc) {
+		dev_err(bd->dev, "Failed to allocate ctx mem rc: %d\n", rc);
+		goto err_free_ctx_mem;
+	}
+
 	rc = bnge_hwrm_func_resc_qcaps(bd);
 	if (rc) {
 		dev_err(bd->dev, "query resc caps failure rc: %d\n", rc);
-		return rc;
+		goto err_free_ctx_mem;
 	}
 
 	rc = bnge_hwrm_func_qcfg(bd);
 	if (rc) {
 		dev_err(bd->dev, "query config failure rc: %d\n", rc);
-		return rc;
+		goto err_free_ctx_mem;
 	}
 
 	rc = bnge_hwrm_vnic_qcaps(bd);
 	if (rc) {
 		dev_err(bd->dev, "vnic caps failure rc: %d\n", rc);
-		return rc;
+		goto err_free_ctx_mem;
 	}
 
 	return 0;
+
+err_free_ctx_mem:
+	bnge_free_ctx_mem(bd);
+	return rc;
 }
 
 static void bnge_fw_unregister_dev(struct bnge_dev *bd)
@@ -132,32 +142,25 @@ static int bnge_fw_register_dev(struct bnge_dev *bd)
 
 	bnge_hwrm_fw_set_time(bd);
 
-	rc =  bnge_hwrm_func_drv_rgtr(bd);
+	/* Get the resources and configuration from firmware */
+	rc = bnge_func_qcaps(bd);
 	if (rc) {
-		dev_err(bd->dev, "Failed to rgtr with firmware rc: %d\n", rc);
+		dev_err(bd->dev, "Failed initial configuration rc: %d\n", rc);
 		return rc;
 	}
 
-	rc = bnge_alloc_ctx_mem(bd);
+	rc = bnge_hwrm_func_drv_rgtr(bd);
 	if (rc) {
-		dev_err(bd->dev, "Failed to allocate ctx mem rc: %d\n", rc);
-		goto err_func_unrgtr;
-	}
-
-	/* Get the resources and configuration from firmware */
-	rc = bnge_func_qcaps(bd);
-	if (rc) {
-		dev_err(bd->dev, "Failed initial configuration rc: %d\n", rc);
-		rc = -ENODEV;
-		goto err_func_unrgtr;
+		dev_err(bd->dev, "Failed to rgtr with firmware rc: %d\n", rc);
+		goto err_free_ctx_mem;
 	}
 
 	bnge_set_dflt_rss_hash_type(bd);
 
 	return 0;
 
-err_func_unrgtr:
-	bnge_fw_unregister_dev(bd);
+err_free_ctx_mem:
+	bnge_free_ctx_mem(bd);
 	return rc;
 }
 
-- 
2.47.1

Re: [PATCH net v2 1/2] bnge: fix initial HWRM sequence

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
bnge: fix initial HWRM sequence

This commit modifies the initial HWRM sequence to allocate the context
memory before querying resource information. This addresses an issue where
firmware might not advertise correct resources.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_core.c b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> @@ -73,25 +73,35 @@ static int bnge_func_qcaps(struct bnge_dev *bd)

[ ... ]

>  	return 0;
> +
> +err_free_ctx_mem:
> +	bnge_free_ctx_mem(bd);
> +	return rc;
>  }

Could this error path leave the firmware with active DMA pointers to freed
memory?

Looking at bnge_alloc_ctx_mem(), it configures the firmware with host DMA
addresses for the backing store via bnge_backing_store_cfg(). If a
subsequent step like bnge_hwrm_func_resc_qcaps() fails, this new error path
directly frees the context memory.

Previously, errors in this sequence triggered bnge_fw_unregister_dev(),
which safely unregistered the firmware before freeing the memory. There is
even a comment in bnge_fw_unregister_dev() documenting this requirement:

static void bnge_fw_unregister_dev(struct bnge_dev *bd)
{
        /* ctx mem free after unrgtr only */
        bnge_hwrm_func_drv_unrgtr(bd);
        bnge_free_ctx_mem(bd);
}

Since the firmware isn't told to stop using the memory before it is freed
here, is there a risk the hardware might DMA into the freed host pages?

> @@ -132,32 +142,25 @@ static int bnge_fw_register_dev(struct bnge_dev *bd)

[ ... ]

> -err_func_unrgtr:
> -	bnge_fw_unregister_dev(bd);
> +err_free_ctx_mem:
> +	bnge_free_ctx_mem(bd);
>  	return rc;
>  }

This error path appears to have the same regression. If
bnge_hwrm_func_drv_rgtr() fails after bnge_func_qcaps() has already
configured the backing store, freeing the context memory directly without
unregistering might allow the hardware to access freed memory.
-- 
pw-bot: cr

Re: [PATCH net v2 1/2] bnge: fix initial HWRM sequence

[Vikas Gupta]

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

> > @@ -73,25 +73,35 @@ static int bnge_func_qcaps(struct bnge_dev *bd)
>
> [ ... ]
>
> >       return 0;
> > +
> > +err_free_ctx_mem:
> > +     bnge_free_ctx_mem(bd);
> > +     return rc;
> >  }
>
> Could this error path leave the firmware with active DMA pointers to freed
> memory?
>
> Looking at bnge_alloc_ctx_mem(), it configures the firmware with host DMA
> addresses for the backing store via bnge_backing_store_cfg(). If a
> subsequent step like bnge_hwrm_func_resc_qcaps() fails, this new error path
> directly frees the context memory.

HW does not access the context memory unless rings are active (traffic
is running).
Therefore, It is safe to free the memory on the host.

>
> Previously, errors in this sequence triggered bnge_fw_unregister_dev(),
> which safely unregistered the firmware before freeing the memory. There is
> even a comment in bnge_fw_unregister_dev() documenting this requirement:

bnge_fw_unregister_dev() is called in both success and failure scenarios.
Therefore, free context memory using bnge_free_ctx_mem() after
bnge_hwrm_func_drv_unrgtr() ensures that the context memory is invalidated
at the hardware level.

>
> static void bnge_fw_unregister_dev(struct bnge_dev *bd)
> {
>         /* ctx mem free after unrgtr only */
>         bnge_hwrm_func_drv_unrgtr(bd);
>         bnge_free_ctx_mem(bd);
> }
>
> Since the firmware isn't told to stop using the memory before it is freed
> here, is there a risk the hardware might DMA into the freed host pages?

There is no communication mechanism to inform the firmware to free the
context memory.

>
> > @@ -132,32 +142,25 @@ static int bnge_fw_register_dev(struct bnge_dev *bd)
>
> [ ... ]
>
> > -err_func_unrgtr:
> > -     bnge_fw_unregister_dev(bd);
> > +err_free_ctx_mem:
> > +     bnge_free_ctx_mem(bd);
> >       return rc;
> >  }
>
> This error path appears to have the same regression. If
> bnge_hwrm_func_drv_rgtr() fails after bnge_func_qcaps() has already
> configured the backing store, freeing the context memory directly without
> unregistering might allow the hardware to access freed memory.

Even if bnge_hwrm_func_drv_rgtr() fails, it is still safe to free the context
memory at the host because the driver unloads from this point.

AI reviews appear to ignore logic related to handling context memory
in the patch.
I see no valid comments on the patch.

Thanks,
Vikas

> --
> pw-bot: cr

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

Re: [PATCH net v2 1/2] bnge: fix initial HWRM sequence

[Jakub Kicinski]

On Fri, 17 Apr 2026 11:46:08 +0530 Vikas Gupta wrote:
> > > -err_func_unrgtr:
> > > -     bnge_fw_unregister_dev(bd);
> > > +err_free_ctx_mem:
> > > +     bnge_free_ctx_mem(bd);
> > >       return rc;
> > >  }  
> >
> > This error path appears to have the same regression. If
> > bnge_hwrm_func_drv_rgtr() fails after bnge_func_qcaps() has already
> > configured the backing store, freeing the context memory directly without
> > unregistering might allow the hardware to access freed memory.  
> 
> Even if bnge_hwrm_func_drv_rgtr() fails, it is still safe to free the context
> memory at the host because the driver unloads from this point.

Looking closer, indeed, the way bnge_hwrm_func_drv_unrgtr() is written
the AI suggestion is pointless. Hopefully you're right cause debugging
FW corrupting host memory after reboot on bnxt is not fun.

> AI reviews appear to ignore logic related to handling context memory
> in the patch.
> I see no valid comments on the patch.

Why is bnge_func_qcaps() allocating context mem? It may be the case
that context mem has to be allocated but bnge_func_qcaps() doesn't
sound like a function that'd perform such key part of init.
Why not just move the alloc earlier in bnge_fw_register_dev() ?

Re: [PATCH net v2 1/2] bnge: fix initial HWRM sequence

[Vikas Gupta]

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

On Fri, Apr 17, 2026 at 8:12 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 17 Apr 2026 11:46:08 +0530 Vikas Gupta wrote:
> > > > -err_func_unrgtr:
> > > > -     bnge_fw_unregister_dev(bd);
> > > > +err_free_ctx_mem:
> > > > +     bnge_free_ctx_mem(bd);
> > > >       return rc;
> > > >  }
> > >
> > > This error path appears to have the same regression. If
> > > bnge_hwrm_func_drv_rgtr() fails after bnge_func_qcaps() has already
> > > configured the backing store, freeing the context memory directly without
> > > unregistering might allow the hardware to access freed memory.
> >
> > Even if bnge_hwrm_func_drv_rgtr() fails, it is still safe to free the context
> > memory at the host because the driver unloads from this point.
>
> Looking closer, indeed, the way bnge_hwrm_func_drv_unrgtr() is written
> the AI suggestion is pointless. Hopefully you're right cause debugging
> FW corrupting host memory after reboot on bnxt is not fun.
>
> > AI reviews appear to ignore logic related to handling context memory
> > in the patch.
> > I see no valid comments on the patch.
>
> Why is bnge_func_qcaps() allocating context mem? It may be the case
> that context mem has to be allocated but bnge_func_qcaps() doesn't
> sound like a function that'd perform such key part of init.
> Why not just move the alloc earlier in bnge_fw_register_dev() ?

I agree that bnge_func_qcaps(), which appears to be a query function,
should not allocate memory. I can refactor bnge_func_qcaps() and
move bnge_alloc_ctx_mem() to bnge_fw_register_dev() so that
bnge_func_qcaps() remains solely a query function.
I`ll make changes in v3.

Thanks,
Vikas

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

[PATCH net v2 2/2] bnge: remove unsupported backing store type

[Vikas Gupta]

The backing store type, BNGE_CTX_MRAV, is not applicable in Thor Ultra
devices. Remove it from the backing store configuration, as the firmware
will not populate entities in this backing store type, due to which the
driver load fails.

Fixes: 29c5b358f385 ("bng_en: Add backing store support")
Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
Reviewed-by: Dharmender Garg <dharmender.garg@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnge/bnge_rmem.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c b/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
index 94f15e08a88c..b066ee887a09 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
@@ -324,7 +324,6 @@ int bnge_alloc_ctx_mem(struct bnge_dev *bd)
 	u32 l2_qps, qp1_qps, max_qps;
 	u32 ena, entries_sp, entries;
 	u32 srqs, max_srqs, min;
-	u32 num_mr, num_ah;
 	u32 extra_srqs = 0;
 	u32 extra_qps = 0;
 	u32 fast_qpmd_qps;
@@ -390,21 +389,6 @@ int bnge_alloc_ctx_mem(struct bnge_dev *bd)
 	if (!bnge_is_roce_en(bd))
 		goto skip_rdma;
 
-	ctxm = &ctx->ctx_arr[BNGE_CTX_MRAV];
-	/* 128K extra is needed to accommodate static AH context
-	 * allocation by f/w.
-	 */
-	num_mr = min_t(u32, ctxm->max_entries / 2, 1024 * 256);
-	num_ah = min_t(u32, num_mr, 1024 * 128);
-	ctxm->split_entry_cnt = BNGE_CTX_MRAV_AV_SPLIT_ENTRY + 1;
-	if (!ctxm->mrav_av_entries || ctxm->mrav_av_entries > num_ah)
-		ctxm->mrav_av_entries = num_ah;
-
-	rc = bnge_setup_ctxm_pg_tbls(bd, ctxm, num_mr + num_ah, 2);
-	if (rc)
-		return rc;
-	ena |= FUNC_BACKING_STORE_CFG_REQ_ENABLES_MRAV;
-
 	ctxm = &ctx->ctx_arr[BNGE_CTX_TIM];
 	rc = bnge_setup_ctxm_pg_tbls(bd, ctxm, l2_qps + qp1_qps + extra_qps, 1);
 	if (rc)
-- 
2.47.1

Re: [PATCH net v2 2/2] bnge: remove unsupported backing store type

[Przemek Kitszel]

On 4/15/26 17:16, Vikas Gupta wrote:
> The backing store type, BNGE_CTX_MRAV, is not applicable in Thor Ultra
> devices. Remove it from the backing store configuration, as the firmware

I guess the removed code was needed for previous devices, what is the
impact for them?

> will not populate entities in this backing store type, due to which the
> driver load fails.
> 
> Fixes: 29c5b358f385 ("bng_en: Add backing store support")
> Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
> Reviewed-by: Dharmender Garg <dharmender.garg@broadcom.com>
> ---
>   drivers/net/ethernet/broadcom/bnge/bnge_rmem.c | 16 ----------------
>   1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c b/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
> index 94f15e08a88c..b066ee887a09 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
> @@ -324,7 +324,6 @@ int bnge_alloc_ctx_mem(struct bnge_dev *bd)
>   	u32 l2_qps, qp1_qps, max_qps;
>   	u32 ena, entries_sp, entries;
>   	u32 srqs, max_srqs, min;
> -	u32 num_mr, num_ah;
>   	u32 extra_srqs = 0;
>   	u32 extra_qps = 0;
>   	u32 fast_qpmd_qps;
> @@ -390,21 +389,6 @@ int bnge_alloc_ctx_mem(struct bnge_dev *bd)
>   	if (!bnge_is_roce_en(bd))
>   		goto skip_rdma;
>   
> -	ctxm = &ctx->ctx_arr[BNGE_CTX_MRAV];
> -	/* 128K extra is needed to accommodate static AH context
> -	 * allocation by f/w.
> -	 */
> -	num_mr = min_t(u32, ctxm->max_entries / 2, 1024 * 256);
> -	num_ah = min_t(u32, num_mr, 1024 * 128);
> -	ctxm->split_entry_cnt = BNGE_CTX_MRAV_AV_SPLIT_ENTRY + 1;
> -	if (!ctxm->mrav_av_entries || ctxm->mrav_av_entries > num_ah)
> -		ctxm->mrav_av_entries = num_ah;
> -
> -	rc = bnge_setup_ctxm_pg_tbls(bd, ctxm, num_mr + num_ah, 2);
> -	if (rc)
> -		return rc;
> -	ena |= FUNC_BACKING_STORE_CFG_REQ_ENABLES_MRAV;
> -
>   	ctxm = &ctx->ctx_arr[BNGE_CTX_TIM];
>   	rc = bnge_setup_ctxm_pg_tbls(bd, ctxm, l2_qps + qp1_qps + extra_qps, 1);
>   	if (rc)

Re: [PATCH net v2 2/2] bnge: remove unsupported backing store type

[Vikas Gupta]

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

On Thu, Apr 16, 2026 at 9:24 AM Przemek Kitszel
<przemyslaw.kitszel@intel.com> wrote:
>
> On 4/15/26 17:16, Vikas Gupta wrote:
> > The backing store type, BNGE_CTX_MRAV, is not applicable in Thor Ultra
> > devices. Remove it from the backing store configuration, as the firmware
>
> I guess the removed code was needed for previous devices, what is the
> impact for them?

This driver does not support previous devices. Thor Ultra devices have
split MRAV
into two separate contexts, MR and AV. Support for them will be added
in a future
patch series.

>
> > will not populate entities in this backing store type, due to which the
> > driver load fails.
> >
> > Fixes: 29c5b358f385 ("bng_en: Add backing store support")
> > Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
> > Reviewed-by: Dharmender Garg <dharmender.garg@broadcom.com>
> > ---
> >   drivers/net/ethernet/broadcom/bnge/bnge_rmem.c | 16 ----------------
> >   1 file changed, 16 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c b/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
> > index 94f15e08a88c..b066ee887a09 100644
> > --- a/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
> > +++ b/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
> > @@ -324,7 +324,6 @@ int bnge_alloc_ctx_mem(struct bnge_dev *bd)
> >       u32 l2_qps, qp1_qps, max_qps;
> >       u32 ena, entries_sp, entries;
> >       u32 srqs, max_srqs, min;
> > -     u32 num_mr, num_ah;
> >       u32 extra_srqs = 0;
> >       u32 extra_qps = 0;
> >       u32 fast_qpmd_qps;
> > @@ -390,21 +389,6 @@ int bnge_alloc_ctx_mem(struct bnge_dev *bd)
> >       if (!bnge_is_roce_en(bd))
> >               goto skip_rdma;
> >
> > -     ctxm = &ctx->ctx_arr[BNGE_CTX_MRAV];
> > -     /* 128K extra is needed to accommodate static AH context
> > -      * allocation by f/w.
> > -      */
> > -     num_mr = min_t(u32, ctxm->max_entries / 2, 1024 * 256);
> > -     num_ah = min_t(u32, num_mr, 1024 * 128);
> > -     ctxm->split_entry_cnt = BNGE_CTX_MRAV_AV_SPLIT_ENTRY + 1;
> > -     if (!ctxm->mrav_av_entries || ctxm->mrav_av_entries > num_ah)
> > -             ctxm->mrav_av_entries = num_ah;
> > -
> > -     rc = bnge_setup_ctxm_pg_tbls(bd, ctxm, num_mr + num_ah, 2);
> > -     if (rc)
> > -             return rc;
> > -     ena |= FUNC_BACKING_STORE_CFG_REQ_ENABLES_MRAV;
> > -
> >       ctxm = &ctx->ctx_arr[BNGE_CTX_TIM];
> >       rc = bnge_setup_ctxm_pg_tbls(bd, ctxm, l2_qps + qp1_qps + extra_qps, 1);
> >       if (rc)
>

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