netdev - Re: [PATCH net-next 4/6] net: gianfar: use devm for register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKxU2N8jOTyKAGeUm7JGwrsDbQviwtSkGuuonYxj2ZV3ni82Ag@mail.gmail.com>
Date: Wed, 2 Oct 2024 12:40:48 -0700
From: Rosen Penev <rosenp@...il.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: netdev@...r.kernel.org, andrew@...n.ch, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	linux-kernel@...r.kernel.org, claudiu.manoil@....com
Subject: Re: [PATCH net-next 4/6] net: gianfar: use devm for register_netdev

On Wed, Oct 2, 2024 at 12:29 AM Maxime Chevallier
<maxime.chevallier@...tlin.com> wrote:
>
> Hi,
>
> On Tue,  1 Oct 2024 14:22:02 -0700
> Rosen Penev <rosenp@...il.com> wrote:
>
> > Avoids manual unregister netdev.
> >
> > Signed-off-by: Rosen Penev <rosenp@...il.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
>
>