netdev - Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii

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: <CAJq09z64o96jURg-2ROgMRjQ9FTnL51kXQQcEpff1=TN11ShKw@mail.gmail.com>
Date: Sun, 28 Jan 2024 23:12:25 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: netdev@...r.kernel.org, linus.walleij@...aro.org, alsi@...g-olufsen.dk, 
	andrew@...n.ch, f.fainelli@...il.com, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	arinc.unal@...nc9.com, ansuelsmth@...il.com
Subject: Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii_bus setup

> On Tue, Jan 23, 2024 at 06:56:00PM -0300, Luiz Angelo Daros de Luca wrote:
> > The line assigning dev.of_node in mdio_bus has been removed since the
> > subsequent of_mdiobus_register will always overwrite it.
>
> Please use present tense and imperative mood. "Remove the line assigning
> dev.of_node, because ...".

OK

> >
> > ds->user_mii_bus is not assigned anymore[1].
>
> "As discussed in [1], allow the DSA core to be simplified, by not
> assigning ds->user_mii_bus when the MDIO bus is described in OF, as it
> is unnecessary."

OK

> > It should work as before as long as the switch ports have a valid
> > phy-handle property.
> >
> > Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
> > to mdiobus"), we can put the "mdio" node just after the MDIO bus
> > registration.
>
> > The switch unregistration was moved into realtek_common_remove() as
> > both interfaces now use the same code path.
>
> Hopefully you can sort this part out in a separate patch that's
> unrelated to the user_mii_bus cleanup, ideally in "net: dsa: realtek:
> common rtl83xx module".

Yes. With the introduction of rtl83xx_unregister_switch, this part is gone.

> >
> > [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> > ---
> >  drivers/net/dsa/realtek/realtek-mdio.c |  5 -----
> >  drivers/net/dsa/realtek/realtek-smi.c  | 15 ++-------------
> >  drivers/net/dsa/realtek/rtl83xx.c      |  2 ++
> >  3 files changed, 4 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> > index 0171185ec665..c75b4550802c 100644
> > --- a/drivers/net/dsa/realtek/realtek-mdio.c
> > +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> > @@ -158,11 +158,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev)
> >  {
> >       struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
> >
> > -     if (!priv)
> > -             return;
> > -
>
> The way I would structure these guards is I would keep them here, and
> not in rtl83xx_remove() and rtl83xx_shutdown(). Then I would make sure
> that rtl83xx_remove() is the exact opposite of just rtl83xx_probe(), and
> rtl83xx_unregister_switch() is the exact opposite of just rtl83xx_register_switch().

It looks like it is now, although "remove" mostly leaves the job for devm.

I'm still not sure if we have the correct shutdown/remove code. From
what I could understand, driver shutdown is called during system
shutdown while remove is called when the driver is removed. However,
it looks like that both might be called in sequence. Would it be
shutdown,remove? (it's probably that because there is the
dev_set_drvdata(priv->dev, NULL) in shutdown).
However, if shutdown should prepare the system for another OS, I
believe it should be asserting the hw reset as well or remove should
stop doing it. Are the dsa_switch_shutdown and dsa_switch_unregister
enough to prevent leaking traffic after the driver is gone? It does
disable all ports. Or should we have a fallback "isolate all ports"
when a hw reset is missing? I guess the u-boot driver does something
like that.

I don't think it is mandatory for this series but if we got something
wrong, it would be nice to fix it.

> And I would fix the error handling path of realtek_smi_probe() and
> realtek_mdio_probe() to call rtl83xx_remove() when rtl83xx_register_switch()
> fails.

With the rtl83xx_unregister_switch, it was kept in realtek_smi_probe() and
realtek_mdio_probe.

Regards,

Luiz