Re: [net,v5,4/6] bnxt_en: Handle buffer allocation failure in bnxt_rx_ring_reset()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] Commit message misdescribes the failure mechanism it is
  fixing.
- [Low] The new error path in `bnxt_rx_ring_reset()`
  (drivers/net/ethernet/broadcom/bnxt/bnxt.c:14606-14616) loses the…

--

bnxt_en: Handle buffer allocation failure in bnxt_rx_ring_reset()

bnxt_rx_ring_reset() used to ignore the return value of
bnxt_alloc_one_rx_ring().  The patch checks it and, on failure, logs a
warning and falls back to a global reset instead of re-enabling TPA with a
partially populated rxr->rx_tpa[] array.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260828190900.1767611-1-joe%40dama.to

A couple of questions below, one about the commit message and one about the
new error path.

>     bnxt_alloc_one_rx_ring() can fail in bnxt_alloc_one_tpa_info_data(), which
>     returns -ENOMEM on the first failed allocation and leaves the remaining
>     rxr->rx_tpa[] entries zeroed.
>
>     This means that when the agg_id from hardware is mapped to a SW index in
>     rxr->rx_tpa[], an uninitialized slot can be chosen which would hand a
>     zero DMA address to the device.

[Severity: Medium]
Is the "entries zeroed" / "zero DMA address" description accurate for the
reset path?

bnxt_rx_ring_reset() calls bnxt_free_one_rx_ring_skbs() ->
bnxt_free_one_tpa_info_data() first, and that only clears ->data:

    for (i = 0; i < bp->max_tpa; i++) {
            struct bnxt_tpa_info *tpa_info = &rxr->rx_tpa[i];
            u8 *data = tpa_info->data;

            if (!data)
                    continue;

            tpa_info->data = NULL;
            page_pool_free_va(rxr->head_pool, data, false);
    }

->data_ptr and ->mapping are left pointing at the buffer that was just
returned to the head pool.  bnxt_alloc_one_tpa_info_data() then bails out
at the first failure:

    for (i = 0; i < bp->max_tpa; i++) {
            data = __bnxt_alloc_rx_frag(bp, &mapping, rxr, GFP_KERNEL);
            if (!data)
                    return -ENOMEM;
            ...
    }

so entries [i, bp->max_tpa) keep the stale mapping from before the reset
rather than becoming zero.  In the unfixed code bnxt_tpa_start() would then
program that stale address into the RX BD:

    mapping = tpa_info->mapping;
    prod_rx_buf->mapping = mapping;
    prod_bd = &rxr->rx_desc_ring[RX_RING(bp, prod)][RX_IDX(prod)];
    prod_bd->rx_bd_haddr = cpu_to_le64(mapping);

Would it be more precise to say the device would DMA into a recycled
page-pool buffer (and prod_rx_buf->data would be NULL), instead of a zero
DMA address?  Since this is tagged for stable, the description is what
backporters will read.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 3755a30f8d403..a8e5fdfcdf591 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -14604,7 +14604,14 @@ static void bnxt_rx_ring_reset(struct bnxt *bp)
>  		rxr->rx_sw_agg_prod = 0;
>  		rxr->rx_next_cons = 0;
>  		rxr->bnapi->in_reset = false;
> -		bnxt_alloc_one_rx_ring(bp, i);
> +		rc = bnxt_alloc_one_rx_ring(bp, i);
> +		if (rc) {
> +			netdev_warn(bp->dev, "RX ring reset failed to allocate buffers, rc = %d, falling back to global reset\n",
> +				    rc);
> +			bnxt_reset_task(bp, true);
> +			bnxt_rtnl_unlock_sp(bp);
> +			return;
> +		}
>  		cpr = &rxr->bnapi->cp_ring;
>  		cpr->sw_stats->rx.rx_resets++;

[Severity: Low]
Does this new path lose the rx_resets accounting for the reset it just did?

rxr->bnapi->in_reset = false is done just above the allocation, and the new
error path returns before reaching cpr->sw_stats->rx.rx_resets++.

The fallback bnxt_reset_task(bp, true) -> bnxt_close_nic() ->
bnxt_disable_napi() is the only other place that bumps this counter, and it
is gated on in_reset:

    if (bnapi->in_reset)
            cpr->sw_stats->rx.rx_resets++;

which is already false for this ring, so the event is counted nowhere.

Contrast with the sibling failure path in the same function: the
bnxt_hwrm_rx_ring_reset() failure does a break with in_reset still set, so
its global reset does get counted by bnxt_disable_napi().  That leaves two
paths that both do an RX ring reset plus a global reset reporting different
rx_resets values via ethtool -S.

cpr->sw_stats is only freed by bnxt_free_ring_stats(), which is not reached
for an irq_re_init == false close, so the missing increment stays visible in
rx_resets and rx_total_resets.

Would incrementing rx_resets before the early return, or only clearing
in_reset after a successful bnxt_alloc_one_rx_ring(), be preferable here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902015652.2421609-1-joe%40dama.to