of/fdt: Some bug fixes and cleanups

This patch series fixes several bugs related to dt_root_addr_cells and dt_root_size_cells, and performs some cleanup.

Links to the previous related patches:

https://lore.kernel.org/lkml/CAL_JsqJxar7z+VcBXwPTw5-Et2oC9bQmH_CtMtKhoo_-=zN2XQ@mail.gmail.com/

Yuntao Wang (10):
of/fdt: Introduce dt_root_addr_size_cells() and
dt_root_addr_size_bytes()
of/reserved_mem: Use dt_root_addr_size_bytes() instead of open-coding
it
of/reserved_mem: Use dt_root_addr_size_bytes() instead of open-coding
it
of/reserved_mem: Use dt_root_addr_size_bytes() instead of open-coding
it
of/fdt: Use dt_root_addr_size_bytes() instead of open-coding it of/fdt: Fix the len check in early_init_dt_check_for_elfcorehdr() of/fdt: Fix the len check in
early_init_dt_check_for_usable_mem_range()
of/fdt: Use dt_root_addr_size_bytes() instead of open-coding it of/fdt: Fix incorrect use of dt_root_addr_cells in
early_init_dt_check_kho()
of/address: Remove the incorrect and misleading comment

 drivers/of/address.c         |  4 ----
 drivers/of/fdt.c             | 14 +++++++-------
 drivers/of/of_reserved_mem.c |  6 +++---
 include/linux/of_fdt.h       | 11 +++++++++++
 4 files changed, 21 insertions(+), 14 deletions(-)

2.51.0

Currently, many places use the result of the expression (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32) for various checks.

To improve code maintainability and reduce the chance of errors, extract this expression into two helper functions.

---
 include/linux/of_fdt.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

index b8d6c0c20876..9ce2433865ae 100644
@@ -30,6 +30,17 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
 /* TBD: Temporary export of fdt globals - remove when code fully merged */
 extern int __initdata dt_root_addr_cells;
 extern int __initdata dt_root_size_cells;
+
+static inline int dt_root_addr_size_cells(void)
+{
+	return dt_root_addr_cells + dt_root_size_cells;
+}
+
+static inline int dt_root_addr_size_bytes(void)
+{
+	return dt_root_addr_size_cells() * sizeof(__be32);
+}
+
 extern void *initial_boot_params;
 extern phys_addr_t initial_boot_params_pa;
 

2.51.0

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Use dt_root_addr_size_bytes() instead of open-coding it in __reserved_mem_reserve_reg() to improve code maintainability.

---
 drivers/of/of_reserved_mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2e9ea751ed2d..67c0ccd373c2 100644
@@ -154,7 +154,7 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
 static int __init __reserved_mem_reserve_reg(unsigned long node,
 					     const char *uname)
 {
-	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
+	int t_len = dt_root_addr_size_bytes();
 	phys_addr_t base, size;
 	int len;
 	const __be32 *prop;

2.51.0

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Use dt_root_addr_size_bytes() instead of open-coding it in fdt_scan_reserved_mem_reg_nodes() to improve code maintainability.

---
 drivers/of/of_reserved_mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 67c0ccd373c2..0e6b0e7968a9 100644
@@ -230,7 +230,7 @@ static void __init __rmem_check_for_overlap(void);
  */
 void __init fdt_scan_reserved_mem_reg_nodes(void)
 {
-	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
+	int t_len = dt_root_addr_size_bytes();
 	const void *fdt = initial_boot_params;
 	phys_addr_t base, size;
 	const __be32 *prop;

2.51.0

But please combine with the previous patch with the same subject.

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Use dt_root_addr_size_bytes() instead of open-coding it in __reserved_mem_alloc_size() to improve code maintainability.

---
 drivers/of/of_reserved_mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 0e6b0e7968a9..8d9139f4297f 100644
@@ -401,7 +401,7 @@ static int __init __reserved_mem_alloc_in_range(phys_addr_t size,
  */
 static int __init __reserved_mem_alloc_size(unsigned long node, const char *uname)
 {
-	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
+	int t_len = dt_root_addr_size_bytes();
 	phys_addr_t start = 0, end = 0;
 	phys_addr_t base = 0, align = 0, size;
 	int len;

2.51.0

But please combine with the previous patch with the same subject.

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Use dt_root_addr_size_bytes() instead of open-coding it in of_fdt_limit_memory() to improve code maintainability.

---
 drivers/of/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 0edd639898a6..a1db3c9ac981 100644
@@ -52,7 +52,7 @@ void __init of_fdt_limit_memory(int limit)
 	int memory;
 	int len;
 	const void *val;
-	int cell_size = sizeof(uint32_t)*(dt_root_addr_cells + dt_root_size_cells);
+	int cell_size = dt_root_addr_size_bytes();
 
 	memory = fdt_path_offset(initial_boot_params, "/memory");
 	if (memory > 0) {

2.51.0

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

The len value is in bytes, while dt_root_addr_cells + dt_root_size_cells is in cells (4 bytes per cell).

Comparing them directly is incorrect. Convert units before comparison.

---
 drivers/of/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a1db3c9ac981..716ebe8c23d3 100644
@@ -821,7 +821,7 @@ static void __init early_init_dt_check_for_elfcorehdr(unsigned long node)
 	pr_debug("Looking for elfcorehdr property... ");
 
 	prop = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
-	if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
+	if (!prop || len < dt_root_addr_size_bytes())
 		return;
 
 	elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &prop);

2.51.0

The len value is in bytes, while dt_root_addr_cells + dt_root_size_cells is in cells (4 bytes per cell).

The modulo calculation between them is incorrect, the units must be converted first.

---
 drivers/of/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 716ebe8c23d3..0f95f3b356ea 100644
@@ -859,7 +859,7 @@ void __init early_init_dt_check_for_usable_mem_range(void)
 	pr_debug("Looking for usable-memory-range property... ");
 
 	prop = of_get_flat_dt_prop(node, "linux,usable-memory-range", &len);
-	if (!prop || (len % (dt_root_addr_cells + dt_root_size_cells)))
+	if (!prop || len % dt_root_addr_size_bytes())
 		return;
 
 	endp = prop + (len / sizeof(__be32));

2.51.0

Hi Yuntao,

Thanks for your patch!

That commit merely changed "<" to "%".

The code that added the bad expression was:

usable memory range property")
However, that code was copied from:

property, usable-memory-range")

So I think you want to list these two Fixes-tags instead.

For the actual change:

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Use dt_root_addr_size_bytes() instead of open-coding it in early_init_dt_check_kho() to improve code maintainability.

---
 drivers/of/fdt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 0f95f3b356ea..fa1703612530 100644
@@ -890,14 +890,14 @@ static void __init early_init_dt_check_kho(void)
 		return;
 
 	p = of_get_flat_dt_prop(node, "linux,kho-fdt", &l);
-	if (l != (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32))
+	if (l != dt_root_addr_size_bytes())
 		return;
 
 	fdt_start = dt_mem_next_cell(dt_root_addr_cells, &p);
 	fdt_size = dt_mem_next_cell(dt_root_addr_cells, &p);
 
 	p = of_get_flat_dt_prop(node, "linux,kho-scratch", &l);
-	if (l != (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32))
+	if (l != dt_root_addr_size_bytes())
 		return;
 
 	scratch_start = dt_mem_next_cell(dt_root_addr_cells, &p);

2.51.0

But please combine with the other patch with the same subject.

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

When reading the fdt_size value, the argument passed to dt_mem_next_cell() is dt_root_addr_cells, but it should be dt_root_size_cells.

The same issue occurs when reading the scratch_size value.

Fix these issues.

---
 drivers/of/fdt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index fa1703612530..3b34c7e71ebc 100644
@@ -894,14 +894,14 @@ static void __init early_init_dt_check_kho(void)
 		return;
 
 	fdt_start = dt_mem_next_cell(dt_root_addr_cells, &p);
-	fdt_size = dt_mem_next_cell(dt_root_addr_cells, &p);
+	fdt_size = dt_mem_next_cell(dt_root_size_cells, &p);
 
 	p = of_get_flat_dt_prop(node, "linux,kho-scratch", &l);
 	if (l != dt_root_addr_size_bytes())
 		return;
 
 	scratch_start = dt_mem_next_cell(dt_root_addr_cells, &p);
-	scratch_size = dt_mem_next_cell(dt_root_addr_cells, &p);
+	scratch_size = dt_mem_next_cell(dt_root_size_cells, &p);
 
 	kho_populate(fdt_start, fdt_size, scratch_start, scratch_size);
 }

2.51.0

Hi Yuntao,

Thanks for your patch!

    /* TBD: Temporary export of fdt globals - remove when code fully merged */
    extern int __initdata dt_root_addr_cells;
    extern int __initdata dt_root_size_cells;

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

The of_bus_default_match() function appears to have been copied from of_bus_default_flags_match() with some modifications.

However, the comment was left unchanged and still describes the behavior of of_bus_default_flags_match(), it is incorrect and misleading, remove it.

---
 drivers/of/address.c | 4 ----
 1 file changed, 4 deletions(-)

index f0f8f0dd191c..4034d798c55a 100644
@@ -328,10 +328,6 @@ static int of_bus_default_flags_match(struct device_node *np)
 
 static int of_bus_default_match(struct device_node *np)
 {
-	/*
-	 * Check for presence first since of_bus_n_addr_cells() will warn when
-	 * walking parent nodes.
-	 */
 	return of_property_present(np, "#address-cells");
 }
 

2.51.0

translating non-translatable addresses")

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

There is no reason to put this patch in this series. It is completely unrelated.

Applied, thanks!

Your aim in writing subjects should be to write something that is unique for every commit in the past or future. Because you can never make the same change twice, right? (I'm excluding 'fix typos/spelling' type commits). Certainly the same subject in one series is never right.

This is not what I meant. We have multiple copies of this where only the property name changes:

	prop = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
	if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
		return;

	elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
	elfcorehdr_size = dt_mem_next_cell(dt_root_size_cells, &prop);

Instead, add a function something like this:

static void early_init_dt_read_address(unsigned long node, const char *prop, u64 *addr, u64*size)

{
        prop = of_get_flat_dt_prop(node, prop, &len);
        if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
                return;

        *addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
        *size = dt_mem_next_cell(dt_root_size_cells, &prop);
}

Then we only have the length checks in one place.

That still leaves the cases with more than 1 entry open coded. So instead, to cover that case to something like this:

const __be32 *of_get_flat_dt_address_prop(unsigned long node, const char *propname, int *len)

{
	prop = of_get_flat_dt_prop(node, propname, &len);
	if (!prop || (*len % (dt_root_addr_cells + dt_root_size_cells))) {
		*len = 0;
		return NULL;
	}

	*len /= (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
	return prop;
}

And then a user would look something like this:

prop = of_get_flat_dt_address(node, "linux,usable-memory-range", &len);

for (i = 0; i < len; i++) {
	of_read_address_idx(prop, i, &addr, &size);
	...
}

Here 'len' is number of addr+size entries.

And the simple case of reading 1 entry could be just:

of_read_address_idx(of_get_flat_dt_address(node, "linux,elfcorehdr", NULL), 0, &addr, &size);

Rob

Hi Rob,

The link to the new patch series:

https://lore.kernel.org/linux-devicetree/[email protected]/t/

Thanks,
Yuntao