[PATCH net v3 0/2] bnge fixes

[PATCH net v3 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

v2->v3:
  Addressed Jakub Kicinski's comments.
https://lore.kernel.org/netdev/CAHLZf_uARgZzoTPnnPjxRu5AGeHEOw3yyTEbNHYP3brfwuW0Sw@mail.gmail.com/

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    | 30 ++++++++++++++-----
 .../net/ethernet/broadcom/bnge/bnge_rmem.c    | 16 ----------
 2 files changed, 22 insertions(+), 24 deletions(-)

-- 
2.47.1

[PATCH net v3 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    | 30 ++++++++++++++-----
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_core.c b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
index 1c14c5fe8d61..68b74eb2c3a2 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_core.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
@@ -74,6 +74,13 @@ static int bnge_func_qcaps(struct bnge_dev *bd)
 		return rc;
 	}
 
+	return 0;
+}
+
+static int bnge_func_qrcaps_qcfg(struct bnge_dev *bd)
+{
+	int rc;
+
 	rc = bnge_hwrm_func_resc_qcaps(bd);
 	if (rc) {
 		dev_err(bd->dev, "query resc caps failure rc: %d\n", rc);
@@ -133,23 +140,28 @@ 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 querying caps rc: %d\n", rc);
 		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_func_unrgtr;
+		goto err_free_ctx_mem;
 	}
 
-	/* Get the resources and configuration from firmware */
-	rc = bnge_func_qcaps(bd);
+	rc = bnge_hwrm_func_drv_rgtr(bd);
 	if (rc) {
-		dev_err(bd->dev, "Failed initial configuration rc: %d\n", rc);
-		rc = -ENODEV;
+		dev_err(bd->dev, "Failed to rgtr with firmware rc: %d\n", rc);
+		goto err_free_ctx_mem;
+	}
+
+	rc = bnge_func_qrcaps_qcfg(bd);
+	if (rc) {
+		dev_err(bd->dev, "Failed querying resources rc: %d\n", rc);
 		goto err_func_unrgtr;
 	}
 
@@ -158,7 +170,9 @@ static int bnge_fw_register_dev(struct bnge_dev *bd)
 	return 0;
 
 err_func_unrgtr:
-	bnge_fw_unregister_dev(bd);
+	bnge_hwrm_func_drv_unrgtr(bd);
+err_free_ctx_mem:
+	bnge_free_ctx_mem(bd);
 	return rc;
 }
 
-- 
2.47.1

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

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

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

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.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_core.c b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> index 1c14c5fe8d613..68b74eb2c3a29 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
[ ... ]
> @@ -133,23 +140,28 @@ 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 querying caps rc: %d\n", rc);
>  		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_func_unrgtr;
> +		goto err_free_ctx_mem;
>  	}
>  
> -	/* Get the resources and configuration from firmware */
> -	rc = bnge_func_qcaps(bd);
> +	rc = bnge_hwrm_func_drv_rgtr(bd);
>  	if (rc) {
> -		dev_err(bd->dev, "Failed initial configuration rc: %d\n", rc);
> -		rc = -ENODEV;
> +		dev_err(bd->dev, "Failed to rgtr with firmware rc: %d\n", rc);
> +		goto err_free_ctx_mem;

Does this regression leave the hardware with active DMA pointers to freed
memory?

If bnge_alloc_ctx_mem() allocates host DMA memory and passes the addresses
to the firmware via bnge_hwrm_func_backing_store(), and then
bnge_hwrm_func_drv_rgtr() fails, the code jumps directly to err_free_ctx_mem.

> +	}
> +
> +	rc = bnge_func_qrcaps_qcfg(bd);
> +	if (rc) {
> +		dev_err(bd->dev, "Failed querying resources rc: %d\n", rc);
>  		goto err_func_unrgtr;
>  	}
>  
> @@ -158,7 +170,9 @@ static int bnge_fw_register_dev(struct bnge_dev *bd)
>  	return 0;
>  
>  err_func_unrgtr:
> -	bnge_fw_unregister_dev(bd);
> +	bnge_hwrm_func_drv_unrgtr(bd);
> +err_free_ctx_mem:
> +	bnge_free_ctx_mem(bd);

Since the err_func_unrgtr label is bypassed, is the firmware ever told to
drop the backing store configuration before the host frees the memory in
bnge_free_ctx_mem()?

In the pre-patch sequence, the driver was registered first, so any failure
subsequently triggered a driver unregister, which implicitly told the
firmware to drop the backing store configuration. With the new order, does
the firmware maintain active DMA pointers to the freed host memory?

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

[Vikas Gupta]

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

Hi Simon,
We have discussed the AI review comments on the behaviour of
bnge_alloc_ctx_mem().
Please review the explanation and let me know if it does not provide
sufficient clarity.

The discussion on v2 is available at:
https://lore.kernel.org/netdev/CAHLZf_uARgZzoTPnnPjxRu5AGeHEOw3yyTEbNHYP3brfwuW0Sw@mail.gmail.com/
https://lore.kernel.org/netdev/CAHLZf_voEPTrfkuO6pJcchaQaOqJin8m7j-+hwMrjJcGFmJv0A@mail.gmail.com/

Thanks,
Vikas

On Wed, Apr 22, 2026 at 10:36 PM Simon Horman <horms@kernel.org> wrote:
>
> From: 'Simon Horman' <horms@kernel.org>
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> bnge: fix initial HWRM sequence
>
> 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.
>
> > diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_core.c b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> > index 1c14c5fe8d613..68b74eb2c3a29 100644
> > --- a/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> > +++ b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> [ ... ]
> > @@ -133,23 +140,28 @@ 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 querying caps rc: %d\n", rc);
> >               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_func_unrgtr;
> > +             goto err_free_ctx_mem;
> >       }
> >
> > -     /* Get the resources and configuration from firmware */
> > -     rc = bnge_func_qcaps(bd);
> > +     rc = bnge_hwrm_func_drv_rgtr(bd);
> >       if (rc) {
> > -             dev_err(bd->dev, "Failed initial configuration rc: %d\n", rc);
> > -             rc = -ENODEV;
> > +             dev_err(bd->dev, "Failed to rgtr with firmware rc: %d\n", rc);
> > +             goto err_free_ctx_mem;
>
> Does this regression leave the hardware with active DMA pointers to freed
> memory?
>
> If bnge_alloc_ctx_mem() allocates host DMA memory and passes the addresses
> to the firmware via bnge_hwrm_func_backing_store(), and then
> bnge_hwrm_func_drv_rgtr() fails, the code jumps directly to err_free_ctx_mem.
>
> > +     }
> > +
> > +     rc = bnge_func_qrcaps_qcfg(bd);
> > +     if (rc) {
> > +             dev_err(bd->dev, "Failed querying resources rc: %d\n", rc);
> >               goto err_func_unrgtr;
> >       }
> >
> > @@ -158,7 +170,9 @@ static int bnge_fw_register_dev(struct bnge_dev *bd)
> >       return 0;
> >
> >  err_func_unrgtr:
> > -     bnge_fw_unregister_dev(bd);
> > +     bnge_hwrm_func_drv_unrgtr(bd);
> > +err_free_ctx_mem:
> > +     bnge_free_ctx_mem(bd);
>
> Since the err_func_unrgtr label is bypassed, is the firmware ever told to
> drop the backing store configuration before the host frees the memory in
> bnge_free_ctx_mem()?
>
> In the pre-patch sequence, the driver was registered first, so any failure
> subsequently triggered a driver unregister, which implicitly told the
> firmware to drop the backing store configuration. With the new order, does
> the firmware maintain active DMA pointers to the freed host memory?

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

[PATCH net v3 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