Re: [PATCH 1/3] add generic builtin command line

[Daniel Walker]

On Wed, Nov 28, 2018 at 09:12:12PM -0800, Andrew Morton wrote:
> On Fri,  9 Nov 2018 09:34:31 -0800 Daniel Walker <danielwa@xxxxxxxxx> wrote:
> 
> > 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.
> 
> I'm not sure what's happened to this and I haven't seen the other two
> patches but...

It's been sitting in -next since the last merge window. 

> 
> > [maksym.kokhan@xxxxxxxxxxxxxxx: fix cmdline_add_builtin() macro]
> > Cc: Daniel Walker <dwalker@xxxxxxxxxx>
> > Cc: Daniel Walker <danielwa@xxxxxxxxx>
> > Cc: xe-linux-external@xxxxxxxxx
> > Signed-off-by: Daniel Walker <danielwa@xxxxxxxxx>
> > Signed-off-by: Maksym Kokhan <maksym.kokhan@xxxxxxxxxxxxxxx>
> 
> Two SOB's is nice, but some reviews and acks would be nicer.
 
Would love to add some, but no one is reviewing it.. 

> > --- /dev/null
> > +++ b/include/linux/cmdline.h
> > @@ -0,0 +1,79 @@
> > +/* 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);
> > +		strlcat(dest, src, length);
> > +	}
> > +
> > +	strlcat(dest, " ", length);
> > +
> > +	if (sizeof(CONFIG_CMDLINE_APPEND) > 1)
> > +		strlcat(dest, CONFIG_CMDLINE_APPEND, length);
> > +
> > +	/*
> > +	 * You need to convert you old style CONFIG_CMDLINE to use
> 
> "your"
> 
> > +	 * the prepend, or append defines. Some architectures use one
> 
> "one and"
> 
> > +	 * some use the other. You need to figure out which ones is
> 
> "one"

Can fix these..


> 
> > +	 * right for your situation. I would recommend prepending
> > +	 * because it's the safest (i.e. CONFIG_CMDLINE_PREPEND).
> > +	 */
> > +	BUILD_BUG_ON(sizeof(CONFIG_CMDLINE) != 1);
> > +
> > +	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {
> > +		strlcpy(tmp, CONFIG_CMDLINE_PREPEND, length);
> > +		strlcat(tmp, " ", length);
> > +		strlcat(tmp, dest, length);
> > +		strlcpy(dest, tmp, length);
> > +	}
> > +}
> 
> And... holy cow.  Does this monster really need to be inlined?
> 
> > +#define cmdline_add_builtin(dest, src, length)				    \
> > +{									    \
> > +	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {			    \
> > +		static char cmdline_tmp_space[length] __initdata;	    \
> > +		_cmdline_add_builtin(dest, src, cmdline_tmp_space, length); \
> > +	} else {							    \
> > +		_cmdline_add_builtin(dest, src, NULL, length);		    \
> > +	}								    \
> > +}
> 
> And this will generate __initdata storage at each invocation site.  Can
> it be redone in real, non-inlined C?


It's intended to be used once, maybe twice, per architecture. Powerpc uses it
twice I think. There's no reason for it to be used more than twice, and it replaces
code which is similar to the code above. I can' imagine a situation where people
go nuts and start calling this everywhere.

So it seemed easier to just inline it.

Daniel