Re: [PATCH net-next] net: moxart_ether: use devm in probe

From: Rosen Penev
Date: Thu Aug 08 2024 - 16:21:49 EST


On Thu, Aug 8, 2024 at 1:18 AM Christophe JAILLET
<christophe.jaillet@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
>
> 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[] = {
>