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

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