Re: [PATCH net-next v3 5/5] bnge: make NQ0 persistent across open/stop cycles

[Bhargava Chenna Marreddy]

On Sun, Oct 18, 2026 at 9:02 PM Bhargava Chenna Marreddy
<bhargava.marreddy@xxxxxxxxxxxx> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
>
> Source: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013200645.1346299-1-bhargava.marreddy@xxxxxxxxxxxx
>
> On Wed, Oct 14, 2026 at 1:38 AM Bhargava Marreddy
> <bhargava.marreddy@xxxxxxxxxxxx> wrote:
> > diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> > index dc6cf3525034..c6649a2c5876 100644
> > --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> > +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> > @@ -1209,10 +1209,20 @@ static int bnge_init_ring_grps(struct bnge_net *bn)
> >                 bn->grp_info[i].agg_fw_ring_id = INVALID_HW_RING_ID;
> >                 bn->grp_info[i].nq_fw_ring_id = INVALID_HW_RING_ID;
> >         }
> > +       /* NQ0 stays allocated in FW across open/close */
> > +       bn->grp_info[0].nq_fw_ring_id =
> > +               (u16)bn->nq0->nq_ring.ring_struct.fw_ring_id;
>
> Discovered by: gpt-6-astra, opus-5-5 · Ranked Medium by: gpt-6-astra
>
> bnge_nq0_init() now allocates NQ0 in firmware and sets it as the async
> event completion ring only once. After that, every open reuses the
> probe-time fw_ring_id here, and bnge_nq0_resume() re-arms the cached NQ0
> doorbell.
>
> On the way back up, bnge_hwrm_if_change(bd, true) returns only rc and
> does not check any response flags:
>
>     if (up)
>         req->flags = cpu_to_le32(FUNC_DRV_IF_CHANGE_REQ_FLAGS_UP);
>     return bnge_hwrm_req_send(bd, req);
>
> Suppose firmware resets or reclaims the function's NQ while the
> interface is down. Would queue 0's completion ring then be allocated
> against a stale nq_fw_ring_id? NQ0 doorbell writes would target a ring
> firmware no longer owns, and async events would stop with no error.
>
> The commit message acknowledges this ("Recreating NQ0 after a firmware
> reset is not handled here"). Other probe-only firmware state would also
> be stale in that case, such as the context memory from
> bnge_alloc_ctx_mem() and the registration from
> bnge_hwrm_func_drv_rgtr(). So the driver does not handle a firmware
> reset while down today either.
>
> Does firmware guarantee that it keeps NQ0 and the async event CR
> configuration across FUNC_DRV_IF_CHANGE down/up?

Yes. Firmware does not touch NQ0 or async event support across
FUNC_DRV_IF_CHANGE down/up transitions, so the fw_ring_id cached during probe
remains valid across open/stop cycles.

A full firmware reset is a different case. NQ0, context memory, and driver
registration would all need to be recreated—which the driver does not support
today, with or without this patch. That recovery path is out of scope for this
series, as noted in the commit message.

>
> > +
> > +       rc = bnge_hwrm_set_async_event_cr(bd, nqr->ring_struct.fw_ring_id);
> > +       if (rc) {
> > +               bnge_err(bd, "Failed to set async event completion ring: %d\n",
> > +                        rc);
> > +               goto err_free_irq;
> > +       }
>
> The commit message says probe will "configure it as the async event
> completion ring (a failure is only warned about, as before)". Does that
> still match the code?
>
> Here, if bnge_hwrm_set_async_event_cr() fails, the error is logged with
> bnge_err() and the code jumps to err_free_irq. That tears NQ0 down and
> returns rc. bnge_netdev_alloc() then treats the error as fatal:
>
>     rc = bnge_nq0_init(bn);
>     if (rc) {
>         dev_err(bd->dev, "NQ0 init failed rc: %d\n", rc);
>         goto err_free_port_stats;
>     }
>
> So register_netdev() is never called and probe fails.
>
> Before this patch, bnge_hwrm_ring_alloc() only warned and kept going:
>
>     if (!i) {
>         rc = bnge_hwrm_set_async_event_cr(bd, ring->fw_ring_id);
>         if (rc)
>             netdev_warn(bn->netdev, "Failed to set async event
> completion ring.\n");
>     }
>
> Before, a firmware rejection of this FUNC_CFG request still left a
> working interface, just without async events. Now the port gets no
> netdev at all.
>
> Should this failure stay warn-only, as the commit message says? If it
> is meant to be fatal at probe, could the commit message say so?

The failing probe is intentional. Without async event support, the driver
cannot receive firmware async events, so a netdev coming up without it would be
only partially functional. The current commit message is inaccurate on this
point, which I can fix in a future revision.

Thanks,
Bhargava, Marreddy

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature