Re: [PATCH v1 2/4] driver core: fw_devlink: Link to supplier ancestor if no device

[Miquel Raynal]

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@xxxxx 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.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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èl