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

[Rosen Penev]

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[] = {
>