Re: [Minios-devel] [UNIKRAFT PATCHv4 1/9] plat/common: Introduce fdt_getprop_u32_by_offset helper

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Jia He,

To summarize all the changes requested:

* fdt_getprop_u32_by_offset : Moves to lib/fdt and we add this as an 
exported function. Once we do it, we do not need to use any internal fdt 
header file from the platform library. We will create the separate 
source file in the library uk_fdt.c. If possible use 
fdt_get_property_namelen instead of fdt_get_property.
* Move other plat/common/fdt.c to plat/driver/ofw/fdt.c and its header 
files to plat/driver/include/ofw/fdt.h. exportsyms.uk not needed for the 
platform library.


Thanks & Regards
Sharan

On 4/4/19 3:32 PM, Sharan Santhanam wrote:
> Hello,
>
> Please find the review comments inline.
>
> Thanks & Regards
> Sharan
>
> On 3/27/19 3:34 AM, Jia He wrote:
> > From: Jianyong Wu <jianyong.wu@xxxxxxx>
> >
> > From: Jianyong Wu <jianyong.wu@xxxxxxx>
> > This helper will be used very frequently to u32 from properties.
> > So we provide this helper here to avoid using fdt_getprop and
> > fdt32_to_cpu everywhere.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> > Signed-off-by: Jia He <justin.he@xxxxxxx>
> > ---
> >   plat/common/fdt.c         | 59 +++++++++++++++++++++++++++++++++++++++
> >   plat/common/include/fdt.h | 59 +++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 118 insertions(+)
> >   create mode 100644 plat/common/fdt.c
> >   create mode 100644 plat/common/include/fdt.h
> >
> > diff --git a/plat/common/fdt.c b/plat/common/fdt.c
> > new file mode 100644
> > index 0000000..47f7c74
> > --- /dev/null
> > +++ b/plat/common/fdt.c
> > @@ -0,0 +1,59 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause */
> > +/*
> > + * Authors: Wei Chen <Wei.Chen@xxxxxxx>
> > + *          Jianyong Wu <Jianyong.Wu@xxxxxxx>
> > + *
> > + * Copyright (c) 2018, Arm Ltd. All rights reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + *
> > + * 1. Redistributions of source code must retain the above copyright
> > + *    notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *    notice, this list of conditions and the following disclaimer in 
> > the
> > + *    documentation and/or other materials provided with the 
> > distribution.
> > + * 3. Neither the name of the copyright holder nor the names of its
> > + *    contributors may be used to endorse or promote products derived 
> > from
> > + *    this software without specific prior written permission.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND 
> > CONTRIBUTORS "AS IS"
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED 
> > TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR 
> > PURPOSE
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR 
> > CONTRIBUTORS BE
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR 
> > BUSINESS
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, 
> > WHETHER IN
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR 
> > OTHERWISE)
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF 
> > ADVISED OF THE
> > + * POSSIBILITY OF SUCH DAMAGE.
> > + *
> > + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> > + */
> Why do you prefer to add this to the plat/common instead of adding it to 
> the lib/fdt?
>   I prefer if we add it the lib/fdt and add this function to exportsyms.uk
> > +#include "libfdt_env.h"
> > +
> > +#include <fdt.h>
> > +#include <libfdt.h>
> > +
> The libfdt_internal.h is internal to the lib/fdt. Why are we including 
> it here?
> > +#include "libfdt_internal.h"
> Prefer to use <> instead of the "" for print.h.
> > +#include "uk/print.h"
> > +
> > +int fdt_getprop_u32_by_offset(const void *fdt, int offset,
> > +        const char *name, uint32_t *out)
> > +{
> > +    const fdt32_t *prop;
> > +    int prop_len;
> > +
> UK_ASSERT(out); or you can report an error?
>
> We can call the fdt_get_property_namelen directly as it rest of the 
> functions.
> > +    prop = fdt_getprop(fdt, offset, name, &prop_len);
> > +    if (!prop)
> > +        return prop_len;
> > +
> > +    if (prop_len >= (int)sizeof(fdt32_t)) {
> > +        *out = fdt32_to_cpu(prop[0]);
> > +        return 0;
> > +    }
> > +
> > +    return -FDT_ERR_NOTFOUND;
> > +}
> > diff --git a/plat/common/include/fdt.h b/plat/common/include/fdt.h
> > new file mode 100644
> > index 0000000..8ff88d7
> > --- /dev/null
> > +++ b/plat/common/include/fdt.h
> > @@ -0,0 +1,59 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause */
> > +/*
> > + * Authors: Wei Chen <Wei.Chen@xxxxxxx>
> > + *          Jianyong Wu <Jianyong.Wu@xxxxxxx>
> > + *
> > + * Copyright (c) 2018, Arm Ltd. All rights reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + *
> > + * 1. Redistributions of source code must retain the above copyright
> > + *    notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *    notice, this list of conditions and the following disclaimer in 
> > the
> > + *    documentation and/or other materials provided with the 
> > distribution.
> > + * 3. Neither the name of the copyright holder nor the names of its
> > + *    contributors may be used to endorse or promote products derived 
> > from
> > + *    this software without specific prior written permission.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND 
> > CONTRIBUTORS "AS IS"
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED 
> > TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR 
> > PURPOSE
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR 
> > CONTRIBUTORS BE
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR 
> > BUSINESS
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, 
> > WHETHER IN
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR 
> > OTHERWISE)
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF 
> > ADVISED OF THE
> > + * POSSIBILITY OF SUCH DAMAGE.
> > + *
> > + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> > + */
> There are multiple fdt.h with a similar path. The "fdt.h" from the 
> library gets preference because of the order include. We should try and 
> avoid such situation.
>   1. lib/fdt/include/fdt.h
>
>   2. plat/common/include/fdt.h
> > +#ifndef _PLAT_COMMON_FDT_H
> > +#define _PLAT_COMMON_FDT_H
> > +
> > +/**
> > + * fdt_getprop_u32_by_offset - retrieve u32 of a given property
> > + * @fdt: pointer to the device tree blob
> > + * @nodeoffset: offset of the node whose property to find
> > + * @name: name of the property to find
> > + * @out: pointer to u32 variable (will be overwritten) or NULL
> > + *
> > + * fdt_getprop_u32_by_offset() retrieves u32 to the value of the 
> > property
> > + * named 'name' of the node at offset nodeoffset (this will be a
> > + * pointer to within the device blob itself, not a copy of the value).
> > + * If out is non-NULL, the u32 of the property value is returned.
> > + *
> > + * returns:
> > + *    0, on success
> > + *        out contains the u32 of a given property at nodeoffset.
> > + *    -FDT_ERR_NOTFOUND, node does not have named property
> > + *    -FDT_ERR_BADNCELLS,
> > + */
> > +int fdt_getprop_u32_by_offset(const void *fdt, int nodeoffset,
> > +        const char *name, uint32_t *out);
> > +
> > +#endif
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel