Re: [PATCH v5 2/2] device property: add test cases for fwnode_for_each_child_node()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The KUnit test fails to use `kunit_add_action()` for global state cleanup, permanently leaking software nodes on test aborts.

Pre-existing issues:
- [High] The newly added tests expose pre-existing infinite loop and double-put bugs in `fwnode_get_next_child_node()` when transitioning between primary OF nodes and secondary SW nodes.
--

--- Patch [2]: [PATCH v5 2/2] device property: add test cases for fwnode_for_each_child_node() ---
commit 6f128f7ffab2a2787ad6820b967d94a17fcbcbc2
Author: Xu Yang <xu.yang_2@nxp.com>
device property: add test cases for fwnode_for_each_child_node()

Add test cases for fwnode_for_each_child_node() API.

> diff --git a/drivers/base/test/property-entry-test.c b/drivers/base/test/property-entry-test.c
> index a8657eb06f94e..a3d1caf28cc7d 100644
> --- a/drivers/base/test/property-entry-test.c
> +++ b/drivers/base/test/property-entry-test.c
[ ... ]
> @@ -489,6 +491,139 @@ static void pe_test_reference(struct kunit *test)
[ ... ]
> +	/* 2. Test SW node child iteration */
> +
> +	error = software_node_register_node_group(group);
> +	KUNIT_ASSERT_EQ(test, error, 0);
> +
> +	sw_node = software_node_fwnode(&node);
> +
> +	i = 0;
> +	num = ARRAY_SIZE(sw_child_array);
> +	fwnode_for_each_child_node(sw_node, child) {
> +		KUNIT_ASSERT_LT(test, i, num);

[Severity: Medium]
If KUNIT_ASSERT_LT() fails, it will immediately abort the test execution.
Does this bypass the manual software_node_unregister_node_group() call at
the end of the function?

To prevent leaking the software nodes and causing sysfs conflicts on
subsequent test runs, should kunit_add_action() be used to register the
cleanup immediately after registration?

> +		KUNIT_EXPECT_STREQ(test, sw_child_array[i++], fwnode_get_name(child));
> +	}
> +	KUNIT_EXPECT_PTR_EQ(test, child, NULL);
> +
> +	/* 3. Test OF (primary) + SW (secondary) node child iteration */
> +
> +	of_node->secondary = sw_node;
> +	sw_node->secondary = ERR_PTR(-ENODEV);
> +
> +	i = 0;
> +	num = ARRAY_SIZE(of_sw_child_array);
> +	fwnode_for_each_child_node(of_node, child) {
> +		KUNIT_ASSERT_LT(test, i, num);

[Severity: High]
This isn't a bug introduced by this patch, but these new tests appear to
expose a pre-existing infinite loop and double-put bug in the core fwnode
iterator.

When fwnode_for_each_child_node() transitions from primary OF children to
secondary SW children, of_fwnode_get_next_child_node() drops the reference
on the last OF child and returns NULL. The core iterator then passes this
same child pointer to the secondary node's iterator:

drivers/base/property.c:fwnode_get_next_child_node() {
    ...
    next = fwnode_call_ptr_op(fwnode, get_next_child_node, child);
    if (next)
        return next;
    return fwnode_call_ptr_op(fwnode->secondary, get_next_child_node, child);
}

Does this cause the secondary iterator (software_node_get_next_child) to
unconditionally drop the reference a second time?

Furthermore, when iterating over the secondary children, the returned SW node
is eventually passed back to the primary iterator. Since to_of_node()
evaluates to NULL for SW nodes, of_get_next_available_child() restarts from
the first OF child:

drivers/of/property.c:of_fwnode_get_next_child_node() {
    ...
    return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode),
                                                        to_of_node(child)));
}

Will this restart cause an infinite loop in the test runner when testing
mixed primary and secondary firmware nodes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720104730.1285552-1-xu.yang_2@oss.nxp.com?part=2