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

[Bhargava Chenna Marreddy]

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?

> +
> +       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?

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