Re: [Minios-devel] [UNIKRAFT PATCHv7 1/8] plat/common: Introduce fdt_getprop_u32_by_offset helper

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Jianyong Wu,

Please find the review comment inline

Thanks & Regards
Sharan Santhanam


On 6/27/19 10:55 AM, Jia He wrote:
> 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/drivers/include/ofw/fdt.h | 59 +++++++++++++++++++++++++++++++++
>   plat/drivers/ofw/fdt.c         | 60 ++++++++++++++++++++++++++++++++++
>   2 files changed, 119 insertions(+)
>   create mode 100644 plat/drivers/include/ofw/fdt.h
>   create mode 100644 plat/drivers/ofw/fdt.c
>
> diff --git a/plat/drivers/include/ofw/fdt.h b/plat/drivers/include/ofw/fdt.h
> new file mode 100644
> index 0000000..c202671
> --- /dev/null
> +++ b/plat/drivers/include/ofw/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.
> + */
> +#ifndef _PLAT_DRIVER_OFW_FDT_H
> +#define _PLAT_DRIVER_OFW_FDT_H
> +
Does this helper function belong plat/driver/ofw or lib/fdt? Was there a 
reason why we added it to plat/driver/ofw because in the last patch 
series discussion v4 [1] we had decided on adding to lib/fdt with an 
entry into exportsyms.uk.
> +/**
> + * 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
> diff --git a/plat/drivers/ofw/fdt.c b/plat/drivers/ofw/fdt.c
> new file mode 100644
> index 0000000..e23b7a3
> --- /dev/null
> +++ b/plat/drivers/ofw/fdt.c
> @@ -0,0 +1,60 @@
> +/* 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.
> + */
> +#include <libfdt_env.h>
> +#include <fdt.h>
> +#include <libfdt.h>
> +
need uk/assert.h?
> +#include <uk/print.h>
> +
> +int fdt_getprop_u32_by_offset(const void *fdt, int offset,
> +               const char *name, uint32_t *out)
> +{
> +       const struct fdt_property *prop;
> +       int prop_len;
> +
> +       UK_ASSERT(out);
> +
> +       prop = fdt_get_property_namelen(fdt, offset, name, strlen(name),
> +                                       &prop_len);
> +       if (!prop)
> +               return prop_len;
> +
Why should this be >= sizeof(fdt32_t) instead of == sizeof(fdt_32t)?
> +       if (prop_len >= (int)sizeof(fdt32_t)) {
> +               *out = fdt32_to_cpu(*(fdt32_t *)prop->data);
> +               return 0;
> +       }
> +
> +       return -FDT_ERR_NOTFOUND;
> +}
[1] 
https://lists.xenproject.org/archives/html/minios-devel/2019-04/msg00128.html
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel