Date: Wed, 13 Dec 2023 15:04:19 +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 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.

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.

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

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

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.

Powered by blists - more mailing lists