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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2021年8月25日 21:39
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Jan Beulich
> <jbeulich@xxxxxxxx>
> 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.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.
> 

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