[PATCH] arm64: Fix early pointer print plus improve comment

[PATCH] arm64: Fix early pointer print plus improve comment

[Guilherme G. Piccoli]

When facing a really early issue on DT parsing we have currently
a message that shows both the physical and virtual address of the
FDT. The printk pointer modifier there was not right for the physical
address, due to the hashed address stuff, so hereby we fix that.

Also, we tried to improve a bit commenting on that function, given that
if kernel fails there, it just hangs forever in a cpu_relax() loop.
The reason we cannot BUG/panic there is that is too early to do so;
thanks to Mark Brown for pointing that on IRC.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 arch/arm64/kernel/setup.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index be5f85b0a24d..0d5c51a9549e 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -189,11 +189,15 @@ static void __init setup_machine_fdt(phys_addr_t dt_phys)
 
 	if (!dt_virt || !early_init_dt_scan(dt_virt)) {
 		pr_crit("\n"
-			"Error: invalid device tree blob at physical address %pa (virtual address 0x%p)\n"
+			"Error: invalid device tree blob at physical address %px (virtual address 0x%p)\n"
 			"The dtb must be 8-byte aligned and must not exceed 2 MB in size\n"
 			"\nPlease check your bootloader.",
 			&dt_phys, dt_virt);
 
+		/* Note that in this _really_ early stage we cannot even BUG()
+		 * or oops, so the least terrible thing to do is cpu_relax(),
+		 * or else we could end-up printing non-initialized data, etc.
+		 */
 		while (true)
 			cpu_relax();
 	}
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: Fix early pointer print plus improve comment

[Robin Murphy]

On 2021-12-20 15:54, Guilherme G. Piccoli wrote:
> When facing a really early issue on DT parsing we have currently
> a message that shows both the physical and virtual address of the
> FDT. The printk pointer modifier there was not right for the physical
> address, due to the hashed address stuff, so hereby we fix that.
> 
> Also, we tried to improve a bit commenting on that function, given that
> if kernel fails there, it just hangs forever in a cpu_relax() loop.
> The reason we cannot BUG/panic there is that is too early to do so;
> thanks to Mark Brown for pointing that on IRC.
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
>   arch/arm64/kernel/setup.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index be5f85b0a24d..0d5c51a9549e 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -189,11 +189,15 @@ static void __init setup_machine_fdt(phys_addr_t dt_phys)
>   
>   	if (!dt_virt || !early_init_dt_scan(dt_virt)) {
>   		pr_crit("\n"
> -			"Error: invalid device tree blob at physical address %pa (virtual address 0x%p)\n"
> +			"Error: invalid device tree blob at physical address %px (virtual address 0x%p)\n"

%pa is the correct specifier for printing a physical address, so I don't 
really follow what's being "fixed" here. Especially given that what this 
will now do is print the virtual address of the local dt_phys variable 
itself. How is that useful?

Robin.

>   			"The dtb must be 8-byte aligned and must not exceed 2 MB in size\n"
>   			"\nPlease check your bootloader.",
>   			&dt_phys, dt_virt);
>   
> +		/* Note that in this _really_ early stage we cannot even BUG()
> +		 * or oops, so the least terrible thing to do is cpu_relax(),
> +		 * or else we could end-up printing non-initialized data, etc.
> +		 */
>   		while (true)
>   			cpu_relax();
>   	}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: Fix early pointer print plus improve comment

[Guilherme G. Piccoli]

On 21/12/2021 07:11, Robin Murphy wrote:
> [...]
> %pa is the correct specifier for printing a physical address, so I don't 
> really follow what's being "fixed" here. Especially given that what this 
> will now do is print the virtual address of the local dt_phys variable 
> itself. How is that useful?

Hi Robin, thanks a lot for your review, and I owe you (and the list) an
apology. I confused myself, this is not the one to fix - the %p on
virtual address is the one that is bogus!

This patch (the doc part) was stuck here for some months..and I
remembered that there was a pointer hashed that I also fixed to see its
value (in an issue I was investigating that time), but I swapped the
pointers! My bad, it was a lame confusion.

I'll submit a V2! Just for the sake of completeness, I've tested and
having the virt address with %p got me a "0x(____ptrval____)", while
with %px I can see "0xfffffbfffdc00000)".

Cheers,


Guilherme

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel