1. Patchew
  2. Xen
  3. View series

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

Paran Lee posted 1 patch 2 years, 7 months ago
Test gitlab-ci passed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220422185251.GA7124@DESKTOP-NK4TH6S.localdomain
xen/arch/arm/bootfdt.c  | 12 ++++++++++--
xen/common/libfdt/fdt.c | 10 +++++-----
2 files changed, 15 insertions(+), 7 deletions(-)
[PATCH] xen/arm: fdt fix duplicated ternary operator, shift operations
Posted by Paran Lee 2 years, 7 months ago 
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;
+        }
 
         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
@@ -165,7 +165,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
 uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 {
 	const fdt32_t *tagp, *lenp;
-	uint32_t tag;
+	uint32_t tag, len;
 	int offset = startoffset;
 	const char *p;
 
@@ -192,11 +192,11 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 		if (!can_assume(VALID_DTB) && !lenp)
 			return FDT_END; /* premature end */
 		/* skip-name offset, length and value */
-		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
-			+ fdt32_to_cpu(*lenp);
+		len = fdt32_to_cpu(*lenp);
+		offset += sizeof(struct fdt_property) - FDT_TAGSIZE + len;
 		if (!can_assume(LATEST) &&
-		    fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
-		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
+		    fdt_version(fdt) < 0x10 && len >= 8 &&
+		    ((offset - len) % 8) != 0)
 			offset += 4;
 		break;
 
-- 
2.25.1
Re: [PATCH] xen/arm: fdt fix duplicated ternary operator, shift operations
Posted by Julien Grall 2 years, 7 months ago 
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
Re: [PATCH] xen/arm: fdt fix duplicated ternary operator, shift operations
Posted by Stefano Stabellini 2 years, 7 months ago 
On Sat, 23 Apr 2022, 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>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  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;
> +        }
>  
>          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
> @@ -165,7 +165,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>  uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  {
>  	const fdt32_t *tagp, *lenp;
> -	uint32_t tag;
> +	uint32_t tag, len;
>  	int offset = startoffset;
>  	const char *p;
>  
> @@ -192,11 +192,11 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  		if (!can_assume(VALID_DTB) && !lenp)
>  			return FDT_END; /* premature end */
>  		/* skip-name offset, length and value */
> -		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> -			+ fdt32_to_cpu(*lenp);
> +		len = fdt32_to_cpu(*lenp);
> +		offset += sizeof(struct fdt_property) - FDT_TAGSIZE + len;
>  		if (!can_assume(LATEST) &&
> -		    fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
> -		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
> +		    fdt_version(fdt) < 0x10 && len >= 8 &&
> +		    ((offset - len) % 8) != 0)
>  			offset += 4;
>  		break;
>  
> -- 
> 2.25.1
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.