[PATCH] fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ

[Aswath Govindraju]

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 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)