Message-ID: <CAGETcx_kH1Z7YuciybYac+q5qpA3EQb+7q77vx5OV+dk8_uwCQ@mail.gmail.com>
Date: Tue, 13 Feb 2024 19:28:35 -0800
From: Saravana Kannan <saravanak@...gle.com>
To: Michael Pratt <mcpratt@...me>
Cc: devicetree@...r.kernel.org, gregkh@...uxfoundation.org, 
	linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org, 
	abel.vesa@...aro.org, alexander.stein@...tq-group.com, 
	andriy.shevchenko@...ux.intel.com, bigunclemax@...il.com, brgl@...ev.pl, 
	colin.foster@...advantage.com, djrscally@...il.com, 
	dmitry.baryshkov@...aro.org, festevam@...il.com, fido_max@...ox.ru, 
	frowand.list@...il.com, geert@...ux-m68k.org, heikki.krogerus@...ux.intel.com, 
	kernel@...gutronix.de, linus.walleij@...aro.org, linux@...ck-us.net, 
	luca.weiss@...rphone.com, magnus.damm@...il.com, martin.kepplinger@...i.sm, 
	miquel.raynal@...tlin.com, rafal@...ecki.pl, ansuelsmth@...il.com, 
	richard@....at, sakari.ailus@...ux.intel.com, sudeep.holla@....com, 
	tglx@...utronix.de, tony@...mide.com, vigneshr@...com, dianders@...omium.org, 
	jpb@...nel.org, rafael@...nel.org
Subject: Re: [PATCH v1 0/4] fw_devlink: generically handle bad links to child fwnodes

On Mon, Feb 5, 2024 at 12:06 PM Saravana Kannan <saravanak@...gle.com> wrote:
>
> On Mon, Jan 22, 2024 at 5:46 PM Michael Pratt <mcpratt@...me> wrote:
> >
> > Hi all,
> >
> > First off, if you are CC'ed here, it's likely because you were
> > involved in or CC'ed in a patch series by Saravana Kannan for fw_devlink,
> > and for consistency, I'm kindly asking for review from you all as well.
> > If you think this may not affect your use cases, feel free to ignore this.
> > I'm also CC'ing Christian and Rafal from the Openwrt project.
> >
> > This series is following up on some excellent work from Saravana [1]
> > which is another patch series that includes some changes
> > that helped make it possible for fw_devlink to work with MTD partitions
> > that are a supplier fwnode by being a NVMEM provider. For an MTD partition
> > to become an NVMEM provider, it must be registered as a platform device
> > using of_platform_populate() or similar function
> > which was done in a previous commit [2]
> > but this also resulted in fw_devlink to apply
> > to those fwnodes and their child fwnodes.
> >
> > That regression caused consumer devices to defer indefinitely
> > while waiting for probing that will never happen or for device links
> > to form from fwnode links with fwnodes that are not associated
> > with any real device or driver that probes
> > (e.g. describing the location of a MAC address).
> >
> > Saravana's patch series helped in two ways:
> > First, by moving consumers from a child fwnode (in this case,
> > the "nvmem-cells" compatible node) to an ancestor fwnode
> > that represents the actual supplier device when that device probes,
> > which handles the case where
> > the supplier device has a probe attempt first. [3] [4]
> > And secondly, by specifically marking "nvmem-cells" compatible nodes
> > as populated during MTD partition parsing which also occurs during
> > the supplier device probe, which handles both cases of initial probe order,
> > but only for that node type. [5]
> >
> > However, there's some drawbacks to the second solution
>
> Oh, somehow missed this thread entirely until it saw some activity today.
>
> > from having to manually name which nodes need the workaround
> > for the corner case with the compatible string.
> > Most notably, that it's possible for this corner case
> > to apply to other nodes types, but also the fact that initial probe order
> > deciding whether or not everything probes in the intended order, if at all,
> > through driver core alone is still an issue with fw_devlink,
> > despite the fact that controlling probe order in driver core
> > is one of it's goals. In other words, the real problem is in driver core,
> > but the fix is in the mtd driver.
>
> It's been a while since I looked at MTD code, but the real problem is
> not in driver core, but how it's used incorrectly by the MTD/nvmem
> frameworks. Adding devices to a bus that'll never probe is wrong. I
> think there's also a case of two devices being created off the same DT
> node. While not technically wrong, it's weird when one of them never
> probes.
>
> I'll take a closer look and respond to this series later. Hopefully
> this week, but if not, then next week.
>
> As I said in the other patch, I don't like the series in the current
> form because it's changing APIs in not so great ways.
>
> fwnode_link_add() is supposed to be super dumb and simple. It's
> literally there just to avoid reparsing DT nodes multiple times.
> Nothing more ever. It just denotes the "pointers" between fwnode or DT
> nodes.
>
> The "smarts" should either be where fwnode links are converted into
> device links (and not have to fix them up) or where nvmem-cells is
> being parsed and converted to fwnode links.
>
> As I said in the other patch, let me take a closer look this week or
> next and get back. These things needs several hours of uninterrupted
> time for me to debug and unwind.

As promised, I took a closer look and left some comments in Patch 1
and 2. Patch 1 is 100% broken/wrong so the series will not be
accepted.

Just for the sake of understanding, can you send a patch that'll add
these additional compatible strings similar to how I handled
nvmem-cells and show my how much worse it is than this series?

>
> >
> > Unfortunately, with the Openwrt project
> > we are experiencing this regression again
> > by implementing the new NVMEM layout "fixed-layout"
> > after it was accepted into the kernel. [6]
> > This causes some subsystems of an SOC, like
> > ethernet or wireless or both depending on hardware and DTS,
> > and in some cases a completely different function like USB
> > to never probe once again, and the temporary fix, like before,
> > is by disabling fw_devlink. [7]
> >
> > Below is a simplified DTS of an Atheros device with the NVMEM layout:
> >
> >
> > &eth0 {
> >         nvmem-cells = <&macaddr_art_0>;
> >         nvmem-cell-names = "mac-address";
> > };
> >
> > &wifi {
> >         nvmem-cells = <&caldata_art_1000>;
> >         nvmem-cell-names = "calibration";
> > };
> >
> > &spi {
> >         status = "okay";
> >
> >         flash@0 {
> >                 compatible = "jedec,spi-nor";
> >
> >                 partitions {
> >                         compatible = "fixed-partitions";
> >
> >                         partition@...000 {
> >                                 label = "art";
> >                                 reg = <0xff0000 0x010000>;
> >                                 read-only;
> >
> >                                 nvmem-layout {
> >                                         compatible = "fixed-layout";
> >
> >                                         macaddr_art_0: macaddr@0 {
> >                                                 reg = <0x0 0x6>;
> >                                         };
> >
> >                                         caldata_art_1000: caldata@...0 {
> >                                                 reg = <0x1000 0x440>;
> >                                         };
> >                                 };
> >                         };
> >                 };
> >         };
> > };

In this example, can you walk me through the probe attempts/successes
of the nvmem supplier and it's consumers and at what point does
fw_devlink makes the wrong decision? You also said fw_devlink depends
on the order in which devices probe to work correctly. This is
definitely not the case/intention. So, if you can show me an example
of that, I'll fix that too.

I'm fairly certain this example will end up being a case of the nvmem
framework creating pointless devices or using a "bus" when it needs a
"class". But I don't want to assume.

I'm asking all these question to understand your case better and maybe
suggest a better fix that doesn't break fw_devlink or effectively
disable it.

Thanks,
Saravana

> >
> >
> > When the DTS is written this way, not only is there a different node
> > involved for NVMEM, but this node is a new node that is yet another
> > descendant in the tree. In other words, the "partition@...000" node
> > used to be what designated this device as the NVMEM provider
> > with the "nvmem-cells" compatible, so the node that represents
> > the actual probing device is now 4 parent nodes up in the tree
> > in this example instead of 3 parent nodes up in the tree as before.
> >
> > For the past year, and even before the "fixed-layout" issue came up,
> > I had been experimenting with a way to handle these reverse probe order
> > and linking of distant descendant node issues in a generic way instead of
> > naming exceptions with the compatible string, and this series is the
> > culmination of those efforts. It may look strange at first,
> > but there are a myriad set of cases to handle and other reasons
> > that led me to develop this approach of using existing bad device links
> > in order to find the correct fwnode to link to, and then redo
> > the relevant links for that consumer device altogether.
> > I'm concerned that doing this another way would be a much more massive
> > project that would basically rewrite what the fw_devlink feature does.
> > Or perhaps there would have to be a new, third form of device links
> > that would be a "placeholder" before it becomes a fwnode link.
> > Eventually, I came to the conclusion that
> > there simply is not enough information to form the correct fwnode link
> > before the real supplier device has a successful probe.
> >
> > Some of the changes proposed here are made on the extreme side of caution,
> > for example, checking for null dereference when it might not be necessary,
> > and reducing the activity of some functions in order to reduce
> > the amount of assumptions taking place in the middle of driver core
> > in cases where the new functions proposed here handles that just as well
> > and closer to a possible probe defer event
> > (e.g. not declaring a fwnode as NOT_DEVICE before
> > a probe attempt is expected to have happened).
> >
> > I have tried to make the commit messages as self-explanatory as I can,
> > but they might have ended up a little too verbose, and therefore confusing
> > but I'm ready to explain whatever has not been explained well already.
> > Lastly, this is my first attempt at sending a larger change to the kernel,
> > so I appreciate your time and patience very much.
> >
> > MCP
> >
> >
> > [1] https://lore.kernel.org/lkml/20230207014207.1678715-1-saravanak@google.com/
> >
> > [2] bcdf0315a61a29eb753a607d3a85a4032de72d94
> >
> > [3] 3a2dbc510c437ca392516b0105bad8e7970e6614
> >
> > [4] 411c0d58ca6faa9bc4b9f5382118a31c7bb92a6f
> >
> > [5] fb42378dcc7f247df56f0ecddfdae85487495fbc
> >
> > [6] 27f699e578b1a72df89dfa3bc42e093a01dc8d10
> >
> > [7] https://github.com/openwrt/openwrt/commit/703d667a0cdf6dfa402c08e72d0c77a257ca5009
> >
> >
> > Michael Pratt (4):
> >   driver core: fw_devlink: Use driver to determine probe ability
> >   driver core: fw_devlink: Link to supplier ancestor if no device
> >   driver core: fw_devlink: Add function device_links_fixup_suppliers()
> >   mtd: mtdpart: Allow fwnode links to NVMEM compatible fwnodes
> >
> >  drivers/base/base.h    |   1 +
> >  drivers/base/core.c    | 144 ++++++++++++++++++++++++++++++++++++++---
> >  drivers/base/dd.c      |   2 +
> >  drivers/mtd/mtdpart.c  |  10 ---
> >  include/linux/fwnode.h |   4 ++
> >  5 files changed, 143 insertions(+), 18 deletions(-)
> >
> >
> > base-commit: b0d326da462e20285236e11e4cbc32085de9f363
> > --
> > 2.30.2
> >
> >

Powered by blists - more mailing lists