| Message ID | 20190812222844.9636-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,1/7] xen/arm: pass node to device_tree_for_each_node | expand |
Hi Stefano, Stefano Stabellini writes: > Add a new parameter to device_tree_for_each_node: node, the node to > start the search from. Passing 0 triggers the old behavior. > > Set min_depth to depth of the current node + 1 and replace the for > loop with a do/while loop to avoid scanning siblings of the initial node > passed as an argument. > > We need this change because in follow-up patches we want to be able to > use reuse device_tree_for_each_node to call a function for each children > nodes of a provided node and the node itself. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> You can have my Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com> providing that you'll fix formatting issue below. > --- > Changes in v5: > - go back to v3 > - code style improvement in acpi/boot.c > - improve comments and commit message > - increase min_depth to avoid parsing siblings > - replace for with do/while loop and increase min_depth to avoid > scanning siblings of the initial node > - pass only node, calculate depth > > Changes in v3: > - improve commit message > - improve in-code comments > - improve code style > > Changes in v2: > - new > --- > xen/arch/arm/acpi/boot.c | 8 +++++--- > xen/arch/arm/bootfdt.c | 19 ++++++++++--------- > xen/include/xen/device_tree.h | 6 +++--- > 3 files changed, 18 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c > index 9b29769a10..d4957cca06 100644 > --- a/xen/arch/arm/acpi/boot.c > +++ b/xen/arch/arm/acpi/boot.c > @@ -246,9 +246,11 @@ int __init acpi_boot_table_init(void) > * - the device tree is not empty (it has more than just a /chosen node) > * and ACPI has not been force enabled (acpi=force) > */ > - if ( param_acpi_off || ( !param_acpi_force > - && device_tree_for_each_node(device_tree_flattened, > - dt_scan_depth1_nodes, NULL))) > + if ( param_acpi_off) > + goto disable; > + if ( !param_acpi_force && > + device_tree_for_each_node(device_tree_flattened, 0, > + dt_scan_depth1_nodes, NULL) ) There is 3 tabs, followed by spaces. This file missed emacs magic at the end. I think, this is cause for this formatting issue. > goto disable; > > /* > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 891b4b66ff..a872ea57d6 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -77,6 +77,7 @@ static u32 __init device_tree_get_u32(const void *fdt, int node, > /** > * device_tree_for_each_node - iterate over all device tree nodes > * @fdt: flat device tree. > + * @node: node to start the search from > * @func: function to call for each node. > * @data: data to pass to @func. > * > @@ -85,20 +86,17 @@ static u32 __init device_tree_get_u32(const void *fdt, int node, > * Returns 0 if all nodes were iterated over successfully. If @func > * returns a value different from 0, that value is returned immediately. > */ > -int __init device_tree_for_each_node(const void *fdt, > +int __init device_tree_for_each_node(const void *fdt, int node, > device_tree_node_func func, > void *data) > { > - int node; > - int depth; > + int depth = fdt_node_depth(fdt, node); > + int min_depth = depth + 1; > u32 address_cells[DEVICE_TREE_MAX_DEPTH]; > u32 size_cells[DEVICE_TREE_MAX_DEPTH]; > int ret; > > - for ( node = 0, depth = 0; > - node >=0 && depth >= 0; > - node = fdt_next_node(fdt, node, &depth) ) > - { > + do { > const char *name = fdt_get_name(fdt, node, NULL); > u32 as, ss; > > @@ -120,7 +118,10 @@ int __init device_tree_for_each_node(const void *fdt, > ret = func(fdt, node, name, depth, as, ss, data); > if ( ret != 0 ) > return ret; > - } > + > + node = fdt_next_node(fdt, node, &depth); > + } while ( node >= 0 && depth >= min_depth ); > + > return 0; > } > > @@ -357,7 +358,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr) > > add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false); > > - device_tree_for_each_node((void *)fdt, early_scan_node, NULL); > + device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL); > early_print_info(); > > return fdt_totalsize(fdt); > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h > index 83156297e2..9a7a8f2dab 100644 > --- a/xen/include/xen/device_tree.h > +++ b/xen/include/xen/device_tree.h > @@ -158,9 +158,9 @@ typedef int (*device_tree_node_func)(const void *fdt, > > extern const void *device_tree_flattened; > > -int device_tree_for_each_node(const void *fdt, > - device_tree_node_func func, > - void *data); > +int device_tree_for_each_node(const void *fdt, int node, > + device_tree_node_func func, > + void *data); > > /** > * dt_unflatten_host_device_tree - Unflatten the host device tree
Hi, On 8/12/19 11:28 PM, Stefano Stabellini wrote: > Add a new parameter to device_tree_for_each_node: node, the node to > start the search from. Passing 0 triggers the old behavior. > > Set min_depth to depth of the current node + 1 and replace the for > loop with a do/while loop to avoid scanning siblings of the initial node > passed as an argument. > > We need this change because in follow-up patches we want to be able to > use reuse device_tree_for_each_node to call a function for each children > nodes of a provided node and the node itself. I have to say this would be fairly confusing for reserved-memory because you are only expecting to parse the subnode. Furthermore, in the unlikely event to first node does have a property "regs", then #address-cells and #size-cells is going to be incorrect (we don't look up for its parent...). So I think it would be best to consider to ignore the first node. This should not be an issue as none of the user care about the root node (i.e /). It would also makes the interface more straightforward. Cheers,
On Tue, 13 Aug 2019, Julien Grall wrote: > Hi, > > On 8/12/19 11:28 PM, Stefano Stabellini wrote: > > Add a new parameter to device_tree_for_each_node: node, the node to > > start the search from. Passing 0 triggers the old behavior. > > > > Set min_depth to depth of the current node + 1 and replace the for > > loop with a do/while loop to avoid scanning siblings of the initial node > > passed as an argument. > > > > We need this change because in follow-up patches we want to be able to > > use reuse device_tree_for_each_node to call a function for each children > > nodes of a provided node and the node itself. > > I have to say this would be fairly confusing for reserved-memory because you > are only expecting to parse the subnode. > > Furthermore, in the unlikely event to first node does have a property "regs", > then #address-cells and #size-cells is going to be incorrect (we don't look up > for its parent...). > > So I think it would be best to consider to ignore the first node. This should > not be an issue as none of the user care about the root node (i.e /). It would > also makes the interface more straightforward. Yes, I can do that. It is a good idea.
On Tue, 13 Aug 2019, Volodymyr Babchuk wrote: > Hi Stefano, > > Stefano Stabellini writes: > > > Add a new parameter to device_tree_for_each_node: node, the node to > > start the search from. Passing 0 triggers the old behavior. > > > > Set min_depth to depth of the current node + 1 and replace the for > > loop with a do/while loop to avoid scanning siblings of the initial node > > passed as an argument. > > > > We need this change because in follow-up patches we want to be able to > > use reuse device_tree_for_each_node to call a function for each children > > nodes of a provided node and the node itself. > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > You can have my > > Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com> > > providing that you'll fix formatting issue below. Thank you! I'll fix the formatting issue, but won't add reviewed-by for now as I'll change the function a bit to skip calling func() on the first node. > > --- > > Changes in v5: > > - go back to v3 > > - code style improvement in acpi/boot.c > > - improve comments and commit message > > - increase min_depth to avoid parsing siblings > > - replace for with do/while loop and increase min_depth to avoid > > scanning siblings of the initial node > > - pass only node, calculate depth > > > > Changes in v3: > > - improve commit message > > - improve in-code comments > > - improve code style > > > > Changes in v2: > > - new > > --- > > xen/arch/arm/acpi/boot.c | 8 +++++--- > > xen/arch/arm/bootfdt.c | 19 ++++++++++--------- > > xen/include/xen/device_tree.h | 6 +++--- > > 3 files changed, 18 insertions(+), 15 deletions(-) > > > > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c > > index 9b29769a10..d4957cca06 100644 > > --- a/xen/arch/arm/acpi/boot.c > > +++ b/xen/arch/arm/acpi/boot.c > > @@ -246,9 +246,11 @@ int __init acpi_boot_table_init(void) > > * - the device tree is not empty (it has more than just a /chosen node) > > * and ACPI has not been force enabled (acpi=force) > > */ > > - if ( param_acpi_off || ( !param_acpi_force > > - && device_tree_for_each_node(device_tree_flattened, > > - dt_scan_depth1_nodes, NULL))) > > + if ( param_acpi_off) > > + goto disable; > > + if ( !param_acpi_force && > > + device_tree_for_each_node(device_tree_flattened, 0, > > + dt_scan_depth1_nodes, NULL) ) > There is 3 tabs, followed by spaces. > > This file missed emacs magic at the end. I think, this is cause for this > formatting issue. > > > goto disable; > > > > /* > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > index 891b4b66ff..a872ea57d6 100644 > > --- a/xen/arch/arm/bootfdt.c > > +++ b/xen/arch/arm/bootfdt.c > > @@ -77,6 +77,7 @@ static u32 __init device_tree_get_u32(const void *fdt, int node, > > /** > > * device_tree_for_each_node - iterate over all device tree nodes > > * @fdt: flat device tree. > > + * @node: node to start the search from > > * @func: function to call for each node. > > * @data: data to pass to @func. > > * > > @@ -85,20 +86,17 @@ static u32 __init device_tree_get_u32(const void *fdt, int node, > > * Returns 0 if all nodes were iterated over successfully. If @func > > * returns a value different from 0, that value is returned immediately. > > */ > > -int __init device_tree_for_each_node(const void *fdt, > > +int __init device_tree_for_each_node(const void *fdt, int node, > > device_tree_node_func func, > > void *data) > > { > > - int node; > > - int depth; > > + int depth = fdt_node_depth(fdt, node); > > + int min_depth = depth + 1; > > u32 address_cells[DEVICE_TREE_MAX_DEPTH]; > > u32 size_cells[DEVICE_TREE_MAX_DEPTH]; > > int ret; > > > > - for ( node = 0, depth = 0; > > - node >=0 && depth >= 0; > > - node = fdt_next_node(fdt, node, &depth) ) > > - { > > + do { > > const char *name = fdt_get_name(fdt, node, NULL); > > u32 as, ss; > > > > @@ -120,7 +118,10 @@ int __init device_tree_for_each_node(const void *fdt, > > ret = func(fdt, node, name, depth, as, ss, data); > > if ( ret != 0 ) > > return ret; > > - } > > + > > + node = fdt_next_node(fdt, node, &depth); > > + } while ( node >= 0 && depth >= min_depth ); > > + > > return 0; > > } > > > > @@ -357,7 +358,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr) > > > > add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false); > > > > - device_tree_for_each_node((void *)fdt, early_scan_node, NULL); > > + device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL); > > early_print_info(); > > > > return fdt_totalsize(fdt); > > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h > > index 83156297e2..9a7a8f2dab 100644 > > --- a/xen/include/xen/device_tree.h > > +++ b/xen/include/xen/device_tree.h > > @@ -158,9 +158,9 @@ typedef int (*device_tree_node_func)(const void *fdt, > > > > extern const void *device_tree_flattened; > > > > -int device_tree_for_each_node(const void *fdt, > > - device_tree_node_func func, > > - void *data); > > +int device_tree_for_each_node(const void *fdt, int node, > > + device_tree_node_func func, > > + void *data); > > > > /** > > * dt_unflatten_host_device_tree - Unflatten the host device tree > > > -- > Volodymyr Babchuk at EPAM
diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c index 9b29769a10..d4957cca06 100644 --- a/xen/arch/arm/acpi/boot.c +++ b/xen/arch/arm/acpi/boot.c @@ -246,9 +246,11 @@ int __init acpi_boot_table_init(void) * - the device tree is not empty (it has more than just a /chosen node) * and ACPI has not been force enabled (acpi=force) */ - if ( param_acpi_off || ( !param_acpi_force - && device_tree_for_each_node(device_tree_flattened, - dt_scan_depth1_nodes, NULL))) + if ( param_acpi_off) + goto disable; + if ( !param_acpi_force && + device_tree_for_each_node(device_tree_flattened, 0, + dt_scan_depth1_nodes, NULL) ) goto disable; /* diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 891b4b66ff..a872ea57d6 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -77,6 +77,7 @@ static u32 __init device_tree_get_u32(const void *fdt, int node, /** * device_tree_for_each_node - iterate over all device tree nodes * @fdt: flat device tree. + * @node: node to start the search from * @func: function to call for each node. * @data: data to pass to @func. * @@ -85,20 +86,17 @@ static u32 __init device_tree_get_u32(const void *fdt, int node, * Returns 0 if all nodes were iterated over successfully. If @func * returns a value different from 0, that value is returned immediately. */ -int __init device_tree_for_each_node(const void *fdt, +int __init device_tree_for_each_node(const void *fdt, int node, device_tree_node_func func, void *data) { - int node; - int depth; + int depth = fdt_node_depth(fdt, node); + int min_depth = depth + 1; u32 address_cells[DEVICE_TREE_MAX_DEPTH]; u32 size_cells[DEVICE_TREE_MAX_DEPTH]; int ret; - for ( node = 0, depth = 0; - node >=0 && depth >= 0; - node = fdt_next_node(fdt, node, &depth) ) - { + do { const char *name = fdt_get_name(fdt, node, NULL); u32 as, ss; @@ -120,7 +118,10 @@ int __init device_tree_for_each_node(const void *fdt, ret = func(fdt, node, name, depth, as, ss, data); if ( ret != 0 ) return ret; - } + + node = fdt_next_node(fdt, node, &depth); + } while ( node >= 0 && depth >= min_depth ); + return 0; } @@ -357,7 +358,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr) add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false); - device_tree_for_each_node((void *)fdt, early_scan_node, NULL); + device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL); early_print_info(); return fdt_totalsize(fdt); diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index 83156297e2..9a7a8f2dab 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -158,9 +158,9 @@ typedef int (*device_tree_node_func)(const void *fdt, extern const void *device_tree_flattened; -int device_tree_for_each_node(const void *fdt, - device_tree_node_func func, - void *data); +int device_tree_for_each_node(const void *fdt, int node, + device_tree_node_func func, + void *data); /** * dt_unflatten_host_device_tree - Unflatten the host device tree
Add a new parameter to device_tree_for_each_node: node, the node to start the search from. Passing 0 triggers the old behavior. Set min_depth to depth of the current node + 1 and replace the for loop with a do/while loop to avoid scanning siblings of the initial node passed as an argument. We need this change because in follow-up patches we want to be able to use reuse device_tree_for_each_node to call a function for each children nodes of a provided node and the node itself. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> --- Changes in v5: - go back to v3 - code style improvement in acpi/boot.c - improve comments and commit message - increase min_depth to avoid parsing siblings - replace for with do/while loop and increase min_depth to avoid scanning siblings of the initial node - pass only node, calculate depth Changes in v3: - improve commit message - improve in-code comments - improve code style Changes in v2: - new --- xen/arch/arm/acpi/boot.c | 8 +++++--- xen/arch/arm/bootfdt.c | 19 ++++++++++--------- xen/include/xen/device_tree.h | 6 +++--- 3 files changed, 18 insertions(+), 15 deletions(-)