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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 5 Feb 2024 16:00:11 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Michael Pratt <mcpratt@...me>
Cc: devicetree@...r.kernel.org, gregkh@...uxfoundation.org,
 linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org,
 saravanak@...gle.com, 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, 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 2/4] driver core: fw_devlink: Link to supplier
 ancestor if no device

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