Re: [PATCH] xen/arm: fdt fix duplicated ternary operator, shift operations

[Julien Grall <julien@xen.org>]

Hi,

On 22/04/2022 19:52, Paran Lee wrote:
> It doesn't seem necessary to do duplicate ternary operation and calculation
> of order shift using fdt32_to_cpu macro.
> 
> Signed-off-by: Paran Lee <p4ranlee@gmail.com>
> ---
>   xen/arch/arm/bootfdt.c  | 12 ++++++++++--
>   xen/common/libfdt/fdt.c | 10 +++++-----
>   2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index e318ef9603..e5b885a7f2 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -159,8 +159,16 @@ int __init device_tree_for_each_node(const void *fdt, int node,
>               continue;
>           }
>   
> -        as = depth > 0 ? address_cells[depth-1] : DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
> -        ss = depth > 0 ? size_cells[depth-1] : DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
> +        if ( depth > 0 )
> +        {
> +            as = address_cells[depth-1];
> +            ss = size_cells[depth-1];
> +        }
> +        else
> +        {
> +            as = DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
> +            ss = DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
> +        }
IHMO the original code is easier to read. That said, in the two cases, I 
think this is a bit pointless to check if the depth is > 0 at every 
iteration because it will mostly be only always true but for one node.

So I would go with the following code (not tested):

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index e318ef960386..a382e10065f9 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -144,10 +144,13 @@ int __init device_tree_for_each_node(const void 
*fdt, int node,
       */
      int depth = 0;
      const int first_node = node;
-    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
-    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
+    u32 address_cells[DEVICE_TREE_MAX_DEPTH + 1];
+    u32 size_cells[DEVICE_TREE_MAX_DEPTH + 1];
      int ret;

+    address_cells[0] = DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
+    size_cells[0] = DT_ROOT_NOT_SIZE_CELLS_DEFAULT;
+
      do {
          const char *name = fdt_get_name(fdt, node, NULL);
          u32 as, ss;
@@ -159,13 +162,13 @@ int __init device_tree_for_each_node(const void 
*fdt, int node,
              continue;
          }

-        as = depth > 0 ? address_cells[depth-1] : 
DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
-        ss = depth > 0 ? size_cells[depth-1] : 
DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
+        as = address_cells[depth];
+        ss = size_cells[depth];

-        address_cells[depth] = device_tree_get_u32(fdt, node,
+        address_cells[depth + 1] = device_tree_get_u32(fdt, node,
                                                     "#address-cells", as);
-        size_cells[depth] = device_tree_get_u32(fdt, node,
-                                                "#size-cells", ss);
+        size_cells[depth + 1] = device_tree_get_u32(fdt, node,
+                                                    "#size-cells", ss);

          /* skip the first node */
          if ( node != first_node )

Any thoughts?

>   
>           address_cells[depth] = device_tree_get_u32(fdt, node,
>                                                      "#address-cells", as);
> diff --git a/xen/common/libfdt/fdt.c b/xen/common/libfdt/fdt.c
> index 9fe7cf4b74..a507169d29 100644
> --- a/xen/common/libfdt/fdt.c
> +++ b/xen/common/libfdt/fdt.c

fdt.c is an (old) verbatim copy from DTC (see [1]). I would rather 
prefer to keep this directory as a verbatim copy because it makes easier 
to sync with the upstream version.

In fact, there are a patch on xen-devel ([1]) to re-sync. So any of your 
changes would be lost. Therefore, please send this change to the DTC 
mailing list. We will pick it up on the next re-sync.

Cheers,

[1] https://github.com/dgibson/dtc
[2] 
https://lore.kernel.org/xen-devel/1636702040-116895-1-git-send-email-fnu.vikram@xilinx.com/

-- 
Julien Grall