From: Jonathan Matthew <jonathan@d14n.org>
Subject: Re: bnxt_queue_up: free completion ring on failure
To: Stefan Sperling <stsp@stsp.name>
Cc: tech@openbsd.org
Date: Mon, 13 Oct 2025 10:24:04 +1000

On Tue, Oct 07, 2025 at 03:34:56PM +0200, Stefan Sperling wrote:
> If bnxt firmware fails to allocate a Tx ring in bnxt_queue_up()
> then we jump into an error path which leaks a completion ring.
> 
> Found while running ping traffic across bnxt0 in parallel with:
> 
>   while true; do ifconfig bnxt0 down up; done
> 
> The patch below avoids leaking a completion ring but it does not
> resolve the problem where the above down/up loop eventually fails:
> 
>   ifconfig: bnxt0: SIOCSIFFLAGS: Cannot allocate memory
> 
>   bnxt0: HWRM_RING_ALLOC command returned RESOURCE_ALLOC_ERROR error.
>   bnxt0: failed to set up tx ring
> 
> There might be another resource leak caused by the driver, or perhaps
> firmware is leaking memory and will eventually always hit this problem.
> 
> bnxt0 at pci14 dev 0 function 0 "Broadcom BCM57414" rev 0x01: fw ver 234.0.150, msix, 8 queues, address xx:xx:xx:xx:xx:xx
> 
> 
> ok?

ok jmatthew@

> 
> free completion ring on failure in bnxt_queue_up()
> 
> M  sys/dev/pci/if_bnxt.c  |  13+  3-
> 
> 1 file changed, 13 insertions(+), 3 deletions(-)
> 
> commit - dfd988efd85f876f66c82e5f98ff3ff8ac9ed907
> commit + bdb26caee92dfd55bf4a07741f3e3b15cf02cfc7
> blob - 49b4a418a8801b88632d1a78fe29d589c5188f92
> blob + 87a8f220796fc433431e49a7484e81e4b8ebd50e
> --- sys/dev/pci/if_bnxt.c
> +++ sys/dev/pci/if_bnxt.c
> @@ -821,13 +821,13 @@ bnxt_queue_up(struct bnxt_softc *sc, struct bnxt_queue
>  		    HWRM_NA_SIGNATURE, 1) != 0) {
>  			printf("%s: failed to allocate completion queue %d\n",
>  			    DEVNAME(sc), bq->q_index);
> -			goto free_rx;
> +			goto free_cp_ring_mem;
>  		}
>  
>  		if (bnxt_set_cp_ring_aggint(sc, cp) != 0) {
>  			printf("%s: failed to set interrupt %d aggregation\n",
>  			    DEVNAME(sc), bq->q_index);
> -			goto free_rx;
> +			goto free_cp_ring;
>  		}
>  		bnxt_write_cp_doorbell(sc, &cp->ring, 1);
>  	}
> @@ -836,7 +836,7 @@ bnxt_queue_up(struct bnxt_softc *sc, struct bnxt_queue
>  	    BNXT_DMA_DVA(sc->sc_stats_ctx_mem) +
>  	    (bq->q_index * sizeof(struct ctx_hw_stats))) != 0) {
>  		printf("%s: failed to set up stats context\n", DEVNAME(sc));
> -		goto free_rx;
> +		goto free_cp_ring;
>  	}
>  
>  	tx->tx_ring.phys_id = (uint16_t)HWRM_NA_SIGNATURE;
> @@ -998,6 +998,16 @@ dealloc_rx:
>  	    &rx->rx_ring);
>  dealloc_stats:
>  	bnxt_hwrm_stat_ctx_free(sc, cp);
> +free_cp_ring:
> +	if (sc->sc_intrmap != NULL) {
> +		bnxt_hwrm_ring_free(sc,
> +		    HWRM_RING_ALLOC_INPUT_RING_TYPE_L2_CMPL, &cp->ring);
> +	}
> +free_cp_ring_mem:
> +	if (sc->sc_intrmap != NULL) {
> +		bnxt_dmamem_free(sc, cp->ring_mem);
> +		cp->ring_mem = NULL;
> +	}
>  free_rx:
>  	bnxt_dmamem_free(sc, rx->rx_ring_mem);
>  	rx->rx_ring_mem = NULL;
>