Date: Mon, 11 Dec 2023 19:11:43 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc: Alvin Šipraga <ALSI@...g-olufsen.dk>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
	"andrew@...n.ch" <andrew@...n.ch>,
	"f.fainelli@...il.com" <f.fainelli@...il.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next 2/7] net: dsa: realtek: put of node after MDIO
 registration

On Fri, Dec 08, 2023 at 03:05:41PM -0300, Luiz Angelo Daros de Luca wrote:
> Reviewing the code again, I believe it was not just misplacing the
> of_put_node() but probably calling it twice.
> 
> devm_mdiobus_alloc() doesn't set the dev in mii_bus. So, dev is all
> zeros. The dev.of_node normal place to be defined is:
> 
> devm_of_mdiobus_register()
>   __devm_of_mdiobus_register()
>     __of_mdiobus_register()
>       device_set_node()
> 
> The only way for that value, set by the line I removed, to persist is
> when the devm_of_mdiobus_register() fails before device_set_node().

Did you consider that __of_mdiobus_register() -> device_set_node() is
actually overwriting priv->user_mii_bus->dev.of_node with the same value?
So the reference to mdio_np persists even if technically overwritten.
The fact that the assignment looks redundant is another story.

> My guess is that it was set to be used by realtek_smi_remove() if it
> is called when registration fails. However, in that case, both
> realtek_smi_setup_mdio() and realtek_smi_setup_mdio() would put the

You listed the same function name twice. You meant realtek_smi_remove()
the second time?

> node. So, either the line is useless or it will effectively result in
> calling of_node_put() twice.

False logic, since realtek_smi_remove() is not called when probe() fails.
ds->ops->setup() is called from probe() context. So no double of_node_put().
That's a general rule with the kernel API. When a setup API function fails,
it is responsible of cleaning up the temporary things it did. The
teardown API function is only called when the setup was performed fully.

(the only exception I'm aware of is the Qdisc API, but that's not
exactly the best model to follow)

> If I really needed to put that node in the realtek_smi_remove(), I
> would use a dedicated field in realtek_priv instead of reusing a
> reference for it inside another structure.
> 
> I'll add some notes to the commit message about all these but moving
> the of_node_put() to the same function that gets the node solved all
> the issues.

"Solved all the issues" - what are those issues, first of all?

The simple fact is: of_get_compatible_child() returns an OF node with an
elevated refcount. It passes it to of_mdiobus_register() which does not
take ownership of it per se, but assigns it to bus->dev.of_node, which
is accessible until device_del() from mdiobus_unregister().

The PHY library does not make the ownership rules of the of_node very
clear, but since it takes no reference on it, it will fail in subtle
ways if you pull the carpet from under its feet.

For example, I expect of_mdio_find_bus() to fail. That is used only
rarely, like by the MDIO mux driver which I suppose you haven't tested.

If you want, you could make the OF MDIO API get() and put() the reference,
instead of using something it doesn't fully own. But currently the code
doesn't do that. Try to acknowledge what exists, first.

Powered by blists - more mailing lists