[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.