[PATCH] fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ
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 using - phandles 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;
val = trailing_strtol(name); if (val != -1) {
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 and the offset that we get from fdt_path_offset(blob, prop) is the offset of the aliases property. I don't think these both offsets will match for any case. That is the reason behind comparing phandles and not offsets.
Also, as fdt_path_offset() slow and this would effect the U-Boot startup. To avert the time penalty on all boards, this extra check put under a config option.
Thanks, Aswath
val = trailing_strtol(name); if (val != -1)
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.]
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".
Rasmus
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
On 19/05/2022 13.50, Aswath Govindraju wrote:
Understood, thanks for the explanation. I am good with this patch.
Acked-by: Aswath Govindraju a-govindraju@ti.com
Thanks.
For completeness, to expand on this:
it's somewhat fragile to rely on (at least one of) the nodes in question to even have a phandle.
One way in which to ensure all nodes (with a label) do get a phandle is to build with the -@ flag to dtc, which is implied by setting CONFIG_OF_LIBFDT_OVERLAY=y. Three of the four _defconfigs that set PHANDLE_CHECK_SEQ also set that option, so for them it was guaranteed to work, but mostly by chance - if I randomly discovered CONFIG_PHANDLE_CHECK_SEQ and found I needed to enable it for my board, I wouldn't know to also enable CONFIG_OF_LIBFDT_OVERLAY. And if I didn't need overlay support as such, it would also bloat the .dtb (and U-Boot itself) needlessly; the difference for sama7g5ek_mmc_defconfig is 34784 bytes for the current .dtb and 26920 bytes if one disables OF_LIBFDT_OVERLAY.
The last _defconfig with PHANDLE_CHECK_SEQ=y didn't actually seem to need it; I've built it and looked at u-boot.dtb, and there are no collisions in basenames in the aliases. Commit ddd778ae doesn't say anything about why it was added.
Rasmus
On 5/19/22 3:16 PM, Rasmus Villemoes wrote:
On 19/05/2022 13.50, Aswath Govindraju wrote:
Understood, thanks for the explanation. I am good with this patch.
Acked-by: Aswath Govindraju a-govindraju@ti.com
Thanks.
For completeness, to expand on this:
it's somewhat fragile to rely on (at least one of) the nodes in question to even have a phandle.
One way in which to ensure all nodes (with a label) do get a phandle is to build with the -@ flag to dtc, which is implied by setting CONFIG_OF_LIBFDT_OVERLAY=y. Three of the four _defconfigs that set PHANDLE_CHECK_SEQ also set that option, so for them it was guaranteed to work, but mostly by chance - if I randomly discovered CONFIG_PHANDLE_CHECK_SEQ and found I needed to enable it for my board, I wouldn't know to also enable CONFIG_OF_LIBFDT_OVERLAY. And if I didn't need overlay support as such, it would also bloat the .dtb (and U-Boot itself) needlessly; the difference for sama7g5ek_mmc_defconfig is 34784 bytes for the current .dtb and 26920 bytes if one disables OF_LIBFDT_OVERLAY.
The last _defconfig with PHANDLE_CHECK_SEQ=y didn't actually seem to need it; I've built it and looked at u-boot.dtb, and there are no collisions in basenames in the aliases. Commit ddd778ae doesn't say anything about why it was added.
Rasmus
Hi Rasmus,
For sama7g5 boards, we have the following situation:
some node { i2c1: i2c@600: { some props; }; };
some other node { i2c2: i2c@600: { other props; }; };
and then we want to be able to reference aliases { i2c0 = &i2c1; i2c1 = &i2c2; };
Without CONFIG_PHANDLE_CHECK_SEQ , both i2c0 and i2c1 aliases pointed to the same 'i2c@600' node, because the code was searching through the DT for 'i2c@600' and it was hitting the first in both cases.
With your patch, it can happen that we hit the same problem again ? Or we should be safe ?
Thanks, Eugen
On 08/06/2022 16.18, Eugen.Hristev@microchip.com wrote:
On 5/19/22 3:16 PM, Rasmus Villemoes wrote:
On 19/05/2022 13.50, Aswath Govindraju wrote:
Understood, thanks for the explanation. I am good with this patch.
Acked-by: Aswath Govindraju a-govindraju@ti.com
Thanks.
For completeness, to expand on this:
it's somewhat fragile to rely on (at least one of) the nodes in question to even have a phandle.
One way in which to ensure all nodes (with a label) do get a phandle is to build with the -@ flag to dtc, which is implied by setting CONFIG_OF_LIBFDT_OVERLAY=y. Three of the four _defconfigs that set PHANDLE_CHECK_SEQ also set that option, so for them it was guaranteed to work, but mostly by chance - if I randomly discovered CONFIG_PHANDLE_CHECK_SEQ and found I needed to enable it for my board, I wouldn't know to also enable CONFIG_OF_LIBFDT_OVERLAY. And if I didn't need overlay support as such, it would also bloat the .dtb (and U-Boot itself) needlessly; the difference for sama7g5ek_mmc_defconfig is 34784 bytes for the current .dtb and 26920 bytes if one disables OF_LIBFDT_OVERLAY.
The last _defconfig with PHANDLE_CHECK_SEQ=y didn't actually seem to need it; I've built it and looked at u-boot.dtb, and there are no collisions in basenames in the aliases. Commit ddd778ae doesn't say anything about why it was added.
Rasmus
Hi Rasmus,
For sama7g5 boards, we have the following situation:
some node { i2c1: i2c@600: { some props; }; };
some other node { i2c2: i2c@600: { other props; }; };
and then we want to be able to reference aliases { i2c0 = &i2c1; i2c1 = &i2c2; };
Without CONFIG_PHANDLE_CHECK_SEQ , both i2c0 and i2c1 aliases pointed to the same 'i2c@600' node, because the code was searching through the DT for 'i2c@600' and it was hitting the first in both cases.
With your patch, it can happen that we hit the same problem again ? Or we should be safe ?
I removed the config option partly because one shouldn't need to know about some magic config option to get correct behaviour. The check which was previously hidden behind that config knob is now done unconditionally, and doesn't implicitly rely on the .dtb being built with -@, and at a lower cost (because two redundant phandle lookups are gone, and those phandle lookups was what was making the check non-robust anyway).
Rasmus
And no, the aliases were each pointing at the correct node (the i2c0 = &i2c1 is merely a shorthand for i2c0 = "/the/full/path/to/the/node"). What this check is about is when one comes with a pointer to some node and wants to know if there is some i2cX alias pointing at that node, and if so, what the value of X is - and without properly comparing the full paths but merely the name of the node passed in to the basename of the value of the alias, one would incorrectly assign the seq number 0 to both nodes.
On Thu, May 19, 2022 at 11:10:43AM +0200, 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 Acked-by: Aswath Govindraju a-govindraju@ti.com
Applied to u-boot/next, thanks!
participants (4)
-
Aswath Govindraju -
Eugen.Hristev＠microchip.com -
Rasmus Villemoes -
Tom Rini