Add support for DEVNAME:0.0 style hardware based addressing

While adding a preferred console handling for serial_core for serial port hardware based device addressing, Jiri suggested we constify name for add_preferred_console(). The gets copied anyways. This allows serial core to add a preferred console using serial drv->dev_name without copying it.

Note that constifying options causes changes all over the place because of struct console for match().

Cc John Ogness <[email protected]>

Cc Petr Mladek <[email protected]>

Cc Sergey Senozhatsky <[email protected]>

Cc Steven Rostedt <[email protected]>

Suggested-by Jiri Slaby <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

Quoted 3 lines · Tony Lindgren

While adding a preferred console handling for serial_core for serial port hardware based device addressing, Jiri suggested we constify name for add_preferred_console(). The gets copied anyways. This allows serial core

Quoted 11 lines

to add a preferred console using serial drv->dev_name without copying it.

Note that constifying options causes changes all over the place because of struct console for match().

Cc John Ogness <[email protected]>

Cc Petr Mladek <[email protected]>

Cc Sergey Senozhatsky <[email protected]>

Cc Steven Rostedt <[email protected]>

Suggested-by Jiri Slaby <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

Makes sense:

Reviewed-by Petr Mladek <[email protected]>

I assume that this patch would via Greg's tree together with the rest of the patchset. Please, tell me if you would prefer to queue this via printk tree.

Best Regards,
Petr

I assume that this patch would via Greg's tree together with the rest of the patchset. Please, tell me if you would prefer to queue this via printk tree.

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.

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/tty/serial/Makefile          |   3 +
 drivers/tty/serial/serial_base.h     |  11 +++
 drivers/tty/serial/serial_base_con.c | 133 +++++++++++++++++++++++++++
 drivers/tty/serial/serial_core.c     |   4 +
 4 files changed, 151 insertions(+)
 create mode 100644 drivers/tty/serial/serial_base_con.c

drivers/tty/serial/Makefile +3 -0

@@ -3,6 +3,9 @@
 # Makefile for the kernel serial device drivers.
 #
 
+# Parse kernel command line consoles before the serial drivers probe
+obj-$(CONFIG_SERIAL_CORE_CONSOLE) += serial_base_con.o
+
 obj-$(CONFIG_SERIAL_CORE) += serial_base.o
 serial_base-y := serial_core.o serial_base_bus.o serial_ctrl.o serial_port.o
 

drivers/tty/serial/serial_base.h +11 -0

@@ -45,3 +45,14 @@ void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port
 
 int serial_core_register_port(struct uart_driver *drv, struct uart_port *port);
 void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port);
+
+#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)
+{
+	return 0;
+}
+#endif

drivers/tty/serial/serial_base_con.c new file +11 -0

new file mode 100644
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Serial base console options handling
+ *
+ * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
+ * Author: Tony Lindgren <[email protected]>
+ */
+
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/kernel.h>

drivers/tty/serial/serial_base_con.c new file +112 -0

+	char *opt;
+};
+
+/*
+ * Adds a preferred console for a serial port if console=DEVNAME:0.0
+ * style addressing is used for the kernel command line. Translates
+ * from DEVNAME:0.0 to port->dev_name such as ttyS. Duplicates are
+ * ignored by add_preferred_console().
+ */
+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);
+	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);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(serial_base_add_preferred_console);
+
+/* 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)
+			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++;
+	}
+
+	if (!strlen(val))
+		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().
+ */
+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;
+}
+
+arch_initcall(serial_base_opts_init);

drivers/tty/serial/serial_core.c +4 -0

@@ -3358,6 +3358,10 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 	if (ret)
 		goto err_unregister_ctrl_dev;
 
+	ret = serial_base_add_preferred_console(drv, port);
+	if (ret)
+		goto err_unregister_port_dev;
+
 	ret = serial_core_add_one_port(drv, port);
 	if (ret)
 		goto err_unregister_port_dev;

drivers/tty/serial/serial_base_con.c line 21

Quoted 3 lines · Tony Lindgren

drivers/tty/serial/serial_base_con.c new file +3 -0

+struct serial_base_console {
+	struct list_head node;
+	char *name;

Can't this be const char as too?

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.

drivers/tty/serial/serial_base.h +4 -0

+{
+	return 0;
+}
+#endif

drivers/tty/serial/serial_base_con.c new file +2 -0

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

+static LIST_HEAD(serial_base_consoles);

drivers/tty/serial/serial_base_con.c new file +5 -0

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

drivers/tty/serial/serial_base.h +2 -0

+	return 0;
 
+}

+EXPORT_SYMBOL_GPL(serial_base_add_preferred_console);

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

drivers/tty/serial/serial_base_con.c new file +5 -0

+{
+	struct serial_base_console *con;
+
+	con = kzalloc(sizeof(*con), GFP_KERNEL);
+	if (!con)

(quoted diff) +2 -0

+	return -ENOMEM;
+}

drivers/tty/serial/serial_base_con.c new file +6 -0

+static int __init serial_base_parse_one(char *param, char *val,
+					const char *unused, void *arg)
+{
+	char *opt;
+
+	if (strcmp(param, "console"))

(quoted diff) +2 -0

+	return serial_base_add_con(val, opt);
+}

Quoted 1 line · Tony Lindgren

+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?

drivers/tty/serial/serial_base_con.c line 38

Quoted 2 lines

drivers/tty/serial/serial_base_con.c new file +2 -0

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

What about starting using cleanup.h?

Quoted 1 line

+EXPORT_SYMBOL_GPL(serial_base_add_preferred_console);

Can we use (start using) namespaced exports?

...

Quoted 1 line

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

const name
const opt
?

drivers/tty/serial/serial_base_con.c line 104

Quoted 5 lines

drivers/tty/serial/serial_base_con.c new file +5 -0

+	opt = strchr(val, ',');
+	if (opt) {
+		opt[0] = '\0';
+		opt++;
+	}

strsep() ?

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

Review

Quoted 2 lines

(quoted diff) +2 -0

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

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

drivers/tty/serial/serial_base_con.c new file +0 -0

drivers/tty/serial/serial_base_con.c new file +6 -0

+/* 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);

drivers/tty/serial/serial_base_con.c new file +23 -0

+			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"))

+ */

drivers/tty/serial/serial_base_con.c new file +5 -0

+static int __init serial_base_opts_init(void)
+{
+	char *command_line;
+
+	command_line = kstrdup(boot_command_line, GFP_KERNEL);

Quoted 4 lines · Tony Lindgren

+/*
+ * 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.

Let's allow the userspace to find out the ttyS style name for a serial core port device if a tty exists. This can be done with:

$ grep DEVNAME /sys/bus/serial-base/devices/*/tty/uevent
/sys/bus/serial-base/devices/00:04:0.0/tty/uevent:DEVNAME=ttyS0
/sys/bus/serial-base/devices/serial8250:0.1/tty/uevent:DEVNAME=ttyS1
/sys/bus/serial-base/devices/serial8250:0.2/tty/uevent:DEVNAME=ttyS2
/sys/bus/serial-base/devices/serial8250:0.3/tty/uevent:DEVNAME=ttyS3

With this change, we can add /dev/serial/by-id symlinks for the serial core port device instances. This allows using hardware based port addressing in addition to the legacy ttyS style naming.

The serial core port naming is DEVNAME:0.0, such as the 00:04:0.0 above. The 0.0 above are serial core controller id and port id. The port id and controller id are typically both zero unless the serial port hardware controller has multiple controllers or ports.

drivers/tty/serial/serial_core.c +4 -0

+		put_device(tty_dev);
+		if (ret)
+			goto err_remove_port;
+	}

(quoted diff) +2 -0

+		put_device(tty_dev);
+	}

Review

Quoted 1 line · Tony Lindgren

(quoted diff) +1 -0

+	tty_dev = device_find_child(port->dev, &match, serial_match_port);

Can be written as

	tty_dev = device_find_child(phys_dev, &match, serial_match_port);

?

drivers/tty/serial/serial_core.c void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port · line 3420

Quoted 2 lines

drivers/tty/serial/serial_core.c +2 -0

+	if (tty_dev) {
+		sysfs_remove_link(&port->port_dev->dev.kobj, "tty");

Can be written as

		sysfs_remove_link(&port_dev->dev.kobj, "tty");

can't be?