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

[Ding Hui]

Hi Lorenzo,

Thanks for your review comments.

At 2026-09-06 18:16:39, "Lorenzo Bianconi" <lorenzo.bianconi@xxxxxxxxxxxxxxxx> wrote:
>> 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()?
>

tx_skbuff_dma is a per-queue resource, and stmmac_free_tx_buffer()
references it in multiple places across the function body. Moving the
NULL check into stmmac_free_tx_buffer() would require repeating it on
every call, introducing O(n) overhead proportional to dma_tx_size.
So I put it at the entry of 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);
>>  }

>> @@ -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
>
Excellent suggestion, will use addr directly in v3.

Thanks,
Ding Hui