Hi Julien,
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: 2021年8月25日 21:39
> To: Wei Chen <wei.c...@arm.com>; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org
> Cc: Bertrand Marquis <bertrand.marq...@arm.com>; Jan Beulich
> <jbeul...@suse.com>
> Subject: Re: [XEN RFC PATCH 19/40] xen: fdt: Introduce a helper to check
> fdt node type
>
> 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.c...@arm.com>
> > ---
> > 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.
>
Oh, if we need to upstream this change in libfdt. I think I'd better
to remove this change in libfdt. Because we can implement type checking
in other place, and I don't want to introduce a dependency on external
repo upstream in this series.
> > 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?
>
Yes, I think you're right. This function was based on the modification
of fdt_node_check_compatible, and I forgot to replace fdt_stringlist_contains.
And, as I reply above, we can simply the check. And I will implement it
out of libfdt.
> > + 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
