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

[Julien Grall <julien.grall@arm.com>]

Hi,

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@xilinx.com>
> ---
> 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 node,
>       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;
>           bootinfo.mem.nr_banks++;
>       }
> +
> +    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)
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel