Re: [PATCH net-next 05/10] bnxt_en: Refactor bnxt_free_tx_rings() to free per Tx ring

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Sun, Jan 12, 2025 at 10:39:22PM -0800, Michael Chan wrote:
> From: Somnath Kotur <somnath.kotur@broadcom.com>
> 
> Modify bnxt_free_tx_rings() to free the skbs per Tx ring.
> This will be useful later in the series.
> 
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 115 ++++++++++++----------
>  1 file changed, 61 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 4c5cb4dd7420..4336a5b54289 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -3314,74 +3314,81 @@ static int bnxt_poll_p5(struct napi_struct *napi, int budget)
>  	return work_done;
>  }
>  
> -static void bnxt_free_tx_skbs(struct bnxt *bp)
> +static void bnxt_free_one_tx_ring_skbs(struct bnxt *bp,
> +				       struct bnxt_tx_ring_info *txr, int idx)
>  {
>  	int i, max_idx;
>  	struct pci_dev *pdev = bp->pdev;
>  
> -	if (!bp->tx_ring)
> -		return;
> -
>  	max_idx = bp->tx_nr_pages * TX_DESC_CNT;
> -	for (i = 0; i < bp->tx_nr_rings; i++) {
> -		struct bnxt_tx_ring_info *txr = &bp->tx_ring[i];
> -		int j;
>  
> -		if (!txr->tx_buf_ring)
> +	for (i = 0; i < max_idx;) {
> +		struct bnxt_sw_tx_bd *tx_buf = &txr->tx_buf_ring[i];
> +		struct sk_buff *skb;
> +		int j, last;
> +
> +		if (idx  < bp->tx_nr_rings_xdp &&
> +		    tx_buf->action == XDP_REDIRECT) {
> +			dma_unmap_single(&pdev->dev,
> +					 dma_unmap_addr(tx_buf, mapping),
> +					 dma_unmap_len(tx_buf, len),
> +					 DMA_TO_DEVICE);
> +			xdp_return_frame(tx_buf->xdpf);
> +			tx_buf->action = 0;
> +			tx_buf->xdpf = NULL;
> +			i++;
>  			continue;
> +		}
>  
> -		for (j = 0; j < max_idx;) {
> -			struct bnxt_sw_tx_bd *tx_buf = &txr->tx_buf_ring[j];
> -			struct sk_buff *skb;
> -			int k, last;
> -
> -			if (i < bp->tx_nr_rings_xdp &&
> -			    tx_buf->action == XDP_REDIRECT) {
> -				dma_unmap_single(&pdev->dev,
> -					dma_unmap_addr(tx_buf, mapping),
> -					dma_unmap_len(tx_buf, len),
> -					DMA_TO_DEVICE);
> -				xdp_return_frame(tx_buf->xdpf);
> -				tx_buf->action = 0;
> -				tx_buf->xdpf = NULL;
> -				j++;
> -				continue;
> -			}
> +		skb = tx_buf->skb;
> +		if (!skb) {
> +			i++;
> +			continue;
> +		}
>  
> -			skb = tx_buf->skb;
> -			if (!skb) {
> -				j++;
> -				continue;
> -			}
> +		tx_buf->skb = NULL;
>  
> -			tx_buf->skb = NULL;
> +		if (tx_buf->is_push) {
> +			dev_kfree_skb(skb);
> +			i += 2;
> +			continue;
> +		}
>  
> -			if (tx_buf->is_push) {
> -				dev_kfree_skb(skb);
> -				j += 2;
> -				continue;
> -			}
> +		dma_unmap_single(&pdev->dev,
> +				 dma_unmap_addr(tx_buf, mapping),
> +				 skb_headlen(skb),
> +				 DMA_TO_DEVICE);
>  
> -			dma_unmap_single(&pdev->dev,
> -					 dma_unmap_addr(tx_buf, mapping),
> -					 skb_headlen(skb),
> -					 DMA_TO_DEVICE);
> +		last = tx_buf->nr_frags;
> +		i += 2;
> +		for (j = 0; j < last; j++, i++) {
> +			int ring_idx = i & bp->tx_ring_mask;
> +			skb_frag_t *frag = &skb_shinfo(skb)->frags[j];
>  
> -			last = tx_buf->nr_frags;
> -			j += 2;
> -			for (k = 0; k < last; k++, j++) {
> -				int ring_idx = j & bp->tx_ring_mask;
> -				skb_frag_t *frag = &skb_shinfo(skb)->frags[k];
> -
> -				tx_buf = &txr->tx_buf_ring[ring_idx];
> -				dma_unmap_page(
> -					&pdev->dev,
> -					dma_unmap_addr(tx_buf, mapping),
> -					skb_frag_size(frag), DMA_TO_DEVICE);
> -			}
> -			dev_kfree_skb(skb);
> +			tx_buf = &txr->tx_buf_ring[ring_idx];
> +			dma_unmap_page(&pdev->dev,
> +				       dma_unmap_addr(tx_buf, mapping),
> +				       skb_frag_size(frag), DMA_TO_DEVICE);
>  		}
> -		netdev_tx_reset_queue(netdev_get_tx_queue(bp->dev, i));
> +		dev_kfree_skb(skb);
> +	}
> +	netdev_tx_reset_queue(netdev_get_tx_queue(bp->dev, idx));
> +}
> +
> +static void bnxt_free_tx_skbs(struct bnxt *bp)
> +{
> +	int i;
> +
> +	if (!bp->tx_ring)
> +		return;
> +
> +	for (i = 0; i < bp->tx_nr_rings; i++) {
> +		struct bnxt_tx_ring_info *txr = &bp->tx_ring[i];
> +
> +		if (!txr->tx_buf_ring)
> +			continue;
> +
> +		bnxt_free_one_tx_ring_skbs(bp, txr, i);
>  	}
>  }

Looks fine, I didn't find any functional changes from previous version.
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

>  
> -- 
> 2.30.1