Re: [PATCH net-next v2] net: stmmac: fix error path cleanup in DMA descriptor ring allocation
From: Lorenzo Bianconi <hidden>
Date: 2026-09-06 10:16:43
Also in:
lkml, netdev
From: Ding Hui <redacted>
Hi Ding Hui, just a couple of nits inline. Regards, Lorenzo [...]
quoted hunk ↗ jump to hunk
.../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()?
quoted hunk ↗ jump to hunk
- 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
quoted hunk ↗ jump to hunk
+ 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
Attachments
- signature.asc [application/pgp-signature] 228 bytes