Avoids manual unregister netdev.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 66818d63cced..07936dccc389 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -3272,7 +3272,7 @@ static int gfar_probe(struct platform_device *ofdev)
 	/* Carrier starts down, phylib will bring it up */
 	netif_carrier_off(dev);
 
-	err = register_netdev(dev);
+	err = devm_register_netdev(&ofdev->dev, dev);
 
 	if (err) {
 		pr_err("%s: Cannot register net device, aborting\n", dev->name);
@@ -3338,8 +3338,6 @@ static void gfar_remove(struct platform_device *ofdev)
 	of_node_put(priv->phy_node);
 	of_node_put(priv->tbi_node);
 
-	unregister_netdev(priv->ndev);
-
 	if (of_phy_is_fixed_link(np))
 		of_phy_deregister_fixed_link(np);
 
-- 
2.46.2

Hi,

On Tue,  1 Oct 2024 14:22:02 -0700
Rosen Penev <rosenp@gmail.com> wrote:

> Avoids manual unregister netdev.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/ethernet/freescale/gianfar.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 66818d63cced..07936dccc389 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -3272,7 +3272,7 @@ static int gfar_probe(struct platform_device *ofdev)
>  	/* Carrier starts down, phylib will bring it up */
>  	netif_carrier_off(dev);
>  
> -	err = register_netdev(dev);
> +	err = devm_register_netdev(&ofdev->dev, dev);

I wonder if this is not a good opportunity to also move the
registration at the end of this function. Here, the netdev is
registered but some configuration is still being done afterwards, such
as WoL init and internal filter configuration.

There's the ever so slightly chance that traffic can start flowing
before these filters are configured, which could lead to unexpected
side effects. We usually register the netdev as a very last step, once
all initial configuration is done and the device is ready to be used.

As you're doing some cleanup on the registration code itself, it seems
like a good opportunity to change that.

Thanks,

Maxime

On Wed, Oct 2, 2024 at 12:29 AM Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:
>
> Hi,
>
> On Tue,  1 Oct 2024 14:22:02 -0700
> Rosen Penev <rosenp@gmail.com> wrote:
>
> > Avoids manual unregister netdev.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/net/ethernet/freescale/gianfar.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> > index 66818d63cced..07936dccc389 100644
> > --- a/drivers/net/ethernet/freescale/gianfar.c
> > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > @@ -3272,7 +3272,7 @@ static int gfar_probe(struct platform_device *ofdev)
> >       /* Carrier starts down, phylib will bring it up */
> >       netif_carrier_off(dev);
> >
> > -     err = register_netdev(dev);
> > +     err = devm_register_netdev(&ofdev->dev, dev);
>
> I wonder if this is not a good opportunity to also move the
> registration at the end of this function. Here, the netdev is
> registered but some configuration is still being done afterwards, such
> as WoL init and internal filter configuration.
>
> There's the ever so slightly chance that traffic can start flowing
> before these filters are configured, which could lead to unexpected
> side effects. We usually register the netdev as a very last step, once
> all initial configuration is done and the device is ready to be used.
>
> As you're doing some cleanup on the registration code itself, it seems
> like a good opportunity to change that.
There seem to be a bunch of netdev_info calls. I assume those need a
registered netdev.

Additionally, the irqs are allocated in _open instead of _probe. I
assume those would need to be moved.
>
> Thanks,
>
> Maxime
>
>