ll_temac: free everything on error path
Subsystems (3)
PATCH 3/4 ll_temac: free everything on error path
temac_dma_bd_init() must free all allocated resources: memory, dma, skbs.
--- drivers/net/ll_temac_main.c | 30 +++++++++++++++++++++++++----- 1 files changed, 25 insertions(+), 5 deletions(-)
drivers/net/ll_temac_main.c +25 -5
@@ -200,6 +200,7 @@ static int temac_dma_bd_init(struct net_device *ndev)struct temac_local *lp =netdev_priv (ndev);struct sk_buff *skb;int i; +int tx_bd_v_size, rx_bd_v_size; lp->rx_skb =kzalloc (sizeof (*lp->rx_skb) *RX_BD_NUM ,GFP_KERNEL );if (!lp->rx_skb) {
@@ -209,21 +210,23 @@ static int temac_dma_bd_init(struct net_device *ndev) }/* allocate the tx and rx ring buffer descriptors. */ /* returns a virtual addres and a physical address. */ + tx_bd_v_size =sizeof (*lp->tx_bd_v) *TX_BD_NUM ; lp->tx_bd_v =dma_alloc_coherent (ndev->dev.parent, -sizeof (*lp->tx_bd_v) *TX_BD_NUM , + tx_bd_v_size, &lp->tx_bd_p,GFP_KERNEL );if (!lp->tx_bd_v) {dev_err (&ndev->dev,"unable to allocate DMA TX buffer descriptors" ); - goto out; + goto err_rx_skb; } + rx_bd_v_size =sizeof (*lp->rx_bd_v) *RX_BD_NUM ; lp->rx_bd_v =dma_alloc_coherent (ndev->dev.parent, -sizeof (*lp->rx_bd_v) *RX_BD_NUM , + rx_bd_v_size, &lp->rx_bd_p,GFP_KERNEL );if (!lp->rx_bd_v) {dev_err (&ndev->dev,"unable to allocate DMA RX buffer descriptors" ); - goto out; + goto err_tx_bd; }memset (lp->tx_bd_v,0 ,sizeof (*lp->tx_bd_v) *TX_BD_NUM );
@@ -242,7 +245,7 @@ static int temac_dma_bd_init(struct net_device *ndev)if (skb =0 ) {dev_err (&ndev->dev,"alloc_skb error %d\n" , i); - goto out; + goto err_dma_single; } lp->rx_skb[i] = skb;/* returns physical address of skb->data */
@@ -274,6 +277,23 @@ static int temac_dma_bd_init(struct net_device *ndev)return 0 ; +err_dma_single: +for (i =0 ; i <RX_BD_NUM ; i++) { +if (lp->rx_skb[i] =NULL ) +break ; + +kfree_skb (lp->rx_skb[i]); +dma_unmap_single (ndev->dev.parent, lp->rx_bd_v[i].phys, +XTE_MAX_JUMBO_FRAME_SIZE ,DMA_FROM_DEVICE ); + } + +dma_free_coherent (ndev->dev.parent, rx_bd_v_size, + lp->rx_bd_v, lp->rx_bd_p); +err_tx_bd: +dma_free_coherent (ndev->dev.parent, tx_bd_v_size, + lp->tx_bd_v, lp->tx_bd_p); +err_rx_skb: +kfree (lp->rx_skb); out:return -ENOMEM ; }
Signature · 1 line
1.7.0.4
Quoted 77 lines · Kulikov Vasiliy
temac_dma_bd_init()must free all allocated resources: memory, dma, skbs.Signed-off-by Kulikov Vasiliy <[email protected]>---
drivers/net/ll_temac_main.c | 30 +++++++++++++++++++++++++-----1 files changed, 25 insertions(+), 5 deletions(-)drivers/net/ll_temac_main.c +25
-5@@ -200,6 +200,7 @@ static int temac_dma_bd_init(struct net_device *ndev) struct temac_local *lp = netdev_priv(ndev); struct sk_buff *skb; int i; + int tx_bd_v_size, rx_bd_v_size; lp->rx_skb = kzalloc(sizeof(*lp->rx_skb) * RX_BD_NUM, GFP_KERNEL); if (!lp->rx_skb) {@@ -209,21 +210,23 @@ static int temac_dma_bd_init(struct net_device *ndev) } /* allocate the tx and rx ring buffer descriptors. */ /* returns a virtual addres and a physical address. */ + tx_bd_v_size = sizeof(*lp->tx_bd_v) * TX_BD_NUM; lp->tx_bd_v = dma_alloc_coherent(ndev->dev.parent, - sizeof(*lp->tx_bd_v) * TX_BD_NUM, + tx_bd_v_size, &lp->tx_bd_p, GFP_KERNEL); if (!lp->tx_bd_v) { dev_err(&ndev->dev, "unable to allocate DMA TX buffer descriptors"); - goto out; + goto err_rx_skb; } + rx_bd_v_size = sizeof(*lp->rx_bd_v) * RX_BD_NUM; lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent, - sizeof(*lp->rx_bd_v) * RX_BD_NUM, + rx_bd_v_size, &lp->rx_bd_p, GFP_KERNEL); if (!lp->rx_bd_v) { dev_err(&ndev->dev, "unable to allocate DMA RX buffer descriptors"); - goto out; + goto err_tx_bd; } memset(lp->tx_bd_v, 0, sizeof(*lp->tx_bd_v) * TX_BD_NUM);@@ -242,7 +245,7 @@ static int temac_dma_bd_init(struct net_device *ndev) if (skb = 0) { dev_err(&ndev->dev, "alloc_skb error %d\n", i); - goto out; + goto err_dma_single; } lp->rx_skb[i] = skb; /* returns physical address of skb->data */@@ -274,6 +277,23 @@ static int temac_dma_bd_init(struct net_device *ndev) return 0; +err_dma_single: + for (i = 0; i < RX_BD_NUM; i++) { + if (lp->rx_skb[i] = NULL) + break; + + kfree_skb(lp->rx_skb[i]); + dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, + XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE); + } + + dma_free_coherent(ndev->dev.parent, rx_bd_v_size, + lp->rx_bd_v, lp->rx_bd_p); +err_tx_bd: + dma_free_coherent(ndev->dev.parent, tx_bd_v_size, + lp->tx_bd_v, lp->tx_bd_p); +err_rx_skb: + kfree(lp->rx_skb);
Quoted 5 lines
out:
return-ENOMEM;
}
--
1.7.0.4
This is not enough. DMA resources also must be released on exit.
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 6 lines
Regards, Denis -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 18 lines · Denis Kirjanov
Quoted 17 lines · Kulikov Vasiliy
temac_dma_bd_init()must free all allocated resources: memory, dma, skbs.Signed-off-by Kulikov Vasiliy <[email protected]>---
drivers/net/ll_temac_main.c | 30 +++++++++++++++++++++++++-----1 files changed, 25 insertions(+), 5 deletions(-)drivers/net/ll_temac_main.c +1
-0@@ -200,6 +200,7 @@ static int temac_dma_bd_init(struct net_device *ndev) struct temac_local *lp = netdev_priv(ndev); struct sk_buff *skb; int i; + int tx_bd_v_size, rx_bd_v_size;
Quoted 8 lines
drivers/net/ll_temac_main.c static int temac_dma_bd_init(struct net_device *ndev) · line 214
Quoted 8 lines
drivers/net/ll_temac_main.c +1
-0lp->rx_skb = kzalloc(sizeof(*lp->rx_skb) * RX_BD_NUM, GFP_KERNEL); if (!lp->rx_skb) {@@ -209,21 +210,23 @@ static int temac_dma_bd_init(struct net_device *ndev) } /* allocate the tx and rx ring buffer descriptors. */ /* returns a virtual addres and a physical address. */ + tx_bd_v_size = sizeof(*lp->tx_bd_v) * TX_BD_NUM; lp->tx_bd_v = dma_alloc_coherent(ndev->dev.parent,
^^^^^^^^^^^
1stQuoted 11 lines
drivers/net/ll_temac_main.c static int temac_dma_bd_init(struct net_device *ndev) · line 223
Quoted 11 lines
drivers/net/ll_temac_main.c +3
-2- sizeof(*lp->tx_bd_v) * TX_BD_NUM, + tx_bd_v_size, &lp->tx_bd_p, GFP_KERNEL); if (!lp->tx_bd_v) { dev_err(&ndev->dev, "unable to allocate DMA TX buffer descriptors"); - goto out; + goto err_rx_skb; } + rx_bd_v_size = sizeof(*lp->rx_bd_v) * RX_BD_NUM; lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
^^^^^^^^^^^
2ndQuoted 24 lines
Quoted 24 lines
drivers/net/ll_temac_main.c +3
-3- sizeof(*lp->rx_bd_v) * RX_BD_NUM, + rx_bd_v_size, &lp->rx_bd_p, GFP_KERNEL); if (!lp->rx_bd_v) { dev_err(&ndev->dev, "unable to allocate DMA RX buffer descriptors"); - goto out; + goto err_tx_bd; } memset(lp->tx_bd_v, 0, sizeof(*lp->tx_bd_v) * TX_BD_NUM);@@ -242,7 +245,7 @@ static int temac_dma_bd_init(struct net_device *ndev) if (skb = 0) { dev_err(&ndev->dev, "alloc_skb error %d\n", i); - goto out; + goto err_dma_single; } lp->rx_skb[i] = skb; /* returns physical address of skb->data */@@ -274,6 +277,23 @@ static int temac_dma_bd_init(struct net_device *ndev) return 0;
Quoted 12 lines
drivers/net/ll_temac_main.c static int temac_dma_bd_init(struct net_device *ndev) · line 291
Quoted 12 lines
drivers/net/ll_temac_main.c +12
-0+err_dma_single: + for (i = 0; i < RX_BD_NUM; i++) { + if (lp->rx_skb[i] = NULL) + break; + + kfree_skb(lp->rx_skb[i]); + dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, + XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE); + } + + dma_free_coherent(ndev->dev.parent, rx_bd_v_size, + lp->rx_bd_v, lp->rx_bd_p);
^^^^^^^^^^^
2ndQuoted 3 lines
drivers/net/ll_temac_main.c static int temac_dma_bd_init(struct net_device *ndev) · line 294
Quoted 3 lines
drivers/net/ll_temac_main.c +3
-0+err_tx_bd: + dma_free_coherent(ndev->dev.parent, tx_bd_v_size, + lp->tx_bd_v, lp->tx_bd_p);
^^^^^^^^^^^
1stQuoted 8 lines
Quoted 7 lines
drivers/net/ll_temac_main.c +2
-0+err_rx_skb: + kfree(lp->rx_skb);out:
return-ENOMEM;
}
--
1.7.0.4This is not enough. DMA resources also must be released on exit.
Could you point to it? I see only two variables with allocated DMA (see above). --
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 92 lines · Kulikov Vasiliy
Quoted 18 lines · Denis Kirjanov
Quoted 17 lines · Kulikov Vasiliy
temac_dma_bd_init()must free all allocated resources: memory, dma, skbs.Signed-off-by Kulikov Vasiliy <[email protected]>---
drivers/net/ll_temac_main.c | 30 +++++++++++++++++++++++++-----1 files changed, 25 insertions(+), 5 deletions(-)drivers/net/ll_temac_main.c +1
-0@@ -200,6 +200,7 @@ static int temac_dma_bd_init(struct net_device *ndev) struct temac_local *lp = netdev_priv(ndev); struct sk_buff *skb; int i; + int tx_bd_v_size, rx_bd_v_size;Quoted 8 lines
drivers/net/ll_temac_main.c static int temac_dma_bd_init(struct net_device *ndev) · line 214
Quoted 8 lines
drivers/net/ll_temac_main.c +1
-0lp->rx_skb = kzalloc(sizeof(*lp->rx_skb) * RX_BD_NUM, GFP_KERNEL); if (!lp->rx_skb) {@@ -209,21 +210,23 @@ static int temac_dma_bd_init(struct net_device *ndev) } /* allocate the tx and rx ring buffer descriptors. */ /* returns a virtual addres and a physical address. */ + tx_bd_v_size = sizeof(*lp->tx_bd_v) * TX_BD_NUM; lp->tx_bd_v = dma_alloc_coherent(ndev->dev.parent,^^^^^^^^^^^
1stQuoted 11 lines
drivers/net/ll_temac_main.c static int temac_dma_bd_init(struct net_device *ndev) · line 223
Quoted 11 lines
drivers/net/ll_temac_main.c +3
-2- sizeof(*lp->tx_bd_v) * TX_BD_NUM, + tx_bd_v_size, &lp->tx_bd_p, GFP_KERNEL); if (!lp->tx_bd_v) { dev_err(&ndev->dev, "unable to allocate DMA TX buffer descriptors"); - goto out; + goto err_rx_skb; } + rx_bd_v_size = sizeof(*lp->rx_bd_v) * RX_BD_NUM; lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,^^^^^^^^^^^
2ndQuoted 24 lines
Quoted 24 lines
drivers/net/ll_temac_main.c +3
-3- sizeof(*lp->rx_bd_v) * RX_BD_NUM, + rx_bd_v_size, &lp->rx_bd_p, GFP_KERNEL); if (!lp->rx_bd_v) { dev_err(&ndev->dev, "unable to allocate DMA RX buffer descriptors"); - goto out; + goto err_tx_bd; } memset(lp->tx_bd_v, 0, sizeof(*lp->tx_bd_v) * TX_BD_NUM);@@ -242,7 +245,7 @@ static int temac_dma_bd_init(struct net_device *ndev) if (skb = 0) { dev_err(&ndev->dev, "alloc_skb error %d\n", i); - goto out; + goto err_dma_single; } lp->rx_skb[i] = skb; /* returns physical address of skb->data */@@ -274,6 +277,23 @@ static int temac_dma_bd_init(struct net_device *ndev) return 0;Quoted 12 lines
drivers/net/ll_temac_main.c static int temac_dma_bd_init(struct net_device *ndev) · line 291
Quoted 12 lines
drivers/net/ll_temac_main.c +12
-0+err_dma_single: + for (i = 0; i < RX_BD_NUM; i++) { + if (lp->rx_skb[i] = NULL) + break; + + kfree_skb(lp->rx_skb[i]); + dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, + XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE); + } + + dma_free_coherent(ndev->dev.parent, rx_bd_v_size, + lp->rx_bd_v, lp->rx_bd_p);^^^^^^^^^^^
2ndQuoted 3 lines
drivers/net/ll_temac_main.c static int temac_dma_bd_init(struct net_device *ndev) · line 294
Quoted 3 lines
drivers/net/ll_temac_main.c +3
-0+err_tx_bd: + dma_free_coherent(ndev->dev.parent, tx_bd_v_size, + lp->tx_bd_v, lp->tx_bd_p);^^^^^^^^^^^
1stQuoted 7 lines
Quoted 7 lines
drivers/net/ll_temac_main.c +2
-0+err_rx_skb: + kfree(lp->rx_skb);out:
return-ENOMEM;
}
--
1.7.0.4
Quoted 3 lines
Quoted 1 line
This is not enough. DMA resources also must be released on exit.
Could you point to it? I see only two variables with allocated DMA (see above). --
I just sent a patch that fixes this.
Quoted 4 lines
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 6 lines
Regards, Denis -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 1 line
I just sent a patch that fixes this.
Hey, you forgot to CC me/kernel-janitors in your patch.
Anyway,
Review
Quoted 10 lines
/**
+ * *temac_dma_bd_release- Release buffer descriptor rings
+ */(quoted diff) +7
-0+static void temac_dma_bd_release(struct net_device *ndev) +{ + struct temac_local *lp = netdev_priv(ndev); + int i; + + for (i = 0; i < RX_BD_NUM; i++) { + if (lp->rx_skb[i]) {
^^^^^^^^^^
This can be NULL if kzalloc() failed.
drivers/net/ll_temac_main.c static int temac_dma_bd_init(struct net_device *ndev) · line 294
Quoted 9 lines
drivers/net/ll_temac_main.c +9
-0+dma_unmap_single (ndev->dev.parent, lp->rx_bd_v[i].phys, +XTE_MAX_JUMBO_FRAME_SIZE ,DMA_FROM_DEVICE ); +dev_kfree_skb (lp->rx_skb[i]); + } + } +dma_free_coherent (ndev->dev.parent,sizeof (*lp->rx_bd_v) *RX_BD_NUM , + lp->rx_bd_v, lp->rx_bd_p); +dma_free_coherent (ndev->dev.parent,sizeof (*lp->tx_bd_v) *TX_BD_NUM , + lp->tx_bd_v, lp->tx_bd_p);
These two DMA's can be unallocated if dma_alloc_coherent() failed.
I wanted to introduce many labels to divide these error cases because of it.
Quoted 2 lines
(quoted diff) +2
-0+ kfree(lp->rx_skb); +}
PATCH v3 3/4 ll_temac: fix DMA and memory leaks
In reply to Denis Kirjanovtemac_dma_bd_init() doesn't free DMA and memory on error. Also temac_stop() must free them.
--- drivers/net/ll_temac_main.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 files changed, 38 insertions(+), 0 deletions(-)
drivers/net/ll_temac_main.c +38 -0
@@ -193,6 +193,41 @@ static int temac_dcr_setup(struct temac_local *lp, struct of_device *op,#endif /** +* * temac_dma_bd_release - Release buffer descriptor rings +*/ +static void temac_dma_bd_release (struct net_device *ndev) +{ +struct temac_local *lp =netdev_priv (ndev); +int i; + +if (lp->rx_skb) { +for (i =0 ; i <RX_BD_NUM ; i++) { +if (!lp->rx_skb[i]) +break ; +dma_unmap_single (ndev->dev.parent, lp->rx_bd_v[i].phys, +XTE_MAX_JUMBO_FRAME_SIZE ,DMA_FROM_DEVICE ); +dev_kfree_skb (lp->rx_skb[i]); + } +kfree (lp->rx_skb); + lp->rx_skb =NULL ; + } + +if (lp->rx_bd_v) { +dma_free_coherent (ndev->dev.parent, +sizeof (*lp->rx_bd_v) *RX_BD_NUM , + lp->rx_bd_v, lp->rx_bd_p); + lp->rx_bd_v =NULL ; + } + +if (lp->tx_bd_v) { +dma_free_coherent (ndev->dev.parent, +sizeof (*lp->tx_bd_v) *TX_BD_NUM , + lp->tx_bd_v, lp->tx_bd_p); + lp->tx_bd_v =NULL ; + } +} + +/** * temac_dma_bd_init - Setup buffer descriptor rings */ static int temac_dma_bd_init (struct net_device *ndev)
@@ -275,6 +310,7 @@ static int temac_dma_bd_init(struct net_device *ndev)return 0 ; out: +temac_dma_bd_release (ndev);return -ENOMEM ; }
@@ -858,6 +894,8 @@ static int temac_stop(struct net_device *ndev)phy_disconnect (lp->phy_dev); lp->phy_dev =NULL ; +temac_dma_bd_release (ndev); +return 0 ; }
Signature · 1 line
1.7.0.4