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

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func


On 8/12/19 11:28 PM, Stefano Stabellini wrote:
Change the signature of process_memory_node to match
device_tree_node_func. Thanks to this change, the next patch will be
able to use device_tree_for_each_node to call process_memory_node on all
the children of a provided node.

Return error if there is no reg property or if nr_banks is reached. Let
the caller deal with the error.

Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
Changes in v5:
- return -ENOENT if address_cells or size_cells are not properly set

Changes in v4:
- return error if there is no reg propery, remove printk
- return error if nr_banks is reached

Changes in v3:
- improve commit message
- check return value of process_memory_node

Changes in v2:
- new
  xen/arch/arm/bootfdt.c | 29 +++++++++++++++--------------
  1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index a872ea57d6..590b14304c 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -125,9 +125,10 @@ int __init device_tree_for_each_node(const void *fdt, int 
      return 0;
-static void __init process_memory_node(const void *fdt, int node,
-                                       const char *name,
-                                       u32 address_cells, u32 size_cells)
+static int __init process_memory_node(const void *fdt, int node,
+                                      const char *name, int depth,
+                                      u32 address_cells, u32 size_cells,
+                                      void *data)
      const struct fdt_property *prop;
      int i;
@@ -137,18 +138,11 @@ static void __init process_memory_node(const void *fdt, 
int node,
      u32 reg_cells = address_cells + size_cells;
if ( address_cells < 1 || size_cells < 1 )
-    {
-        printk("fdt: node `%s': invalid #address-cells or #size-cells",
-               name);
-        return;
-    }
+        return -ENOENT;

I saw your answer on the previous version and didn't get the chance. Missing the "regs" property and wrong {address,size}-cells values are two different things.

In the former case, it just means the reserved-region will be allocated dynamically.

In the latter case, this is an error in the device-tree configuration. If you ignore it, you are at risk to not take into account a reserved-region.

I agree this is a bug in the Device-Tree, but doing basic sanity check for the users is always helpful.

With that in mind, I would keep the printk and return a different error here.

prop = fdt_get_property(fdt, node, "reg", NULL);
      if ( !prop )
-    {
-        printk("fdt: node `%s': missing `reg' property\n", name);
-        return;
-    }
+        return -ENOENT;
cell = (const __be32 *)prop->data;
      banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
@@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, 
int node,
          bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
+    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
+        return -ENOSPC;
+    return 0;
static void __init process_multiboot_node(const void *fdt, int node,
@@ -293,15 +291,18 @@ static int __init early_scan_node(const void *fdt,
                                    u32 address_cells, u32 size_cells,
                                    void *data)
+    int rc = 0;
      if ( device_tree_node_matches(fdt, node, "memory") )
-        process_memory_node(fdt, node, name, address_cells, size_cells);
+        rc = process_memory_node(fdt, node, name, depth,
+                                 address_cells, size_cells, NULL);

As a small NIT there are no way for the user to know that the parsing failed because of the reg property were missing. Could you print an error message either here or maybe in device_tree_for_each() to say parsing as failed for node N?

      else if ( depth <= 3 && (device_tree_node_compatible(fdt, node, 
"xen,multiboot-module" ) ||
                device_tree_node_compatible(fdt, node, "multiboot,module" )))
          process_multiboot_node(fdt, node, name, address_cells, size_cells);
      else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
          process_chosen_node(fdt, node, name, address_cells, size_cells);
- return 0;
+    return rc;
static void __init early_print_info(void)


Julien Grall

Xen-devel mailing list



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