Date: Tue, 12 Dec 2023 23:58:01 +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 Tue, Dec 12, 2023 at 12:47:57AM -0300, Luiz Angelo Daros de Luca wrote:
> The unregistration happens only in mdiobus_unregister(), where, I
> guess, it should avoid OF-specific code. Even if we put OF code there,
> we would need to know during mdiobus_unregister() if the
> bus->dev.of_node was gotten by of_mdio or someone else.
> 
> I believe it is not nice to externally assign dev.of_node directly to
> mdiobus but realtek switch driver is doing just that and others might
> be doing the same thing.

Well, make up your mind: earlier you said the user_mii_bus->dev.of_node
assignment from the Realtek DSA driver is redundant, because
devm_of_mdiobus_register() -> ... -> __of_mdiobus_register() does it
anyway. So if it's redundant, you can remove it and nothing changes.
What's so "not nice" about it that's worth complaining?

Are you trying to say that you're concerned that some drivers might be
populating the mii_bus->dev.of_node manually, and then proceeding to
call the _non-OF_ mdiobus_register() variant?

Some drivers like bcm_sf2.c? :)

That will be a problem, yes. If a clean result is the goal, I guess some
consolidation needs to be done before any new rule could be added.
Otherwise, yeah, we can just snap on one more lazy layer of complexity,
no problem. My 2 cents.

> The delegation of of_node_get/put to the caller seems to be just an
> easy workaround the fact that there is no good place to put a node
> that of_mdio would get. For devm functions, we could include the
> get/put call creating a new devm_of_mdiobus_unregister() but I believe
> devm and non-devm needs to be equivalent (except for the resource
> deallocation).

How did we get here, who suggested to get and put the references to the
OF node outside of the OF MDIO API?

> > 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.
> 
> What I saw in other drivers outside drivers/net is that one that
> allocates the dev will get the node before assigning dev.of_node and
> put it before freeing the device. The mdiobus case seems to be
> different. I believe it would make the code more robust if we could
> fix that inside OF MDIO API and not just document its behavior. It
> will also not break existing uses as extra get/put's are OK.
> 
> I believe we could add an unregister callback to mii_bus. It wouldn't
> be too complex:
> 
> From b5b059ea4491e9f745872220fb94d8105e2d7d43 Mon Sep 17 00:00:00 2001
> From: Luiz Angelo Daros de Luca <luizluca@...il.com>
> Date: Tue, 12 Dec 2023 00:26:06 -0300
> Subject: [PATCH] net: mdio: get/put device node during (un)registration
> 
> __of_mdiobus_register() was storing the device node in dev.of_node
> without increasing its refcount. It was implicitly delegating to the
> caller to maintain the node allocated until mdiobus was unregistered.
> 
> Now, the __of_mdiobus_register() will get the node before assigning it,
> and of_mdiobus_unregister_callback() will be called at the end of
> mdio_unregister().
> 
> Drivers can now put the node just after the MDIO registration.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> ---
> drivers/net/mdio/of_mdio.c | 12 +++++++++++-
> drivers/net/phy/mdio_bus.c |  3 +++
> include/linux/phy.h        |  3 +++
> 3 files changed, 17 insertions(+), 1 deletion(-)

I don't mean to be rude, but I don't have the time to dig into this any
further, sorry. If you are truly committed to better the phylib API,
please bring it up with the phylib people instead. I literally only
care about the thing that Alvin pointed out, which is that you made
unjustified changes to a DSA driver.

> If we don't fix that in OF MDIO API, we would need to fix
> fe7324b932222 as well, moving the put to the dsa_switch_teardown().

Oh, couldn't we straight-up revert that instead? :) The user_mii_bus
is created by DSA for compatibility with non-OF. I cannot understand
why you insist to attach an OF node to it.

But otherwise, yes, it is the same situation: of_node_put(), called
before unregistering an MDIO bus registered with of_mdiobus_register(),
means that the full OF API on this MDIO bus may not work correctly.
I don't know the exact conditions though. It might be marginal or even
a bug that's impossible to trigger. I haven't tested anything.

In any case, while I encourage you to make OF node refcounting work in
the way that you think is intuitive, I want to be clear about one thing,
and that is that I'm not onboard with modifying phylib to make a non
use-case in DSA work, aka OF-aware user_mii_bus (an oxymoron).

I understand why a driver may want a ds->user_mii_bus. And I understand
why a driver may want an MDIO bus with an of_node. What I don't understand
is who might want both at the same time, and why.

Powered by blists - more mailing lists