Re: [XEN RFC PATCH 19/40] xen: fdt: Introduce a helper to check fdt node type

[Julien Grall <julien@xxxxxxx>]

Hi Wei,

On 11/08/2021 11:24, Wei Chen wrote:
> In later patches, we will parse CPU and memory NUMA information
> from device tree. FDT is using device type property to indicate
> CPU nodes and memory nodes. So we introduce fdt_node_check_type
> in this patch to avoid redundant code in subsequent patches.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
>   xen/common/libfdt/fdt_ro.c      | 15 +++++++++++++++
>   xen/include/xen/libfdt/libfdt.h | 25 +++++++++++++++++++++++++
This is meant to be a verbatim copy of libfdt. So I am not entirely in 
favor of adding a new function therefore without been upstreamed to 
libfdt first.
>   2 files changed, 40 insertions(+)
>
> diff --git a/xen/common/libfdt/fdt_ro.c b/xen/common/libfdt/fdt_ro.c
> index 36f9b480d1..ae7794d870 100644
> --- a/xen/common/libfdt/fdt_ro.c
> +++ b/xen/common/libfdt/fdt_ro.c
> @@ -545,6 +545,21 @@ int fdt_node_check_compatible(const void *fdt, int 
> nodeoffset,
>                 return 1;
>   }
>   
> +int fdt_node_check_type(const void *fdt, int nodeoffset,
> +                             const char *type)
> +{
> +       const void *prop;
> +       int len;
> +
> +       prop = fdt_getprop(fdt, nodeoffset, "device_type", &len);
> +       if (!prop)
> +               return len;
> +       if (fdt_stringlist_contains(prop, len, type))
The "device_type" is not a list of string. So I am a bit confused why 
you are using this helper. Shouldn't we simply check that the property 
value and type matches?
> +               return 0;
> +       else
> +               return 1;
> +}
> +
>   int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
>                                   const char *compatible)
>   {
> diff --git a/xen/include/xen/libfdt/libfdt.h b/xen/include/xen/libfdt/libfdt.h
> index 7c75688a39..7e4930dbcd 100644
> --- a/xen/include/xen/libfdt/libfdt.h
> +++ b/xen/include/xen/libfdt/libfdt.h
> @@ -799,6 +799,31 @@ int fdt_node_offset_by_phandle(const void *fdt, uint32_t 
> phandle);
>   int fdt_node_check_compatible(const void *fdt, int nodeoffset,
>                               const char *compatible);
>   
> +/**
> + * fdt_node_check_type: check a node's device_type property
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of a tree node
> + * @type: string to match against
> + *
> + *
> + * fdt_node_check_type() returns 0 if the given node contains a 'device_type'
> + * property with the given string as one of its elements, it returns non-zero
> + * otherwise, or on error.
> + *
> + * returns:
> + *     0, if the node has a 'device_type' property listing the given string
> + *     1, if the node has a 'device_type' property, but it does not list
> + *             the given string
> + *     -FDT_ERR_NOTFOUND, if the given node has no 'device_type' property
> + *     -FDT_ERR_BADOFFSET, if nodeoffset does not refer to a BEGIN_NODE tag
> + *     -FDT_ERR_BADMAGIC,
> + *     -FDT_ERR_BADVERSION,
> + *     -FDT_ERR_BADSTATE,
> + *     -FDT_ERR_BADSTRUCTURE, standard meanings
> + */
> +int fdt_node_check_type(const void *fdt, int nodeoffset,
> +                             const char *type);
> +
>   /**
>    * fdt_node_offset_by_compatible - find nodes with a given 'compatible' value
>    * @fdt: pointer to the device tree blob
Cheers,

--
Julien Grall