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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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;
> +}

...

> +++ b/drivers/base/test/fwnode-link-test.c

> +#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;
> +}

...

> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h

> +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