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

[Andy Shevchenko]

On Tue, Sep 12, 2023 at 02:03:44PM +0300, Tony Lindgren wrote:
> We can now add hardware based addressing to serial ports. Starting with
> commit 84a9582fd203 ("serial: core: Start managing serial controllers to
> enable runtime PM"), and all the related fixes to this commit, the serial
> core now knows to which serial port controller the ports are connected.
> 
> The serial ports can be addressed with DEVNAME:0.0 style naming. The names
> are something like 00:04:0.0 for a serial port on qemu, and something like
> 2800000.serial:0.0 on platform device using systems like ARM64 for example.
> 
> The DEVNAME is the unique serial port hardware controller device name, AKA
> the name for port->dev. The 0.0 are the serial core controller id and port
> id.
> 
> Typically 0.0 are used for each controller and port instance unless the
> serial port hardware controller has multiple controllers or ports.
> 
> Using DEVNAME:0.0 style naming actually solves two long term issues for
> addressing the serial ports:
> 
> 1. According to Andy Shevchenko, using DEVNAME:0.0 style naming fixes an
>    issue where depending on the BIOS settings, the kernel serial port ttyS
>    instance number may change if HSUART is enabled
> 
> 2. Device tree using architectures no longer necessarily need to specify
>    aliases to find a specific serial port, and we can just allocate the
>    ttyS instance numbers dynamically in whatever probe order
> 
> To do this, we need a custom init time parser for the console= command
> line option as printk already handles parsing it with console_setup().
> Also early_param() gets handled by console_setup() if "console" and
> "earlycon" are used.

...

> +#ifdef CONFIG_SERIAL_CORE_CONSOLE
> +int serial_base_add_preferred_console(struct uart_driver *drv,
> +				      struct uart_port *port);
> +#else

> +static inline int serial_base_add_preferred_console(struct uart_driver *drv,
> +						    struct uart_port *port)

Maybe

static inline
int serial_base_add_preferred_console(struct uart_driver *drv,
				      struct uart_port *port)

for being aligned with the above?


> +{
> +	return 0;
> +}
> +#endif

...

> +#include <linux/init.h>
> +#include <linux/list.h>

> +#include <linux/kernel.h>

Hmm... Can we use better header(s) instead?
types.h, etc?


> +#include <linux/serial_core.h>
> +#include <linux/slab.h>

...

> +static LIST_HEAD(serial_base_consoles);

Don't you need a locking to access this list?
If not, perhaps a comment why it's okay?

...

> +int serial_base_add_preferred_console(struct uart_driver *drv,
> +				      struct uart_port *port)
> +{
> +	struct serial_base_console *entry;
> +	char *port_match;

...

> +	port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
> +			       port->ctrl_id, port->port_id);

What about starting using cleanup.h?

> +	if (!port_match)
> +		return -ENOMEM;
> +
> +	list_for_each_entry(entry, &serial_base_consoles, node) {
> +		if (!strcmp(port_match, entry->name)) {
> +			add_preferred_console(drv->dev_name, port->line,
> +					      entry->opt);
> +			break;
> +		}
> +	}
> +
> +	kfree(port_match);

Also (with the above) this can be written as

	list_for_each_entry(entry, &serial_base_consoles, node) {
		if (!strcmp(port_match, entry->name))
			break;
	}
	if (list_entry_is_head(entry, &serial_base_consoles, node)
		return 0; // Hmm... it maybe -ENOENT, but okay.

	add_preferred_console(drv->dev_name, port->line, entry->opt);

> +	return 0;

> +}

...

> +EXPORT_SYMBOL_GPL(serial_base_add_preferred_console);

Can we use (start using) namespaced exports?

...

> +static int __init serial_base_add_con(char *name, char *opt)

const name
const 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)

Are you sure? I think it's c&p typo here.

> +			goto free_name;
> +	}
> +
> +	list_add_tail(&con->node, &serial_base_consoles);
> +
> +	return 0;
> +
> +free_name:
> +	kfree(con->name);
> +
> +free_con:
> +	kfree(con);

With cleanup.h this will look much better.

> +	return -ENOMEM;
> +}

...

> +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++;
> +	}

strsep() ?

Actually param_array() uses strcspn() in similar situation.

> +	if (!strlen(val))
> +		return 0;

Btw, have you seen lib/cmdline.c? Can it be helpful here?

> +	return serial_base_add_con(val, opt);
> +}

-- 
With Best Regards,
Andy Shevchenko