Re: [PATCH net-next v2] net: stmmac: fix error path cleanup in DMA descriptor ring allocation

[Lorenzo Bianconi]

> From: Ding Hui <dinghui@xxxxxxxxxxx>

Hi Ding Hui,

just a couple of nits inline.

Regards,
Lorenzo

[...]

>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 59 ++++++++++++++++---
>  1 file changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f2fc89176654..f0e06c011b8d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1728,7 +1728,7 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
>  					 DMA_TO_DEVICE);
>  	}
>  

Is it more appropriate to move the tx_q->tx_skbuff_dma check here from
dma_free_tx_skbufs()?

> -	if (tx_q->xdpf[i] &&
> +	if (tx_q->xdpf && tx_q->xdpf[i] &&
>  	    (tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XDP_TX ||
>  	     tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XDP_NDO)) {
>  		xdp_return_frame(tx_q->xdpf[i]);
> @@ -1738,7 +1738,7 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
>  	if (tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XSK_TX)
>  		tx_q->xsk_frames_done++;
>  
> -	if (tx_q->tx_skbuff[i] &&
> +	if (tx_q->tx_skbuff && tx_q->tx_skbuff[i] &&
>  	    tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_SKB) {
>  		dev_kfree_skb_any(tx_q->tx_skbuff[i]);
>  		tx_q->tx_skbuff[i] = NULL;
> @@ -1761,6 +1761,10 @@ static void dma_free_rx_skbufs(struct stmmac_priv *priv,
>  	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
>  	int i;
>  
> +	/* buf_pool may not be allocated if alloc failed early */
> +	if (!rx_q->buf_pool)
> +		return;
> +
>  	for (i = 0; i < dma_conf->dma_rx_size; i++)
>  		stmmac_free_rx_buffer(priv, rx_q, i);
>  }
> @@ -1802,6 +1806,10 @@ static void dma_free_rx_xskbufs(struct stmmac_priv *priv,
>  	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
>  	int i;
>  
> +	/* buf_pool may not be allocated if alloc failed early */
> +	if (!rx_q->buf_pool)
> +		return;
> +
>  	for (i = 0; i < dma_conf->dma_rx_size; i++) {
>  		struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
>  
> @@ -2097,6 +2105,10 @@ static void dma_free_tx_skbufs(struct stmmac_priv *priv,
>  	struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[queue];
>  	int i;
>  
> +	/* tx_skbuff_dma may not be allocated if alloc failed early */
> +	if (!tx_q->tx_skbuff_dma)
> +		return;
> +
>  	tx_q->xsk_frames_done = 0;
>  
>  	for (i = 0; i < dma_conf->dma_tx_size; i++)
> @@ -2272,15 +2284,19 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
>  	}
>  
>  	rx_q->buf_pool = kzalloc_objs(*rx_q->buf_pool, dma_conf->dma_rx_size);
> -	if (!rx_q->buf_pool)
> -		return -ENOMEM;
> +	if (!rx_q->buf_pool) {
> +		ret = -ENOMEM;
> +		goto err_destroy_pool;
> +	}
>  
>  	size = stmmac_get_rx_desc_size(priv) * dma_conf->dma_rx_size;
>  
>  	addr = dma_alloc_coherent(priv->device, size, &rx_q->dma_rx_phy,
>  				  GFP_KERNEL);
> -	if (!addr)
> -		return -ENOMEM;
> +	if (!addr) {
> +		ret = -ENOMEM;
> +		goto err_free_buf_pool;
> +	}
>  
>  	if (priv->extend_desc)
>  		rx_q->dma_erx = addr;
> @@ -2296,10 +2312,27 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
>  	ret = xdp_rxq_info_reg(&rx_q->xdp_rxq, priv->dev, queue, napi_id);
>  	if (ret) {
>  		netdev_err(priv->dev, "Failed to register xdp rxq info\n");
> -		return -EINVAL;
> +		goto err_free_dma;
>  	}
>  
>  	return 0;
> +
> +err_free_dma:
> +	if (priv->extend_desc)
> +		dma_free_coherent(priv->device, size, rx_q->dma_erx,
> +				  rx_q->dma_rx_phy);
> +	else
> +		dma_free_coherent(priv->device, size, rx_q->dma_rx,
> +				  rx_q->dma_rx_phy);

I guess you can use addr here and remove the if/else block

> +	rx_q->dma_erx = NULL;
> +	rx_q->dma_rx = NULL;
> +err_free_buf_pool:
> +	kfree(rx_q->buf_pool);
> +	rx_q->buf_pool = NULL;
> +err_destroy_pool:
> +	page_pool_destroy(rx_q->page_pool);
> +	rx_q->page_pool = NULL;
> +	return ret;
>  }
>  
>  static int alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
> @@ -2352,14 +2385,14 @@ static int __alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
>  
>  	tx_q->tx_skbuff = kzalloc_objs(struct sk_buff *, dma_conf->dma_tx_size);
>  	if (!tx_q->tx_skbuff)
> -		return -ENOMEM;
> +		goto err_free_skbuff_dma;
>  
>  	size = stmmac_get_tx_desc_size(priv, tx_q) * dma_conf->dma_tx_size;
>  
>  	addr = dma_alloc_coherent(priv->device, size,
>  				  &tx_q->dma_tx_phy, GFP_KERNEL);
>  	if (!addr)
> -		return -ENOMEM;
> +		goto err_free_skbuff;
>  
>  	if (priv->extend_desc)
>  		tx_q->dma_etx = addr;
> @@ -2369,6 +2402,14 @@ static int __alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
>  		tx_q->dma_tx = addr;
>  
>  	return 0;
> +
> +err_free_skbuff:
> +	kfree(tx_q->tx_skbuff);
> +	tx_q->tx_skbuff = NULL;
> +err_free_skbuff_dma:
> +	kfree(tx_q->tx_skbuff_dma);
> +	tx_q->tx_skbuff_dma = NULL;
> +	return -ENOMEM;
>  }
>  
>  static int alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
> -- 
> 2.34.1
> 
>

Attachment: signature.asc
Description: PGP signature