From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: interface multiqueue timout race
To: Jonathan Matthew <jonathan@d14n.org>
Cc: David Gwynne <david@gwynne.id.au>, Openbsd Tech <tech@openbsd.org>
Date: Fri, 4 Oct 2024 15:23:04 +0200

On Wed, Oct 02, 2024 at 05:01:39PM +1000, Jonathan Matthew wrote:
> I'd rather fix the driver so it doesn't need this.
> The problem here seems to be that bnxt has two rings to refill, and
> the timeout tries to refill them both, even if one of them is not empty.
> Can you try this diff instead?

Works for me.  OK bluhm@

> diff --git a/sys/dev/pci/if_bnxt.c b/sys/dev/pci/if_bnxt.c
> index 52165d95f2c..5a40908cb1e 100644
> --- a/sys/dev/pci/if_bnxt.c
> +++ b/sys/dev/pci/if_bnxt.c
> @@ -315,7 +315,7 @@ void		bnxt_write_rx_doorbell(struct bnxt_softc *, struct bnxt_ring *,
>  void		bnxt_write_tx_doorbell(struct bnxt_softc *, struct bnxt_ring *,
>  		    int);
>  
> -int		bnxt_rx_fill(struct bnxt_queue *);
> +int		bnxt_rx_fill(struct bnxt_queue *, int, int);
>  u_int		bnxt_rx_fill_slots(struct bnxt_softc *, struct bnxt_ring *, void *,
>  		    struct bnxt_slot *, uint *, int, uint16_t, u_int);
>  void		bnxt_refill(void *);
> @@ -954,7 +954,7 @@ bnxt_queue_up(struct bnxt_softc *sc, struct bnxt_queue *bq)
>  	rx->rx_cons = 0;
>  	rx->rx_ag_prod = 0;
>  	rx->rx_ag_cons = 0;
> -	bnxt_rx_fill(bq);
> +	bnxt_rx_fill(bq, 1, 1);
>  
>  	tx->tx_cons = 0;
>  	tx->tx_prod = 0;
> @@ -1658,7 +1658,7 @@ bnxt_intr(void *xq)
>  			if_rxr_livelocked(&rx->rxr[1]);
>  		}
>  
> -		bnxt_rx_fill(q);
> +		bnxt_rx_fill(q, 1, 1);
>  		if ((rx->rx_cons == rx->rx_prod) ||
>  		    (rx->rx_ag_cons == rx->rx_ag_prod))
>  			timeout_add(&rx->rx_refill, 0);
> @@ -2216,33 +2216,37 @@ bnxt_rx_fill_slots(struct bnxt_softc *sc, struct bnxt_ring *ring, void *ring_mem
>  }
>  
>  int
> -bnxt_rx_fill(struct bnxt_queue *q)
> +bnxt_rx_fill(struct bnxt_queue *q, int rxring, int agring)
>  {
>  	struct bnxt_rx_queue *rx = &q->q_rx;
>  	struct bnxt_softc *sc = q->q_sc;
>  	u_int slots;
>  	int rv = 0;
>  
> -	slots = if_rxr_get(&rx->rxr[0], rx->rx_ring.ring_size);
> -	if (slots > 0) {
> -		slots = bnxt_rx_fill_slots(sc, &rx->rx_ring,
> -		    BNXT_DMA_KVA(rx->rx_ring_mem), rx->rx_slots,
> -		    &rx->rx_prod, MCLBYTES,
> -		    RX_PROD_PKT_BD_TYPE_RX_PROD_PKT, slots);
> -		if_rxr_put(&rx->rxr[0], slots);
> -	} else
> -		rv = 1;
> -
> -	slots = if_rxr_get(&rx->rxr[1],  rx->rx_ag_ring.ring_size);
> -	if (slots > 0) {
> -		slots = bnxt_rx_fill_slots(sc, &rx->rx_ag_ring,
> -		    BNXT_DMA_KVA(rx->rx_ring_mem) + PAGE_SIZE,
> -		    rx->rx_ag_slots, &rx->rx_ag_prod,
> -		    BNXT_AG_BUFFER_SIZE,
> -		    RX_PROD_AGG_BD_TYPE_RX_PROD_AGG, slots);
> -		if_rxr_put(&rx->rxr[1], slots);
> -	} else
> -		rv = 1;
> +	if (rxring != 0) {
> +		slots = if_rxr_get(&rx->rxr[0], rx->rx_ring.ring_size);
> +		if (slots > 0) {
> +			slots = bnxt_rx_fill_slots(sc, &rx->rx_ring,
> +			    BNXT_DMA_KVA(rx->rx_ring_mem), rx->rx_slots,
> +			    &rx->rx_prod, MCLBYTES,
> +			    RX_PROD_PKT_BD_TYPE_RX_PROD_PKT, slots);
> +			if_rxr_put(&rx->rxr[0], slots);
> +		} else
> +			rv = 1;
> +	}
> +
> +	if (agring != 0) {
> +		slots = if_rxr_get(&rx->rxr[1],  rx->rx_ag_ring.ring_size);
> +		if (slots > 0) {
> +			slots = bnxt_rx_fill_slots(sc, &rx->rx_ag_ring,
> +			    BNXT_DMA_KVA(rx->rx_ring_mem) + PAGE_SIZE,
> +			    rx->rx_ag_slots, &rx->rx_ag_prod,
> +			    BNXT_AG_BUFFER_SIZE,
> +			    RX_PROD_AGG_BD_TYPE_RX_PROD_AGG, slots);
> +			if_rxr_put(&rx->rxr[1], slots);
> +		} else
> +			rv = 1;
> +	}
>  
>  	return (rv);
>  }
> @@ -2253,9 +2257,10 @@ bnxt_refill(void *xq)
>  	struct bnxt_queue *q = xq;
>  	struct bnxt_rx_queue *rx = &q->q_rx;
>  
> -	bnxt_rx_fill(q);
> +	bnxt_rx_fill(q, rx->rx_cons == rx->rx_prod, rx->rx_ag_cons == rx->rx_ag_prod);
>  
> -	if (rx->rx_cons == rx->rx_prod)
> +	if ((rx->rx_cons == rx->rx_prod) ||
> +	    (rx->rx_ag_cons == rx->rx_ag_prod))
>  		timeout_add(&rx->rx_refill, 1);
>  }
>