Message-ID: <CAJq09z6veGUNymR6hxabBPercR6+7gFC-FhwiVM+wScm5xDREA@mail.gmail.com>
Date: Wed, 13 Dec 2023 01:37:27 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Vladimir Oltean <olteanv@...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

> > 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?

Yes. Just like that. :)

> Some drivers like bcm_sf2.c? :)

Yeah.

> 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?

It is not a suggestion. If it was a suggestion (like in a comment), it
would be a little bit better. Some got it right and some didn't.

The OF API will only return you a node with the refcount incremented.
You need to put it in somewhere after that. That will happen no matter
how you use the node and that's OK. The problem is when I pass that
reference to another function, I need to somehow know if it keeps a
reference to that node and not increments the refconf. If it does not
keep the reference, it is OK. If it keeps the reference and gets it,
it is also OK.

The answer "just read (all the multiple level and different) code
(paths)" is fated to fail. The put after registration in DSA core code
is just an example of how it did not work.

> > > 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.

Sure, phylib is still for netdev, right?

I'll redo this patch to avoid putting the node before unregistration.

> > 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.

Please, not before this patch series gets merged or you'll break
MDIO-connected Realtek DSA switches, at least the IRQ handling.
I'll send the revert myself afterwards.

> 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.

OK. I'll not try to fix that but revert it as soon as possible without
breaking existing code. You need too much conditions to make it
trigger a bug:
1) use dynamic OF
2) no other code also keep a reference to that node
3) a call that actually reads the of_node from user_mii.dev

But, as you pointed out, OF and user_mii should not work together.

> 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).

The change to MDIO code would not be a requirement for this patch
series. I'll deal with each front independently.

> 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.

That one I might be novice enough to answer :).

When you start to write a new driver, you read the docs to get a
general idea. However, as code moves faster than docs, you mainly rely
on code. So, you just choose a driver (or a couple of them) to inspire
you. You normally prefer a small driver because it is less code to
read and it might be just enough to get started. As long as it is
mainline, nothing indicates it should not be used as a reference.

I wasn't the one that wrote most of the Realtek DSA driver but I see
the same OF + user_mii_bus pattern in more than one driver. If you
want to stop spreading, as rewriting all affected drivers might not be
an option, a nice /* TODO: convert to user YXZ */ comment might do the
trick. An updated doc suggesting a driver to be used as an example
would also be nice.

The doc update you sent about the "user MDIO bus documentation"
telling us we should not use user_mii_bus when we describe the
internal MDIO in the device-tree made me more confused. But I'll
comment on that thread.

Regards,

Luiz

Powered by blists - more mailing lists