libfdt: overlay: Refactor overlay_fixup_phandle and update test case #132
Conversation
| phandle = fdt_get_phandle(fdt, symbol_off); | ||
| if (!phandle) | ||
| return -FDT_ERR_NOTFOUND; | ||
|
|
There was a problem hiding this comment.
This is a nice refactor, thanks!.
libfdt/fdt_overlay.c
Outdated
| path, path_len, name, name_len, | ||
| poffset, label); | ||
| path, path_len, name, name_len, | ||
| poffset, phandle); |
There was a problem hiding this comment.
These lines are oddly over-indented in the revised version, can you fix this up please.
| @@ -269,7 +269,7 @@ libfdt_overlay_tests () { | |||
| for test in $BAD_FIXUP_TREES; do | |||
| tree="overlay_bad_fixup_$test" | |||
| run_dtc_test -I dts -O dtb -o $tree.test.dtb "$SRCDIR/$tree.dts" | |||
| run_test overlay_bad_fixup overlay_base_no_symbols.test.dtb $tree.test.dtb | |||
| run_test overlay_bad_fixup overlay_base_manual_symbols.test.dtb $tree.test.dtb | |||
There was a problem hiding this comment.
This change is unrelated, logically or code-wise to the other one. Can you please make them separate patches / commits (they can still go in the same PR on github).
There was a problem hiding this comment.
The test case update is necessary due to the refactoring, as it's essential for the tests to pass with the modified code logic. Specifically, the change prevents premature test exits due to the updated phandle handling. For the detailed rationale, please refer to point 2 in my original commit message. Nonetheless, I've split the commits for clarity as you suggested.
There was a problem hiding this comment.
Sorry, I missed that. I had the impression that the first patch didn't make any behavioural change, just optimised the existing behaviour. Since it clearly does, I need to consider more carefully if this is the right idea.
I'm having trouble understanding point 2 from your commit message. Can you clarify for me why the behaviour is different with the overlay change?
There was a problem hiding this comment.
To clarify point two, the crucial distinction between overlay_base_manual_symbols.test.dtb and overlay_base_no_symbols.test.dtb lies in the presence of the __symbols__ node in the former. This node allows us to retrieve elements like symbol_path, symbol_off, and phandle successfully. In the past, the code would validate the fixup string format before these elements were gathered. Now, we’ve inverted the process; we ensure these elements are in place before we evaluate the string format. Since the test's purpose is to verify the string format, having a __symbols__ node in the base FDT is essential to avoid an early test failure.
There was a problem hiding this comment.
Ah, I see. Previously the generated trees here had two defects: first they were missing the symbols, and second the actual defect we introduced deliberately to test. With your other change we're now failing on that first defect instead of the second one we're trying to test.
Refactored overlay_fixup_phandle to optimize efficiency by moving the phandle lookup logic based on label outside the overlay_fixup_one_phandle call. This avoids redundant phandle lookups when a single label is associated with multiple modifications. Signed-off-by: Zheng Guangyuan <[email protected]>
Changed the target DTS from overlay_base_no_symbols.test.dtb to overlay_base_manual_symbols.test.dtb. This ensures that the test case doesn't exit prematurely due to the absence of label-linked phandle in the symbols node. The update guarantees that the test case appropriately checks the validity of the fixup string linked to the label, as intended. Signed-off-by: Zheng Guangyuan <[email protected]>
|
Thanks for the clarifications. I've now merged this - I reversed the order of the two patches so that there isn't a temporary test failure when bisecting. |
This commit makes two main changes:
Refactored overlay_fixup_phandle to optimize efficiency by moving the phandle lookup logic based on label outside the overlay_fixup_one_phandle call. This avoids redundant phandle lookups when a single label is associated with multiple modifications.
Updated the test case for overlay_bad_fixup. Changed the target DTS from overlay_base_no_symbols.test.dtb to overlay_base_manual_symbols.test.dtb. This ensures that the test case doesn't exit prematurely due to the absence of label-linked phandle in the symbols node. The update guarantees that the test case appropriately checks the validity of the fixup string linked to the label, as intended.