Date: Sat, 16 Dec 2023 01:26:20 -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

> On Wed, Dec 13, 2023 at 01:37:27AM -0300, Luiz Angelo Daros de Luca wrote:
> > 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 try to think critically about the changes you make, you'll get
> better at it with time. Reading code is what we all do, really, and
> making educated guesses about what we saw. It's error prone for everyone
> involved, which is why we use review to confront what we all understand.

I'm not trying to blame anyone. We all make mistakes. However, when we
have the same mistake happening in different situations, maybe we are
only dealing with the consequences and not the cause.

> This is not only an encouragement, but also a subtle hint that you will
> endlessly frustrate those who have to read more than you did, in order
> to review your proposals, only for you to complain that you have to read
> too much when making changes.

Sorry if that's what my words said. It really wasn't my intention. I'm
not complaining about reading the code. It is actually a pleasure. But
I'll bother the MDIO guys on that matter :)

> Anyway.
>
> > > 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?
>
> ETHERNET PHY LIBRARY
> M:      Andrew Lunn <andrew@...n.ch>
> M:      Heiner Kallweit <hkallweit1@...il.com>
> R:      Russell King <linux@...linux.org.uk>
> L:      netdev@...r.kernel.org
> S:      Maintained
> F:      Documentation/ABI/testing/sysfs-class-net-phydev
> F:      Documentation/devicetree/bindings/net/ethernet-phy.yaml
> F:      Documentation/devicetree/bindings/net/mdio*
> F:      Documentation/devicetree/bindings/net/qca,ar803x.yaml
> F:      Documentation/networking/phy.rst
> F:      drivers/net/mdio/
> F:      drivers/net/mdio/acpi_mdio.c
> F:      drivers/net/mdio/fwnode_mdio.c
> F:      drivers/net/mdio/of_mdio.c
> F:      drivers/net/pcs/
> F:      drivers/net/phy/
> F:      include/dt-bindings/net/qca-ar803x.h
> F:      include/linux/*mdio*.h
> F:      include/linux/linkmode.h
> F:      include/linux/mdio/*.h
> F:      include/linux/mii.h
> F:      include/linux/of_net.h
> F:      include/linux/phy.h
> F:      include/linux/phy_fixed.h
> F:      include/linux/phylib_stubs.h
> F:      include/linux/platform_data/mdio-bcm-unimac.h
> F:      include/linux/platform_data/mdio-gpio.h
> F:      include/trace/events/mdio.h
> F:      include/uapi/linux/mdio.h
> F:      include/uapi/linux/mii.h
> F:      net/core/of_net.c
>
> > > 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.
>
> Ok, I'm not saying "revert it NOW". But it would be good if you could
> revert it as part of the realtek-common series, as a last patch
> (provided that you've done your homework and nobody else relies on it).
> Or at least not forget about it.

I'll add the revert patch. I believe I just need to pay attention to
those cases where phy_read/phy_write are included in ds_ops.

> > > 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.
>
> And when you consider that DSA has better documentation than most
> subsystems out there....
>
> Would it blow your mind away if I told you that the documentation is
> written based on the code? The same code from which you draw a lazy
> conclusion.
>
> You have perfectly laid out why the code is not the problem, and why the
> documentation is not the solution. The problem is the unwillingness to
> spend time to understand, but to want to push your changes nonetheless.
> The problem is on your end. I'm sorry, it has to be said.

Oh, I did spend a lot of time on it, hundreds from my free and sleep
time :). Sometimes you just cannot understand by yourself. And yes, I
do believe in most cases I'm wrong.

> > 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.
>
> I don't have time, Luiz. I spent 1 and a half hours today replying
> to just your emails, and one and a half hours yesterday. I have a job.

Vladimir, I cannot thank you enough. I probably wouldn't get it (and
maybe I still didn't) without your help.

> You've made me see that I'm wasting my time writing documentation for
> people who want instant gratification instead. I don't know how to get
> to them. I'll think about it some more.

I wouldn't say submitting patches to the kernel brings some kind of
gratification (except if you are a type of masochist). I'm just trying
to help.

Probably you are spending more time answering my emails than you
should. ML are great but their knowledge tends to fade away with time.
The answer to some topics I brought should be there, not here. Save
your time for reviewing the code.

I normally work a lot more than I should just to avoid the chance of
reworking on it. A comment in code like /* This behavior is
deprecated. Please see https://kernel.org/doc/xxx */ would both avoid
the code to be spread and alert maintainers that there is some work to
be done. We all have nicer things to do instead but it could save some
future headaches. My 2 cents.

Regards,

Luiz

Powered by blists - more mailing lists