[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN][PATCH v5 02/17] common/device_tree: change __unflatten_device_tree()
Hi, On 4/14/23 11:09 AM, Julien Grall wrote: Thanks for explanation. Will add error handling for unflatten_dt_node() and unflatten_device_tree() for failures.Hi, On 14/04/2023 18:51, Vikram Garhwal wrote:On 4/13/23 3:03 AM, Julien Grall wrote:Looks like i misunderstood v4 comments. Will change it to propagate an error in case of failure here to the handle_add_overlay_nodes() caller and that will further forward to error to toolstack.Hi, On 11/04/2023 20:16, Vikram Garhwal wrote:Following changes are done to __unflatten_device_tree():1. __unflatten_device_tree() is renamed to unflatten_device_tree().2. Remove static function type.3. Add handling of memory allocation. This will be useful in dynamic node programming when we unflatten the dt during runtime memory allocationcan fail. Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx> --- xen/common/device_tree.c | 10 ++++++---- xen/include/xen/device_tree.h | 5 +++++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index aed38ff63c..bf847b2584 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c@@ -2047,7 +2047,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"@@ -2056,8 +2056,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 void __unflatten_device_tree(const void *fdt, - struct dt_device_node **mynodes)+void unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes){ unsigned long start, mem, size; struct dt_device_node **allnextp = mynodes;@@ -2079,6 +2078,9 @@ static void __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 ) + panic("Cannot allocate memory for unflatten device tree\n");After your series, unflatten_device_tree() will be called after boot, so we should not unconditionally called panic(). Instead, unflatten_device_tree() should return an error and let the caller decide what to do.I suggest to read misc/xen-error-handling.txt to understand when to use panic()/BUG() & co. For...+ ((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeef); dt_dprintk(" unflattening %lx...\n", mem);@@ -2179,7 +2181,7 @@ 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); + unflatten_device_tree(device_tree_flattened, &dt_host);... this caller this should be a panic() (this is OK here because it is boot code).But for your new caller, you should properly report the error back the toolstack.Understood, will change it in next version.for dynamic_programming, while adding node(check patch: [XEN][PATCH v5 14/17] xen/arm: Implement device tree node addition functionalities), fdt_overlay_apply() is called before unflatten_device_tree() is called. fdt_overlay_apply() will catch the invalid device-tree overlay nodes.Also, unflatten_dt_node() (called by __unflatten_device_tree()) seems to have some failure cases. Can you explain why they are not properly propagated in your case? Are you trusting the device-tree to always be valid?I agree that in theory fdt_overlay_apply() will catch invalid device-tree. However, neither of the functions are exempts of bugs and there is no code shared between the two (they are not even coming from the same project).So we could end up in a situation where fdt_overlay_apply() works but not unflatten_device_tree(). Therefore, I would rather prefer if the latter function properly handle any errors.Note that unflatten_dt_node() already have check the validity of the DT and will return. We just need to make sure they are treated as error rather than been ignored. Cheers,
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |