netdev - Re: [PATCH net-next] net: dsa: OF-ware slave_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 PHC
Open Source and information security mailing list archives

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

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

Date:   Sat, 12 Feb 2022 00:39:20 -0300
From:   Luiz Angelo Daros de Luca <luizluca@...il.com>
To:     Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "olteanv@...il.com" <olteanv@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next] net: dsa: OF-ware slave_mii_bus (RFC)

> > @@ -924,7 +926,9 @@ static int dsa_switch_setup(struct dsa_switch *ds)
> >
> >               dsa_slave_mii_bus_init(ds);
> >
> > -             err = mdiobus_register(ds->slave_mii_bus);
> > +             dn = of_get_child_by_name(ds->dn, "mdio");
>
> of_node_put(dn) after registration? Or else who will put it?

Thanks Alvin, I'll need some help here.

Is it expected for every code that receives a device_node and keeps a
reference to it to call of_node_get()?  I checked of_get_child_by_name
and it seems it just parses the device_node but does not save it.
I'll put it after the registration.

I got confused looking at the dsa_port->dn usage. It is initialized at
dsa_switch_parse_ports_of() with:

for_each_available_child_of_node() {
      ...
      dsa_port_parse_of(dp, port);
}

dsa_port_parse_of(dp, dn) {
      dp->dn = dn;
      ...
}

But nobody calls of_node_put(port) when there is no error.

Should it have used a of_node_put(port) after dsa_port_parse_of() is
called and 'dp->dn = of_node_get(dn)' with a corresponding
of_node_put() when the port is destroyed?

And I dropped "ds->dn" as I can use "ds->dev->of_node". The code
indicates it can be null but it looks like all methods I use seem to
play nice with null values. It's getting even smaller:

 #include <linux/slab.h>
#include <linux/rtnetlink.h>
#include <linux/of.h>
+#include <linux/of_mdio.h>
#include <linux/of_net.h>
#include <net/devlink.h>
#include <net/sch_generic.h>
@@ -869,6 +870,7 @@ static int dsa_switch_setup_tag_protocol(struct
dsa_switch *ds)
static int dsa_switch_setup(struct dsa_switch *ds)
{
       struct dsa_devlink_priv *dl_priv;
+       struct device_node *dn;
       struct dsa_port *dp;
       int err;

@@ -924,7 +926,10 @@ static int dsa_switch_setup(struct dsa_switch *ds)

               dsa_slave_mii_bus_init(ds);

-               err = mdiobus_register(ds->slave_mii_bus);
+               dn = of_get_child_by_name(ds->dev->of_node, "mdio");
+
+               err = of_mdiobus_register(ds->slave_mii_bus, dn);
+               of_node_put(dn);
               if (err < 0)
                       goto free_slave_mii_bus;
       }

Regards,

Luiz