From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E2229C433EF for ; Tue, 19 Apr 2022 14:03:36 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 384A683BBA; Tue, 19 Apr 2022 16:03:34 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=collabora.com header.i=@collabora.com header.b="bITXKFaB"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 05FE083BC2; Tue, 19 Apr 2022 15:58:11 +0200 (CEST) Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 6056383BBA for ; Tue, 19 Apr 2022 15:58:07 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=detlev.casanova@collabora.com Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: detlev) with ESMTPSA id 787A21F424C1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1650376686; bh=0Hdh6+WkwU+nWh97dvkMO+rrw0eVIdKGUNoI7ro69Bw=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=bITXKFaBqxiDtYQ+ICjKfUCXfFuxiCYyp1N5OG5q3Y9aiO166ZzG6CXASHSqe48h4 0/dSyaAwujpJy0cDhDhN5U+POtgpYVy71dPmRfBZ0nUiL/MHy6ApqVtvcmeStlf/gY SHOhunGs8FXwJ3cEhirTya7pd0l1DGWGjtEnGb45m0stI808PHXLxx7ivbsc3gVtBF 1D2sBQcsJOghzroRouVYsuAI7W0M7Q5vtIWczjrCDqxLn3BXu/uSASK7gKwECwmktU CaloMUvRoqVZusNuaQCgDZPNLoaxNJwa28M7IrhV0JBsMZpguzTik3Unxv6bnpxms/ YFN+bX0M5iedA== From: Detlev Casanova To: Daniel Golle Cc: u-boot@lists.denx.de, =?ISO-8859-1?Q?Fr=E9d=E9ric?= Danis , Simon Glass Subject: Re: [PATCH v2] pstore: Support already existing reserved-memory node Date: Tue, 19 Apr 2022 09:58:00 -0400 Message-ID: <8051828.T7Z3S40VBb@falcon9> In-Reply-To: References: <20220207160230.38662-1-detlev.casanova@collabora.com> <2234374.ElGaqSPkdT@falcon9> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Mailman-Approved-At: Tue, 19 Apr 2022 16:03:33 +0200 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean 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 > > > > --- > > > > > > > > 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 > > > > #include > > > > #include > > > > > > > > +#include > > > > > > > > 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,