Hi Rasmus,
On 19/05/22 16:58, Rasmus Villemoes wrote:
On 19/05/2022 12.41, Aswath Govindraju wrote:
Hi Rasmus,
On 19/05/22 14:40, Rasmus Villemoes wrote:
Asking if the alias we found actually points at the device tree node we passed in (in the guise of its offset from blob) can be done simply by asking if the fdt_path_offset() of the alias' path is identical to offset.
In fact, the current method suffers from the possibility of false negatives: dtc does not necessarily emit a phandle property for a node just because it is referenced in /aliases; it only emits a phandle property for a node if it is referenced in <angle brackets> somewhere. So if both the node we passed in and the alias node we're considering don't have phandles, fdt_get_phandle() returns 0 for both.
Since the proper check is so simple, there's no reason to hide that behind a config option (and if one really wanted that, it should be called something else because there's no need to involve phandle in the check).
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
configs/am65x_evm_a53_defconfig | 1 - configs/evb-ast2600_defconfig | 1 - configs/sama7g5ek_mmc1_defconfig | 1 - configs/sama7g5ek_mmc_defconfig | 1 - lib/Kconfig | 7 ------- lib/fdtdec.c | 7 ++----- 6 files changed, 2 insertions(+), 16 deletions(-)
diff --git a/configs/am65x_evm_a53_defconfig b/configs/am65x_evm_a53_defconfig index 9f41b397c3..a635d6d137 100644 --- a/configs/am65x_evm_a53_defconfig +++ b/configs/am65x_evm_a53_defconfig @@ -170,4 +170,3 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x0451 CONFIG_USB_GADGET_PRODUCT_NUM=0x6162 CONFIG_USB_GADGET_DOWNLOAD=y CONFIG_OF_LIBFDT_OVERLAY=y -CONFIG_PHANDLE_CHECK_SEQ=y diff --git a/configs/evb-ast2600_defconfig b/configs/evb-ast2600_defconfig index ea75762926..015df20f09 100644 --- a/configs/evb-ast2600_defconfig +++ b/configs/evb-ast2600_defconfig @@ -84,4 +84,3 @@ CONFIG_WDT=y CONFIG_SHA384=y CONFIG_HEXDUMP=y # CONFIG_EFI_LOADER is not set -CONFIG_PHANDLE_CHECK_SEQ=y diff --git a/configs/sama7g5ek_mmc1_defconfig b/configs/sama7g5ek_mmc1_defconfig index 42d96f7c02..3f33eb1142 100644 --- a/configs/sama7g5ek_mmc1_defconfig +++ b/configs/sama7g5ek_mmc1_defconfig @@ -76,4 +76,3 @@ CONFIG_TIMER=y CONFIG_MCHP_PIT64B_TIMER=y CONFIG_OF_LIBFDT_OVERLAY=y # CONFIG_EFI_LOADER_HII is not set -CONFIG_PHANDLE_CHECK_SEQ=y diff --git a/configs/sama7g5ek_mmc_defconfig b/configs/sama7g5ek_mmc_defconfig index e03a6ba9af..6266eb0b59 100644 --- a/configs/sama7g5ek_mmc_defconfig +++ b/configs/sama7g5ek_mmc_defconfig @@ -76,4 +76,3 @@ CONFIG_TIMER=y CONFIG_MCHP_PIT64B_TIMER=y CONFIG_OF_LIBFDT_OVERLAY=y # CONFIG_EFI_LOADER_HII is not set -CONFIG_PHANDLE_CHECK_SEQ=y diff --git a/lib/Kconfig b/lib/Kconfig index acc0ac081a..884569f9b1 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -958,11 +958,4 @@ config LMB_RESERVED_REGIONS Define the number of supported reserved regions in the library logical memory blocks.
-config PHANDLE_CHECK_SEQ
- bool "Enable phandle check while getting sequence number"
- help
When there are multiple device tree nodes with same name,enable this config option to distinguish them usingphandles in fdtdec_get_alias_seq() function.endmenu diff --git a/lib/fdtdec.c b/lib/fdtdec.c index e20f6aad9c..ffa78f97ca 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -516,11 +516,8 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset, * Adding an extra check to distinguish DT nodes with * same name */
if (IS_ENABLED(CONFIG_PHANDLE_CHECK_SEQ)) {if (fdt_get_phandle(blob, offset) !=fdt_get_phandle(blob, fdt_path_offset(blob, prop)))continue;}
if (offset != fdt_path_offset(blob, prop))continue;The offset over here is the offset of the dt node
Yes.
and the offset that we get from fdt_path_offset(blob, prop) is the offset of the aliases property.
No. Here "prop" is the value of the property under the /aliases node with the name "name", i.e. if you have
aliases { ethernet0 = "/soc@0/bus@30800000/ethernet@30be0000"; }then name would be "ethernet0" and "prop" would be "/soc@0/bus@30800000/ethernet@30be0000". That's why there's some sanity checks on prop before this, i.e. the value must start with a '/' and must be a proper nul-terminated string:
if (len < find_namelen || *prop != '/' || prop[len - 1] || strncmp(name, base, base_len)) continue;
The next check then matches the basename of that path against the name of the node passed in:
slash = strrchr(prop, '/'); if (strcmp(slash + 1, find_name)) continue;
And then fdt_path_offset() simply follows the given path from the root of blob to that node and returns its offset. [Yes, fdt_path_offset() also supports non-absolute paths and in that case does an alias lookup by itself, but we're not passing the alias, we're passing the value of that alias which is the absolute path.]
Understood, my bad I stand corrected.
I don't think these both offsets will match for any case. That is the reason behind comparing phandles and not offsets.
It does, I've tested it.
Also, as fdt_path_offset() slow and this would effect the U-Boot
startup.
First, we only get to do that fdt_path_offset() if the node names actually match, so it's not for each and every alias lookup. Second, as I pointed out, it's somewhat fragile to rely on (at least one of) the nodes in question to even have a phandle.
To avert the time penalty on all boards, this extra check put under a config option.
I prefer correctness out-of-the-box instead of having to discover some config knob, while also somehow making sure that all nodes do get phandles. And since this no longer does two fdt_get_phandle() calls, the overhead is much smaller. I really doubt you can measure any difference in boot time. If you can, let's add a config option, but make it opt-out with a help text that says "only disable this if you know you never have two nodes with the same node name".
Understood, thanks for the explanation. I am good with this patch.
Acked-by: Aswath Govindraju a-govindraju@ti.com