Re: [XEN][PATCH v6 02/19] common/device_tree: handle memory allocation failure in __unflatten_device_tree()

[Vikram Garhwal <vikram.garhwal@xxxxxxx>]

Hi Michal,

On 5/5/23 2:38 AM, Michal Orzel wrote:
> On 03/05/2023 01:36, Vikram Garhwal wrote:
> > Change __unflatten_device_tree() return type to integer so it can propagate
> > memory allocation failure. Add panic() in dt_unflatten_host_device_tree() for
> > memory allocation failure during boot.
> >
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> I think we are missing a Fixes tag.
Like the below line?
Fixes: fb97eb6 ("xen/arm: Create a hierarchical device tree")

Original patch for your reference: 
https://github.com/xen-project/xen/commit/fb97eb614acfbcc812098bbbe5dde99271fe0a0d
Regards,
Vikram

> > ---
> >   xen/common/device_tree.c | 13 ++++++++++---
> >   1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 5f7ae45304..fc38a0b3dd 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -2056,8 +2056,8 @@ static unsigned long unflatten_dt_node(const void *fdt,
> >    * @fdt: The fdt to expand
> >    * @mynodes: The device_node tree created by the call
> >    */
> > -static void __init __unflatten_device_tree(const void *fdt,
> > -                                           struct dt_device_node **mynodes)
> > +static int __init __unflatten_device_tree(const void *fdt,
> > +                                          struct dt_device_node **mynodes)
> >   {
> >       unsigned long start, mem, size;
> >       struct dt_device_node **allnextp = mynodes;
> > @@ -2078,6 +2078,8 @@ static void __init __unflatten_device_tree(const void 
> > *fdt,
> >
> >       /* Allocate memory for the expanded device tree */
> >       mem = (unsigned long)_xmalloc (size + 4, __alignof__(struct 
> > dt_device_node));
> > +    if ( !mem )
> > +        return -ENOMEM;
> >
> >       ((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeef);
> >
> > @@ -2095,6 +2097,8 @@ static void __init __unflatten_device_tree(const void 
> > *fdt,
> >       *allnextp = NULL;
> >
> >       dt_dprintk(" <- unflatten_device_tree()\n");
> > +
> > +    return 0;
> >   }
> >
> >   static void dt_alias_add(struct dt_alias_prop *ap,
> > @@ -2179,7 +2183,10 @@ dt_find_interrupt_controller(const struct 
> > dt_device_match *matches)
> >
> >   void __init dt_unflatten_host_device_tree(void)
> >   {
> > -    __unflatten_device_tree(device_tree_flattened, &dt_host);
> > +    int error = __unflatten_device_tree(device_tree_flattened, &dt_host);
> NIT: there should be a blank line between definitions and rest of the code
>
> > +    if ( error )
> > +        panic("__unflatten_device_tree failed with error %d\n", error);
> > +
> >       dt_alias_scan();
> >   }
> >
> > --
> > 2.17.1
> FWICS, patches 2 and 4 are not strictly related to DTBO and are fixing issues
> and propagating errors which is always good. Therefore by moving them to the 
> start
> of the series, they could be merged right away reducing the number of patches 
> to review.
> At the moment, they can't be because patch 3 placed in-between is strictly 
> related to the series.
>
> @julien?
>
> ~Michal