From: Christophe Leroy <christophe.leroy@c-s.fr>
To: Daniel Walker <danielwa@cisco.com>,
Andrew Morton <akpm@linux-foundation.org>
Cc: Maksym Kokhan <maksym.kokhan@globallogic.com>,
linux-kernel@vger.kernel.org, Paul Mackerras <paulus@samba.org>,
xe-linux-external@cisco.com, Daniel Walker <dwalker@fifo99.com>,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 1/3] add generic builtin command line
Date: Mon, 4 Mar 2019 15:05:47 +0100 [thread overview]
Message-ID: <701a46a6-57e6-f8c7-1fee-18c50e72c38c@c-s.fr> (raw)
In-Reply-To: <1551469472-53043-2-git-send-email-danielwa@cisco.com>
Le 01/03/2019 à 20:44, Daniel Walker a écrit :
> This code allows architectures to use a generic builtin command line.
> The state of the builtin command line options across architecture is
> diverse. On x86 and mips they have pretty much the same code and the
> code prepends the builtin command line onto the boot loader provided
> one. On powerpc there is only a builtin override and nothing else.
>
> The code in this commit unifies the mips and x86 code into a generic
> header file under the CONFIG_GENERIC_CMDLINE option. When this
> option is enabled the architecture can call the cmdline_add_builtin()
> to add the builtin command line.
>
> [maksym.kokhan@globallogic.com: fix cmdline_add_builtin() macro]
> Cc: Daniel Walker <dwalker@fifo99.com>
> Cc: Daniel Walker <danielwa@cisco.com>
> Cc: xe-linux-external@cisco.com
> Signed-off-by: Daniel Walker <danielwa@cisco.com>
> Signed-off-by: Maksym Kokhan <maksym.kokhan@globallogic.com>
> ---
> include/linux/cmdline.h | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
> init/Kconfig | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 141 insertions(+)
> create mode 100644 include/linux/cmdline.h
>
> diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
> new file mode 100644
> index 000000000000..126fa52f55d2
> --- /dev/null
> +++ b/include/linux/cmdline.h
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_CMDLINE_H
> +#define _LINUX_CMDLINE_H
> +
> +/*
> + *
> + * Copyright (C) 2015. Cisco Systems, Inc.
> + *
> + * Generic Append/Prepend cmdline support.
> + */
> +
> +#if defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_CMDLINE_BOOL)
> +
> +#ifndef CONFIG_CMDLINE_OVERRIDE
> +/*
> + * This function will append or prepend a builtin command line to the command
> + * line provided by the bootloader. Kconfig options can be used to alter
> + * the behavior of this builtin command line.
> + * @dest: The destination of the final appended/prepended string
> + * @src: The starting string or NULL if there isn't one.
> + * @tmp: temporary space used for prepending
> + * @length: the maximum length of the strings above.
> + */
> +static inline void
> +_cmdline_add_builtin(char *dest, char *src, char *tmp, unsigned long length)
> +{
> + if (src != dest && src != NULL) {
> + strlcpy(dest, " ", length);
Why prepend a space here ?
> + strlcat(dest, src, length);
> + }
> +
> + strlcat(dest, " ", length);
Why a space here ? When there is nothing to append, there is no need of
a space here. The space could simply be concatenated below in from of
CONFIG_CMDLINE_APPEND
> +
> + if (sizeof(CONFIG_CMDLINE_APPEND) > 1)
> + strlcat(dest, CONFIG_CMDLINE_APPEND, length);
> +
> + if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {
> + strlcpy(tmp, CONFIG_CMDLINE_PREPEND, length);
> + strlcat(tmp, " ", length);
Why not concatenate the space to above, ie:
strlcpy(tmp, CONFIG_CMDLINE_PREPEND " ", length);
> + strlcat(tmp, dest, length);
> + strlcpy(dest, tmp, length);
> + }
> +}
> +
> +#define cmdline_add_builtin_section(dest, src, length, section) \
> +{ \
> + if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) { \
> + static char cmdline_tmp_space[length] section; \
> + _cmdline_add_builtin(dest, src, cmdline_tmp_space, length); \
> + } else { \
> + _cmdline_add_builtin(dest, src, NULL, length); \
> + } \
> +}
> +#else
> +#define cmdline_add_builtin_section(dest, src, length, section) \
> +{ \
> + strlcpy(dest, CONFIG_CMDLINE_PREPEND " " CONFIG_CMDLINE_APPEND, \
> + length); \
> +}
> +#endif /* !CONFIG_CMDLINE_OVERRIDE */
> +
> +#else
> +#define cmdline_add_builtin_section(dest, src, length, section) { \
> + if (src != NULL) \
> + strlcpy(dest, src, length); \
> +}
> +#endif /* CONFIG_GENERIC_CMDLINE */
> +
> +#define cmdline_add_builtin(dest, src, length) \
> + cmdline_add_builtin_section(dest, src, length, __initdata)
All those ifdefs are uggly, I don't think it is worth making something
so complex just for the fun of getting something generic. See my
comments to following patch.
> +
> +#endif /* _LINUX_CMDLINE_H */
> diff --git a/init/Kconfig b/init/Kconfig
> index a4112e95724a..b59e856511e1 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1775,6 +1775,75 @@ config PROFILING
> config TRACEPOINTS
> bool
>
> +config GENERIC_CMDLINE
> + bool
> +
> +if GENERIC_CMDLINE
> +
> +config CMDLINE_BOOL
> + bool "Built-in kernel command line"
> + help
> + Allow for specifying boot arguments to the kernel at
> + build time. On some systems (e.g. embedded ones), it is
> + necessary or convenient to provide some or all of the
> + kernel boot arguments with the kernel itself (that is,
> + to not rely on the boot loader to provide them.)
> +
> + To compile command line arguments into the kernel,
> + set this option to 'Y', then fill in the
> + the boot arguments in CONFIG_CMDLINE.
> +
> + Systems with fully functional boot loaders (i.e. non-embedded)
> + should leave this option set to 'N'.
> +
> +config CMDLINE_APPEND
> + string "Built-in kernel command string append"
> + depends on CMDLINE_BOOL
Would be better to remove this 'depends' and put the condition on the
previous line. That way we would always have CONFIG_CMDLINE_APPEND and
we could do something more simple in the code.
> + default ""
> + help
> + Enter arguments here that should be compiled into the kernel
> + image and used at boot time. If the boot loader provides a
> + command line at boot time, this string is appended to it to
> + form the full kernel command line, when the system boots.
> +
> + However, you can use the CONFIG_CMDLINE_OVERRIDE option to
> + change this behavior.
> +
> + In most cases, the command line (whether built-in or provided
> + by the boot loader) should specify the device for the root
> + file system.
> +
> +config CMDLINE_PREPEND
> + string "Built-in kernel command string prepend"
> + depends on CMDLINE_BOOL
Would be better to remove this 'depends' and put the condition on the
previous line. That way we would always have CONFIG_CMDLINE_APPEND and
we could do something more simple in the code.
> + default ""
> + help
> + Enter arguments here that should be compiled into the kernel
> + image and used at boot time. If the boot loader provides a
> + command line at boot time, this string is prepended to it to
> + form the full kernel command line, when the system boots.
> +
> + However, you can use the CONFIG_CMDLINE_OVERRIDE option to
> + change this behavior.
> +
> + In most cases, the command line (whether built-in or provided
> + by the boot loader) should specify the device for the root
> + file system.
> +
> +config CMDLINE_OVERRIDE
> + bool "Built-in command line overrides boot loader arguments"
> + depends on CMDLINE_BOOL
> + help
> + Set this option to 'Y' to have the kernel ignore the boot loader
> + command line, and use ONLY the built-in command line. In this case
> + append and prepend strings are concatenated to form the full
> + command line.
> +
> + This is used to work around broken boot loaders. This should
> + be set to 'N' under normal conditions.
> +
> +endif
> +
> endmenu # General setup
>
> source "arch/Kconfig"
>
Christophe
next prev parent reply other threads:[~2019-03-04 14:06 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-01 19:44 [PATCH 1/3] add generic builtin command line Daniel Walker
2019-03-04 14:05 ` Christophe Leroy [this message]
-- strict thread matches above, loose matches on Subject: below --
2018-11-09 17:34 Daniel Walker
2018-11-29 5:12 ` Andrew Morton
2018-11-29 15:51 ` Daniel Walker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=701a46a6-57e6-f8c7-1fee-18c50e72c38c@c-s.fr \
--to=christophe.leroy@c-s.fr \
--cc=akpm@linux-foundation.org \
--cc=danielwa@cisco.com \
--cc=dwalker@fifo99.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=maksym.kokhan@globallogic.com \
--cc=paulus@samba.org \
--cc=xe-linux-external@cisco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).