Re: [PATCH v2] pstore: Support already existing reserved-memory node

[Daniel Golle <daniel@makrotopia.org>]

Hi Detlev,

I've recently tried to update U-Boot from 2022.01 to 2022.04 and
noticed a regression introduced by the commit below.

While the unwanted error message ("Add 'reserved-memory' node failed:
FDT_ERR_EXISTS") no longer appears, I now see a lengthy kernel stack
trace in Linux instead:
...
[    0.008295] sysfs: cannot create duplicate filename '/devices/platform/42ff0000.ramoops'
[    0.008303] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S                5.15.33 #0
[    0.008312] Hardware name: Bananapi BPI-R64 (DT)
[    0.008318] Call trace:
[    0.008322]  dump_backtrace+0x0/0x15c
[    0.008337]  show_stack+0x14/0x30
[    0.008345]  dump_stack_lvl+0x64/0x7c
[    0.008355]  dump_stack+0x14/0x2c
[    0.008362]  sysfs_warn_dup+0x5c/0x74
[    0.008373]  sysfs_create_dir_ns+0xb0/0xc0
[    0.008381]  kobject_add_internal+0xbc/0x330
[    0.008392]  kobject_add+0x80/0xe0
[    0.008400]  device_add+0xd4/0x82c
[    0.008410]  of_device_add+0x4c/0x5c
[    0.008420]  of_platform_device_create_pdata+0xb8/0xf0
[    0.008428]  of_platform_default_populate_init+0x58/0xcc
[    0.008437]  do_one_initcall+0x4c/0x1b0
[    0.008444]  kernel_init_freeable+0x230/0x294
[    0.008456]  kernel_init+0x20/0x120
[    0.008463]  ret_from_fork+0x10/0x20
[    0.008471] kobject_add_internal failed for 42ff0000.ramoops with -EEXIST, don't try to register things with the same name in the same directory.
...

I assume that fdt_find_or_add_subnode() doesn't behave as expected and
apparently adds a duplicate node having the same name.
The pstore/ramoops reserved-memory is already defined in the fdt
contained in the FIT image (for compatibility with older U-Boot which
didn't care about pstore), so it should be not added again.


On Mon, Feb 07, 2022 at 11:02:30AM -0500, Detlev Casanova wrote:
> The pstore command tries to create a reserved-memory node but fails if
> it is already present with:
> 
>     Add 'reserved-memory' node failed: FDT_ERR_EXISTS
> 
> This patch creates the node only if it does not exist and adapts the reg
> values sizes depending on already present #address-cells and #size-cells
> values.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
> 
> Changes in v2:
>  - Move declarations at beginning of function
>  - Use if instead of switch
>  - Reword Subject
> 
>  cmd/pstore.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/cmd/pstore.c b/cmd/pstore.c
> index 9fac8c7218..cd6f6feb2f 100644
> --- a/cmd/pstore.c
> +++ b/cmd/pstore.c
> @@ -11,6 +11,7 @@
>  #include <mapmem.h>
>  #include <memalign.h>
>  #include <part.h>
> +#include <fdt_support.h>
>  
>  struct persistent_ram_buffer {
>  	u32    sig;
> @@ -485,6 +486,8 @@ void fdt_fixup_pstore(void *blob)
>  {
>  	char node[32];
>  	int  nodeoffset;	/* node offset from libfdt */
> +	u32 addr_cells;
> +	u32 size_cells;
>  
>  	nodeoffset = fdt_path_offset(blob, "/");
>  	if (nodeoffset < 0) {
> @@ -493,14 +496,18 @@ void fdt_fixup_pstore(void *blob)
>  		return;
>  	}
>  
> -	nodeoffset = fdt_add_subnode(blob, nodeoffset, "reserved-memory");
> +	nodeoffset = fdt_find_or_add_subnode(blob, nodeoffset, "reserved-memory");
>  	if (nodeoffset < 0) {
>  		log_err("Add 'reserved-memory' node failed: %s\n",
>  				fdt_strerror(nodeoffset));
>  		return;
>  	}
> -	fdt_setprop_u32(blob, nodeoffset, "#address-cells", 2);
> -	fdt_setprop_u32(blob, nodeoffset, "#size-cells", 2);
> +
> +	addr_cells = fdt_getprop_u32_default_node(blob, nodeoffset, 0, "#address-cells", 2);
> +	size_cells = fdt_getprop_u32_default_node(blob, nodeoffset, 0, "#size-cells", 2);
> +	fdt_setprop_u32(blob, nodeoffset, "#address-cells", addr_cells);
> +	fdt_setprop_u32(blob, nodeoffset, "#size-cells", size_cells);
> +
>  	fdt_setprop_empty(blob, nodeoffset, "ranges");
>  
>  	sprintf(node, "ramoops@%llx", (unsigned long long)pstore_addr);
> @@ -509,14 +516,36 @@ void fdt_fixup_pstore(void *blob)
>  		log_err("Add '%s' node failed: %s\n", node, fdt_strerror(nodeoffset));
>  		return;
>  	}
> +
>  	fdt_setprop_string(blob, nodeoffset, "compatible", "ramoops");
> -	fdt_setprop_u64(blob, nodeoffset, "reg", pstore_addr);
> -	fdt_appendprop_u64(blob, nodeoffset, "reg", pstore_length);
> +
> +	if (addr_cells == 1) {
> +		fdt_setprop_u32(blob, nodeoffset, "reg", pstore_addr);
> +	} else if (addr_cells == 2) {
> +		fdt_setprop_u64(blob, nodeoffset, "reg", pstore_addr);
> +	} else {
> +		log_err("Unsupported #address-cells: %u\n", addr_cells);
> +		goto clean_ramoops;
> +	}
> +
> +	if (size_cells == 1) {
> +		// Let's consider that the pstore_length fits in a 32 bits value
> +		fdt_appendprop_u32(blob, nodeoffset, "reg", pstore_length);
> +	} else if (size_cells == 2) {
> +		fdt_appendprop_u64(blob, nodeoffset, "reg", pstore_length);
> +	} else {
> +		log_err("Unsupported #size-cells: %u\n", addr_cells);
> +		goto clean_ramoops;
> +	}
> +
>  	fdt_setprop_u32(blob, nodeoffset, "record-size", pstore_record_size);
>  	fdt_setprop_u32(blob, nodeoffset, "console-size", pstore_console_size);
>  	fdt_setprop_u32(blob, nodeoffset, "ftrace-size", pstore_ftrace_size);
>  	fdt_setprop_u32(blob, nodeoffset, "pmsg-size", pstore_pmsg_size);
>  	fdt_setprop_u32(blob, nodeoffset, "ecc-size", pstore_ecc_size);
> +
> +clean_ramoops:
> +	fdt_del_node_and_alias(blob, node);
>  }
>  
>  U_BOOT_CMD(pstore, 10, 0, do_pstore,
> -- 
> 2.35.1
>