* Re: [PATCH] xen/arm: fdt fix duplicated ternary operator, shift operations
2022-04-22 18:52 [PATCH] xen/arm: fdt fix duplicated ternary operator, shift operations Paran Lee
@ 2022-04-22 23:27 ` Stefano Stabellini
2022-04-24 15:46 ` Julien Grall
1 sibling, 0 replies; 3+ messages in thread
From: Stefano Stabellini @ 2022-04-22 23:27 UTC (permalink / raw)
To: Paran Lee
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Austin Kim, xen-devel
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] xen/arm: fdt fix duplicated ternary operator, shift operations
2022-04-22 18:52 [PATCH] xen/arm: fdt fix duplicated ternary operator, shift operations Paran Lee
2022-04-22 23:27 ` Stefano Stabellini
@ 2022-04-24 15:46 ` Julien Grall
1 sibling, 0 replies; 3+ messages in thread
From: Julien Grall @ 2022-04-24 15:46 UTC (permalink / raw)
To: Paran Lee, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Cc: Austin Kim, xen-devel
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
^ permalink raw reply related [flat|nested] 3+ messages in thread