[PATCH v2 0/3] Add support for DEVNAME:0.0 style hardware based addressing

[PATCH v2 0/3] Add support for DEVNAME:0.0 style hardware based addressing

[Tony Lindgren]

Hi all,

With the recent serial core changes in v6.5, we can now add DEVNAME:0.0
style addressing for the serial ports. When using DEVNAME:0.0 naming, we
don't need to care which ttyS instance number is allocated depending on
HSUART settings or if the devicetree has added aliases for all the ports.

With these changes the port mapping is visible for usespace in sysfs with:

$ grep DEVNAME /sys/bus/serial-base/devices/*/tty/uevent

Regards,

Tony

Changes since v1:

- Constify printk add_preferred_console() as suggested by Jiri

- Use proper kernel command line helpers for parsing console as
  suggested by Jiri

- Update description for HSUART based on Andy's comments

- Standardize on DEVNAME:0.0 style naming as suggested by Andy

- Added missing put_device() calls paired with device_find_child()

Tony Lindgren (3):
  printk: Constify name for add_preferred_console()
  serial: core: Add support for DEVNAME:0.0 style naming for kernel
    console
  serial: core: Add sysfs links for serial core port instances for ttys

 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     |  26 ++++++
 include/linux/console.h              |   2 +-
 kernel/printk/printk.c               |   4 +-
 6 files changed, 176 insertions(+), 3 deletions(-)
 create mode 100644 drivers/tty/serial/serial_base_con.c


base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
-- 
2.42.0

[PATCH v2 1/3] printk: Constify name for add_preferred_console()

[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
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 <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Suggested-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 include/linux/console.h | 2 +-
 kernel/printk/printk.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/console.h b/include/linux/console.h
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -337,7 +337,7 @@ enum con_flush_mode {
 	CONSOLE_REPLAY_ALL,
 };
 
-extern int add_preferred_console(char *name, int idx, char *options);
+extern int add_preferred_console(const char *name, int idx, char *options);
 extern void console_force_preferred_locked(struct console *con);
 extern void register_console(struct console *);
 extern int unregister_console(struct console *);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2400,7 +2400,7 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified)
 	console_set_on_cmdline = 1;
 }
 
-static int __add_preferred_console(char *name, int idx, char *options,
+static int __add_preferred_console(const char *name, int idx, char *options,
 				   char *brl_options, bool user_specified)
 {
 	struct console_cmdline *c;
@@ -2509,7 +2509,7 @@ __setup("console=", console_setup);
  * commonly to provide a default console (ie from PROM variables) when
  * the user has not supplied one.
  */
-int add_preferred_console(char *name, int idx, char *options)
+int add_preferred_console(const char *name, int idx, char *options)
 {
 	return __add_preferred_console(name, idx, options, NULL, false);
 }
-- 
2.42.0

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

[Tony Lindgren]

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 <tony@atomide.com>
---
 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

diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -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
 
diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h
--- a/drivers/tty/serial/serial_base.h
+++ b/drivers/tty/serial/serial_base.h
@@ -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
diff --git a/drivers/tty/serial/serial_base_con.c b/drivers/tty/serial/serial_base_con.c
new file mode 100644
--- /dev/null
+++ b/drivers/tty/serial/serial_base_con.c
@@ -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 <tony@atomide.com>
+ */
+
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/kernel.h>
+#include <linux/serial_core.h>
+#include <linux/slab.h>
+
+#include "serial_base.h"
+
+static LIST_HEAD(serial_base_consoles);
+
+struct serial_base_console {
+	struct list_head node;
+	char *name;
+	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);
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -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;
-- 
2.42.0

[PATCH v2 3/3] serial: core: Add sysfs links for serial core port instances for ttys

[Tony Lindgren]

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.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/serial_core.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3328,6 +3328,8 @@ static int serial_core_port_device_add(struct serial_ctrl_device *ctrl_dev,
 int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 {
 	struct serial_ctrl_device *ctrl_dev, *new_ctrl_dev = NULL;
+	struct uart_match match = {port, drv};
+	struct device *tty_dev;
 	int ret;
 
 	mutex_lock(&port_mutex);
@@ -3368,10 +3370,22 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 
 	port->flags &= ~UPF_DEAD;
 
+	tty_dev = device_find_child(port->dev, &match, serial_match_port);
+	if (tty_dev) {
+		ret = sysfs_create_link(&port->port_dev->dev.kobj, &tty_dev->kobj,
+					"tty");
+		put_device(tty_dev);
+		if (ret)
+			goto err_remove_port;
+	}
+
 	mutex_unlock(&port_mutex);
 
 	return 0;
 
+err_remove_port:
+	serial_core_remove_one_port(drv, port);
+
 err_unregister_port_dev:
 	serial_base_port_device_remove(port->port_dev);
 
@@ -3393,12 +3407,20 @@ void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port
 	struct device *phys_dev = port->dev;
 	struct serial_port_device *port_dev = port->port_dev;
 	struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
+	struct uart_match match = {port, drv};
 	int ctrl_id = port->ctrl_id;
+	struct device *tty_dev;
 
 	mutex_lock(&port_mutex);
 
 	port->flags |= UPF_DEAD;
 
+	tty_dev = device_find_child(port->dev, &match, serial_match_port);
+	if (tty_dev) {
+		sysfs_remove_link(&port->port_dev->dev.kobj, "tty");
+		put_device(tty_dev);
+	}
+
 	serial_core_remove_one_port(drv, port);
 
 	/* Note that struct uart_port *port is no longer valid at this point */
-- 
2.42.0

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

[Ilpo Järvinen]

On Tue, 12 Sep 2023, 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.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  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
> 
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -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
>  
> diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h
> --- a/drivers/tty/serial/serial_base.h
> +++ b/drivers/tty/serial/serial_base.h
> @@ -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
> diff --git a/drivers/tty/serial/serial_base_con.c b/drivers/tty/serial/serial_base_con.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base_con.c
> @@ -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 <tony@atomide.com>
> + */
> +
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/kernel.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +
> +#include "serial_base.h"
> +
> +static LIST_HEAD(serial_base_consoles);
> +
> +struct serial_base_console {
> +	struct list_head node;
> +	char *name;

Can't this be const char as too?

-- 
 i.


> +	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);
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -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;
> 

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

Re: [PATCH v2 3/3] serial: core: Add sysfs links for serial core port instances for ttys

[Andy Shevchenko]

On Tue, Sep 12, 2023 at 02:03:45PM +0300, Tony Lindgren wrote:
> 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.

...

> +	struct uart_match match = {port, drv};

A nit:

	struct uart_match match = { .port = port, .driver = drv };

...

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

> +		ret = sysfs_create_link(&port->port_dev->dev.kobj, &tty_dev->kobj,
> +					"tty");

I would do it on a single line (you already over 80 anyway).

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

...

> +	struct uart_match match = {port, drv};

As per above.

...

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

?

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

> +		put_device(tty_dev);
> +	}

-- 
With Best Regards,
Andy Shevchenko

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

[Tony Lindgren]

* Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> [230912 12:24]:
> On Tue, 12 Sep 2023, Tony Lindgren wrote:
> > +struct serial_base_console {
> > +	struct list_head node;
> > +	char *name;
> 
> Can't this be const char as too?

Yes thanks,

Tony

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

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@intel.com> [230912 15:07]:
> On Tue, Sep 12, 2023 at 02:03:44PM +0300, Tony Lindgren wrote:
> > +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?

It's updated at arch_initcall() time only, I'll add a comment.

> > +	port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
> > +			       port->ctrl_id, port->port_id);
> 
> What about starting using cleanup.h?

OK seems to simplify things nicely :)

> > +EXPORT_SYMBOL_GPL(serial_base_add_preferred_console);
> 
> Can we use (start using) namespaced exports?

Sorry forgot about the namespace stuff already..

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

For name yes, opt has issues as noted in the first patch in this
series.

> > +	opt = strchr(val, ',');
> > +	if (opt) {
> > +		opt[0] = '\0';
> > +		opt++;
> > +	}
> 
> strsep() ?
> 
> Actually param_array() uses strcspn() in similar situation.

OK I'll change to use strcspn().

> > +	if (!strlen(val))
> > +		return 0;
> 
> Btw, have you seen lib/cmdline.c? Can it be helpful here?

I don't think so as at this point we don't have param=value
pairs and param is the port name.

Will fix up the rest of the stuff you commented too thanks.

Regards,

Tony

Re: [PATCH v2 3/3] serial: core: Add sysfs links for serial core port instances for ttys

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@intel.com> [230912 15:17]:
> On Tue, Sep 12, 2023 at 02:03:45PM +0300, Tony Lindgren wrote:
> > +	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);
> 
> ?
> 
> > +	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?

Yes that's shorter.

Thanks,

Tony

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

[Jiri Slaby]

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

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

[Tony Lindgren]

* Jiri Slaby <jirislaby@kernel.org> [230914 05:43]:
> On 12. 09. 23, 13:03, Tony Lindgren wrote:
> > +/*
> > + * 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.

Well console_setup() knows nothing about the probing serial port controller
device, tries to call __add_preferred_console() based on a few hardcoded
device names and some attempted guessing, and is stuffed into printk.c :)

I don't think we should pile on more stuff into printk.c for this.

If we wanted to do something, let's set up the console list somewhere else,
and then just have console_setup() add every console option to that list
and leave the rest of console_setup in place to avoid breaking things all
over the place.

Then we can export some find_named_console() type function for serial core
to use. Or do you have some better ideas in mind?

Regards,

Tony

Re: [PATCH v2 1/3] printk: Constify name for add_preferred_console()

[Petr Mladek]

On Tue 2023-09-12 14:03:43, Tony Lindgren wrote:
> 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 <john.ogness@linutronix.de>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Suggested-by: Jiri Slaby <jirislaby@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Makes sense:

Reviewed-by: Petr Mladek <pmladek@suse.com>

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

Re: [PATCH v2 1/3] printk: Constify name for add_preferred_console()

[Ilpo Järvinen]

On Wed, 20 Sep 2023, Petr Mladek wrote:

> On Tue 2023-09-12 14:03:43, Tony Lindgren wrote:
> > 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

Hi Tony,

"The gets copied" seems to lack a word (name?).

-- 
 i.


> > 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 <john.ogness@linutronix.de>
> > Cc: Petr Mladek <pmladek@suse.com>
> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Suggested-by: Jiri Slaby <jirislaby@kernel.org>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> Makes sense:
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> 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
> 

Re: [PATCH v2 1/3] printk: Constify name for add_preferred_console()

[Tony Lindgren]

Hi,

* Petr Mladek <pmladek@suse.com> [230920 10:17]:
> 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.

Thanks looks like I'll have two other printk console related patches coming
related to after working on changes related to Jiri's serial core comments.
I'm hoping to repost v3 patchset within a few days, so maybe let's wait for
that and see if something should be merged before the whole set of patches.

Regards,

Tony