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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 16/08/2019 00:36, 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.
This sentence does not match the change below. Only 2 of the new error paths are 
described here.
> Add a printk when device tree parsing fails.
>
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> ---
> Changes in v6:
> - fix out of space check
> - bring back printk when address_cells or size_cells are not properly set
> - return -EINVAL in that case (different from reg missing)
> - add printk when parsing fails
> - return -ENOENT when memory size is 0
>
> 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, 18 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index f1614ef7fc..9dc2c1352d 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -130,9 +130,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;
> @@ -145,15 +146,12 @@ static void __init process_memory_node(const void *fdt, 
> int node,
>       {
>           printk("fdt: node `%s': invalid #address-cells or #size-cells",
>                  name);
> -        return;
> +        return -EINVAL;
>       }
>   
>       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,11 +160,15 @@ static void __init process_memory_node(const void *fdt, 
> int node,
>       {
>           device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>           if ( !size )
> -            continue;
> +            return -ENOENT;
I don't think we can treat the same way the lack of "regs" properties and a size 
of 0.
The former is expected as binding allow you to do it for reserved-memory. The 
latter is the user not writing the property correctly. So ignoring the latter 
will result to Xen potentially missing some reserved-regions (not great!).
So, similar to #address-cells/#size-cells discussion, we should return an error 
we are able to distinguish. Probably -EINVAL.
Cheers,

--
Julien Grall

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