Re: [PATCH net-next v3 1/4] driver core: add fw_devlink supplier-copy helper

On Tue, Aug 18, 2026 at 12:58:57AM -0600, James Hilliard wrote:

Some firmware nodes describe resources shared by devices instantiated for
their children, but the container node itself is never converted to a
struct device. The firmware parser should retain the topology as
described, while the framework which creates the children can identify
the actual consumers.

Add fw_devlink_copy_suppliers() so such a framework can copy the direct
supplier links from a container to a real consumer firmware node before
the consumer is registered. The normal device_add() path then converts
the copied dependencies into device links at the correct point in device
registration.

Leave the source links in place for other children, suppress duplicate
links and roll back newly allocated links if a copy fails. Skip ignored
links and clear cycle flags on newly copied links, since cycle
classification must be recomputed for the new consumer topology. Reject
calls after the target firmware node has been associated with a device.
Check that association while holding the fwnode-link lock so a
concurrent device_add() either observes the copied links or makes the
helper reject the request.

What is this paragraph about? The workflow?

Add KUnit coverage for filtering, cycle-flag handling, idempotency, the
pre-registration contract and conversion into an active device link.

...

+int fw_devlink_copy_suppliers(struct fwnode_handle *to,
+			      struct fwnode_handle *from)
+{
+	struct list_head *first;
+	struct fwnode_link *link;
+	int ret;
+
+	if (!to || !from)
+		return -EINVAL;
+	if (!fw_devlink_flags || to == from)
+		return 0;

I think if to == NULL and from == NULL, it's fine to return 0.

	if (to == from)
		return 0;
	if (!to || !from)
		return -EINVAL;
	if (!fw_devlink_flags)
		return 0;

+	fw_devlink_parse_fwnode(from);
+
+	guard(mutex)(&fwnode_link_lock);

+ blank line.

+	if (READ_ONCE(to->dev))
+		return -EBUSY;
+
+	first = to->suppliers.next;

No, we have list.h and APIs for a reason.

+	list_for_each_entry(link, &from->suppliers, c_hook) {
+		u8 flags = link->flags & ~FWLINK_FLAG_CYCLE;
+
+		if (flags & FWLINK_FLAG_IGNORE)
+			continue;
+
+		ret = __fwnode_link_add(to, link->supplier, flags);
+		if (ret)
+			goto rollback;
+	}
+
+	return 0;
+
+rollback:
+	while (to->suppliers.next != first) {

Same here. I think the above and this needs to be thought through as this looks
like an AI shortcut without thinking of the existing APIs and possible different
(better) implementation.

+		link = list_first_entry(&to->suppliers, struct fwnode_link,
+					c_hook);
+		__fwnode_link_del(link);
+	}
+
+	return ret;
+}

...

+#include <kunit/platform_device.h>
+#include <kunit/test.h>
+
+#include <linux/device.h>
+#include <linux/fwnode.h>

+ list.h

+#include <linux/platform_device.h>
+
+#define FWNODE_LINK_TEST_DRIVER_NAME	"fwnode-link-test"
+
+struct fwnode_link_test_context {
+	struct fwnode_handle consumer;
+	struct fwnode_handle container;
+	struct fwnode_handle supplier_a;
+	struct fwnode_handle supplier_b;
+};
+
+static int fwnode_link_test_probe(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct platform_driver fwnode_link_test_driver = {
+	.probe = fwnode_link_test_probe,
+	.driver = {
+		.name = FWNODE_LINK_TEST_DRIVER_NAME,
+	},
+};
+
+static void fwnode_link_test_cleanup(void *data)
+{
+	struct fwnode_link_test_context *context = data;
+
+	fwnode_links_purge(&context->consumer);
+	fwnode_links_purge(&context->container);
+	fwnode_links_purge(&context->supplier_a);
+	fwnode_links_purge(&context->supplier_b);
+}
+
+static struct fwnode_link_test_context *
+fwnode_link_test_init(struct kunit *test)
+{
+	struct fwnode_link_test_context *context;
+	int ret;
+
+	context = kunit_kzalloc(test, sizeof(*context), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, context);
+
+	fwnode_init(&context->consumer, NULL);
+	fwnode_init(&context->container, NULL);
+	fwnode_init(&context->supplier_a, NULL);
+	fwnode_init(&context->supplier_b, NULL);
+	ret = kunit_add_action_or_reset(test, fwnode_link_test_cleanup,
+					context);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	return context;
+}
+
+static struct platform_device *
+fwnode_link_test_register_pdev(struct kunit *test,
+			       struct fwnode_handle *fwnode)
+{
+	struct platform_device *pdev;
+	int ret;
+
+	pdev = kunit_platform_device_alloc(test, FWNODE_LINK_TEST_DRIVER_NAME,
+					   PLATFORM_DEVID_AUTO);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+	device_set_node(&pdev->dev, fwnode);
+	ret = kunit_platform_device_add(test, pdev);
+	if (ret) {
+		KUNIT_FAIL(test, "failed to register platform device: %d", ret);
+		return NULL;
+	}
+
+	return pdev;
+}
+
+static unsigned int fwnode_supplier_count(struct fwnode_handle *fwnode)
+{
+	struct fwnode_link *link;
+	unsigned int count = 0;
+
+	list_for_each_entry(link, &fwnode->suppliers, c_hook)
+		count++;

We have an existing API for this. I recommend to stop using AI for a moment
and just read the existing code thoroughly (list.h) and see what we have
in the kernel.

+	return count;
+}

...

+int fw_devlink_copy_suppliers(struct fwnode_handle *to,
+			      struct fwnode_handle *from);
 bool fw_devlink_is_strict(void);

It might be better to split a test into a separate patch. But I don't care
about this much.

-- 
With Best Regards,
Andy Shevchenko