Received-SPF: pass (google.com: domain of linux-kernel+bounces-52868-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 5 Feb 2024 16:00:11 +0100
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Michael Pratt <mcpratt@pm.me>
Cc: devicetree@vger.kernel.org, gregkh@linuxfoundation.org,
 linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org,
 saravanak@google.com, abel.vesa@linaro.org,
 alexander.stein@ew.tq-group.com, andriy.shevchenko@linux.intel.com,
 bigunclemax@gmail.com, brgl@bgdev.pl, colin.foster@in-advantage.com,
 djrscally@gmail.com, dmitry.baryshkov@linaro.org, festevam@gmail.com,
 fido_max@inbox.ru, frowand.list@gmail.com, geert@linux-m68k.org,
 heikki.krogerus@linux.intel.com, kernel@pengutronix.de,
 linus.walleij@linaro.org, linux@roeck-us.net, luca.weiss@fairphone.com,
 magnus.damm@gmail.com, martin.kepplinger@puri.sm, rafal@milecki.pl,
 ansuelsmth@gmail.com, richard@nod.at, sakari.ailus@linux.intel.com,
 sudeep.holla@arm.com, tglx@linutronix.de, tony@atomide.com,
 vigneshr@ti.com, dianders@chromium.org, jpb@kernel.org, rafael@kernel.org
Subject: Re: [PATCH v1 2/4] driver core: fw_devlink: Link to supplier
 ancestor if no device
Message-ID: <20240205160011.42d1cf80@xps-13>
In-Reply-To: <20240123014517.5787-3-mcpratt@pm.me>
References: <20240123014517.5787-1-mcpratt@pm.me>
 <20240123014517.5787-3-mcpratt@pm.me>
Organization: Bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi Michael,

First, I want to say, this is a great job as fw_devlinks in mtd and
nvmem are really not easy to handle. I am willing to help, despite my
very light understanding of what the core actually does with these
flags.

mcpratt@pm.me wrote on Tue, 23 Jan 2024 01:46:40 +0000:

> Driver core currently supports linking to the next parent fwnode,
> but is not yet handling cases where that parent
> is also a firmware child node not representing a real device,
> which can lead to an indefinite deferred probe in some cases.
> In this case, the fwnode that should actually be linked to
> is multiple ancestors up which presents a challenge where
> it is unknown how many ancestors up the node that
> represents the real probing device is. This makes the usage of
> fwnode_get_next_parent_dev() insufficient because the real device's
> fwnode may or may not be an ancestor of the next parent fwnode as well.
>=20
> Introduce flag FWNODE_FLAG_PARENT_IS_DEV
> in order to mark child firmware nodes of a device
> as having a parent device that can probe.
>=20
> Allow fwnode link creation to the original supplier fwnode's ancestors
> when the original supplier fwnode and any fwnodes in between are flagged
> as FWNODE_FLAG_NOT_DEVICE and/or FWNODE_FLAG_PARENT_IS_DEV
> with a new function __fwnode_link_add_parents() which then creates
> the fwnode link to a real device that provides the supplier's function.
>=20
> This depends on other functions to label a supplier fwnode
> as not a real device, which must be done before the fwnode links
> are created, and if after that, relevant links to the supplier
> would have to be deleted and have links recreated, otherwise,
> the fwnode link would be dropped before the device link is attempted
> or a fwnode link would not be able to become a device link at all,
> because they were created before these fwnode flags can have any effect.
>=20
> It also depends on the supplier device to actually probe first
> in order to have the fwnode flags in place to know for certain
> which fwnodes are non-probing child nodes
> of the fwnode for the supplier device.
>=20
> The use case of function __fw_devlink_pickup_dangling_consumers()
> is designed so that the parameters are always a supplier fwnode
> and one of it's parent fwnodes, so it is safer to assume and more specific
> that the flag PARENT_IS_DEV should be added there, rather than
> declaring the original supplier fwnode as NOT_DEVICE at that point.
> Because this function is called when the real supplier device probes
> and recursively calls itself for all child nodes of the device's fwnode,
> set the new flag here in order to let it propagate down
> to all descendant nodes, thereby providing the info needed later
> in order to link to the proper fwnode representing the supplier device.
>=20
> If a fwnode is flagged as FWNODE_FLAG_NOT_DEVICE
> by the time a device link is to be made with it,
> but not flagged as FWNODE_FLAG_PARENT_IS_DEV,
> the link is dropped, otherwise the device link
> is still made with the original supplier fwnode.
> Theoretically, we can also handle linking to an ancestor
> of the supplier fwnode when forming device links, but there
> are still cases where the necessary fwnode flags are still missing
> because the real supplier device did not probe yet.

I am not sure I follow this. In the following case, I would expect any
dependency towards node-c to be made against node-a. But the above
paragraph seems to tell otherwise: that the the link would be dropped
(and thus, not enforced) because recursively searching for a parent
that would be a device could be endless? It feels wrong, so I probably
mis

node-a {
	# IS DEV
	node-b {
		# PARENT IS DEV
		node-c {
			# PARENT IS DEV
		};
	};
};

Besides that, the commit feels like a good idea.

Thanks,
Miqu=C3=A8l