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

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

[Tony Lindgren]

Hi all,

With the recent serial core changes, 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.

This also allows us to also drop the old console_setup() parsing for
character device names.

Regards,

Tony

Changes since v2:

- Console name got constified and already applied as suggested by Ilpo
  and Andy

- Add printk/conopt.c to save console command line options

- Add a patch to drop old console_setup() character device name parsing

- Use cleanup.h to simplify freeing as suggested by Andy

- Use types.h instead of kernel.h as suggested by Andy

- Use strcspn() as suggested by Andy

- Various coding improvments suggested by Andy

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: Save console options for add_preferred_console_match()
  serial: core: Add support for DEVNAME:0.0 style naming for kernel
    console
  serial: core: Move console character device handling from printk

 drivers/tty/serial/serial_base.h     |  14 ++++
 drivers/tty/serial/serial_base_bus.c | 104 ++++++++++++++++++++++++
 drivers/tty/serial/serial_core.c     |   4 +
 include/linux/printk.h               |   3 +
 kernel/printk/Makefile               |   2 +-
 kernel/printk/conopt.c               | 115 +++++++++++++++++++++++++++
 kernel/printk/console_cmdline.h      |   4 +
 kernel/printk/printk.c               |  41 +++-------
 8 files changed, 254 insertions(+), 33 deletions(-)
 create mode 100644 kernel/printk/conopt.c

-- 
2.42.1

[PATCH v3 1/3] printk: Save console options for add_preferred_console_match()

[Tony Lindgren]

Driver subsystems may need to translate the preferred console name to the
character device name used. We already do some of this in console_setup()
with a few hardcoded names, but that does not scale well.

The console options are parsed early in console_setup(), and the consoles
are added with __add_preferred_console(). At this point we don't know much
about the character device names and device drivers getting probed.

To allow drivers subsystems to set up a preferred console, let's save the
kernel command line console options. To add a preferred console, let's add
a new function add_preferred_console_match().

This allows the serial core layer to support console=DEVNAME:0.0 style
hardware based addressing in addition to the current console=ttyS0 style
naming. And we can start moving console_setup() character device parsing
to the driver subsystem specific code.

We use a separate array from the console_cmdline array as the character
device name and index may be unknown at the console_setup() time. And we do
not want to call __add_preferred_console() until the character device name
and index are known.

Adding the console name in addition to the character device name, and a
flag for an added console, could be added to the struct console_cmdline.
And the console_cmdline array handling modified accordingly. But that
complicates things compared saving the console options, and then adding
the consoles when the subsystems handling the consoles are ready.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 include/linux/printk.h          |   3 +
 kernel/printk/Makefile          |   2 +-
 kernel/printk/conopt.c          | 115 ++++++++++++++++++++++++++++++++
 kernel/printk/console_cmdline.h |   4 ++
 kernel/printk/printk.c          |  11 ++-
 5 files changed, 131 insertions(+), 4 deletions(-)
 create mode 100644 kernel/printk/conopt.c

diff --git a/include/linux/printk.h b/include/linux/printk.h
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -60,6 +60,9 @@ static inline const char *printk_skip_headers(const char *buffer)
 #define CONSOLE_LOGLEVEL_DEFAULT CONFIG_CONSOLE_LOGLEVEL_DEFAULT
 #define CONSOLE_LOGLEVEL_QUIET	 CONFIG_CONSOLE_LOGLEVEL_QUIET
 
+int add_preferred_console_match(const char *match, const char *name,
+				const short idx);
+
 extern int console_printk[];
 
 #define console_loglevel (console_printk[0])
diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile
--- a/kernel/printk/Makefile
+++ b/kernel/printk/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-y	= printk.o
+obj-y	= printk.o conopt.o
 obj-$(CONFIG_PRINTK)	+= printk_safe.o nbcon.o
 obj-$(CONFIG_A11Y_BRAILLE_CONSOLE)	+= braille.o
 obj-$(CONFIG_PRINTK_INDEX)	+= index.o
diff --git a/kernel/printk/conopt.c b/kernel/printk/conopt.c
new file mode 100644
--- /dev/null
+++ b/kernel/printk/conopt.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Kernel command line console options for hardware based addressing
+ *
+ * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
+ * Author: Tony Lindgren <tony@atomide.com>
+ */
+
+#include <linux/console.h>
+#include <linux/kernel.h>
+
+#include "console_cmdline.h"
+
+/*
+ * Allow longer DEVNAME:0.0 style console naming such as abcd0000.serial:0.0
+ * in addition to the legacy ttyS0 style naming.
+ */
+#define CONSOLE_NAME_MAX	32
+
+#define CONSOLE_OPT_MAX		16
+
+struct console_option {
+	char name[CONSOLE_NAME_MAX];
+	char opt[CONSOLE_OPT_MAX];
+};
+
+/* Updated only at console_setup() time, no locking needed */
+static struct console_option conopt[MAX_CMDLINECONSOLES];
+
+/**
+ * console_opt_save - Saves kernel command line console option for driver use
+ * @str: Kernel command line console name and option
+ *
+ * Saves a kernel command line console option for driver subsystems to use for
+ * adding a preferred console during init. Called from console_setup() only.
+ */
+int __init console_opt_save(char *str)
+{
+	struct console_option *con;
+	const char *opt = NULL;
+	size_t namelen, optlen;
+	int i;
+
+	namelen = strcspn(str, ",");
+	if (!namelen)
+		return -EINVAL;
+
+	optlen = strlen(str) - namelen;
+	if (optlen > 1)
+		opt = str + namelen + 1;
+
+	if (namelen >= CONSOLE_NAME_MAX || optlen >= CONSOLE_OPT_MAX)
+		return -EINVAL;
+
+	for (i = 0; i < MAX_CMDLINECONSOLES; i++) {
+		con = &conopt[i];
+
+		if (con->name[0]) {
+			if (!strncmp(str, con->name, namelen))
+				return 0;
+			continue;
+		}
+
+		strscpy(con->name, str, namelen + 1);
+		if (opt)
+			strscpy(con->opt, opt, optlen + 1);
+
+		return 0;
+	}
+
+	return -ENOMEM;
+}
+
+static struct console_option *console_opt_find(const char *name)
+{
+	struct console_option *con;
+	int i;
+
+	for (i = 0; i < MAX_CMDLINECONSOLES; i++) {
+		con = &conopt[i];
+		if (!strcmp(name, con->name))
+			return con;
+	}
+
+	return NULL;
+}
+
+/**
+ * add_preferred_console_match - Adds a preferred console if a match is found
+ * @match: Expected console on kernel command line, such as console=DEVNAME:0.0
+ * @name: Name of the console character device to add such as ttyS
+ * @idx: Index for the console
+ *
+ * Allows driver subsystems to add a console after translating the command
+ * line name to the character device name used for the console. Options are
+ * added automatically based on the kernel command line. Duplicate preferred
+ * consoles are ignored by __add_preferred_console().
+ */
+int add_preferred_console_match(const char *match, const char *name,
+				const short idx)
+{
+	struct console_option *con;
+
+	if (!match || !strlen(match) || !name || !strlen(name) ||
+	    idx < 0)
+		return -EINVAL;
+
+	con = console_opt_find(match);
+	if (!con)
+		return -ENOENT;
+
+	add_preferred_console(name, idx, con->opt);
+
+	return 0;
+}
diff --git a/kernel/printk/console_cmdline.h b/kernel/printk/console_cmdline.h
--- a/kernel/printk/console_cmdline.h
+++ b/kernel/printk/console_cmdline.h
@@ -2,6 +2,10 @@
 #ifndef _CONSOLE_CMDLINE_H
 #define _CONSOLE_CMDLINE_H
 
+#define MAX_CMDLINECONSOLES 8
+
+int console_opt_save(char *str);
+
 struct console_cmdline
 {
 	char	name[16];			/* Name of the driver	    */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -360,9 +360,6 @@ static int console_locked;
 /*
  *	Array of consoles built from command line options (console=)
  */
-
-#define MAX_CMDLINECONSOLES 8
-
 static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
 
 static int preferred_console = -1;
@@ -2445,6 +2442,14 @@ static int __init console_setup(char *str)
 	char *s, *options, *brl_options = NULL;
 	int idx;
 
+	/*
+	 * Save the console for possible driver subsystem use. Bail out early
+	 * for DEVICE:0.0 style console names as the character device name is
+	 * be unknown at this point.
+	 */
+	if (!console_opt_save(str) && strchr(str, ':'))
+		return 1;
+
 	/*
 	 * console="" or console=null have been suggested as a way to
 	 * disable console output. Use ttynull that has been created
-- 
2.42.1

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

[Tony Lindgren]

We can now add hardware based addressing for 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, let's match the hardware addressing style console name to
the character device name used, and add a preferred console using the
character device device name.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/serial_base.h     | 14 ++++++++++
 drivers/tty/serial/serial_base_bus.c | 38 ++++++++++++++++++++++++++++
 drivers/tty/serial/serial_core.c     |  4 +++
 3 files changed, 56 insertions(+)

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,17 @@ 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_bus.c b/drivers/tty/serial/serial_base_bus.c
--- a/drivers/tty/serial/serial_base_bus.c
+++ b/drivers/tty/serial/serial_base_bus.c
@@ -204,6 +204,44 @@ void serial_base_port_device_remove(struct serial_port_device *port_dev)
 	put_device(&port_dev->dev);
 }
 
+#ifdef CONFIG_SERIAL_CORE_CONSOLE
+
+/*
+ * serial_base_add_preferred_console - Adds a preferred console
+ * @drv: Serial port device driver
+ * @port: Serial port instance
+ *
+ * Tries to add a preferred console for a serial port if specified in the
+ * kernel command line. Supports both the traditional character device such
+ * as console=ttyS0, and a hardware addressing based console=DEVNAME:0.0
+ * style name.
+ *
+ * Translates the kernel command line option using a hardware based addressing
+ * console=DEVNAME:0.0 to the serial port character device such as ttyS0.
+ *
+ * Note that duplicates are ignored by add_preferred_console().
+ */
+int serial_base_add_preferred_console(struct uart_driver *drv,
+				      struct uart_port *port)
+{
+	const char *port_match __free(kfree);
+	int ret;
+
+	port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
+			       port->ctrl_id, port->port_id);
+	if (!port_match)
+		return -ENOMEM;
+
+	/* Translate a hardware addressing style console=DEVNAME:0.0 */
+	ret = add_preferred_console_match(port_match, drv->dev_name, port->line);
+	if (ret && ret != -ENOENT)
+		return ret;
+
+	return 0;
+}
+
+#endif
+
 static int serial_base_init(void)
 {
 	int ret;
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
@@ -3359,6 +3359,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.1

[PATCH v3 3/3] serial: core: Move console character device handling from printk

[Tony Lindgren]

There's no need for console_setup() to try to figure out the console
character device name for serial ports early on before uart_add_one_port()
has been called. And for early debug console we have earlycon.

Let's handle the serial port specific commandline options in the serial
core subsystem and drop the handling from printk. The serial core
subsystem can just call add_preferred_console() when the serial port is
getting initialized.

Note that eventually we may want to set up driver specific console quirk
handling for the serial port device drivers to use. But we need to figure
out which driver(s) need to call the quirk. So for now, we just move the
sparc quirk and handle it directly.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/serial_base_bus.c | 66 ++++++++++++++++++++++++++++
 kernel/printk/printk.c               | 30 +------------
 2 files changed, 67 insertions(+), 29 deletions(-)

diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
--- a/drivers/tty/serial/serial_base_bus.c
+++ b/drivers/tty/serial/serial_base_bus.c
@@ -206,6 +206,43 @@ void serial_base_port_device_remove(struct serial_port_device *port_dev)
 
 #ifdef CONFIG_SERIAL_CORE_CONSOLE
 
+#ifdef __sparc__
+
+/* Handle Sparc ttya and ttyb options as done in console_setup() */
+static int serial_base_add_sparc_console(struct uart_driver *drv,
+					 struct uart_port *port)
+{
+	const char *name = NULL;
+	int ret;
+
+	switch (port->line) {
+	case 0:
+		name = "ttya";
+		break;
+	case 1:
+		name = "ttyb";
+		break;
+	default:
+		return 0;
+	}
+
+	ret = add_preferred_console_match(name, drv->dev_name, port->line);
+	if (ret && ret != -ENOENT)
+		return ret;
+
+	return 0;
+}
+
+#else
+
+static inline int serial_base_add_sparc_console(struct uart_driver *drv,
+						struct uart_port *port)
+{
+	return 0;
+}
+
+#endif
+
 /*
  * serial_base_add_preferred_console - Adds a preferred console
  * @drv: Serial port device driver
@@ -225,6 +262,8 @@ int serial_base_add_preferred_console(struct uart_driver *drv,
 				      struct uart_port *port)
 {
 	const char *port_match __free(kfree);
+	const char *char_match __free(kfree);
+	const char *nmbr_match __free(kfree);
 	int ret;
 
 	port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
@@ -232,6 +271,33 @@ int serial_base_add_preferred_console(struct uart_driver *drv,
 	if (!port_match)
 		return -ENOMEM;
 
+	char_match = kasprintf(GFP_KERNEL, "%s%i", drv->dev_name, port->line);
+	if (!char_match)
+		return -ENOMEM;
+
+	/* Handle ttyS specific options */
+	if (!strncmp(drv->dev_name, "ttyS", 4)) {
+		/* No name, just a number */
+		nmbr_match = kasprintf(GFP_KERNEL, "%i", port->line);
+		if (!nmbr_match)
+			return -ENODEV;
+
+		ret = add_preferred_console_match(nmbr_match, drv->dev_name,
+						  port->line);
+		if (ret && ret != -ENOENT)
+			return ret;
+
+		/* Sparc ttya and ttyb */
+		ret = serial_base_add_sparc_console(drv, port);
+		if (ret)
+			return ret;
+	}
+
+	/* Handle the traditional character device name style console=ttyS0 */
+	ret = add_preferred_console_match(char_match, drv->dev_name, port->line);
+	if (ret && ret != -ENOENT)
+		return ret;
+
 	/* Translate a hardware addressing style console=DEVNAME:0.0 */
 	ret = add_preferred_console_match(port_match, drv->dev_name, port->line);
 	if (ret && ret != -ENOENT)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2438,9 +2438,7 @@ __setup("console_msg_format=", console_msg_format_setup);
  */
 static int __init console_setup(char *str)
 {
-	char buf[sizeof(console_cmdline[0].name) + 4]; /* 4 for "ttyS" */
-	char *s, *options, *brl_options = NULL;
-	int idx;
+	char *brl_options = NULL;
 
 	/*
 	 * Save the console for possible driver subsystem use. Bail out early
@@ -2463,32 +2461,6 @@ static int __init console_setup(char *str)
 	if (_braille_console_setup(&str, &brl_options))
 		return 1;
 
-	/*
-	 * Decode str into name, index, options.
-	 */
-	if (str[0] >= '0' && str[0] <= '9') {
-		strcpy(buf, "ttyS");
-		strncpy(buf + 4, str, sizeof(buf) - 5);
-	} else {
-		strncpy(buf, str, sizeof(buf) - 1);
-	}
-	buf[sizeof(buf) - 1] = 0;
-	options = strchr(str, ',');
-	if (options)
-		*(options++) = 0;
-#ifdef __sparc__
-	if (!strcmp(str, "ttya"))
-		strcpy(buf, "ttyS0");
-	if (!strcmp(str, "ttyb"))
-		strcpy(buf, "ttyS1");
-#endif
-	for (s = buf; *s; s++)
-		if (isdigit(*s) || *s == ',')
-			break;
-	idx = simple_strtoul(s, NULL, 10);
-	*s = 0;
-
-	__add_preferred_console(buf, idx, options, brl_options, true);
 	return 1;
 }
 __setup("console=", console_setup);
-- 
2.42.1

Re: [PATCH v3 1/3] printk: Save console options for add_preferred_console_match()

[Andy Shevchenko]

On Tue, Nov 21, 2023 at 01:31:55PM +0200, Tony Lindgren wrote:
> Driver subsystems may need to translate the preferred console name to the
> character device name used. We already do some of this in console_setup()
> with a few hardcoded names, but that does not scale well.
> 
> The console options are parsed early in console_setup(), and the consoles
> are added with __add_preferred_console(). At this point we don't know much
> about the character device names and device drivers getting probed.
> 
> To allow drivers subsystems to set up a preferred console, let's save the
> kernel command line console options. To add a preferred console, let's add
> a new function add_preferred_console_match().
> 
> This allows the serial core layer to support console=DEVNAME:0.0 style
> hardware based addressing in addition to the current console=ttyS0 style
> naming. And we can start moving console_setup() character device parsing
> to the driver subsystem specific code.
> 
> We use a separate array from the console_cmdline array as the character
> device name and index may be unknown at the console_setup() time. And we do
> not want to call __add_preferred_console() until the character device name
> and index are known.
> 
> Adding the console name in addition to the character device name, and a
> flag for an added console, could be added to the struct console_cmdline.
> And the console_cmdline array handling modified accordingly. But that
> complicates things compared saving the console options, and then adding
> the consoles when the subsystems handling the consoles are ready.

...

> +#include <linux/console.h>

> +#include <linux/kernel.h>

I think instead of kernel.h you may want to see these:

linux/init.h
linux/string.h

asm/errno.h

> +#include "console_cmdline.h"

...

> +/**
> + * console_opt_save - Saves kernel command line console option for driver use
> + * @str: Kernel command line console name and option
> + *
> + * Saves a kernel command line console option for driver subsystems to use for
> + * adding a preferred console during init. Called from console_setup() only.

	scripts/kernel-doc -v -none -Wall ...

most likely will complain (no Return section).

> + */
> +int __init console_opt_save(char *str)

str is not const? Hmm...

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Nov 21, 2023 at 01:31:56PM +0200, Tony Lindgren wrote:
> We can now add hardware based addressing for 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, let's match the hardware addressing style console name to
> the character device name used, and add a preferred console using the
> character device device name.

...

> +int serial_base_add_preferred_console(struct uart_driver *drv,
> +				      struct uart_port *port)
> +{
> +	const char *port_match __free(kfree);
> +	int ret;
> +
> +	port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
> +			       port->ctrl_id, port->port_id);
> +	if (!port_match)
> +		return -ENOMEM;
> +
> +	/* Translate a hardware addressing style console=DEVNAME:0.0 */
> +	ret = add_preferred_console_match(port_match, drv->dev_name, port->line);
> +	if (ret && ret != -ENOENT)
> +		return ret;
> +
> +	return 0;

Maybe

	ret = add_preferred_console_match(port_match, drv->dev_name, port->line);
	if (ret == -ENOENT)
		return 0;

	return ret;

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] serial: core: Move console character device handling from printk

[Andy Shevchenko]

On Tue, Nov 21, 2023 at 01:31:57PM +0200, Tony Lindgren wrote:
> There's no need for console_setup() to try to figure out the console
> character device name for serial ports early on before uart_add_one_port()
> has been called. And for early debug console we have earlycon.
> 
> Let's handle the serial port specific commandline options in the serial
> core subsystem and drop the handling from printk. The serial core
> subsystem can just call add_preferred_console() when the serial port is
> getting initialized.
> 
> Note that eventually we may want to set up driver specific console quirk
> handling for the serial port device drivers to use. But we need to figure
> out which driver(s) need to call the quirk. So for now, we just move the
> sparc quirk and handle it directly.

...

> +	ret = add_preferred_console_match(name, drv->dev_name, port->line);
> +	if (ret && ret != -ENOENT)
> +		return ret;
> +
> +	return 0;

2nd time and so on, perhaps deserves a helper?

static inline int add_preferred_console...(...)
{
	int ret;

	ret = add_preferred_console_match(name, drv->dev_name, port->line);
	if (ret == -ENOENT)
		return 0;
	return ret;

}

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] printk: Save console options for add_preferred_console_match()

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@intel.com> [231121 17:53]:
> On Tue, Nov 21, 2023 at 01:31:55PM +0200, Tony Lindgren wrote:
> > +#include <linux/console.h>
> 
> > +#include <linux/kernel.h>
> 
> I think instead of kernel.h you may want to see these:
> 
> linux/init.h
> linux/string.h
> 
> asm/errno.h
> 
> > +#include "console_cmdline.h"

OK

> > +/**
> > + * console_opt_save - Saves kernel command line console option for driver use
> > + * @str: Kernel command line console name and option
> > + *
> > + * Saves a kernel command line console option for driver subsystems to use for
> > + * adding a preferred console during init. Called from console_setup() only.
> 
> 	scripts/kernel-doc -v -none -Wall ...
> 
> most likely will complain (no Return section).

OK adding.

> > + */
> > +int __init console_opt_save(char *str)
> 
> str is not const? Hmm...

Nice yes it can be const char *str here. Hmm maybe with the third patch
also console_setup() can use const char * now.. Will check.

Thanks,

Tony

Re: [PATCH v3 1/3] printk: Save console options for add_preferred_console_match()

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [231122 08:18]:
> * Andy Shevchenko <andriy.shevchenko@intel.com> [231121 17:53]:
> > On Tue, Nov 21, 2023 at 01:31:55PM +0200, Tony Lindgren wrote:
> > > +int __init console_opt_save(char *str)
> > 
> > str is not const? Hmm...
> 
> Nice yes it can be const char *str here. Hmm maybe with the third patch
> also console_setup() can use const char * now.. Will check.

Nah console_setup() uses __setup().

Tony

Re: [PATCH v3 3/3] serial: core: Move console character device handling from printk

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@intel.com> [231121 18:00]:
> On Tue, Nov 21, 2023 at 01:31:57PM +0200, Tony Lindgren wrote:
> > +	ret = add_preferred_console_match(name, drv->dev_name, port->line);
> > +	if (ret && ret != -ENOENT)
> > +		return ret;
> > +
> > +	return 0;
> 
> 2nd time and so on, perhaps deserves a helper?
> 
> static inline int add_preferred_console...(...)
> {
> 	int ret;
> 
> 	ret = add_preferred_console_match(name, drv->dev_name, port->line);
> 	if (ret == -ENOENT)
> 		return 0;
> 	return ret;
> 
> }
> 
> ?

Yes good idea, thanks.

Tony

Re: [PATCH v3 3/3] serial: core: Move console character device handling from printk

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [700101 02:00]:
> -	__add_preferred_console(buf, idx, options, brl_options, true);
>  	return 1;

Looks like this can't be dropped yet. We need to keep it for the
brl_options. I'll change it to return early if brl_options is NULL.

Regards,

Tony

Re: [PATCH v3 3/3] serial: core: Move console character device handling from printk

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [231122 09:04]:
> * Tony Lindgren <tony@atomide.com> [700101 02:00]:
> > -	__add_preferred_console(buf, idx, options, brl_options, true);
> >  	return 1;
> 
> Looks like this can't be dropped yet. We need to keep it for the
> brl_options. I'll change it to return early if brl_options is NULL.

Looks like we can drop the parsing from printk :) In console_setup()
we can call console_opt_save() after _braille_console_setup(), and
then save also the brl_options for the port.

Regards,

Tony

Re: [PATCH v3 3/3] serial: core: Move console character device handling from printk

[Dan Carpenter]

Hi Tony,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tony-Lindgren/printk-Save-console-options-for-add_preferred_console_match/20231121-193809
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20231121113203.61341-4-tony%40atomide.com
patch subject: [PATCH v3 3/3] serial: core: Move console character device handling from printk
config: parisc-randconfig-r081-20231122 (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202311221437.5Gil0Pml-lkp@intel.com/

smatch warnings:
drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
drivers/tty/serial/serial_base_bus.c:265 serial_base_add_preferred_console() error: uninitialized symbol 'char_match'.

vim +/nmbr_match +266 drivers/tty/serial/serial_base_bus.c

e4ebdcd790e0f3 Tony Lindgren 2023-11-21  261  int serial_base_add_preferred_console(struct uart_driver *drv,
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  262  				      struct uart_port *port)
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  263  {
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  264  	const char *port_match __free(kfree);
b1b8726ec3f40b Tony Lindgren 2023-11-21 @265  	const char *char_match __free(kfree);
b1b8726ec3f40b Tony Lindgren 2023-11-21 @266  	const char *nmbr_match __free(kfree);

These need to be initialized to NULL.

	const char *char_match __free(kfree) = NULL;

e4ebdcd790e0f3 Tony Lindgren 2023-11-21  267  	int ret;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  268  
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  269  	port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  270  			       port->ctrl_id, port->port_id);
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  271  	if (!port_match)
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  272  		return -ENOMEM;

Otherwise in this error path we'll call kfree(char_match) and
kfree(nmbr_match) when the haven't been initialized.

e4ebdcd790e0f3 Tony Lindgren 2023-11-21  273  
b1b8726ec3f40b Tony Lindgren 2023-11-21  274  	char_match = kasprintf(GFP_KERNEL, "%s%i", drv->dev_name, port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21  275  	if (!char_match)
b1b8726ec3f40b Tony Lindgren 2023-11-21  276  		return -ENOMEM;
b1b8726ec3f40b Tony Lindgren 2023-11-21  277  
b1b8726ec3f40b Tony Lindgren 2023-11-21  278  	/* Handle ttyS specific options */
b1b8726ec3f40b Tony Lindgren 2023-11-21  279  	if (!strncmp(drv->dev_name, "ttyS", 4)) {
b1b8726ec3f40b Tony Lindgren 2023-11-21  280  		/* No name, just a number */
b1b8726ec3f40b Tony Lindgren 2023-11-21  281  		nmbr_match = kasprintf(GFP_KERNEL, "%i", port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21  282  		if (!nmbr_match)
b1b8726ec3f40b Tony Lindgren 2023-11-21  283  			return -ENODEV;
b1b8726ec3f40b Tony Lindgren 2023-11-21  284  
b1b8726ec3f40b Tony Lindgren 2023-11-21  285  		ret = add_preferred_console_match(nmbr_match, drv->dev_name,
b1b8726ec3f40b Tony Lindgren 2023-11-21  286  						  port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21  287  		if (ret && ret != -ENOENT)
b1b8726ec3f40b Tony Lindgren 2023-11-21  288  			return ret;
b1b8726ec3f40b Tony Lindgren 2023-11-21  289  
b1b8726ec3f40b Tony Lindgren 2023-11-21  290  		/* Sparc ttya and ttyb */
b1b8726ec3f40b Tony Lindgren 2023-11-21  291  		ret = serial_base_add_sparc_console(drv, port);
b1b8726ec3f40b Tony Lindgren 2023-11-21  292  		if (ret)
b1b8726ec3f40b Tony Lindgren 2023-11-21  293  			return ret;
b1b8726ec3f40b Tony Lindgren 2023-11-21  294  	}
b1b8726ec3f40b Tony Lindgren 2023-11-21  295  
b1b8726ec3f40b Tony Lindgren 2023-11-21  296  	/* Handle the traditional character device name style console=ttyS0 */
b1b8726ec3f40b Tony Lindgren 2023-11-21  297  	ret = add_preferred_console_match(char_match, drv->dev_name, port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21  298  	if (ret && ret != -ENOENT)
b1b8726ec3f40b Tony Lindgren 2023-11-21  299  		return ret;
b1b8726ec3f40b Tony Lindgren 2023-11-21  300  
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  301  	/* Translate a hardware addressing style console=DEVNAME:0.0 */
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  302  	ret = add_preferred_console_match(port_match, drv->dev_name, port->line);
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  303  	if (ret && ret != -ENOENT)
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  304  		return ret;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  305  
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  306  	return 0;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  307  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 3/3] serial: core: Move console character device handling from printk

[Dan Carpenter]

On Thu, Nov 23, 2023 at 10:24:24AM +0300, Dan Carpenter wrote:
> Hi Tony,
> 
> kernel test robot noticed the following build warnings:
> 
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Tony-Lindgren/printk-Save-console-options-for-add_preferred_console_match/20231121-193809
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> patch link:    https://lore.kernel.org/r/20231121113203.61341-4-tony%40atomide.com
> patch subject: [PATCH v3 3/3] serial: core: Move console character device handling from printk
> config: parisc-randconfig-r081-20231122 (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/config)
> compiler: hppa-linux-gcc (GCC) 13.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> | Closes: https://lore.kernel.org/r/202311221437.5Gil0Pml-lkp@intel.com/
> 
> smatch warnings:
> drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
> drivers/tty/serial/serial_base_bus.c:265 serial_base_add_preferred_console() error: uninitialized symbol 'char_match'.
> 
> vim +/nmbr_match +266 drivers/tty/serial/serial_base_bus.c
> 
> e4ebdcd790e0f3 Tony Lindgren 2023-11-21  261  int serial_base_add_preferred_console(struct uart_driver *drv,
> e4ebdcd790e0f3 Tony Lindgren 2023-11-21  262  				      struct uart_port *port)
> e4ebdcd790e0f3 Tony Lindgren 2023-11-21  263  {
> e4ebdcd790e0f3 Tony Lindgren 2023-11-21  264  	const char *port_match __free(kfree);
> b1b8726ec3f40b Tony Lindgren 2023-11-21 @265  	const char *char_match __free(kfree);
> b1b8726ec3f40b Tony Lindgren 2023-11-21 @266  	const char *nmbr_match __free(kfree);
> 
> These need to be initialized to NULL.
> 
> 	const char *char_match __free(kfree) = NULL;
> 

Let's add a todo to make checkpatch warn about this.

KTODO: make checkpatch warn about __free() functions without an initializer

regards,
dan carpenter

Re: [PATCH v3 3/3] serial: core: Move console character device handling from printk

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [231122 10:16]:
> * Tony Lindgren <tony@atomide.com> [231122 09:04]:
> > * Tony Lindgren <tony@atomide.com> [700101 02:00]:
> > > -	__add_preferred_console(buf, idx, options, brl_options, true);
> > >  	return 1;
> > 
> > Looks like this can't be dropped yet. We need to keep it for the
> > brl_options. I'll change it to return early if brl_options is NULL.
> 
> Looks like we can drop the parsing from printk :) In console_setup()
> we can call console_opt_save() after _braille_console_setup(), and
> then save also the brl_options for the port.

I noticed we have more issues remaining trying to drop the console
parsing completely from console_setup().

If add_preferred_console() gets called later, register_console() can
try to call try_enable_default_console() before we get around to call
try_enable_preferred_console(), and that may lead to no serial console.

To avoid that, setting console_set_on_cmdline = 1 in console_setup(),
and patching register_console() console to check for the flag helps.
But looks like that leads to bootconsole not getting disabled and
more patching for that is needed.. And of course we'd need to check
the other register_console() callers too, not just 8250..

So I think for now, it's best to just drop the 8250 and sparc quirks
from console_setup(), that already simplifies things in printk a bit :)

And for 8250, we should have serial8250_isa_init_ports() call
add_preferred_console_match() to avoid console getting registered
later on when the hardware specific driver takes over for x86, m68k,
and alpha that define SERIAL_PORT_DFNS.

Regards,

Tony

Re: [PATCH v3 3/3] serial: core: Move console character device handling from printk

[Tony Lindgren]

* Dan Carpenter <dan.carpenter@linaro.org> [231123 07:29]:
> On Thu, Nov 23, 2023 at 10:24:24AM +0300, Dan Carpenter wrote:
> > Hi Tony,
> > 
> > kernel test robot noticed the following build warnings:
> > 
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Tony-Lindgren/printk-Save-console-options-for-add_preferred_console_match/20231121-193809
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> > patch link:    https://lore.kernel.org/r/20231121113203.61341-4-tony%40atomide.com
> > patch subject: [PATCH v3 3/3] serial: core: Move console character device handling from printk
> > config: parisc-randconfig-r081-20231122 (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/config)
> > compiler: hppa-linux-gcc (GCC) 13.2.0
> > reproduce: (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/reproduce)
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Reported-by: Dan Carpenter <error27@gmail.com>
> > | Closes: https://lore.kernel.org/r/202311221437.5Gil0Pml-lkp@intel.com/
> > 
> > smatch warnings:
> > drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
> > drivers/tty/serial/serial_base_bus.c:265 serial_base_add_preferred_console() error: uninitialized symbol 'char_match'.
> > 
> > vim +/nmbr_match +266 drivers/tty/serial/serial_base_bus.c
> > 
> > e4ebdcd790e0f3 Tony Lindgren 2023-11-21  261  int serial_base_add_preferred_console(struct uart_driver *drv,
> > e4ebdcd790e0f3 Tony Lindgren 2023-11-21  262  				      struct uart_port *port)
> > e4ebdcd790e0f3 Tony Lindgren 2023-11-21  263  {
> > e4ebdcd790e0f3 Tony Lindgren 2023-11-21  264  	const char *port_match __free(kfree);
> > b1b8726ec3f40b Tony Lindgren 2023-11-21 @265  	const char *char_match __free(kfree);
> > b1b8726ec3f40b Tony Lindgren 2023-11-21 @266  	const char *nmbr_match __free(kfree);
> > 
> > These need to be initialized to NULL.
> > 
> > 	const char *char_match __free(kfree) = NULL;
> > 
> 
> Let's add a todo to make checkpatch warn about this.
> 
> KTODO: make checkpatch warn about __free() functions without an initializer

Yes good idea.

Thanks,

Tony

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

[Petr Mladek]

On Tue 2023-11-21 13:31:54, Tony Lindgren wrote:
> Hi all,
> 
> With the recent serial core changes, 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.
> 
> This also allows us to also drop the old console_setup() parsing for
> character device names.
> 
> Tony Lindgren (3):
>   printk: Save console options for add_preferred_console_match()
>   serial: core: Add support for DEVNAME:0.0 style naming for kernel
>     console
>   serial: core: Move console character device handling from printk

First, I appreciate the effort to match aliases to the same console.

Well, my understanding is that it solves the problem only for the newly
added console=DEVICENAME:0.0 format. But it does not handle the
existing problems with matching console names passed via earlycon=
and console= parameters. Am I right?

Now, the bad news. This patchset causes regressions which are
not acceptable. I have found two so far but there might be more.

I used the following kernel command line:

   earlycon=uart8250,io,0x3f8,115200 console=ttyS0,115200 console=tty0 ignore_loglevel log_buf_len=1M


1. The patchset caused that /dev/console became associated with
   ttyS0 instead of tty0, see the "C" flag:

	original # cat /proc/consoles
	tty0                 -WU (EC    )    4:1
	ttyS0                -W- (E  p a)    4:64

   vs.

	patched # cat /proc/consoles
	ttyS0                -W- (EC p a)    4:64
	tty0                 -WU (E     )    4:1

   This is most likely caused by the different ordering of
   __add_preferred_console() calls.

   The ordering is important because it defines which console
   will get associated with /dev/console. It is a so called
   preferred console defined by the last console= parameter.

   Unfortunately also the ordering of the other parameters
   is important when a console defined by the last console=
   parameter is not registered at all. In this case,
   /dev/console gets associated with the first console
   with tty binding according to the order on the command line.

   If you think that it is weird behavior then I agree.
   But it is a historical mess. It is how people used it
   when the various features were added. Many changes
   in this code caused regressions and had to be reverted.

   See the following to get the picture:

       + commit c6c7d83b9c9e6a8 ("Revert "console: don't
	 prefer first registered if DT specifies stdout-path")

       + commit dac8bbbae1d0ccb ("Revert "printk: fix double
	 printing with earlycon"").


2. The serial console gets registered much later with this
   patchset:

	original # dmesg | grep printk:
	[    0.000000] printk: legacy bootconsole [uart8250] enabled
	[    0.000000] printk: debug: ignoring loglevel setting.
	[    0.016859] printk: log_buf_len: 1048576 bytes
	[    0.017324] printk: early log buf free: 259624(99%)
	[    0.141859] printk: legacy console [tty0] enabled
	[    0.142399] printk: legacy bootconsole [uart8250] disabled
	[    0.143032] printk: legacy console [ttyS0] enabled

   vs.

	patched # dmesg | grep printk:
	[    0.000000] printk: legacy bootconsole [uart8250] enabled
	[    0.000000] printk: debug: ignoring loglevel setting.
	[    0.018142] printk: log_buf_len: 1048576 bytes
	[    0.018757] printk: early log buf free: 259624(99%)
	[    0.160706] printk: legacy console [tty0] enabled
	[    0.161213] printk: legacy bootconsole [uart8250] disabled
	[    1.592929] printk: legacy console [ttyS0] enabled

   This is pretty bad because it would complicate or even prevent
   debugging of the boot stage via serial console.

   The graphical console is not usable when the system dies. Also
   finding the right arguments for the earlycon= parameter is
   tricky so that people enable it only when they have to debug
   very early messages.


I am going to look at the patches more closely to see if I could
provide some hints.

Best Regards,
Petr

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

[Tony Lindgren]

* Petr Mladek <pmladek@suse.com> [231201 14:36]:
> Well, my understanding is that it solves the problem only for the newly
> added console=DEVICENAME:0.0 format. But it does not handle the
> existing problems with matching console names passed via earlycon=
> and console= parameters. Am I right?

Yes that's where the remaining problems are.

> Now, the bad news. This patchset causes regressions which are
> not acceptable. I have found two so far but there might be more.
> 
> I used the following kernel command line:
> 
>    earlycon=uart8250,io,0x3f8,115200 console=ttyS0,115200 console=tty0 ignore_loglevel log_buf_len=1M
> 
> 
> 1. The patchset caused that /dev/console became associated with
>    ttyS0 instead of tty0, see the "C" flag:
> 
> 	original # cat /proc/consoles
> 	tty0                 -WU (EC    )    4:1
> 	ttyS0                -W- (E  p a)    4:64
> 
>    vs.
> 
> 	patched # cat /proc/consoles
> 	ttyS0                -W- (EC p a)    4:64
> 	tty0                 -WU (E     )    4:1
> 
>    This is most likely caused by the different ordering of
>    __add_preferred_console() calls.

Yes I noticed that too. We can't drop the console parsing from
console_setup() until we have some solution for flagging
register_console() that we do have a console specified on the
kernel command line and try_enable_default_console() should not
be called. It seems some changes to the console_set_on_cmdline
handling might do the trick here.

>    The ordering is important because it defines which console
>    will get associated with /dev/console. It is a so called
>    preferred console defined by the last console= parameter.
> 
>    Unfortunately also the ordering of the other parameters
>    is important when a console defined by the last console=
>    parameter is not registered at all. In this case,
>    /dev/console gets associated with the first console
>    with tty binding according to the order on the command line.
> 
>    If you think that it is weird behavior then I agree.
>    But it is a historical mess. It is how people used it
>    when the various features were added. Many changes
>    in this code caused regressions and had to be reverted.

Yeah agreed it's a mess :)

>    See the following to get the picture:
> 
>        + commit c6c7d83b9c9e6a8 ("Revert "console: don't
> 	 prefer first registered if DT specifies stdout-path")
> 
>        + commit dac8bbbae1d0ccb ("Revert "printk: fix double
> 	 printing with earlycon"").

OK thanks.

> 2. The serial console gets registered much later with this
>    patchset:
> 
> 	original # dmesg | grep printk:
> 	[    0.000000] printk: legacy bootconsole [uart8250] enabled
> 	[    0.000000] printk: debug: ignoring loglevel setting.
> 	[    0.016859] printk: log_buf_len: 1048576 bytes
> 	[    0.017324] printk: early log buf free: 259624(99%)
> 	[    0.141859] printk: legacy console [tty0] enabled
> 	[    0.142399] printk: legacy bootconsole [uart8250] disabled
> 	[    0.143032] printk: legacy console [ttyS0] enabled
> 
>    vs.
> 
> 	patched # dmesg | grep printk:
> 	[    0.000000] printk: legacy bootconsole [uart8250] enabled
> 	[    0.000000] printk: debug: ignoring loglevel setting.
> 	[    0.018142] printk: log_buf_len: 1048576 bytes
> 	[    0.018757] printk: early log buf free: 259624(99%)
> 	[    0.160706] printk: legacy console [tty0] enabled
> 	[    0.161213] printk: legacy bootconsole [uart8250] disabled
> 	[    1.592929] printk: legacy console [ttyS0] enabled
> 
>    This is pretty bad because it would complicate or even prevent
>    debugging of the boot stage via serial console.

I think I have a patch coming for 8250 isa ports for that issue.
This issue should go away if we call add_preferred_console_match()
from serial8250_isa_init_ports() with options for the port like
"ttyS0", "ttyS", 0.

>    The graphical console is not usable when the system dies. Also
>    finding the right arguments for the earlycon= parameter is
>    tricky so that people enable it only when they have to debug
>    very early messages.
> 
> 
> I am going to look at the patches more closely to see if I could
> provide some hints.

Great, help with the early console handling is much appreciated.

I'll post an updated patchset this week that does not touch
console_setup() beyond saving the console options. And then we
hopefully have something that avoids the regressions and can be
used for further changes later on.

Regards,

Tony

Re: [PATCH v3 3/3] serial: core: Move console character device handling from printk

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20231121113203.61341-4-tony@atomide.com>
References: <20231121113203.61341-4-tony@atomide.com>
TO: Tony Lindgren <tony@atomide.com>
TO: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
TO: Jiri Slaby <jirislaby@kernel.org>
TO: Petr Mladek <pmladek@suse.com>
TO: Steven Rostedt <rostedt@goodmis.org>
TO: John Ogness <john.ogness@linutronix.de>
TO: Sergey Senozhatsky <senozhatsky@chromium.org>
CC: "David S . Miller" <davem@davemloft.net>
CC: Andy Shevchenko <andriy.shevchenko@intel.com>
CC: Dhruva Gole <d-gole@ti.com>
CC: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
CC: Johan Hovold <johan@kernel.org>
CC: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
CC: Vignesh Raghavendra <vigneshr@ti.com>
CC: linux-kernel@vger.kernel.org
CC: linux-serial@vger.kernel.org

Hi Tony,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.7-rc3 next-20231201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tony-Lindgren/printk-Save-console-options-for-add_preferred_console_match/20231121-193809
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20231121113203.61341-4-tony%40atomide.com
patch subject: [PATCH v3 3/3] serial: core: Move console character device handling from printk
:::::: branch date: 11 days ago
:::::: commit date: 11 days ago
config: parisc-randconfig-r081-20231122 (https://download.01.org/0day-ci/archive/20231203/202312030049.kH4sh6rq-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231203/202312030049.kH4sh6rq-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202312030049.kH4sh6rq-lkp@intel.com/

smatch warnings:
drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
drivers/tty/serial/serial_base_bus.c:265 serial_base_add_preferred_console() error: uninitialized symbol 'char_match'.
drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.

vim +/nmbr_match +266 drivers/tty/serial/serial_base_bus.c

b1b8726ec3f40b Tony Lindgren 2023-11-21  245  
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  246  /*
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  247   * serial_base_add_preferred_console - Adds a preferred console
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  248   * @drv: Serial port device driver
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  249   * @port: Serial port instance
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  250   *
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  251   * Tries to add a preferred console for a serial port if specified in the
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  252   * kernel command line. Supports both the traditional character device such
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  253   * as console=ttyS0, and a hardware addressing based console=DEVNAME:0.0
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  254   * style name.
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  255   *
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  256   * Translates the kernel command line option using a hardware based addressing
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  257   * console=DEVNAME:0.0 to the serial port character device such as ttyS0.
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  258   *
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  259   * Note that duplicates are ignored by add_preferred_console().
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  260   */
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  261  int serial_base_add_preferred_console(struct uart_driver *drv,
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  262  				      struct uart_port *port)
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  263  {
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  264  	const char *port_match __free(kfree);
b1b8726ec3f40b Tony Lindgren 2023-11-21 @265  	const char *char_match __free(kfree);
b1b8726ec3f40b Tony Lindgren 2023-11-21 @266  	const char *nmbr_match __free(kfree);
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  267  	int ret;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  268  
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  269  	port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  270  			       port->ctrl_id, port->port_id);
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  271  	if (!port_match)
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  272  		return -ENOMEM;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  273  
b1b8726ec3f40b Tony Lindgren 2023-11-21  274  	char_match = kasprintf(GFP_KERNEL, "%s%i", drv->dev_name, port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21  275  	if (!char_match)
b1b8726ec3f40b Tony Lindgren 2023-11-21  276  		return -ENOMEM;
b1b8726ec3f40b Tony Lindgren 2023-11-21  277  
b1b8726ec3f40b Tony Lindgren 2023-11-21  278  	/* Handle ttyS specific options */
b1b8726ec3f40b Tony Lindgren 2023-11-21  279  	if (!strncmp(drv->dev_name, "ttyS", 4)) {
b1b8726ec3f40b Tony Lindgren 2023-11-21  280  		/* No name, just a number */
b1b8726ec3f40b Tony Lindgren 2023-11-21  281  		nmbr_match = kasprintf(GFP_KERNEL, "%i", port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21  282  		if (!nmbr_match)
b1b8726ec3f40b Tony Lindgren 2023-11-21  283  			return -ENODEV;
b1b8726ec3f40b Tony Lindgren 2023-11-21  284  
b1b8726ec3f40b Tony Lindgren 2023-11-21  285  		ret = add_preferred_console_match(nmbr_match, drv->dev_name,
b1b8726ec3f40b Tony Lindgren 2023-11-21  286  						  port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21  287  		if (ret && ret != -ENOENT)
b1b8726ec3f40b Tony Lindgren 2023-11-21  288  			return ret;
b1b8726ec3f40b Tony Lindgren 2023-11-21  289  
b1b8726ec3f40b Tony Lindgren 2023-11-21  290  		/* Sparc ttya and ttyb */
b1b8726ec3f40b Tony Lindgren 2023-11-21  291  		ret = serial_base_add_sparc_console(drv, port);
b1b8726ec3f40b Tony Lindgren 2023-11-21  292  		if (ret)
b1b8726ec3f40b Tony Lindgren 2023-11-21  293  			return ret;
b1b8726ec3f40b Tony Lindgren 2023-11-21  294  	}
b1b8726ec3f40b Tony Lindgren 2023-11-21  295  
b1b8726ec3f40b Tony Lindgren 2023-11-21  296  	/* Handle the traditional character device name style console=ttyS0 */
b1b8726ec3f40b Tony Lindgren 2023-11-21  297  	ret = add_preferred_console_match(char_match, drv->dev_name, port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21  298  	if (ret && ret != -ENOENT)
b1b8726ec3f40b Tony Lindgren 2023-11-21  299  		return ret;
b1b8726ec3f40b Tony Lindgren 2023-11-21  300  
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  301  	/* Translate a hardware addressing style console=DEVNAME:0.0 */
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  302  	ret = add_preferred_console_match(port_match, drv->dev_name, port->line);
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  303  	if (ret && ret != -ENOENT)
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  304  		return ret;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  305  
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  306  	return 0;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  307  }
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  308  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 3/3] serial: core: Move console character device handling from printk

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20231121113203.61341-4-tony@atomide.com>
References: <20231121113203.61341-4-tony@atomide.com>
TO: Tony Lindgren <tony@atomide.com>
TO: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
TO: Jiri Slaby <jirislaby@kernel.org>
TO: Petr Mladek <pmladek@suse.com>
TO: Steven Rostedt <rostedt@goodmis.org>
TO: John Ogness <john.ogness@linutronix.de>
TO: Sergey Senozhatsky <senozhatsky@chromium.org>
CC: "David S . Miller" <davem@davemloft.net>
CC: Andy Shevchenko <andriy.shevchenko@intel.com>
CC: Dhruva Gole <d-gole@ti.com>
CC: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
CC: Johan Hovold <johan@kernel.org>
CC: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
CC: Vignesh Raghavendra <vigneshr@ti.com>
CC: linux-kernel@vger.kernel.org
CC: linux-serial@vger.kernel.org

Hi Tony,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.7-rc2 next-20231122]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tony-Lindgren/printk-Save-console-options-for-add_preferred_console_match/20231121-193809
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20231121113203.61341-4-tony%40atomide.com
patch subject: [PATCH v3 3/3] serial: core: Move console character device handling from printk
:::::: branch date: 19 hours ago
:::::: commit date: 19 hours ago
config: parisc-randconfig-r081-20231122 (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202311221437.5Gil0Pml-lkp@intel.com/

smatch warnings:
drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
drivers/tty/serial/serial_base_bus.c:265 serial_base_add_preferred_console() error: uninitialized symbol 'char_match'.
drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.

vim +/nmbr_match +266 drivers/tty/serial/serial_base_bus.c

b1b8726ec3f40b Tony Lindgren 2023-11-21  245  
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  246  /*
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  247   * serial_base_add_preferred_console - Adds a preferred console
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  248   * @drv: Serial port device driver
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  249   * @port: Serial port instance
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  250   *
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  251   * Tries to add a preferred console for a serial port if specified in the
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  252   * kernel command line. Supports both the traditional character device such
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  253   * as console=ttyS0, and a hardware addressing based console=DEVNAME:0.0
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  254   * style name.
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  255   *
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  256   * Translates the kernel command line option using a hardware based addressing
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  257   * console=DEVNAME:0.0 to the serial port character device such as ttyS0.
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  258   *
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  259   * Note that duplicates are ignored by add_preferred_console().
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  260   */
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  261  int serial_base_add_preferred_console(struct uart_driver *drv,
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  262  				      struct uart_port *port)
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  263  {
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  264  	const char *port_match __free(kfree);
b1b8726ec3f40b Tony Lindgren 2023-11-21 @265  	const char *char_match __free(kfree);
b1b8726ec3f40b Tony Lindgren 2023-11-21 @266  	const char *nmbr_match __free(kfree);
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  267  	int ret;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  268  
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  269  	port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  270  			       port->ctrl_id, port->port_id);
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  271  	if (!port_match)
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  272  		return -ENOMEM;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  273  
b1b8726ec3f40b Tony Lindgren 2023-11-21  274  	char_match = kasprintf(GFP_KERNEL, "%s%i", drv->dev_name, port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21  275  	if (!char_match)
b1b8726ec3f40b Tony Lindgren 2023-11-21  276  		return -ENOMEM;
b1b8726ec3f40b Tony Lindgren 2023-11-21  277  
b1b8726ec3f40b Tony Lindgren 2023-11-21  278  	/* Handle ttyS specific options */
b1b8726ec3f40b Tony Lindgren 2023-11-21  279  	if (!strncmp(drv->dev_name, "ttyS", 4)) {
b1b8726ec3f40b Tony Lindgren 2023-11-21  280  		/* No name, just a number */
b1b8726ec3f40b Tony Lindgren 2023-11-21  281  		nmbr_match = kasprintf(GFP_KERNEL, "%i", port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21  282  		if (!nmbr_match)
b1b8726ec3f40b Tony Lindgren 2023-11-21  283  			return -ENODEV;
b1b8726ec3f40b Tony Lindgren 2023-11-21  284  
b1b8726ec3f40b Tony Lindgren 2023-11-21  285  		ret = add_preferred_console_match(nmbr_match, drv->dev_name,
b1b8726ec3f40b Tony Lindgren 2023-11-21  286  						  port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21  287  		if (ret && ret != -ENOENT)
b1b8726ec3f40b Tony Lindgren 2023-11-21  288  			return ret;
b1b8726ec3f40b Tony Lindgren 2023-11-21  289  
b1b8726ec3f40b Tony Lindgren 2023-11-21  290  		/* Sparc ttya and ttyb */
b1b8726ec3f40b Tony Lindgren 2023-11-21  291  		ret = serial_base_add_sparc_console(drv, port);
b1b8726ec3f40b Tony Lindgren 2023-11-21  292  		if (ret)
b1b8726ec3f40b Tony Lindgren 2023-11-21  293  			return ret;
b1b8726ec3f40b Tony Lindgren 2023-11-21  294  	}
b1b8726ec3f40b Tony Lindgren 2023-11-21  295  
b1b8726ec3f40b Tony Lindgren 2023-11-21  296  	/* Handle the traditional character device name style console=ttyS0 */
b1b8726ec3f40b Tony Lindgren 2023-11-21  297  	ret = add_preferred_console_match(char_match, drv->dev_name, port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21  298  	if (ret && ret != -ENOENT)
b1b8726ec3f40b Tony Lindgren 2023-11-21  299  		return ret;
b1b8726ec3f40b Tony Lindgren 2023-11-21  300  
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  301  	/* Translate a hardware addressing style console=DEVNAME:0.0 */
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  302  	ret = add_preferred_console_match(port_match, drv->dev_name, port->line);
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  303  	if (ret && ret != -ENOENT)
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  304  		return ret;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  305  
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  306  	return 0;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  307  }
e4ebdcd790e0f3 Tony Lindgren 2023-11-21  308  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki