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 claims to exercise patch 2's logic but bypasses it by passing NULL for target_base.
- [Medium] The test claims to exercise patch 3's logic but bypasses it by using an absolute live-tree path.
--

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

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

This commit adds an overlay and a test runner to verify alias creation and
removal dynamically during device tree overlay apply and revert operations.
It asserts that the alias is correctly added and then removed.

>     exercises all three functional patches earlier in this series
>     together: patch 1's reconfig notifier is what mutates aliases_lookup
>     on apply/revert, patch 2 is what lets target-path="/aliases" resolve
>     to the DT root when target_base is non-NULL, and patch 3 is what
>     rewrites the fragment-internal path in the alias value so
>     of_find_node_by_path() finds the live-tree target. Any one of the
>     three missing turns the middle assertion (get_id -> 99) into
>     -ENODEV.

[Severity: Medium]
Is this description of the test coverage accurate?

As detailed below, the test appears to bypass the logic for both patch 2 and
patch 3. The overlay uses an absolute path instead of an intra-overlay
reference (bypassing patch 3), and overlay_data_apply() passes NULL for
target_base (bypassing patch 2).

> diff --git a/drivers/of/unittest-data/overlay_alias.dtso b/drivers/of/unittest-data/overlay_alias.dtso
> new file mode 100644
> index 0000000000000..32532c80505a7
> --- /dev/null
> +++ b/drivers/of/unittest-data/overlay_alias.dtso
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +/plugin/;
> +
> +/* overlay_alias - declare an alias inside an overlay */
> +
> +&{/aliases} {
> +	testcase-alias99 = "/testcase-data/overlay-node/test-bus/test-unittest100";

[Severity: Medium]
Does this overlay actually test the path rewrite logic from patch 3?

The testcase-alias99 property is defined using an absolute live-tree path
rather than an intra-overlay fragment reference. Because this is already a
valid absolute path, it seems of_alias_create() will successfully resolve it
via of_find_node_by_path() even if the rewrite logic from patch 3 is
missing.

> +};
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index e255f54f4d760..a9f4a0bb30872 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
[ ... ]
> @@ -3475,6 +3475,53 @@ static struct notifier_block of_nb = {
>  	.notifier_call = of_notify,
>  };
>  
> +static void __init of_unittest_overlay_alias(void)
> +{
> +	const char *path = "/testcase-data/overlay-node/test-bus/test-unittest100";
> +	struct device_node *np;
> +	int ovcs_id = 0;
> +	int id, ret;
> +
> +	np = of_find_node_by_path(path);
> +	if (!np) {
> +		unittest(0, "could not find %s for alias test\n", path);
> +		return;
> +	}
> +
> +	id = of_alias_get_id(np, "testcase-alias");
> +	if (id != -ENODEV) {
> +		unittest(0,
> +			 "of_alias_get_id() = %d before overlay, expected -ENODEV\n",
> +			 id);
> +		goto out;
> +	}
> +
> +	ret = overlay_data_apply("overlay_alias", &ovcs_id);

[Severity: Medium]
Does this test actually exercise the target_base logic from patch 2?

Looking at overlay_data_apply(), it calls of_overlay_fdt_apply() with a
hardcoded NULL for target_base:

drivers/of/unittest.c:overlay_data_apply() {
    ...
    ret = of_overlay_fdt_apply(info->dtbo_begin, size, &info->ovcs_id,
                               NULL);
    ...
}

When target_base is NULL, the pre-patch-2 code already resolves
target-path="/aliases" correctly via of_find_node_by_path(). This means the
test might pass and bypass patch 2 entirely, missing the intended test
coverage.

> +	if (!ret) {
> +		unittest(0, "overlay_alias apply failed\n");
> +		goto out;
> +	}
[ ... ]

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