Re: [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The test assertion for verifying alias removal trivially passes because it checks the wrong node, rendering the removal test ineffective.
- [Medium] The error path for `of_overlay_fdt_apply()` skips cleanup, leading to a memory and device tree state leak.
--

--- Patch [4]: [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert ---
commit d95c5208da27c0c2461530192533ee49d2b62c1c
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

of: unittest: cover /aliases updates from overlay apply/revert

This commit adds overlay_alias.dtso and of_unittest_overlay_alias() to
cover the end-to-end flow of aliases inside an overlay. It verifies alias
updates when applying and reverting overlays with a non-NULL target base.

> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index e255f54f4d760..35d75f2b3ef25 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c

[ ... ]

> +	size = info->dtbo_end - info->dtbo_begin;
> +	ret = of_overlay_fdt_apply(info->dtbo_begin, size, &ovcs_id, base);
> +	if (ret < 0) {
> +		unittest(0, "overlay_alias apply failed, ret = %d\n", ret);
> +		goto out_put_base;

[Severity: Medium]
Does this error path skip cleaning up the changeset?

Looking at of_overlay_fdt_apply(), if it fails during the apply phase, it
may still populate ovcs_id. Does this code need to call of_overlay_remove()
to clean up the partial changeset before jumping to out_put_base, to avoid
leaking memory and device tree state?

> +	}
> +
> +	target = of_find_node_by_path(target_path);
> +	if (!target) {
> +		unittest(0, "could not find grafted target %s\n", target_path);
> +		goto out_remove;
> +	}
> +
> +	id = of_alias_get_id(target, "testcase-alias");
> +	unittest(id == 99,
> +		 "of_alias_get_id() = %d after overlay apply, expected 99\n",
> +		 id);
> +	of_node_put(target);
> +
> +out_remove:
> +	ret = of_overlay_remove(&ovcs_id);
> +	if (ret) {
> +		unittest(0, "overlay_alias remove failed, ret = %d\n", ret);
> +		goto out_put_base;
> +	}
> +
> +	id = of_alias_get_id(base, "testcase-alias");

[Severity: Medium]
Does this code test the correct node during the removal assertion?

The assertion checks 'base' for the alias, but the earlier success path
checked 'target'. Doesn't of_alias_get_id(base, "testcase-alias") always
return -ENODEV here, meaning the test will pass even if the overlay removal
failed to actually clean up the alias?

> +	unittest(id == -ENODEV,
> +		 "of_alias_get_id() = %d after overlay remove, expected -ENODEV\n",
> +		 id);
> +
> +out_put_base:
> +	of_node_put(base);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-nh-of-alias-overlay-v1-0-27da6848dd84@nexthop.ai?part=4