[PATCH 0/3] of: Miscellaneous fixes and cleanups

[PATCH 0/3] of: Miscellaneous fixes and cleanups

[Geert Uytterhoeven]

	Hi all,

This patch series contains a fix and two cleanups for the OF code.

Thanks!

Geert Uytterhoeven (3):
  of: Fix truncation of memory sizes on 32-bit platforms
  of: Remove superfluous casts when printing u64 values
  of: kexec: Always use FDT_PROP_INITRD_START and FDT_PROP_INITRD_END

 drivers/of/address.c         | 14 ++++----------
 drivers/of/fdt.c             | 14 ++++++--------
 drivers/of/kexec.c           |  4 ++--
 drivers/of/of_reserved_mem.c |  8 ++++----
 4 files changed, 16 insertions(+), 24 deletions(-)

-- 
2.25.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

[PATCH 1/3] of: Fix truncation of memory sizes on 32-bit platforms

[Geert Uytterhoeven]

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 <geert+renesas@glider.be>
---
 drivers/of/fdt.c             | 8 ++++----
 drivers/of/of_reserved_mem.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index a03d43f95495d8e1..970fa8cdc9303195 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -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) {
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 4592b71aba5cf4a1..333d33bad59d7888 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -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;
@@ -148,8 +148,8 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
 		ret = early_init_dt_alloc_reserved_memory_arch(size, align,
 							0, 0, nomap, &base);
 		if (ret == 0)
-			pr_debug("allocated memory for '%s' node: base %pa, size %ld MiB\n",
-				uname, &base, (unsigned long)size / SZ_1M);
+			pr_debug("allocated memory for '%s' node: base %pa, size %lu MiB\n",
+				uname, &base, (unsigned long)(size / SZ_1M));
 	}
 
 	if (base == 0) {
-- 
2.25.1

Re: [PATCH 1/3] of: Fix truncation of memory sizes on 32-bit platforms

[Marek Szyprowski]

On 16.06.2021 11:27, Geert Uytterhoeven wrote:
> 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 <geert+renesas@glider.be>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   drivers/of/fdt.c             | 8 ++++----
>   drivers/of/of_reserved_mem.c | 8 ++++----
>   2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index a03d43f95495d8e1..970fa8cdc9303195 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -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) {
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 4592b71aba5cf4a1..333d33bad59d7888 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -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;
> @@ -148,8 +148,8 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
>   		ret = early_init_dt_alloc_reserved_memory_arch(size, align,
>   							0, 0, nomap, &base);
>   		if (ret == 0)
> -			pr_debug("allocated memory for '%s' node: base %pa, size %ld MiB\n",
> -				uname, &base, (unsigned long)size / SZ_1M);
> +			pr_debug("allocated memory for '%s' node: base %pa, size %lu MiB\n",
> +				uname, &base, (unsigned long)(size / SZ_1M));
>   	}
>   
>   	if (base == 0) {

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 1/3] of: Fix truncation of memory sizes on 32-bit platforms

[Rob Herring]

On Wed, 16 Jun 2021 11:27:44 +0200, Geert Uytterhoeven wrote:
> 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 <geert+renesas@glider.be>
> ---
>  drivers/of/fdt.c             | 8 ++++----
>  drivers/of/of_reserved_mem.c | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 

Applied, thanks!

[PATCH 2/3] of: Remove superfluous casts when printing u64 values

[Geert Uytterhoeven]

"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 <geert+renesas@glider.be>
---
 drivers/of/address.c | 14 ++++----------
 drivers/of/fdt.c     |  6 ++----
 2 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 350b2baa0af38497..c04fce0fb1790cb6 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -78,9 +78,7 @@ static u64 of_bus_default_map(__be32 *addr, const __be32 *range,
 	s  = of_read_number(range + na + pna, ns);
 	da = of_read_number(addr, na);
 
-	pr_debug("default map, cp=%llx, s=%llx, da=%llx\n",
-		 (unsigned long long)cp, (unsigned long long)s,
-		 (unsigned long long)da);
+	pr_debug("default map, cp=%llx, s=%llx, da=%llx\n", cp, s, da);
 
 	if (da < cp || da >= (cp + s))
 		return OF_BAD_ADDR;
@@ -187,9 +185,7 @@ static u64 of_bus_pci_map(__be32 *addr, const __be32 *range, int na, int ns,
 	s  = of_read_number(range + na + pna, ns);
 	da = of_read_number(addr + 1, na - 1);
 
-	pr_debug("PCI map, cp=%llx, s=%llx, da=%llx\n",
-		 (unsigned long long)cp, (unsigned long long)s,
-		 (unsigned long long)da);
+	pr_debug("PCI map, cp=%llx, s=%llx, da=%llx\n", cp, s, da);
 
 	if (da < cp || da >= (cp + s))
 		return OF_BAD_ADDR;
@@ -302,9 +298,7 @@ static u64 of_bus_isa_map(__be32 *addr, const __be32 *range, int na, int ns,
 	s  = of_read_number(range + na + pna, ns);
 	da = of_read_number(addr + 1, na - 1);
 
-	pr_debug("ISA map, cp=%llx, s=%llx, da=%llx\n",
-		 (unsigned long long)cp, (unsigned long long)s,
-		 (unsigned long long)da);
+	pr_debug("ISA map, cp=%llx, s=%llx, da=%llx\n", cp, s, da);
 
 	if (da < cp || da >= (cp + s))
 		return OF_BAD_ADDR;
@@ -459,7 +453,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
 
  finish:
 	of_dump_addr("parent translation for:", addr, pna);
-	pr_debug("with offset: %llx\n", (unsigned long long)offset);
+	pr_debug("with offset: %llx\n", offset);
 
 	/* Translate it into parent bus space */
 	return pbus->translate(addr, offset, pna);
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 970fa8cdc9303195..344f16bb04ccf081 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -900,8 +900,7 @@ static void __init early_init_dt_check_for_initrd(unsigned long node)
 	phys_initrd_start = start;
 	phys_initrd_size = end - start;
 
-	pr_debug("initrd_start=0x%llx  initrd_end=0x%llx\n",
-		 (unsigned long long)start, (unsigned long long)end);
+	pr_debug("initrd_start=0x%llx  initrd_end=0x%llx\n", start, end);
 }
 #else
 static inline void early_init_dt_check_for_initrd(unsigned long node)
@@ -1027,8 +1026,7 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
 
 		if (size == 0)
 			continue;
-		pr_debug(" - %llx ,  %llx\n", (unsigned long long)base,
-		    (unsigned long long)size);
+		pr_debug(" - %llx, %llx\n", base, size);
 
 		early_init_dt_add_memory_arch(base, size);
 
-- 
2.25.1

Re: [PATCH 2/3] of: Remove superfluous casts when printing u64 values

[Rob Herring]

On Wed, 16 Jun 2021 11:27:45 +0200, Geert Uytterhoeven wrote:
> "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 <geert+renesas@glider.be>
> ---
>  drivers/of/address.c | 14 ++++----------
>  drivers/of/fdt.c     |  6 ++----
>  2 files changed, 6 insertions(+), 14 deletions(-)
> 

Applied, thanks!

[PATCH 3/3] of: kexec: Always use FDT_PROP_INITRD_START and FDT_PROP_INITRD_END

[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.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/of/kexec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
index f335d941a716e841..3fe585d5a82732e7 100644
--- a/drivers/of/kexec.c
+++ b/drivers/of/kexec.c
@@ -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

Re: [PATCH 3/3] of: kexec: Always use FDT_PROP_INITRD_START and FDT_PROP_INITRD_END

[Rob Herring]

On Wed, Jun 16, 2021 at 3:27 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> 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.

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/of/kexec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> index f335d941a716e841..3fe585d5a82732e7 100644
> --- a/drivers/of/kexec.c
> +++ b/drivers/of/kexec.c
> @@ -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
>

Re: [PATCH 3/3] of: kexec: Always use FDT_PROP_INITRD_START and FDT_PROP_INITRD_END

[Geert Uytterhoeven]

Hi Rob,

On Wed, Jun 16, 2021 at 7:14 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Wed, Jun 16, 2021 at 3:27 AM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > 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...

> 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?

> 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...

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 3/3] of: kexec: Always use FDT_PROP_INITRD_START and FDT_PROP_INITRD_END

[Rob Herring]

On Wed, Jun 16, 2021 at 1:36 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Wed, Jun 16, 2021 at 7:14 PM Rob Herring <robh+dt@kernel.org> wrote:
> > On Wed, Jun 16, 2021 at 3:27 AM Geert Uytterhoeven
> > <geert+renesas@glider.be> wrote:
> > > 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...

That all was largely a 'this is what you need to implement' review.

> > 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.

The not well defined ship has sailed, so we need to support either.

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

Given it's a single value, there's not really any need to do that.
Unlike some of the kexec properties which combine the start and length
for example.

> > code to have a common function to read the initrd start and end.
>
> What with code that needs to set the start and end?

I'm not sure we can consolidate that as those are mostly in early arch
boot code.

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

That should be fine given fdt.c is flexible already.

Rob