netdev - Re: [PATCH net-next 04/10] net: dsa: qca8k: put MDIO bus OF node on qca8k_mdio

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]

Date: Thu, 4 Jan 2024 21:25:57 -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 04/10] net: dsa: qca8k: put MDIO bus OF node on
 qca8k_mdio_register() failure

> of_get_child_by_name() gives us an OF node with an elevated refcount,
> which should be dropped when we're done with it. This is so that,
> if (of_node_check_flag(node, OF_DYNAMIC)) is true, the node's memory can
> eventually be freed.
>
> There are 2 distinct paths to be considered in qca8k_mdio_register():
>
> - devm_of_mdiobus_register() succeeds: since commit 3b73a7b8ec38 ("net:
>   mdio_bus: add refcounting for fwnodes to mdiobus"), the MDIO core
>   treats this well.
>
> - devm_of_mdiobus_register() or anything up to that point fails: it is
>   the duty of the qca8k driver to release the OF node.

In both cases, it is qca8k driver duty to put the OF node.
3b73a7b8ec38 just allows you to put it just after mdiobus_registration
and not only after mdiobus_unregistration. This patch does put it
correctly though.

> This change addresses the second case by making sure that the OF node
> reference is not leaked.
>
> The "mdio" node may be NULL, but of_node_put(NULL) is safe.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>  drivers/net/dsa/qca/qca8k-8xxx.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index ec57d9d52072..5f47a290bd6e 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -949,10 +949,15 @@ qca8k_mdio_register(struct qca8k_priv *priv)
>         struct dsa_switch *ds = priv->ds;
>         struct device_node *mdio;
>         struct mii_bus *bus;
> +       int err;
> +
> +       mdio = of_get_child_by_name(priv->dev->of_node, "mdio");

I couldn't get why you moved this here. It will only be used in
of_device_is_available()

>
>         bus = devm_mdiobus_alloc(ds->dev);
> -       if (!bus)
> -               return -ENOMEM;
> +       if (!bus) {
> +               err = -ENOMEM;
> +               goto out_put_node;
> +       }
>
>         bus->priv = (void *)priv;
>         snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d.%d",
> @@ -962,12 +967,12 @@ qca8k_mdio_register(struct qca8k_priv *priv)
>         ds->user_mii_bus = bus;
>
>         /* Check if the devicetree declare the port:phy mapping */
> -       mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
>         if (of_device_is_available(mdio)) {

This is off-topic for this patch but it sounds strange to have an mdio
node marked as disabled. The legacy code seems to cover old
device-trees that do not have the mdio node. Supporting an existing
but disabled mdio might point in the wrong direction. Anyway, it would
be good to have common behavior across drivers. I can update the
realtek driver to match whatever is the recommended usage.

As a bonus, if a disabled node should return an error and not fallback
to the legacy user mii, devm_mdiobus_register could be replaced by
devm_of_mdiobus_register() as it handles mdio==NULL.

>                 bus->name = "qca8k user mii";
>                 bus->read = qca8k_internal_mdio_read;
>                 bus->write = qca8k_internal_mdio_write;
> -               return devm_of_mdiobus_register(priv->dev, bus, mdio);
> +               err = devm_of_mdiobus_register(priv->dev, bus, mdio);
> +               goto out_put_node;
>         }
>
>         /* If a mapping can't be found the legacy mapping is used,
> @@ -976,7 +981,13 @@ qca8k_mdio_register(struct qca8k_priv *priv)
>         bus->name = "qca8k-legacy user mii";
>         bus->read = qca8k_legacy_mdio_read;
>         bus->write = qca8k_legacy_mdio_write;
> -       return devm_mdiobus_register(priv->dev, bus);
> +
> +       err = devm_mdiobus_register(priv->dev, bus);
> +
> +out_put_node:
> +       of_node_put(mdio);
> +
> +       return err;
>  }
>
>  static int
> --
> 2.34.1
>
Regards,

Luiz