[{"id":2660519,"web_url":"http://patchwork.ozlabs.org/comment/2660519/","msgid":"","date":"2021-04-02T17:28:41","subject":"Re: [PATCH 1/8] CMDLINE: add generic builtin command line","submitter":{"id":79086,"url":"http://patchwork.ozlabs.org/api/people/79086/","name":"Christophe Leroy","email":"christophe.leroy@csgroup.eu"},"content":"Le 30/03/2021 à 19:56, Daniel Walker a écrit :\n> This code allows architectures to use a generic builtin command line.\n> The state of the builtin command line options across architecture is\n> diverse. MIPS and X86 once has similar systems, then mips added some\n> options to allow extending the command line. Powerpc did something\n> simiar in adding the ability to extend. Even with mips and powerpc\n> enhancement the needs of Cisco are not met on these platforms.\n\nCan you explain in the commit what is the need ? Nobody mind \"who\" needs it I think, but \"what\" is \nneeded would be valuable to know.\n\n> \n> The code in this commit unifies the code into a generic\n> header file under the CONFIG_GENERIC_CMDLINE option. When this\n> option is enabled the architecture can call the cmdline_add_builtin()\n> to add the builtin command line.\n> \n> This unified implementation offers the same functionality needed by\n> Cisco on all platform which use it.\n\nCisco cisco cisco ... Can we avoid mentionning companies like this ? I can't see patches mentioning \ngoogle or IBM or other companies to that extend.\n\n> \n> Cc: xe-linux-external@cisco.com\n> Signed-off-by: Ruslan Bilovol \n> Signed-off-by: Daniel Walker \n> ---\n> include/linux/cmdline.h | 98 +++++++++++++++++++++++++++++++++++++++++\n> init/Kconfig | 72 ++++++++++++++++++++++++++++++\n> 2 files changed, 170 insertions(+)\n> create mode 100644 include/linux/cmdline.h\n> \n> diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h\n> new file mode 100644\n> index 000000000000..439c4585feba\n> --- /dev/null\n> +++ b/include/linux/cmdline.h\n> @@ -0,0 +1,98 @@\n> +/* SPDX-License-Identifier: GPL-2.0 */\n> +#ifndef _LINUX_CMDLINE_H\n> +#define _LINUX_CMDLINE_H\n> +\n> +/*\n> + *\n> + * Copyright (C) 2006,2021. Cisco Systems, Inc.\n> + *\n> + * Generic Append/Prepend cmdline support.\n> + */\n> +\n> +#if defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_CMDLINE_BOOL)\n> +\n> +#ifndef CONFIG_CMDLINE_OVERRIDE\n> +#define GENERIC_CMDLINE_NEED_STRLCAT\n\nDoes it matter ?\n\nOnly powerpc needs that really. And prom_strlcat() is there anyway, so why not just use it when \nneeded and rely on GCC to optimise it out when possible ?\n\n\n> +#define CMDLINE_PREPEND CONFIG_CMDLINE_PREPEND\n> +#define CMDLINE_APPEND CONFIG_CMDLINE_APPEND\n\nWhat are those defines used for ?\n\n> +\n> +/*\n> + * This function will append or prepend a builtin command line to the command\n> + * line provided by the bootloader. Kconfig options can be used to alter\n> + * the behavior of this builtin command line.\n> + * @dest: The destination of the final appended/prepended string\n> + * @src: The starting string or NULL if there isn't one.\n> + * @tmp: temporary space used for prepending\n> + * @length: the maximum length of the strings above.\n> + * @cmdline_strlcpy: point to a compatible strlcpy\n> + * @cmdline_strlcat: point to a compatible strlcat\n> + */\n> +static inline void\n> +__cmdline_add_builtin(char *dest, const char *src, char *tmp, unsigned long length,\n> +\t\tsize_t (*cmdline_strlcpy)(char *dest, const char *src, size_t size),\n> +\t\tsize_t (*cmdline_strlcat)(char *dest, const char *src, size_t count))\n\nI still can see the advantage of passing strlcpy and strlcat as functions to the function.\n\nCan we instead use macros defined by default that can be overriden by powerpc ?\n\nSomething like\n\n#ifndef cmdline_strlcat\n#define cmdline_strlcat strlcat\n#define cmdline_strlcpy strlcpy\n#endif\n\n> +{\n> +\tif (src != dest && src != NULL) {\n> +\t\tcmdline_strlcpy(dest, \" \", length);\n> +\t\tcmdline_strlcat(dest, src, length);\n> +\t}\n> +\n> +\tif (sizeof(CONFIG_CMDLINE_APPEND) > 1)\n> +\t\tcmdline_strlcat(dest, \" \" CONFIG_CMDLINE_APPEND, length);\n> +\n> +\tif (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {\n> +\t\tcmdline_strlcpy(tmp, CONFIG_CMDLINE_PREPEND \" \", length);\n> +\t\tcmdline_strlcat(tmp, dest, length);\n> +\t\tcmdline_strlcpy(dest, tmp, length);\n> +\t}\n> +}\n> +\n> +#define cmdline_add_builtin_custom(dest, src, length, label, cmdline_strlcpy, cmdline_strlcat)\t\t\t\\\n> +{\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n> +\tif (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {\t\t\t\t\t\t\t\t\\\n> +\t\tstatic label char cmdline_tmp_space[length];\t\t\t\t\t\t\t\\\n> +\t\t__cmdline_add_builtin(dest, src, cmdline_tmp_space, length, cmdline_strlcpy, cmdline_strlcat);\t\\\n> +\t} else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) {\t\t\t\t\t\t\t\t\\\n> +\t\t__cmdline_add_builtin(dest, src, NULL, length, cmdline_strlcpy, cmdline_strlcat);\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n> +}\n\nI still don't like passing section names to a macro that way, just for powerpc.\nThat tmp space is only needed when source and destination are identical, and it is easy to ensure \npowerpc doesn't need that. For others, just use __initdata section.\n\nAlso the variable length is not really necessary, only COMMAND_LINE_SIZE is used everywhere.\n\n> +#define cmdline_add_builtin(dest, src, length)\t\\\n> +\tcmdline_add_builtin_custom(dest, src, length, __initdata, strlcpy, strlcat)\n> +\n> +#else /* CONFIG_CMDLINE_OVERRIDE */\n> +\n> +#define CMDLINE_PREPEND CONFIG_CMDLINE_PREPEND\n> +#define CMDLINE_APPEND CONFIG_CMDLINE_APPEND\n> +\n> +static inline void\n> +__cmdline_add_builtin_custom(char *dest, const char *src, unsigned long length,\n> +\t\tsize_t (*cmdline_strlcpy)(char *dest, const char *src, size_t size))\n> +{\n> +\tcmdline_strlcpy(dest, CONFIG_CMDLINE_PREPEND \" \" CONFIG_CMDLINE_APPEND, length);\n> +}\n> +#define cmdline_add_builtin_custom(dest, src, length, label, cmdline_strlcpy, cmdline_strlcat)\t\\\n> +\t__cmdline_add_builtin_custom(dest, src, length, cmdline_strlcpy)\n> +#define cmdline_add_builtin(dest, src, length)\t\\\n> +\t__cmdline_add_builtin_custom(dest, src, length, strlcpy)\n> +#endif /* !CONFIG_CMDLINE_OVERRIDE */\n> +\n> +#else /* !CONFIG_GENERIC_CMDLINE || !CONFIG_CMDLINE_BOOL */\n> +\n> +#define CMDLINE_PREPEND \"\"\n> +#define CMDLINE_APPEND \"\"\n> +\n> +static inline void\n> +__cmdline_add_builtin_custom(char *dest, const char *src, unsigned long length,\n> +\t\tsize_t (*cmdline_strlcpy)(char *dest, const char *src, size_t size))\n> +{\n> +\tif (src != NULL)\n> +\t\tcmdline_strlcpy(dest, src, length);\n> +}\n> +#define cmdline_add_builtin_custom(dest, src, length, label, cmdline_strlcpy, cmdline_strlcat)\t\\\n> +\t__cmdline_add_builtin_custom(dest, src, length, cmdline_strlcpy)\n> +#define cmdline_add_builtin(dest, src, length)\t\\\n> +\t__cmdline_add_builtin_custom(dest, src, length, strlcpy)\t\\\n> +\n> +#endif /* CONFIG_GENERIC_CMDLINE */\n\nThere are too many alternatives, they could be easily refactored to a single instance reducing the \nnumber of #ifdefs.\n\n> +\n> +#endif /* _LINUX_CMDLINE_H */\n> diff --git a/init/Kconfig b/init/Kconfig\n> index 5f5c776ef192..84f06f62550a 100644\n> --- a/init/Kconfig\n> +++ b/init/Kconfig\n> @@ -2034,6 +2034,78 @@ config PROFILING\n> config TRACEPOINTS\n> \tbool\n> \n> +config GENERIC_CMDLINE\n> +\tbool\n> +\n> +config GENERIC_CMDLINE_OF\n> +\tbool\n\nOh ? A new one ? What do we need something special for OF ?\n\n> +\n> +\n> +if GENERIC_CMDLINE\n> +\n> +config CMDLINE_BOOL\n> +\tbool \"Built-in kernel command line\"\n> +\thelp\n> +\t Allow for specifying boot arguments to the kernel at\n> +\t build time. On some systems (e.g. embedded ones), it is\n> +\t necessary or convenient to provide some or all of the\n> +\t kernel boot arguments with the kernel itself (that is,\n> +\t to not rely on the boot loader to provide them.)\n> +\n> +\t To compile command line arguments into the kernel,\n> +\t set this option to 'Y', then fill in the\n> +\t the boot arguments in CONFIG_CMDLINE.\n> +\n> +\t Systems with fully functional boot loaders (i.e. non-embedded)\n> +\t should leave this option set to 'N'.\n> +\n> +config CMDLINE_APPEND\n> +\tstring \"Built-in kernel command string append\"\n> +\tdepends on CMDLINE_BOOL\n> +\tdefault \"\"\n\nBy doing the following instead of using depends\n\n\tstring \"Built-in kernel command string append\" if CMDLINE_BOOL\n\tdefault \"\"\n\nyou could have CMDLINE_APPEND defined all the time and avoid related #ifdefs.\n\n> +\thelp\n> +\t Enter arguments here that should be compiled into the kernel\n> +\t image and used at boot time. If the boot loader provides a\n> +\t command line at boot time, this string is appended to it to\n> +\t form the full kernel command line, when the system boots.\n> +\n> +\t However, you can use the CONFIG_CMDLINE_OVERRIDE option to\n> +\t change this behavior.\n> +\n> +\t In most cases, the command line (whether built-in or provided\n> +\t by the boot loader) should specify the device for the root\n> +\t file system.\n> +\n> +config CMDLINE_PREPEND\n> +\tstring \"Built-in kernel command string prepend\"\n> +\tdepends on CMDLINE_BOOL\n> +\tdefault \"\"\n\nSame\n\n\n> +\thelp\n> +\t Enter arguments here that should be compiled into the kernel\n> +\t image and used at boot time. If the boot loader provides a\n> +\t command line at boot time, this string is prepended to it to\n> +\t form the full kernel command line, when the system boots.\n> +\n> +\t However, you can use the CONFIG_CMDLINE_OVERRIDE option to\n> +\t change this behavior.\n> +\n> +\t In most cases, the command line (whether built-in or provided\n> +\t by the boot loader) should specify the device for the root\n> +\t file system.\n> +\n> +config CMDLINE_OVERRIDE\n\nMost platforms use CMDLINE_FORCE. Why change the majority to minority ?\n\n> +\tbool \"Built-in command line overrides boot loader arguments\"\n> +\tdepends on CMDLINE_BOOL\n> +\thelp\n> +\t Set this option to 'Y' to have the kernel ignore the boot loader\n> +\t command line, and use ONLY the built-in command line. In this case\n> +\t append and prepend strings are concatenated to form the full\n> +\t command line.\n> +\n> +\t This is used to work around broken boot loaders. This should\n> +\t be set to 'N' under normal conditions.\n> +endif\n> +\n\nLooks like we are still missing the one used at least by powerpc today: CMDLINE_DEFAULT_BOOTLOADER, \nwhich uses bootloader arguments if provided, builtin arguments otherwise.\n\n> endmenu\t\t# General setup\n> \n> source \"arch/Kconfig\"\n>","headers":{"Return-Path":"\n ","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Authentication-Results":["ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=lists.ozlabs.org\n (client-ip=2404:9400:2:0:216:3eff:fee1:b9f1; helo=lists.ozlabs.org;\n envelope-from=linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org;\n receiver=)","lists.ozlabs.org; spf=pass (sender SPF authorized)\n smtp.mailfrom=csgroup.eu (client-ip=93.17.236.30; helo=pegase1.c-s.fr;\n envelope-from=christophe.leroy@csgroup.eu; receiver=)"],"Received":["from lists.ozlabs.org (lists.ozlabs.org\n [IPv6:2404:9400:2:0:216:3eff:fee1:b9f1])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange X25519 server-signature RSA-PSS (4096 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 4FBn933Sjjz9sVt\n\tfor ; Sat, 3 Apr 2021 04:29:11 +1100 (AEDT)","from boromir.ozlabs.org (localhost [IPv6:::1])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 4FBn932TGHz3c0H\n\tfor ; Sat, 3 Apr 2021 04:29:11 +1100 (AEDT)","from pegase1.c-s.fr (pegase1.c-s.fr [93.17.236.30])\n (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n (No client certificate requested)\n by lists.ozlabs.org (Postfix) with ESMTPS id 4FBn8k32v6z3bnx\n for ; Sat, 3 Apr 2021 04:28:50 +1100 (AEDT)","from localhost (mailhub1-int [192.168.12.234])\n by localhost (Postfix) with ESMTP id 4FBn8Y50v7z9v3qQ;\n Fri, 2 Apr 2021 19:28:45 +0200 (CEST)","from pegase1.c-s.fr ([192.168.12.234])\n by localhost (pegase1.c-s.fr [192.168.12.234]) (amavisd-new, port 10024)\n with ESMTP id L7ilypF0b3f4; Fri, 2 Apr 2021 19:28:45 +0200 (CEST)","from messagerie.si.c-s.fr (messagerie.si.c-s.fr [192.168.25.192])\n by pegase1.c-s.fr (Postfix) with ESMTP id 4FBn8Y3vtHz9v3gF;\n Fri, 2 Apr 2021 19:28:45 +0200 (CEST)","from localhost (localhost [127.0.0.1])\n by messagerie.si.c-s.fr (Postfix) with ESMTP id AE9D58BB77;\n Fri, 2 Apr 2021 19:28:47 +0200 (CEST)","from messagerie.si.c-s.fr ([127.0.0.1])\n by localhost (messagerie.si.c-s.fr [127.0.0.1]) (amavisd-new, port 10023)\n with ESMTP id IcQEBCei7ZAe; Fri, 2 Apr 2021 19:28:47 +0200 (CEST)","from [192.168.4.90] (unknown [192.168.4.90])\n by messagerie.si.c-s.fr (Postfix) with ESMTP id 498158BB6F;\n Fri, 2 Apr 2021 19:28:46 +0200 (CEST)"],"X-Virus-Scanned":["Debian amavisd-new at c-s.fr","amavisd-new at c-s.fr"],"Subject":"Re: [PATCH 1/8] CMDLINE: add generic builtin command line","To":"Daniel Walker , Will Deacon ,\n ob Herring ,\n Daniel Gimpelevich ,\n Andrew Morton , x86@kernel.org,\n linux-mips@vger.kernel.org, linuxppc-dev@lists.ozlabs.org","References":"\n <41021d66db2ab427c14255d2a24bb4517c8b58fd.1617126961.git.danielwa@cisco.com>","From":"Christophe Leroy ","Message-ID":"","Date":"Fri, 2 Apr 2021 19:28:41 +0200","User-Agent":"Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:78.0) Gecko/20100101\n Thunderbird/78.9.0","MIME-Version":"1.0","In-Reply-To":"\n <41021d66db2ab427c14255d2a24bb4517c8b58fd.1617126961.git.danielwa@cisco.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"fr","Content-Transfer-Encoding":"8bit","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List ","List-Unsubscribe":",\n ","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n ","Cc":"Ruslan Bilovol , linux-kernel@vger.kernel.org,\n xe-linux-external@cisco.com","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n "}}]