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 AAA09C433F5 for ; Tue, 19 Apr 2022 11:04:59 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 1BD2B83B20; Tue, 19 Apr 2022 13:04:57 +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="d6Zc/RY0"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id ACE478392C; Mon, 18 Apr 2022 20:46:28 +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 C3D4681870 for ; Mon, 18 Apr 2022 20:46:25 +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 E91D11F43409 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1650307585; bh=HxcYDBzOuenl6jHwFp32Mgr7R47qd53krGnSs+UsWPk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=d6Zc/RY0reWLSENwDYHxhQQcf+9aJs4Xwi/7JBrwGQ3Gzt7hEa975ifjzIrKd1/8F msRBVOj1BOeYH8b+jhxjkKyUevBUS6UG5CySkHxKWtJ/k/Ex0cqdPVuRmSTytNnnvg dk5y0068ocPJjBpWf+QlTcaav+GZWhTtawg90jn3/L7i5Af308YP48LX8UQvCXKS49 VRFsDFkGyAyQvBF1eR6tyXatT6ziWvGRX4XmY0Jv1VrjmSsZGBJbLqNfLJ28DWaUXV o02cJ/C0yDx2PgSX2F0eRY4cH8EF5fwD50KaHTWXKjPmw9US13t+XtdARic/zF3ybS 0uPUv5SbyQQXw== 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: Mon, 18 Apr 2022 14:46:21 -0400 Message-ID: <2234374.ElGaqSPkdT@falcon9> In-Reply-To: References: <20220207160230.38662-1-detlev.casanova@collabora.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Mailman-Approved-At: Tue, 19 Apr 2022 13:04:55 +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 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 > > --- > > > > 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,