All of lore.kernel.org
 help / color / mirror / Atom feed
From: hch@infradead.org (Christoph Hellwig)
To: linux-riscv@lists.infradead.org
Subject: [PATCH] RISC-V: Implement built-in command line feature
Date: Tue, 2 Oct 2018 07:34:55 -0700	[thread overview]
Message-ID: <20181002143455.GA1476@infradead.org> (raw)
In-Reply-To: <20181002140449.24147-1-mick@ics.forth.gr>

> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_FORCE
> +	static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
> +#else
> +	static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
> +#endif
> +#endif

Why do you need two variables here?  x86, where this same logic is used
seems to get away with just a builtin_cmdline command line one.

Also please don't indent code inside cpp conditionals.

> +	/* When we return, cmdline_p should point to a temporary

This is not the normal kernel comment style.  Please keep the
/* on a line of its own.

> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_FORCE
> +	/* Built-in args override boot loader args and
> +	 * boot loader args are being ignored.
> +	 */
> +	strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
> +#else
> +	/* Boot loader args override built-in args so we
> +	 * need to put built-in args before boot loader args.
> +	 */
> +	if (builtin_cmdline[0]) {
> +		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> +		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> +		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> +	}
> +#endif
> +#endif

Even if this seems mostly copied - it probably should be #if/#elif
instead of this convoluted magic:

#if defined(CONFIG_CMDLINE_FORCE)
	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
#elif defined(CONFIG_CMDLINE_BOOL)
	if (builtin_cmdline[0]) {
		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
	}
#endif

WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@infradead.org>
To: Nick Kossifidis <mick@ics.forth.gr>
Cc: linux-riscv@lists.infradead.org, palmer@sifive.com,
	aou@eecs.berkeley.edu
Subject: Re: [PATCH] RISC-V: Implement built-in command line feature
Date: Tue, 2 Oct 2018 07:34:55 -0700	[thread overview]
Message-ID: <20181002143455.GA1476@infradead.org> (raw)
Message-ID: <20181002143455.fojwFXfW7N02H5p1Ic0EYUZeirv9d96r8wtPre9kJLM@z> (raw)
In-Reply-To: <20181002140449.24147-1-mick@ics.forth.gr>

> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_FORCE
> +	static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
> +#else
> +	static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
> +#endif
> +#endif

Why do you need two variables here?  x86, where this same logic is used
seems to get away with just a builtin_cmdline command line one.

Also please don't indent code inside cpp conditionals.

> +	/* When we return, cmdline_p should point to a temporary

This is not the normal kernel comment style.  Please keep the
/* on a line of its own.

> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_FORCE
> +	/* Built-in args override boot loader args and
> +	 * boot loader args are being ignored.
> +	 */
> +	strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
> +#else
> +	/* Boot loader args override built-in args so we
> +	 * need to put built-in args before boot loader args.
> +	 */
> +	if (builtin_cmdline[0]) {
> +		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> +		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> +		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> +	}
> +#endif
> +#endif

Even if this seems mostly copied - it probably should be #if/#elif
instead of this convoluted magic:

#if defined(CONFIG_CMDLINE_FORCE)
	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
#elif defined(CONFIG_CMDLINE_BOOL)
	if (builtin_cmdline[0]) {
		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
	}
#endif

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply	other threads:[~2018-10-02 14:34 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-02 14:04 [PATCH] RISC-V: Implement built-in command line feature Nick Kossifidis
2018-10-02 14:04 ` Nick Kossifidis
2018-10-02 14:34 ` Christoph Hellwig [this message]
2018-10-02 14:34   ` Christoph Hellwig
2018-10-02 15:21   ` Nick Kossifidis
2018-10-02 15:21     ` Nick Kossifidis
2018-10-02 14:56 ` Palmer Dabbelt
2018-10-02 14:56   ` Palmer Dabbelt
2018-10-02 16:43   ` Nick Kossifidis
2018-10-02 16:43     ` Nick Kossifidis
2018-10-02 16:56     ` Palmer Dabbelt
2018-10-02 16:56       ` Palmer Dabbelt

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=20181002143455.GA1476@infradead.org \
    --to=hch@infradead.org \
    --cc=linux-riscv@lists.infradead.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.