[PATCH] fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ

[Aswath Govindraju]

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 at 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;
>>
>> 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 at 0/bus at 30800000/ethernet at 30be0000";
>     }
> 
> then name would be "ethernet0" and "prop" would be
> "/soc at 0/bus at 30800000/ethernet at 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 at ti.com>

-- 
Thanks,
Aswath

> Rasmus