of: Miscellaneous fixes and cleanups

Variable "size" has type "phys_addr_t", which can be either 32-bit or 64-bit on 32-bit systems, while "unsigned long" is always 32-bit on 32-bit systems. Hence the cast in

(unsigned long)size / SZ_1M

may truncate a 64-bit size to 32-bit, as casts have a higher operator precedence than divisions.

Fix this by inverting the order of the cast and division, which should be safe for memory blocks smaller than 4 PiB. Note that the division is actually a shift, as SZ_1M is a power-of-two constant, hence there is no need to use div_u64().

While at it, use "%lu" to format "unsigned long".

Fixes e8d9d1f5485b52ec ("drivers: of: add initialization code for static reserved memory")

Fixes 3f0c8206644836e4 ("drivers: of: add initialization code for dynamic reserved memory")

Signed-off-by Geert Uytterhoeven <[email protected]>

---
  drivers/of/fdt.c             | 8 ++++----
  drivers/of/of_reserved_mem.c | 8 ++++----
  2 files changed, 8 insertions(+), 8 deletions(-)

drivers/of/fdt.c +4 -4

index a03d43f95495d8e1..970fa8cdc9303195 100644
@@ -510,11 +510,11 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
 
  		if (size &&
  		    early_init_dt_reserve_memory_arch(base, size, nomap) == 0)
-			pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %ld MiB\n",
-				uname, &base, (unsigned long)size / SZ_1M);
+			pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
+				uname, &base, (unsigned long)(size / SZ_1M));
  		else
-			pr_info("Reserved memory: failed to reserve memory for node '%s': base %pa, size %ld MiB\n",
-				uname, &base, (unsigned long)size / SZ_1M);
+			pr_info("Reserved memory: failed to reserve memory for node '%s': base %pa, size %lu MiB\n",
+				uname, &base, (unsigned long)(size / SZ_1M));
 
  		len -= t_len;
  		if (first) {

drivers/of/of_reserved_mem.c +2 -2

index 4592b71aba5cf4a1..333d33bad59d7888 100644
@@ -136,9 +136,9 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
  			ret = early_init_dt_alloc_reserved_memory_arch(size,
  					align, start, end, nomap, &base);
  			if (ret == 0) {
-				pr_debug("allocated memory for '%s' node: base %pa, size %ld MiB\n",
+				pr_debug("allocated memory for '%s' node: base %pa, size %lu MiB\n",
  					uname, &base,
-					(unsigned long)size / SZ_1M);
+					(unsigned long)(size / SZ_1M));
  				break;
  			}
  			len -= t_len;

Variable "size" has type "phys_addr_t", which can be either 32-bit or 64-bit on 32-bit systems, while "unsigned long" is always 32-bit on 32-bit systems. Hence the cast in

(unsigned long)size / SZ_1M

may truncate a 64-bit size to 32-bit, as casts have a higher operator precedence than divisions.

Fix this by inverting the order of the cast and division, which should be safe for memory blocks smaller than 4 PiB. Note that the division is actually a shift, as SZ_1M is a power-of-two constant, hence there is no need to use div_u64().

While at it, use "%lu" to format "unsigned long".

Fixes e8d9d1f5485b52ec ("drivers: of: add initialization code for static reserved memory")

Fixes 3f0c8206644836e4 ("drivers: of: add initialization code for dynamic reserved memory")

Signed-off-by Geert Uytterhoeven <[email protected]>

---
 drivers/of/fdt.c             | 8 ++++----
 drivers/of/of_reserved_mem.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

"u64" is "unsigned long long" on all architectures now. Hence there is no longer a need to use casts when formatting using the "ll" length modifier.

Signed-off-by Geert Uytterhoeven <[email protected]>

---
 drivers/of/address.c | 14 ++++----------
 drivers/of/fdt.c     |  6 ++----
 2 files changed, 6 insertions(+), 14 deletions(-)

Commit b30be4dc733e5067 ("of: Add a common kexec FDT setup function") introduced macros FDT_PROP_INITRD_* to refer to initrd properties, but didn't use them everywhere. Convert the remaining users from string literals to macros.

Signed-off-by Geert Uytterhoeven <[email protected]>

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

drivers/of/kexec.c +2 -3

index f335d941a716e841..3fe585d5a82732e7 100644
@@ -318,13 +318,13 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
                goto out;
 
        /* Did we boot using an initrd? */
-       prop = fdt_getprop(fdt, chosen_node, "linux,initrd-start", NULL);
+       prop = fdt_getprop(fdt, chosen_node, FDT_PROP_INITRD_START, NULL);
        if (prop) {
                u64 tmp_start, tmp_end, tmp_size;
 
                tmp_start = fdt64_to_cpu(*((const fdt64_t *) prop));
 
-               prop = fdt_getprop(fdt, chosen_node, "linux,initrd-end", NULL);
+               prop = fdt_getprop(fdt, chosen_node, FDT_PROP_INITRD_END, NULL);
                if (!prop) {
                        ret = -EINVAL;
                        goto out;
--

2.25.1

Quoted 4 lines · Geert Uytterhoeven

Commit b30be4dc733e5067 ("of: Add a common kexec FDT setup function") introduced macros FDT_PROP_INITRD_* to refer to initrd properties, but didn't use them everywhere. Convert the remaining users from string literals to macros.

I'm not really a fan of the defines, so if anything I'd get rid of

them. But the bigger problem is what you brought to light with the variable size. As I mentioned, we should refactor this and the fdt.c

code to have a common function to read the initrd start and end.

Hi Rob,

Quoted 8 lines · Rob Herring

Quoted 4 lines · Geert Uytterhoeven

Commit b30be4dc733e5067 ("of: Add a common kexec FDT setup function") introduced macros FDT_PROP_INITRD_* to refer to initrd properties, but didn't use them everywhere. Convert the remaining users from string literals to macros.

I'm not really a fan of the defines, so if anything I'd get rid of

Oh, as you authored that patch, I thought you liked them ;-) And I was thinking of moving them to a header file, so they can be used by other .c files, too...

Upon closer inspection, I see you just copied them from arm64, which was not that visible due to commit ac10be5cdbfa8521 ("arm64: Use common of_kexec_alloc_and_setup_fdt()") being a separate commit...

Quoted 2 lines

them. But the bigger problem is what you brought to light with the variable size. As I mentioned, we should refactor this and the fdt.c

The number of cells to use for the initrd properties doesn't seem to be well-defined.
drivers/of/fdt.c derives it from the length of the property, which more or less always works ("be strict when sending, be liberal when receiving"). Some code hardcodes it to 1 or 2.

I suspect (didn't
check) there's also code out there that uses the root number of cells?

Quoted 1 line

code to have a common function to read the initrd start and end.

What with code that needs to set the start and end?

It needs to use what the receiving end will expect...