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

Vikas Gupta
2026-04-22T18:11:40Z

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