| Message ID | 20241001212204.308758-5-rosenp@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | gianfar cleanups | expand |
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 > >
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);
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(-)