[PATCH v2] pstore: Support already existing reserved-memory node
Detlev Casanova
detlev.casanova at collabora.com
Tue Apr 19 15:58:00 CEST 2022
Hey Daniel,
I see that the node is called ramoops at 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 at 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 at 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,
More information about the U-Boot
mailing list