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 X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 166FEC433E0 for ; Thu, 25 Feb 2021 14:09:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B8CCC64F11 for ; Thu, 25 Feb 2021 14:09:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231843AbhBYOJB (ORCPT ); Thu, 25 Feb 2021 09:09:01 -0500 Received: from mail.kernel.org ([198.145.29.99]:47188 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229498AbhBYOI7 (ORCPT ); Thu, 25 Feb 2021 09:08:59 -0500 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 79F6964F10; Thu, 25 Feb 2021 14:08:17 +0000 (UTC) Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94) (envelope-from ) id 1lFHJP-00Fs5E-2d; Thu, 25 Feb 2021 14:08:15 +0000 Date: Thu, 25 Feb 2021 14:08:11 +0000 Message-ID: <87y2fcz1dg.wl-maz@kernel.org> From: Marc Zyngier To: Will Deacon Cc: linux-kernel@vger.kernel.org, Max Uvarov , Rob Herring , Ard Biesheuvel , Doug Anderson , Tyler Hicks , Frank Rowand , Arnd Bergmann , Palmer Dabbelt , Greg Kroah-Hartman , Catalin Marinas , kernel-team@android.com, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org Subject: Re: [PATCH 2/2] of/fdt: Append bootloader arguments when CMDLINE_EXTEND=y In-Reply-To: <20210225125921.13147-3-will@kernel.org> References: <20210225125921.13147-1-will@kernel.org> <20210225125921.13147-3-will@kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: will@kernel.org, linux-kernel@vger.kernel.org, muvarov@gmail.com, robh@kernel.org, ardb@kernel.org, dianders@chromium.org, tyhicks@linux.microsoft.com, frowand.list@gmail.com, arnd@arndb.de, palmer@dabbelt.com, gregkh@linuxfoundation.org, catalin.marinas@arm.com, kernel-team@android.com, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Thu, 25 Feb 2021 12:59:21 +0000, Will Deacon wrote: > > The Kconfig help text for CMDLINE_EXTEND is sadly duplicated across all > architectures that implement it (arm, arm64, powerpc, riscv and sh), but > they all seem to agree that the bootloader arguments will be appended to > the CONFIG_CMDLINE. For example, on arm64: > > | The command-line arguments provided by the boot loader will be > | appended to the default kernel command string. > > This also matches the behaviour of the EFI stub, which parses the > bootloader arguments first if CMDLINE_EXTEND is set, as well as the > out-of-tree CMDLINE_EXTEND implementation in Android. > > However, the behaviour in the upstream fdt code appears to be the other > way around: CONFIG_CMDLINE is appended to the bootloader arguments. > > Fix the code to follow the documentation by moving the cmdline > processing out into a new function, early_init_dt_retrieve_cmdline(), > and copying CONFIG_CMDLINE to the beginning of the cmdline buffer rather > than concatenating it onto the end. > > Cc: Max Uvarov > Cc: Rob Herring > Cc: Ard Biesheuvel > Cc: Marc Zyngier > Cc: Doug Anderson > Cc: Tyler Hicks > Cc: Frank Rowand > Fixes: 34b82026a507 ("fdt: fix extend of cmd line") > Signed-off-by: Will Deacon > --- > drivers/of/fdt.c | 64 +++++++++++++++++++++++++++++------------------- > 1 file changed, 39 insertions(+), 25 deletions(-) > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index dcc1dd96911a..83b9d065e58d 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -1033,11 +1033,48 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname, > return 0; > } > > +static int __init cmdline_from_bootargs(unsigned long node, void *dst, int sz) > +{ > + int l; > + const char *p = of_get_flat_dt_prop(node, "bootargs", &l); > + > + if (!p || l <= 0) > + return -EINVAL; > + > + return strlcpy(dst, p, min(l, sz)); > +} > + > +/* dst is a zero-initialised buffer of COMMAND_LINE_SIZE bytes */ > +static void __init early_init_dt_retrieve_cmdline(unsigned long node, char *dst) > +{ > + if (IS_ENABLED(CONFIG_CMDLINE_EXTEND)) { > + /* Copy CONFIG_CMDLINE to the start of destination buffer */ > + size_t idx = strlcpy(dst, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > + > + /* Check that we have enough space to concatenate */ > + if (idx + 1 >= COMMAND_LINE_SIZE) > + return; > + > + /* Append the bootloader arguments */ > + dst[idx++] = ' '; > + cmdline_from_bootargs(node, &dst[idx], COMMAND_LINE_SIZE - idx); > + } else if (IS_ENABLED(CONFIG_CMDLINE_FORCE)) { > + /* Just use CONFIG_CMDLINE */ > + strlcpy(dst, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > + } else if (IS_ENABLED(CONFIG_CMDLINE_FROM_BOOTLOADER)) { > + /* Use CONFIG_CMDLINE if no arguments from bootloader. */ > + if (cmdline_from_bootargs(node, dst, COMMAND_LINE_SIZE) <= 0) > + strlcpy(dst, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > + } else { Do we have any arch that can end-up not defining any of the 3 above cases? We should be able to just have the above case as the catch-all, and drop the one below. > + /* Just use bootloader arguments */ > + cmdline_from_bootargs(node, dst, COMMAND_LINE_SIZE); > + } > +} > + > int __init early_init_dt_scan_chosen(unsigned long node, const char *uname, > int depth, void *data) > { > int l; > - const char *p; > const void *rng_seed; > > pr_debug("search \"chosen\", depth: %d, uname: %s\n", depth, uname); > @@ -1047,30 +1084,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname, > return 0; > > early_init_dt_check_for_initrd(node); > - > - /* Retrieve command line */ > - p = of_get_flat_dt_prop(node, "bootargs", &l); > - if (p != NULL && l > 0) > - strlcpy(data, p, min(l, COMMAND_LINE_SIZE)); > - > - /* > - * CONFIG_CMDLINE is meant to be a default in case nothing else > - * managed to set the command line, unless CONFIG_CMDLINE_FORCE > - * is set in which case we override whatever was found earlier. > - */ > -#ifdef CONFIG_CMDLINE > -#if defined(CONFIG_CMDLINE_EXTEND) > - strlcat(data, " ", COMMAND_LINE_SIZE); > - strlcat(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > -#elif defined(CONFIG_CMDLINE_FORCE) > - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > -#else > - /* No arguments from boot loader, use kernel's cmdl*/ > - if (!((char *)data)[0]) > - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > -#endif > -#endif /* CONFIG_CMDLINE */ > - > + early_init_dt_retrieve_cmdline(node, data); > pr_debug("Command line is: %s\n", (char *)data); > > rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l); > -- > 2.30.1.766.gb4fecdf3b7-goog > > Other than the above nit: Reviewed-by: Marc Zyngier Thanks, M. -- Without deviation from the norm, progress is not possible.