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
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 <[email protected]>
---
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 <[email protected]>
+ */
+
+#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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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
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
* Andy Shevchenko <[email protected]> [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
* Tony Lindgren <[email protected]> [231122 08:18]:
> * Andy Shevchenko <[email protected]> [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
* Andy Shevchenko <[email protected]> [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
* Tony Lindgren <[email protected]> [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
* Tony Lindgren <[email protected]> [231122 09:04]:
> * Tony Lindgren <[email protected]> [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
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/[email protected]/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231122/[email protected]/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 <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/
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
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/[email protected]/config)
> compiler: hppa-linux-gcc (GCC) 13.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20231122/[email protected]/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 <[email protected]>
> | Reported-by: Dan Carpenter <[email protected]>
> | Closes: https://lore.kernel.org/r/[email protected]/
>
> 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
* Tony Lindgren <[email protected]> [231122 10:16]:
> * Tony Lindgren <[email protected]> [231122 09:04]:
> > * Tony Lindgren <[email protected]> [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
* Dan Carpenter <[email protected]> [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/[email protected]/config)
> > compiler: hppa-linux-gcc (GCC) 13.2.0
> > reproduce: (https://download.01.org/0day-ci/archive/20231122/[email protected]/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 <[email protected]>
> > | Reported-by: Dan Carpenter <[email protected]>
> > | Closes: https://lore.kernel.org/r/[email protected]/
> >
> > 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
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
* Petr Mladek <[email protected]> [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