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

[Vikas Gupta]

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@xxxxxxxxxxxxxx/
https://lore.kernel.org/netdev/CAHLZf_voEPTrfkuO6pJcchaQaOqJin8m7j-+hwMrjJcGFmJv0A@xxxxxxxxxxxxxx/

Thanks,
Vikas

On Wed, Apr 22, 2026 at 10:36 PM Simon Horman <horms@xxxxxxxxxx> wrote:
>
> From: 'Simon Horman' <horms@xxxxxxxxxx>
>
> 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: smime.p7s
Description: S/MIME Cryptographic Signature