Re: [XEN][PATCH v7 04/19] common/device_tree: change __unflatten_device_tree() type

[Julien Grall <julien@xen.org>]

Hi,

On 17/08/2023 00:49, Vikram Garhwal wrote:
> On Tue, Jun 06, 2023 at 12:09:35PM -0700, Vikram Garhwal wrote:
>> Hi Julien,
>> Will update the commit message regarding why we need to export this for dtbo
>> programming.
>>
>> On 6/5/23 12:04 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> Title:
>>>
>>> 'type' is a bit confusing here. How about "Export
>>> __unflatten_device_tre()"?
>>>
>>> On 02/06/2023 01:48, Vikram Garhwal wrote:
>>>> Following changes are done to __unflatten_device_tree():
>>>>       1. __unflatten_device_tree() is renamed to unflatten_device_tree().
>>>>       2. Remove __init and static function type.
>>>
>>> As there is no external caller yet, please explain why you want to
>>> export the function.
> Update the commit message in v8.
>>>
>>> Cheers,
>>>
>>>>
>>>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
>>>> Reviewed-by: Henry Wang <Henry.Wang@arm.com>
>>>> ---
>>>>    xen/common/device_tree.c      | 9 ++++-----
>>>>    xen/include/xen/device_tree.h | 5 +++++
>>>>    2 files changed, 9 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>>> index bbdab07596..16b4b4e946 100644
>>>> --- a/xen/common/device_tree.c
>>>> +++ b/xen/common/device_tree.c
>>>> @@ -2083,7 +2083,7 @@ static unsigned long unflatten_dt_node(const
>>>> void *fdt,
>>>>    }
>>>>      /**
>>>> - * __unflatten_device_tree - create tree of device_nodes from flat blob
>>>> + * unflatten_device_tree - create tree of device_nodes from flat blob
>>>>     *
>>>>     * unflattens a device-tree, creating the
>>>>     * tree of struct device_node. It also fills the "name" and "type"
>>>> @@ -2092,8 +2092,7 @@ static unsigned long unflatten_dt_node(const
>>>> void *fdt,
>>>>     * @fdt: The fdt to expand
>>>>     * @mynodes: The device_node tree created by the call
>>>>     */
>>>> -static int __init __unflatten_device_tree(const void *fdt,
>>>> -                                          struct dt_device_node
>>>> **mynodes)
>>>> +int unflatten_device_tree(const void *fdt, struct dt_device_node
>>>> **mynodes)
>>>>    {
>>>>        unsigned long start, mem, size;
>>>>        struct dt_device_node **allnextp = mynodes;
>>>> @@ -2230,10 +2229,10 @@ dt_find_interrupt_controller(const struct
>>>> dt_device_match *matches)
>>>>      void __init dt_unflatten_host_device_tree(void)
>>>>    {
>>>> -    int error = __unflatten_device_tree(device_tree_flattened,
>>>> &dt_host);
>>>> +    int error = unflatten_device_tree(device_tree_flattened, &dt_host);
>>>>          if ( error )
>>>> -        panic("__unflatten_device_tree failed with error %d\n", error);
>>>> +        panic("unflatten_device_tree failed with error %d\n", error);
>>>>          dt_alias_scan();
>>>
>>> This function doesn't seem to be called in the case of the overlay
>>> device-tree. Does this mean that it will never contain any alias?
>>>
> I haven't seen any overlay example for FPGA use cases where alias are added.
> I have added a TODO in patch 16/19 where we are calling unflatten_device_tree().
>>>>    }
>>>> diff --git a/xen/include/xen/device_tree.h
>>>> b/xen/include/xen/device_tree.h
>>>> index c2eada7489..2c35c0d391 100644
>>>> --- a/xen/include/xen/device_tree.h
>>>> +++ b/xen/include/xen/device_tree.h
>>>> @@ -178,6 +178,11 @@ int device_tree_for_each_node(const void *fdt,
>>>> int node,
>>>>     */
>>>>    void dt_unflatten_host_device_tree(void);
>>>>    +/**
>>>> + * unflatten any device tree.
>>>
>>> Most of the exported function in device_tre.h have documentation. Can
>>> you do the same here?
> Done!
>>>
>>>> + */
>>>> +int unflatten_device_tree(const void *fdt, struct dt_device_node
>>>> **mynodes);
>>>
>>> NIT: From an external interface perspective, do we actually need to pass
>>> an extra pointer? Can't we instead, return the pointer?
> We will also need the error from the function. So, that's why i kept it as it is.

This can be achieved by using the ERR_PTR() infrastructure which I would 
rather prefer over passing an extra pointer here.

Cheers,

-- 
Julien Grall