* [PATCH net-next] net: moxart_ether: use devm in probe
@ 2024-08-08 4:03 Rosen Penev
2024-08-08 8:18 ` Christophe JAILLET
2024-08-08 14:58 ` Jakub Kicinski
0 siblings, 2 replies; 4+ messages in thread
From: Rosen Penev @ 2024-08-08 4:03 UTC (permalink / raw)
To: netdev; +Cc: u.kleine-koenig, davem, edumazet, kuba, pabeni, linux-kernel
alloc_etherdev and kmalloc_array are called first and destroyed last.
Safe to use devm to remove frees.
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
drivers/net/ethernet/moxa/moxart_ether.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/moxa/moxart_ether.c b/drivers/net/ethernet/moxa/moxart_ether.c
index 96dc69e7141f..06c632c90494 100644
--- a/drivers/net/ethernet/moxa/moxart_ether.c
+++ b/drivers/net/ethernet/moxa/moxart_ether.c
@@ -81,9 +81,6 @@ static void moxart_mac_free_memory(struct net_device *ndev)
dma_free_coherent(&priv->pdev->dev,
RX_REG_DESC_SIZE * RX_DESC_NUM,
priv->rx_desc_base, priv->rx_base);
-
- kfree(priv->tx_buf_base);
- kfree(priv->rx_buf_base);
}
static void moxart_mac_reset(struct net_device *ndev)
@@ -461,15 +458,14 @@ static int moxart_mac_probe(struct platform_device *pdev)
unsigned int irq;
int ret;
- ndev = alloc_etherdev(sizeof(struct moxart_mac_priv_t));
+ ndev = devm_alloc_etherdev(p_dev, sizeof(struct moxart_mac_priv_t));
if (!ndev)
return -ENOMEM;
irq = irq_of_parse_and_map(node, 0);
if (irq <= 0) {
netdev_err(ndev, "irq_of_parse_and_map failed\n");
- ret = -EINVAL;
- goto irq_map_fail;
+ return -EINVAL;
}
priv = netdev_priv(ndev);
@@ -511,15 +507,15 @@ static int moxart_mac_probe(struct platform_device *pdev)
goto init_fail;
}
- priv->tx_buf_base = kmalloc_array(priv->tx_buf_size, TX_DESC_NUM,
- GFP_KERNEL);
+ priv->tx_buf_base = devm_kmalloc_array(p_dev, priv->tx_buf_size,
+ TX_DESC_NUM, GFP_KERNEL);
if (!priv->tx_buf_base) {
ret = -ENOMEM;
goto init_fail;
}
- priv->rx_buf_base = kmalloc_array(priv->rx_buf_size, RX_DESC_NUM,
- GFP_KERNEL);
+ priv->rx_buf_base = devm_kmalloc_array(p_dev, priv->rx_buf_size,
+ RX_DESC_NUM, GFP_KERNEL);
if (!priv->rx_buf_base) {
ret = -ENOMEM;
goto init_fail;
@@ -553,8 +549,6 @@ static int moxart_mac_probe(struct platform_device *pdev)
init_fail:
netdev_err(ndev, "init failed\n");
moxart_mac_free_memory(ndev);
-irq_map_fail:
- free_netdev(ndev);
return ret;
}
@@ -565,7 +559,6 @@ static void moxart_remove(struct platform_device *pdev)
unregister_netdev(ndev);
devm_free_irq(&pdev->dev, ndev->irq, ndev);
moxart_mac_free_memory(ndev);
- free_netdev(ndev);
}
static const struct of_device_id moxart_mac_match[] = {
--
2.45.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] net: moxart_ether: use devm in probe
2024-08-08 4:03 [PATCH net-next] net: moxart_ether: use devm in probe Rosen Penev
@ 2024-08-08 8:18 ` Christophe JAILLET
2024-08-08 20:21 ` Rosen Penev
2024-08-08 14:58 ` Jakub Kicinski
1 sibling, 1 reply; 4+ messages in thread
From: Christophe JAILLET @ 2024-08-08 8:18 UTC (permalink / raw)
To: Rosen Penev, netdev
Cc: u.kleine-koenig, davem, edumazet, kuba, pabeni, linux-kernel
Le 08/08/2024 à 06:03, Rosen Penev a écrit :
> alloc_etherdev and kmalloc_array are called first and destroyed last.
> Safe to use devm to remove frees.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
Hi,
using dmam_alloc_coherent() would go even one step further I think.
It would remove moxart_mac_free_memory() completely.
Then IIUC, using devm_register_netdev() would keep things ordered and
moxart_remove() could also be removed completely.
(by inspection only)
CJ
> drivers/net/ethernet/moxa/moxart_ether.c | 19 ++++++-------------
> 1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/moxa/moxart_ether.c b/drivers/net/ethernet/moxa/moxart_ether.c
> index 96dc69e7141f..06c632c90494 100644
> --- a/drivers/net/ethernet/moxa/moxart_ether.c
> +++ b/drivers/net/ethernet/moxa/moxart_ether.c
> @@ -81,9 +81,6 @@ static void moxart_mac_free_memory(struct net_device *ndev)
> dma_free_coherent(&priv->pdev->dev,
> RX_REG_DESC_SIZE * RX_DESC_NUM,
> priv->rx_desc_base, priv->rx_base);
> -
> - kfree(priv->tx_buf_base);
> - kfree(priv->rx_buf_base);
> }
>
> static void moxart_mac_reset(struct net_device *ndev)
> @@ -461,15 +458,14 @@ static int moxart_mac_probe(struct platform_device *pdev)
> unsigned int irq;
> int ret;
>
> - ndev = alloc_etherdev(sizeof(struct moxart_mac_priv_t));
> + ndev = devm_alloc_etherdev(p_dev, sizeof(struct moxart_mac_priv_t));
> if (!ndev)
> return -ENOMEM;
>
> irq = irq_of_parse_and_map(node, 0);
> if (irq <= 0) {
> netdev_err(ndev, "irq_of_parse_and_map failed\n");
> - ret = -EINVAL;
> - goto irq_map_fail;
> + return -EINVAL;
> }
>
> priv = netdev_priv(ndev);
> @@ -511,15 +507,15 @@ static int moxart_mac_probe(struct platform_device *pdev)
> goto init_fail;
> }
>
> - priv->tx_buf_base = kmalloc_array(priv->tx_buf_size, TX_DESC_NUM,
> - GFP_KERNEL);
> + priv->tx_buf_base = devm_kmalloc_array(p_dev, priv->tx_buf_size,
> + TX_DESC_NUM, GFP_KERNEL);
> if (!priv->tx_buf_base) {
> ret = -ENOMEM;
> goto init_fail;
> }
>
> - priv->rx_buf_base = kmalloc_array(priv->rx_buf_size, RX_DESC_NUM,
> - GFP_KERNEL);
> + priv->rx_buf_base = devm_kmalloc_array(p_dev, priv->rx_buf_size,
> + RX_DESC_NUM, GFP_KERNEL);
> if (!priv->rx_buf_base) {
> ret = -ENOMEM;
> goto init_fail;
> @@ -553,8 +549,6 @@ static int moxart_mac_probe(struct platform_device *pdev)
> init_fail:
> netdev_err(ndev, "init failed\n");
> moxart_mac_free_memory(ndev);
> -irq_map_fail:
> - free_netdev(ndev);
> return ret;
> }
>
> @@ -565,7 +559,6 @@ static void moxart_remove(struct platform_device *pdev)
> unregister_netdev(ndev);
> devm_free_irq(&pdev->dev, ndev->irq, ndev);
> moxart_mac_free_memory(ndev);
> - free_netdev(ndev);
> }
>
> static const struct of_device_id moxart_mac_match[] = {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] net: moxart_ether: use devm in probe
2024-08-08 4:03 [PATCH net-next] net: moxart_ether: use devm in probe Rosen Penev
2024-08-08 8:18 ` Christophe JAILLET
@ 2024-08-08 14:58 ` Jakub Kicinski
1 sibling, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2024-08-08 14:58 UTC (permalink / raw)
To: Rosen Penev
Cc: netdev, u.kleine-koenig, davem, edumazet, pabeni, linux-kernel
On Wed, 7 Aug 2024 21:03:54 -0700 Rosen Penev wrote:
> alloc_etherdev and kmalloc_array are called first and destroyed last.
> Safe to use devm to remove frees.
But why? Refactoring old drivers is often more risk than reward.
How did you test this change?
--
pw-bot: cr
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] net: moxart_ether: use devm in probe
2024-08-08 8:18 ` Christophe JAILLET
@ 2024-08-08 20:21 ` Rosen Penev
0 siblings, 0 replies; 4+ messages in thread
From: Rosen Penev @ 2024-08-08 20:21 UTC (permalink / raw)
To: Christophe JAILLET
Cc: netdev, u.kleine-koenig, davem, edumazet, kuba, pabeni,
linux-kernel
On Thu, Aug 8, 2024 at 1:18 AM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 08/08/2024 à 06:03, Rosen Penev a écrit :
> > alloc_etherdev and kmalloc_array are called first and destroyed last.
> > Safe to use devm to remove frees.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
>
> Hi,
>
> using dmam_alloc_coherent() would go even one step further I think.
> It would remove moxart_mac_free_memory() completely.
>
> Then IIUC, using devm_register_netdev() would keep things ordered and
> moxart_remove() could also be removed completely.
>
> (by inspection only)
Right and that's the issue. I don't have hardware to test this on.
Limiting this to alloc_etherdev and kmalloc_array is safe as they are
the last to go.
>
> CJ
>
>
> > drivers/net/ethernet/moxa/moxart_ether.c | 19 ++++++-------------
> > 1 file changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/moxa/moxart_ether.c b/drivers/net/ethernet/moxa/moxart_ether.c
> > index 96dc69e7141f..06c632c90494 100644
> > --- a/drivers/net/ethernet/moxa/moxart_ether.c
> > +++ b/drivers/net/ethernet/moxa/moxart_ether.c
> > @@ -81,9 +81,6 @@ static void moxart_mac_free_memory(struct net_device *ndev)
> > dma_free_coherent(&priv->pdev->dev,
> > RX_REG_DESC_SIZE * RX_DESC_NUM,
> > priv->rx_desc_base, priv->rx_base);
> > -
> > - kfree(priv->tx_buf_base);
> > - kfree(priv->rx_buf_base);
> > }
> >
> > static void moxart_mac_reset(struct net_device *ndev)
> > @@ -461,15 +458,14 @@ static int moxart_mac_probe(struct platform_device *pdev)
> > unsigned int irq;
> > int ret;
> >
> > - ndev = alloc_etherdev(sizeof(struct moxart_mac_priv_t));
> > + ndev = devm_alloc_etherdev(p_dev, sizeof(struct moxart_mac_priv_t));
> > if (!ndev)
> > return -ENOMEM;
> >
> > irq = irq_of_parse_and_map(node, 0);
> > if (irq <= 0) {
> > netdev_err(ndev, "irq_of_parse_and_map failed\n");
> > - ret = -EINVAL;
> > - goto irq_map_fail;
> > + return -EINVAL;
> > }
> >
> > priv = netdev_priv(ndev);
> > @@ -511,15 +507,15 @@ static int moxart_mac_probe(struct platform_device *pdev)
> > goto init_fail;
> > }
> >
> > - priv->tx_buf_base = kmalloc_array(priv->tx_buf_size, TX_DESC_NUM,
> > - GFP_KERNEL);
> > + priv->tx_buf_base = devm_kmalloc_array(p_dev, priv->tx_buf_size,
> > + TX_DESC_NUM, GFP_KERNEL);
> > if (!priv->tx_buf_base) {
> > ret = -ENOMEM;
> > goto init_fail;
> > }
> >
> > - priv->rx_buf_base = kmalloc_array(priv->rx_buf_size, RX_DESC_NUM,
> > - GFP_KERNEL);
> > + priv->rx_buf_base = devm_kmalloc_array(p_dev, priv->rx_buf_size,
> > + RX_DESC_NUM, GFP_KERNEL);
> > if (!priv->rx_buf_base) {
> > ret = -ENOMEM;
> > goto init_fail;
> > @@ -553,8 +549,6 @@ static int moxart_mac_probe(struct platform_device *pdev)
> > init_fail:
> > netdev_err(ndev, "init failed\n");
> > moxart_mac_free_memory(ndev);
> > -irq_map_fail:
> > - free_netdev(ndev);
> > return ret;
> > }
> >
> > @@ -565,7 +559,6 @@ static void moxart_remove(struct platform_device *pdev)
> > unregister_netdev(ndev);
> > devm_free_irq(&pdev->dev, ndev->irq, ndev);
> > moxart_mac_free_memory(ndev);
> > - free_netdev(ndev);
> > }
> >
> > static const struct of_device_id moxart_mac_match[] = {
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-08-08 20:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-08 4:03 [PATCH net-next] net: moxart_ether: use devm in probe Rosen Penev
2024-08-08 8:18 ` Christophe JAILLET
2024-08-08 20:21 ` Rosen Penev
2024-08-08 14:58 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).