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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Mixing goto-based error handling with guard() cleanup in fw_devlink_copy_suppliers().
--

--- Patch [1]: [PATCH net-next v3 1/4] driver core: add fw_devlink supplier-copy helper ---
commit 143585594c89c21bf4350877fc73c0e66d637db3
Author: James Hilliard <james.hilliard1@gmail.com>

driver core: add fw_devlink supplier-copy helper

This commit adds fw_devlink_copy_suppliers() to allow frameworks to copy
direct supplier links from a container firmware node to a real consumer
firmware node before the consumer is registered. It leaves source links
intact, skips ignored links, and clears cycle flags for newly copied links.

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4d026682944f2..cf3f4133391d0 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
[ ... ]
> @@ -2394,6 +2394,66 @@ static void fw_devlink_link_device(struct device *dev)
[ ... ]
> +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;
> +	fw_devlink_parse_fwnode(from);
> +
> +	guard(mutex)(&fwnode_link_lock);
> +	if (READ_ONCE(to->dev))
> +		return -EBUSY;
> +
> +	first = to->suppliers.next;
> +	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;

[Severity: Medium]
Is it safe to mix goto-based error handling with scope-based cleanup here?

According to the cleanup subsystem guidelines in include/linux/cleanup.h,
usage of goto and cleanup helpers like guard() should never be mixed in the
same function to avoid confusing ownership semantics. 

Could this rollback logic be converted to use scope-based cleanup, or could
the guard() be replaced with traditional mutex_lock and mutex_unlock calls?

> +	}
> +
> +	return 0;
> +
> +rollback:
> +	while (to->suppliers.next != first) {
> +		link = list_first_entry(&to->suppliers, struct fwnode_link,
> +					c_hook);
> +		__fwnode_link_del(link);
> +	}
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-submit-phy-package-fwdevlink-v1-v3-0-40a905ea16b6@gmail.com?part=1