Re: [PATCH v2 2/3] serial: core: Add support for DEVNAME:0.0 style naming for kernel console

[Jiri Slaby <jirislaby@kernel.org>]

On 12. 09. 23, 13:03, Tony Lindgren wrote:
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base_con.c
...
> +/* Adds a command line console to the list of consoles for driver probe time */
> +static int __init serial_base_add_con(char *name, char *opt)
> +{
> +	struct serial_base_console *con;
> +
> +	con = kzalloc(sizeof(*con), GFP_KERNEL);
> +	if (!con)
> +		return -ENOMEM;
> +
> +	con->name = kstrdup(name, GFP_KERNEL);
> +	if (!con->name)
> +		goto free_con;
> +
> +	if (opt) {
> +		con->opt = kstrdup(opt, GFP_KERNEL);
> +		if (!con->name)

con->opt

> +			goto free_name;
> +	}
> +
> +	list_add_tail(&con->node, &serial_base_consoles);
> +
> +	return 0;
> +
> +free_name:
> +	kfree(con->name);
> +
> +free_con:
> +	kfree(con);
> +
> +	return -ENOMEM;
> +}
> +
> +/* Parse console name and options */
> +static int __init serial_base_parse_one(char *param, char *val,
> +					const char *unused, void *arg)
> +{
> +	char *opt;
> +
> +	if (strcmp(param, "console"))
> +		return 0;
> +
> +	if (!val)
> +		return 0;
> +
> +	opt = strchr(val, ',');
> +	if (opt) {
> +		opt[0] = '\0';
> +		opt++;
> +	}

Can this be done without mangling val, i.e. without kstrdup below?

> +	if (!strlen(val))

IOW, can this check be "val - opt > 0" or alike?

> +		return 0;
> +
> +	return serial_base_add_con(val, opt);
> +}
> +
> +/*
> + * The "console=" option is handled by console_setup() in printk. We can't use
> + * early_param() as do_early_param() checks for "console" and "earlycon" options
> + * so console_setup() potentially handles console also early. Use parse_args().

So why not concentrate console= handling on one place, ie. in 
console_setup()? The below (second time console= handling) occurs quite 
illogical to me.

> + */
> +static int __init serial_base_opts_init(void)
> +{
> +	char *command_line;
> +
> +	command_line = kstrdup(boot_command_line, GFP_KERNEL);
> +	if (!command_line)
> +		return -ENOMEM;
> +
> +	parse_args("Setting serial core console", command_line,
> +		   NULL, 0, -1, -1, NULL, serial_base_parse_one);
> +
> +	kfree(command_line);
> +
> +	return 0;
> +}

thanks,
-- 
js
suse labs