netdev - Re: [PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation if its OF node has status = "disabled"

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: <CAJq09z4--Ug+3FAmp=EimQ8HTQYOWOuVon-PUMGB5a1N=RPv4g@mail.gmail.com>
Date: Thu, 4 Jan 2024 23:19:20 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>, 
	Alvin Šipraga <alsi@...g-olufsen.dk>, 
	Linus Walleij <linus.walleij@...aro.org>, Florian Fainelli <florian.fainelli@...adcom.com>, 
	Hauke Mehrtens <hauke@...ke-m.de>, Christian Marangi <ansuelsmth@...il.com>, 
	Arınç ÜNAL <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation if
 its OF node has status = "disabled"

Em qui., 4 de jan. de 2024 às 11:01, Vladimir Oltean
<vladimir.oltean@....com> escreveu:
>
> Currently the driver calls the non-OF devm_mdiobus_register() rather
> than devm_of_mdiobus_register() for this case, but it seems to rather
> be a confusing coincidence, and not a real use case that needs to be
> supported.
>
> If the device tree says status = "disabled" for the MDIO bus, we
> shouldn't need an MDIO bus at all. Instead, just exit as early as
> possible and do not call any MDIO API.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>  drivers/net/dsa/qca/qca8k-8xxx.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 5f47a290bd6e..21e36bc3c015 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -949,9 +949,11 @@ qca8k_mdio_register(struct qca8k_priv *priv)
>         struct dsa_switch *ds = priv->ds;
>         struct device_node *mdio;
>         struct mii_bus *bus;
> -       int err;
> +       int err = 0;
>
>         mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
> +       if (mdio && !of_device_is_available(mdio))
> +               goto out;

Hum.. that's why you moved this call here in the previous patch.

Don't you still need to put the node that is not available? Just put
it unconditionally whenever you exit this function after you get it.
of_node_put() can handle even NULL.

I'm not sure if this and other simple switches can be useful without a
valid MDIO. Anyway, wouldn't it be equivalent to having an empty mdio
node? It looks like it would work as well but without a specific code
path.

>         bus = devm_mdiobus_alloc(ds->dev);
>         if (!bus) {
> @@ -967,7 +969,7 @@ qca8k_mdio_register(struct qca8k_priv *priv)
>         ds->user_mii_bus = bus;
>
>         /* Check if the devicetree declare the port:phy mapping */
> -       if (of_device_is_available(mdio)) {
> +       if (mdio) {
>                 bus->name = "qca8k user mii";
>                 bus->read = qca8k_internal_mdio_read;
>                 bus->write = qca8k_internal_mdio_write;
> @@ -986,7 +988,7 @@ qca8k_mdio_register(struct qca8k_priv *priv)
>
>  out_put_node:
>         of_node_put(mdio);
> -
> +out:
>         return err;
>  }
>
> --
> 2.34.1
>