| Message ID | 20190815233618.31630-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v6,1/8] xen/arm: pass node to device_tree_for_each_node | expand |
Hi, On 16/08/2019 00:36, 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. Here you say 0 triggers the old behavior but... > > Set min_depth to depth of the current node + 1 to avoid scanning > siblings of the initial node passed as an argument. > > Don't call func() on the parent node passed as an argument. Clarify the > change in the comment on top of the function. ... here you mention that the first node will be skipped. So the behavior is now different and should be explained in the commit message why this is fine to skip the root node. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > --- > Changes in v6: > - fix code style > - don't call 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 | 34 ++++++++++++++++++++-------------- > xen/include/xen/device_tree.h | 6 +++--- > 3 files changed, 28 insertions(+), 20 deletions(-) > > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c > index 9b29769a10..bf9c78b02c 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..f1614ef7fc 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -75,9 +75,10 @@ static u32 __init device_tree_get_u32(const void *fdt, int node, > } > > /** > - * device_tree_for_each_node - iterate over all device tree nodes > + * device_tree_for_each_node - iterate over all device tree sub-nodes > * @fdt: flat device tree. > - * @func: function to call for each node. > + * @node: parent node to start the search from > + * @func: function to call for each sub-node. > * @data: data to pass to @func. > * > * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored. > @@ -85,20 +86,18 @@ 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); Sorry I didn't spot this in the previous version. As I pointed out on v4 (and you actually agreed!), you don't need the absolute depth... You only need the relative depth. So this is a waste of processing as you have to go through the FDT to calculate the depth. This is not entirely critical so I would be willing to consider a patch on top of this series. > + int min_depth = depth + 1; > + int first_node = node; NIT: Anything that can't change should really be const to catch any mistake. > 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; > > @@ -117,10 +116,17 @@ int __init device_tree_for_each_node(const void *fdt, > size_cells[depth] = device_tree_get_u32(fdt, node, > "#size-cells", ss); > > - ret = func(fdt, node, name, depth, as, ss, data); > - if ( ret != 0 ) > - return ret; > - } > + /* skip the first node */ > + if ( node != first_node ) > + { > + 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 +363,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 > Cheers,
On Fri, 16 Aug 2019, Julien Grall wrote: > Hi, > > On 16/08/2019 00:36, 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. > > Here you say 0 triggers the old behavior but... > > > > > Set min_depth to depth of the current node + 1 to avoid scanning > > siblings of the initial node passed as an argument. > > > > Don't call func() on the parent node passed as an argument. Clarify the > > change in the comment on top of the function. > > ... here you mention that the first node will be skipped. So the behavior is > now different and should be explained in the commit message why this is fine > to skip the root node. Yes I'll update > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > --- > > Changes in v6: > > - fix code style > > - don't call 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 | 34 ++++++++++++++++++++-------------- > > xen/include/xen/device_tree.h | 6 +++--- > > 3 files changed, 28 insertions(+), 20 deletions(-) > > > > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c > > index 9b29769a10..bf9c78b02c 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..f1614ef7fc 100644 > > --- a/xen/arch/arm/bootfdt.c > > +++ b/xen/arch/arm/bootfdt.c > > @@ -75,9 +75,10 @@ static u32 __init device_tree_get_u32(const void *fdt, > > int node, > > } > > /** > > - * device_tree_for_each_node - iterate over all device tree nodes > > + * device_tree_for_each_node - iterate over all device tree sub-nodes > > * @fdt: flat device tree. > > - * @func: function to call for each node. > > + * @node: parent node to start the search from > > + * @func: function to call for each sub-node. > > * @data: data to pass to @func. > > * > > * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored. > > @@ -85,20 +86,18 @@ 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); > > Sorry I didn't spot this in the previous version. As I pointed out on v4 (and > you actually agreed!), you don't need the absolute depth... > > You only need the relative depth. So this is a waste of processing as you have > to go through the FDT to calculate the depth. > > This is not entirely critical so I would be willing to consider a patch on top > of this series. I tried when I sent this version of the series, but I couldn't quite do it that way. I wanted to get rid of the depth parameter to device_tree_for_each_node, and we need to know the depth of the first node passed as an argument to compare it with the depth of the next node and figure out if it is at the same level or one level deeper. How do you see this being implemented? > > + int min_depth = depth + 1; > > + int first_node = node; > > NIT: Anything that can't change should really be const to catch any mistake. OK > > > 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; > > @@ -117,10 +116,17 @@ int __init device_tree_for_each_node(const void > > *fdt, > > size_cells[depth] = device_tree_get_u32(fdt, node, > > "#size-cells", ss); > > - ret = func(fdt, node, name, depth, as, ss, data); > > - if ( ret != 0 ) > > - return ret; > > - } > > + /* skip the first node */ > > + if ( node != first_node ) > > + { > > + 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 +363,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 > > > > Cheers, > > -- > Julien Grall >
Hi, On 8/17/19 1:29 AM, Stefano Stabellini wrote: > On Fri, 16 Aug 2019, Julien Grall wrote: >> Hi, >> >> On 16/08/2019 00:36, 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. >> >> Here you say 0 triggers the old behavior but... >> >>> >>> Set min_depth to depth of the current node + 1 to avoid scanning >>> siblings of the initial node passed as an argument. >>> >>> Don't call func() on the parent node passed as an argument. Clarify the >>> change in the comment on top of the function. >> >> ... here you mention that the first node will be skipped. So the behavior is >> now different and should be explained in the commit message why this is fine >> to skip the root node. > > Yes I'll update > > >>> >>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> >>> --- >>> Changes in v6: >>> - fix code style >>> - don't call 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 | 34 ++++++++++++++++++++-------------- >>> xen/include/xen/device_tree.h | 6 +++--- >>> 3 files changed, 28 insertions(+), 20 deletions(-) >>> >>> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c >>> index 9b29769a10..bf9c78b02c 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..f1614ef7fc 100644 >>> --- a/xen/arch/arm/bootfdt.c >>> +++ b/xen/arch/arm/bootfdt.c >>> @@ -75,9 +75,10 @@ static u32 __init device_tree_get_u32(const void *fdt, >>> int node, >>> } >>> /** >>> - * device_tree_for_each_node - iterate over all device tree nodes >>> + * device_tree_for_each_node - iterate over all device tree sub-nodes >>> * @fdt: flat device tree. >>> - * @func: function to call for each node. >>> + * @node: parent node to start the search from >>> + * @func: function to call for each sub-node. >>> * @data: data to pass to @func. >>> * >>> * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored. >>> @@ -85,20 +86,18 @@ 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); >> >> Sorry I didn't spot this in the previous version. As I pointed out on v4 (and >> you actually agreed!), you don't need the absolute depth... >> >> You only need the relative depth. So this is a waste of processing as you have >> to go through the FDT to calculate the depth. >> >> This is not entirely critical so I would be willing to consider a patch on top >> of this series. > > I tried when I sent this version of the series, but I couldn't quite do > it that way. I wanted to get rid of the depth parameter to > device_tree_for_each_node, and we need to know the depth of the first > node passed as an argument to compare it with the depth of the next node > and figure out if it is at the same level or one level deeper. fdt_next_node() will increment/decrement whichever depth you pass in argument. So if you pass 0, then any child of the node will be at depth 1. Any node at the same level as the parent node will also be depth 0... Therefore initializing depth to 0 and checking it is strictly positive (i.e depth > 0) is enough for our use case. Cheers,
On Mon, 19 Aug 2019, Julien Grall wrote: > On 8/17/19 1:29 AM, Stefano Stabellini wrote: > > On Fri, 16 Aug 2019, Julien Grall wrote: > > > Hi, > > > > > > On 16/08/2019 00:36, 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. > > > > > > Here you say 0 triggers the old behavior but... > > > > > > > > > > > Set min_depth to depth of the current node + 1 to avoid scanning > > > > siblings of the initial node passed as an argument. > > > > > > > > Don't call func() on the parent node passed as an argument. Clarify the > > > > change in the comment on top of the function. > > > > > > ... here you mention that the first node will be skipped. So the behavior > > > is > > > now different and should be explained in the commit message why this is > > > fine > > > to skip the root node. > > > > Yes I'll update > > > > > > > > > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > > > --- > > > > Changes in v6: > > > > - fix code style > > > > - don't call 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 | 34 ++++++++++++++++++++-------------- > > > > xen/include/xen/device_tree.h | 6 +++--- > > > > 3 files changed, 28 insertions(+), 20 deletions(-) > > > > > > > > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c > > > > index 9b29769a10..bf9c78b02c 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..f1614ef7fc 100644 > > > > --- a/xen/arch/arm/bootfdt.c > > > > +++ b/xen/arch/arm/bootfdt.c > > > > @@ -75,9 +75,10 @@ static u32 __init device_tree_get_u32(const void > > > > *fdt, > > > > int node, > > > > } > > > > /** > > > > - * device_tree_for_each_node - iterate over all device tree nodes > > > > + * device_tree_for_each_node - iterate over all device tree sub-nodes > > > > * @fdt: flat device tree. > > > > - * @func: function to call for each node. > > > > + * @node: parent node to start the search from > > > > + * @func: function to call for each sub-node. > > > > * @data: data to pass to @func. > > > > * > > > > * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored. > > > > @@ -85,20 +86,18 @@ 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); > > > > > > Sorry I didn't spot this in the previous version. As I pointed out on v4 > > > (and > > > you actually agreed!), you don't need the absolute depth... > > > > > > You only need the relative depth. So this is a waste of processing as you > > > have > > > to go through the FDT to calculate the depth. > > > > > > This is not entirely critical so I would be willing to consider a patch on > > > top > > > of this series. > > > > I tried when I sent this version of the series, but I couldn't quite do > > it that way. I wanted to get rid of the depth parameter to > > device_tree_for_each_node, and we need to know the depth of the first > > node passed as an argument to compare it with the depth of the next node > > and figure out if it is at the same level or one level deeper. > > fdt_next_node() will increment/decrement whichever depth you pass in argument. > > So if you pass 0, then any child of the node will be at depth 1. Any node at > the same level as the parent node will also be depth 0... > > Therefore initializing depth to 0 and checking it is strictly positive (i.e > depth > 0) is enough for our use case. That makes a lot of sense and will improve the code. Thanks for the suggestion.
diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c index 9b29769a10..bf9c78b02c 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..f1614ef7fc 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -75,9 +75,10 @@ static u32 __init device_tree_get_u32(const void *fdt, int node, } /** - * device_tree_for_each_node - iterate over all device tree nodes + * device_tree_for_each_node - iterate over all device tree sub-nodes * @fdt: flat device tree. - * @func: function to call for each node. + * @node: parent node to start the search from + * @func: function to call for each sub-node. * @data: data to pass to @func. * * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored. @@ -85,20 +86,18 @@ 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; + int first_node = node; 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; @@ -117,10 +116,17 @@ int __init device_tree_for_each_node(const void *fdt, size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells", ss); - ret = func(fdt, node, name, depth, as, ss, data); - if ( ret != 0 ) - return ret; - } + /* skip the first node */ + if ( node != first_node ) + { + 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 +363,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 to avoid scanning siblings of the initial node passed as an argument. Don't call func() on the parent node passed as an argument. Clarify the change in the comment on top of the function. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> --- Changes in v6: - fix code style - don't call 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 | 34 ++++++++++++++++++++-------------- xen/include/xen/device_tree.h | 6 +++--- 3 files changed, 28 insertions(+), 20 deletions(-)