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

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

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