| Message ID | 20220207160230.38662-1-detlev.casanova@collabora.com |
|---|---|
| State | Accepted |
| Commit | ac52cba63f26eab9c74f3d1c2da00835724d3484 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2] pstore: Support already existing reserved-memory node | expand |
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> Applied to u-boot/master, thanks!
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
>
Hi Daniel, The patch only checks the existence of the reserved-memory node. I guess that before, adding the reserved-memory node failed and so nothing else was happening. Now, the node is found and then, it adds the ramoops subnode to it, making it double because it is already there in your case. But that should not happen, because the fdt_add_subnode() function should fail if the node is already present (like it was failing for reserved-memory). I'm going to look into it. Can you send me the device tree taht you are using (or at least the reserved-memory part) and u-boot config ? Detlev. On Monday, April 11, 2022 7:00:20 P.M. EDT Daniel Golle wrote: > 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,
Hi Detlev, On Mon, Apr 18, 2022 at 02:46:21PM -0400, Detlev Casanova wrote: > Hi Daniel, > > The patch only checks the existence of the reserved-memory node. > > I guess that before, adding the reserved-memory node failed and so nothing > else was happening. Now, the node is found and then, it adds the ramoops > subnode to it, making it double because it is already there in your case. > > But that should not happen, because the fdt_add_subnode() function should fail > if the node is already present (like it was failing for reserved-memory). > > I'm going to look into it. Can you send me the device tree taht you are using > (or at least the reserved-memory part) and u-boot config ? Thank you for your reply and for taking a look at what's going on. reserved-memory bits in dts: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=package/boot/uboot-mediatek/patches/050-mt7622-enable-pstore.patch u-boot config: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=package/boot/uboot-mediatek/patches/404-add-bananapi_bpi-r64_defconfigs.patch Cheers Daniel > > Detlev. > > On Monday, April 11, 2022 7:00:20 P.M. EDT Daniel Golle wrote: > > 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, > > > >
Hey Daniel, I see that the node is called ramoops@0x42ff0000. We usually don't set the 0x in device tree nodes, and that's why the fdt_add_subnode function doesn't see it and add ramoops@42ff0000 (it just compares the text). For Linux, both nodes are the same and it complains. I think the best course of action here is to remove that '0x' in your dts, so that u-boot sees it as the pstore node. Regards, Detlev. On Monday, April 18, 2022 7:50:36 P.M. EDT Daniel Golle wrote: > Hi Detlev, > > On Mon, Apr 18, 2022 at 02:46:21PM -0400, Detlev Casanova wrote: > > Hi Daniel, > > > > The patch only checks the existence of the reserved-memory node. > > > > I guess that before, adding the reserved-memory node failed and so nothing > > else was happening. Now, the node is found and then, it adds the ramoops > > subnode to it, making it double because it is already there in your case. > > > > But that should not happen, because the fdt_add_subnode() function should > > fail if the node is already present (like it was failing for > > reserved-memory). > > > > I'm going to look into it. Can you send me the device tree taht you are > > using (or at least the reserved-memory part) and u-boot config ? > > Thank you for your reply and for taking a look at what's going on. > > reserved-memory bits in dts: > https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=package/boot/uboot-m > ediatek/patches/050-mt7622-enable-pstore.patch > > u-boot config: > https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=package/boot/uboot-m > ediatek/patches/404-add-bananapi_bpi-r64_defconfigs.patch > > > Cheers > > > Daniel > > > Detlev. > > > > On Monday, April 11, 2022 7:00:20 P.M. EDT Daniel Golle wrote: > > > 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,
Hi Detlev, On Tue, Apr 19, 2022 at 09:58:00AM -0400, Detlev Casanova wrote: > Hey Daniel, > > I see that the node is called ramoops@0x42ff0000. We usually don't set the 0x > in device tree nodes, and that's why the fdt_add_subnode function doesn't see > it and add ramoops@42ff0000 (it just compares the text). For Linux, both nodes > are the same and it complains. > > I think the best course of action here is to remove that '0x' in your dts, so > that u-boot sees it as the pstore node. I tried the change you suggest and remove the '0x' prefix and indeed that works, the lengthy kernel warning about duplicate nodes and sysfs entries are gone and pstore works as expected. I've hence committed that change to OpenWrt: https://git.openwrt.org/?p=openwrt/openwrt.git;a=commit;h=fc245338d6e02e61fa7ecbd1a828aed97cdbef88 However, now there is another error message in U-Boot output: Add 'ramoops@42ff0000' node failed: FDT_ERR_EXISTS I figured that this is not a problem as previously (ie. before your patch) even the exitence of the 'reserved-memory' node would result in the 'ramoops' node not being added while now it really just errors out in case of exactly that 'ramoops' node already existing, which is fine. Thank you again for resolving this issue! Cheers Daniel > > Regards, > > Detlev. > > On Monday, April 18, 2022 7:50:36 P.M. EDT Daniel Golle wrote: > > Hi Detlev, > > > > On Mon, Apr 18, 2022 at 02:46:21PM -0400, Detlev Casanova wrote: > > > Hi Daniel, > > > > > > The patch only checks the existence of the reserved-memory node. > > > > > > I guess that before, adding the reserved-memory node failed and so nothing > > > else was happening. Now, the node is found and then, it adds the ramoops > > > subnode to it, making it double because it is already there in your case. > > > > > > But that should not happen, because the fdt_add_subnode() function should > > > fail if the node is already present (like it was failing for > > > reserved-memory). > > > > > > I'm going to look into it. Can you send me the device tree taht you are > > > using (or at least the reserved-memory part) and u-boot config ? > > > > Thank you for your reply and for taking a look at what's going on. > > > > reserved-memory bits in dts: > > https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=package/boot/uboot-m > > ediatek/patches/050-mt7622-enable-pstore.patch > > > > u-boot config: > > https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=package/boot/uboot-m > > ediatek/patches/404-add-bananapi_bpi-r64_defconfigs.patch > > > > > > Cheers > > > > > > Daniel > > > > > Detlev. > > > > > > On Monday, April 11, 2022 7:00:20 P.M. EDT Daniel Golle wrote: > > > > 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, > > > >
Hi Daniel, On Tuesday, April 19, 2022 12:14:07 P.M. EDT Daniel Golle wrote: > Hi Detlev, > > On Tue, Apr 19, 2022 at 09:58:00AM -0400, Detlev Casanova wrote: > > Hey Daniel, > > > > I see that the node is called ramoops@0x42ff0000. We usually don't set the > > 0x in device tree nodes, and that's why the fdt_add_subnode function > > doesn't see it and add ramoops@42ff0000 (it just compares the text). For > > Linux, both nodes are the same and it complains. > > > > I think the best course of action here is to remove that '0x' in your dts, > > so that u-boot sees it as the pstore node. > > I tried the change you suggest and remove the '0x' prefix and indeed that > works, the lengthy kernel warning about duplicate nodes and sysfs > entries are gone and pstore works as expected. > I've hence committed that change to OpenWrt: > > https://git.openwrt.org/?p=openwrt/openwrt.git;a=commit;h=fc245338d6e02e61fa > 7ecbd1a828aed97cdbef88 > > However, now there is another error message in U-Boot output: > Add 'ramoops@42ff0000' node failed: FDT_ERR_EXISTS > > I figured that this is not a problem as previously (ie. before your > patch) even the exitence of the 'reserved-memory' node would result in > the 'ramoops' node not being added while now it really just errors out > in case of exactly that 'ramoops' node already existing, which is fine. That's right, you can see that error message as "You activated pstore in the config to make me add the ramoops node, but it's already there. I cannot do my job" :) > Thank you again for resolving this issue! > > > Cheers > > > Daniel > > > Regards, > > > > Detlev. > > > > On Monday, April 18, 2022 7:50:36 P.M. EDT Daniel Golle wrote: > > > Hi Detlev, > > > > > > On Mon, Apr 18, 2022 at 02:46:21PM -0400, Detlev Casanova wrote: > > > > Hi Daniel, > > > > > > > > The patch only checks the existence of the reserved-memory node. > > > > > > > > I guess that before, adding the reserved-memory node failed and so > > > > nothing > > > > else was happening. Now, the node is found and then, it adds the > > > > ramoops > > > > subnode to it, making it double because it is already there in your > > > > case. > > > > > > > > But that should not happen, because the fdt_add_subnode() function > > > > should > > > > fail if the node is already present (like it was failing for > > > > reserved-memory). > > > > > > > > I'm going to look into it. Can you send me the device tree taht you > > > > are > > > > using (or at least the reserved-memory part) and u-boot config ? > > > > > > Thank you for your reply and for taking a look at what's going on. > > > > > > reserved-memory bits in dts: > > > https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=package/boot/ubo > > > ot-m ediatek/patches/050-mt7622-enable-pstore.patch > > > > > > u-boot config: > > > https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=package/boot/ubo > > > ot-m ediatek/patches/404-add-bananapi_bpi-r64_defconfigs.patch > > > > > > > > > Cheers > > > > > > > > > Daniel > > > > > > > Detlev. > > > > > > > > On Monday, April 11, 2022 7:00:20 P.M. EDT Daniel Golle wrote: > > > > > 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,
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,
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(-)