[v2,2/3] serial: core: Add support for DEVNAME:0.0 style naming for kernel console
Commit Message
We can now add hardware based addressing to serial ports. Starting with
commit 84a9582fd203 ("serial: core: Start managing serial controllers to
enable runtime PM"), and all the related fixes to this commit, the serial
core now knows to which serial port controller the ports are connected.
The serial ports can be addressed with DEVNAME:0.0 style naming. The names
are something like 00:04:0.0 for a serial port on qemu, and something like
2800000.serial:0.0 on platform device using systems like ARM64 for example.
The DEVNAME is the unique serial port hardware controller device name, AKA
the name for port->dev. The 0.0 are the serial core controller id and port
id.
Typically 0.0 are used for each controller and port instance unless the
serial port hardware controller has multiple controllers or ports.
Using DEVNAME:0.0 style naming actually solves two long term issues for
addressing the serial ports:
1. According to Andy Shevchenko, using DEVNAME:0.0 style naming fixes an
issue where depending on the BIOS settings, the kernel serial port ttyS
instance number may change if HSUART is enabled
2. Device tree using architectures no longer necessarily need to specify
aliases to find a specific serial port, and we can just allocate the
ttyS instance numbers dynamically in whatever probe order
To do this, we need a custom init time parser for the console= command
line option as printk already handles parsing it with console_setup().
Also early_param() gets handled by console_setup() if "console" and
"earlycon" are used.
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
drivers/tty/serial/Makefile | 3 +
drivers/tty/serial/serial_base.h | 11 +++
drivers/tty/serial/serial_base_con.c | 133 +++++++++++++++++++++++++++
drivers/tty/serial/serial_core.c | 4 +
4 files changed, 151 insertions(+)
create mode 100644 drivers/tty/serial/serial_base_con.c
Comments
On Tue, 12 Sep 2023, Tony Lindgren wrote:
> We can now add hardware based addressing to serial ports. Starting with
> commit 84a9582fd203 ("serial: core: Start managing serial controllers to
> enable runtime PM"), and all the related fixes to this commit, the serial
> core now knows to which serial port controller the ports are connected.
>
> The serial ports can be addressed with DEVNAME:0.0 style naming. The names
> are something like 00:04:0.0 for a serial port on qemu, and something like
> 2800000.serial:0.0 on platform device using systems like ARM64 for example.
>
> The DEVNAME is the unique serial port hardware controller device name, AKA
> the name for port->dev. The 0.0 are the serial core controller id and port
> id.
>
> Typically 0.0 are used for each controller and port instance unless the
> serial port hardware controller has multiple controllers or ports.
>
> Using DEVNAME:0.0 style naming actually solves two long term issues for
> addressing the serial ports:
>
> 1. According to Andy Shevchenko, using DEVNAME:0.0 style naming fixes an
> issue where depending on the BIOS settings, the kernel serial port ttyS
> instance number may change if HSUART is enabled
>
> 2. Device tree using architectures no longer necessarily need to specify
> aliases to find a specific serial port, and we can just allocate the
> ttyS instance numbers dynamically in whatever probe order
>
> To do this, we need a custom init time parser for the console= command
> line option as printk already handles parsing it with console_setup().
> Also early_param() gets handled by console_setup() if "console" and
> "earlycon" are used.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> drivers/tty/serial/Makefile | 3 +
> drivers/tty/serial/serial_base.h | 11 +++
> drivers/tty/serial/serial_base_con.c | 133 +++++++++++++++++++++++++++
> drivers/tty/serial/serial_core.c | 4 +
> 4 files changed, 151 insertions(+)
> create mode 100644 drivers/tty/serial/serial_base_con.c
>
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -3,6 +3,9 @@
> # Makefile for the kernel serial device drivers.
> #
>
> +# Parse kernel command line consoles before the serial drivers probe
> +obj-$(CONFIG_SERIAL_CORE_CONSOLE) += serial_base_con.o
> +
> obj-$(CONFIG_SERIAL_CORE) += serial_base.o
> serial_base-y := serial_core.o serial_base_bus.o serial_ctrl.o serial_port.o
>
> diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h
> --- a/drivers/tty/serial/serial_base.h
> +++ b/drivers/tty/serial/serial_base.h
> @@ -45,3 +45,14 @@ void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port
>
> int serial_core_register_port(struct uart_driver *drv, struct uart_port *port);
> void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port);
> +
> +#ifdef CONFIG_SERIAL_CORE_CONSOLE
> +int serial_base_add_preferred_console(struct uart_driver *drv,
> + struct uart_port *port);
> +#else
> +static inline int serial_base_add_preferred_console(struct uart_driver *drv,
> + struct uart_port *port)
> +{
> + return 0;
> +}
> +#endif
> diff --git a/drivers/tty/serial/serial_base_con.c b/drivers/tty/serial/serial_base_con.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base_con.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Serial base console options handling
> + *
> + * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
> + * Author: Tony Lindgren <tony@atomide.com>
> + */
> +
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/kernel.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +
> +#include "serial_base.h"
> +
> +static LIST_HEAD(serial_base_consoles);
> +
> +struct serial_base_console {
> + struct list_head node;
> + char *name;
Can't this be const char as too?
On Tue, Sep 12, 2023 at 02:03:44PM +0300, Tony Lindgren wrote:
> We can now add hardware based addressing to serial ports. Starting with
> commit 84a9582fd203 ("serial: core: Start managing serial controllers to
> enable runtime PM"), and all the related fixes to this commit, the serial
> core now knows to which serial port controller the ports are connected.
>
> The serial ports can be addressed with DEVNAME:0.0 style naming. The names
> are something like 00:04:0.0 for a serial port on qemu, and something like
> 2800000.serial:0.0 on platform device using systems like ARM64 for example.
>
> The DEVNAME is the unique serial port hardware controller device name, AKA
> the name for port->dev. The 0.0 are the serial core controller id and port
> id.
>
> Typically 0.0 are used for each controller and port instance unless the
> serial port hardware controller has multiple controllers or ports.
>
> Using DEVNAME:0.0 style naming actually solves two long term issues for
> addressing the serial ports:
>
> 1. According to Andy Shevchenko, using DEVNAME:0.0 style naming fixes an
> issue where depending on the BIOS settings, the kernel serial port ttyS
> instance number may change if HSUART is enabled
>
> 2. Device tree using architectures no longer necessarily need to specify
> aliases to find a specific serial port, and we can just allocate the
> ttyS instance numbers dynamically in whatever probe order
>
> To do this, we need a custom init time parser for the console= command
> line option as printk already handles parsing it with console_setup().
> Also early_param() gets handled by console_setup() if "console" and
> "earlycon" are used.
...
> +#ifdef CONFIG_SERIAL_CORE_CONSOLE
> +int serial_base_add_preferred_console(struct uart_driver *drv,
> + struct uart_port *port);
> +#else
> +static inline int serial_base_add_preferred_console(struct uart_driver *drv,
> + struct uart_port *port)
Maybe
static inline
int serial_base_add_preferred_console(struct uart_driver *drv,
struct uart_port *port)
for being aligned with the above?
> +{
> + return 0;
> +}
> +#endif
...
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/kernel.h>
Hmm... Can we use better header(s) instead?
types.h, etc?
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
...
> +static LIST_HEAD(serial_base_consoles);
Don't you need a locking to access this list?
If not, perhaps a comment why it's okay?
...
> +int serial_base_add_preferred_console(struct uart_driver *drv,
> + struct uart_port *port)
> +{
> + struct serial_base_console *entry;
> + char *port_match;
...
> + port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
> + port->ctrl_id, port->port_id);
What about starting using cleanup.h?
> + if (!port_match)
> + return -ENOMEM;
> +
> + list_for_each_entry(entry, &serial_base_consoles, node) {
> + if (!strcmp(port_match, entry->name)) {
> + add_preferred_console(drv->dev_name, port->line,
> + entry->opt);
> + break;
> + }
> + }
> +
> + kfree(port_match);
Also (with the above) this can be written as
list_for_each_entry(entry, &serial_base_consoles, node) {
if (!strcmp(port_match, entry->name))
break;
}
if (list_entry_is_head(entry, &serial_base_consoles, node)
return 0; // Hmm... it maybe -ENOENT, but okay.
add_preferred_console(drv->dev_name, port->line, entry->opt);
> + return 0;
> +}
...
> +EXPORT_SYMBOL_GPL(serial_base_add_preferred_console);
Can we use (start using) namespaced exports?
...
> +static int __init serial_base_add_con(char *name, char *opt)
const name
const opt
?
> +{
> + struct serial_base_console *con;
> +
> + con = kzalloc(sizeof(*con), GFP_KERNEL);
> + if (!con)
> + return -ENOMEM;
> +
> + con->name = kstrdup(name, GFP_KERNEL);
> + if (!con->name)
> + goto free_con;
> +
> + if (opt) {
> + con->opt = kstrdup(opt, GFP_KERNEL);
> + if (!con->name)
Are you sure? I think it's c&p typo here.
> + goto free_name;
> + }
> +
> + list_add_tail(&con->node, &serial_base_consoles);
> +
> + return 0;
> +
> +free_name:
> + kfree(con->name);
> +
> +free_con:
> + kfree(con);
With cleanup.h this will look much better.
> + return -ENOMEM;
> +}
...
> +static int __init serial_base_parse_one(char *param, char *val,
> + const char *unused, void *arg)
> +{
> + char *opt;
> +
> + if (strcmp(param, "console"))
> + return 0;
> +
> + if (!val)
> + return 0;
> + opt = strchr(val, ',');
> + if (opt) {
> + opt[0] = '\0';
> + opt++;
> + }
strsep() ?
Actually param_array() uses strcspn() in similar situation.
> + if (!strlen(val))
> + return 0;
Btw, have you seen lib/cmdline.c? Can it be helpful here?
> + return serial_base_add_con(val, opt);
> +}
* Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> [230912 12:24]:
> On Tue, 12 Sep 2023, Tony Lindgren wrote:
> > +struct serial_base_console {
> > + struct list_head node;
> > + char *name;
>
> Can't this be const char as too?
Yes thanks,
Tony
* Andy Shevchenko <andriy.shevchenko@intel.com> [230912 15:07]:
> On Tue, Sep 12, 2023 at 02:03:44PM +0300, Tony Lindgren wrote:
> > +static LIST_HEAD(serial_base_consoles);
>
> Don't you need a locking to access this list?
> If not, perhaps a comment why it's okay?
It's updated at arch_initcall() time only, I'll add a comment.
> > + port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
> > + port->ctrl_id, port->port_id);
>
> What about starting using cleanup.h?
OK seems to simplify things nicely :)
> > +EXPORT_SYMBOL_GPL(serial_base_add_preferred_console);
>
> Can we use (start using) namespaced exports?
Sorry forgot about the namespace stuff already..
> ...
>
> > +static int __init serial_base_add_con(char *name, char *opt)
>
> const name
> const opt
> ?
For name yes, opt has issues as noted in the first patch in this
series.
> > + opt = strchr(val, ',');
> > + if (opt) {
> > + opt[0] = '\0';
> > + opt++;
> > + }
>
> strsep() ?
>
> Actually param_array() uses strcspn() in similar situation.
OK I'll change to use strcspn().
> > + if (!strlen(val))
> > + return 0;
>
> Btw, have you seen lib/cmdline.c? Can it be helpful here?
I don't think so as at this point we don't have param=value
pairs and param is the port name.
Will fix up the rest of the stuff you commented too thanks.
Regards,
Tony
On 12. 09. 23, 13:03, Tony Lindgren wrote:
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base_con.c
...
> +/* Adds a command line console to the list of consoles for driver probe time */
> +static int __init serial_base_add_con(char *name, char *opt)
> +{
> + struct serial_base_console *con;
> +
> + con = kzalloc(sizeof(*con), GFP_KERNEL);
> + if (!con)
> + return -ENOMEM;
> +
> + con->name = kstrdup(name, GFP_KERNEL);
> + if (!con->name)
> + goto free_con;
> +
> + if (opt) {
> + con->opt = kstrdup(opt, GFP_KERNEL);
> + if (!con->name)
con->opt
> + goto free_name;
> + }
> +
> + list_add_tail(&con->node, &serial_base_consoles);
> +
> + return 0;
> +
> +free_name:
> + kfree(con->name);
> +
> +free_con:
> + kfree(con);
> +
> + return -ENOMEM;
> +}
> +
> +/* Parse console name and options */
> +static int __init serial_base_parse_one(char *param, char *val,
> + const char *unused, void *arg)
> +{
> + char *opt;
> +
> + if (strcmp(param, "console"))
> + return 0;
> +
> + if (!val)
> + return 0;
> +
> + opt = strchr(val, ',');
> + if (opt) {
> + opt[0] = '\0';
> + opt++;
> + }
Can this be done without mangling val, i.e. without kstrdup below?
> + if (!strlen(val))
IOW, can this check be "val - opt > 0" or alike?
> + return 0;
> +
> + return serial_base_add_con(val, opt);
> +}
> +
> +/*
> + * The "console=" option is handled by console_setup() in printk. We can't use
> + * early_param() as do_early_param() checks for "console" and "earlycon" options
> + * so console_setup() potentially handles console also early. Use parse_args().
So why not concentrate console= handling on one place, ie. in
console_setup()? The below (second time console= handling) occurs quite
illogical to me.
> + */
> +static int __init serial_base_opts_init(void)
> +{
> + char *command_line;
> +
> + command_line = kstrdup(boot_command_line, GFP_KERNEL);
> + if (!command_line)
> + return -ENOMEM;
> +
> + parse_args("Setting serial core console", command_line,
> + NULL, 0, -1, -1, NULL, serial_base_parse_one);
> +
> + kfree(command_line);
> +
> + return 0;
> +}
thanks,
* Jiri Slaby <jirislaby@kernel.org> [230914 05:43]:
> On 12. 09. 23, 13:03, Tony Lindgren wrote:
> > +/*
> > + * The "console=" option is handled by console_setup() in printk. We can't use
> > + * early_param() as do_early_param() checks for "console" and "earlycon" options
> > + * so console_setup() potentially handles console also early. Use parse_args().
>
> So why not concentrate console= handling on one place, ie. in
> console_setup()? The below (second time console= handling) occurs quite
> illogical to me.
Well console_setup() knows nothing about the probing serial port controller
device, tries to call __add_preferred_console() based on a few hardcoded
device names and some attempted guessing, and is stuffed into printk.c :)
I don't think we should pile on more stuff into printk.c for this.
If we wanted to do something, let's set up the console list somewhere else,
and then just have console_setup() add every console option to that list
and leave the rest of console_setup in place to avoid breaking things all
over the place.
Then we can export some find_named_console() type function for serial core
to use. Or do you have some better ideas in mind?
Regards,
Tony
@@ -3,6 +3,9 @@
# Makefile for the kernel serial device drivers.
#
+# Parse kernel command line consoles before the serial drivers probe
+obj-$(CONFIG_SERIAL_CORE_CONSOLE) += serial_base_con.o
+
obj-$(CONFIG_SERIAL_CORE) += serial_base.o
serial_base-y := serial_core.o serial_base_bus.o serial_ctrl.o serial_port.o
@@ -45,3 +45,14 @@ void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port
int serial_core_register_port(struct uart_driver *drv, struct uart_port *port);
void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port);
+
+#ifdef CONFIG_SERIAL_CORE_CONSOLE
+int serial_base_add_preferred_console(struct uart_driver *drv,
+ struct uart_port *port);
+#else
+static inline int serial_base_add_preferred_console(struct uart_driver *drv,
+ struct uart_port *port)
+{
+ return 0;
+}
+#endif
new file mode 100644
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Serial base console options handling
+ *
+ * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
+ * Author: Tony Lindgren <tony@atomide.com>
+ */
+
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/kernel.h>
+#include <linux/serial_core.h>
+#include <linux/slab.h>
+
+#include "serial_base.h"
+
+static LIST_HEAD(serial_base_consoles);
+
+struct serial_base_console {
+ struct list_head node;
+ char *name;
+ char *opt;
+};
+
+/*
+ * Adds a preferred console for a serial port if console=DEVNAME:0.0
+ * style addressing is used for the kernel command line. Translates
+ * from DEVNAME:0.0 to port->dev_name such as ttyS. Duplicates are
+ * ignored by add_preferred_console().
+ */
+int serial_base_add_preferred_console(struct uart_driver *drv,
+ struct uart_port *port)
+{
+ struct serial_base_console *entry;
+ char *port_match;
+
+ port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
+ port->ctrl_id, port->port_id);
+ if (!port_match)
+ return -ENOMEM;
+
+ list_for_each_entry(entry, &serial_base_consoles, node) {
+ if (!strcmp(port_match, entry->name)) {
+ add_preferred_console(drv->dev_name, port->line,
+ entry->opt);
+ break;
+ }
+ }
+
+ kfree(port_match);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(serial_base_add_preferred_console);
+
+/* Adds a command line console to the list of consoles for driver probe time */
+static int __init serial_base_add_con(char *name, char *opt)
+{
+ struct serial_base_console *con;
+
+ con = kzalloc(sizeof(*con), GFP_KERNEL);
+ if (!con)
+ return -ENOMEM;
+
+ con->name = kstrdup(name, GFP_KERNEL);
+ if (!con->name)
+ goto free_con;
+
+ if (opt) {
+ con->opt = kstrdup(opt, GFP_KERNEL);
+ if (!con->name)
+ goto free_name;
+ }
+
+ list_add_tail(&con->node, &serial_base_consoles);
+
+ return 0;
+
+free_name:
+ kfree(con->name);
+
+free_con:
+ kfree(con);
+
+ return -ENOMEM;
+}
+
+/* Parse console name and options */
+static int __init serial_base_parse_one(char *param, char *val,
+ const char *unused, void *arg)
+{
+ char *opt;
+
+ if (strcmp(param, "console"))
+ return 0;
+
+ if (!val)
+ return 0;
+
+ opt = strchr(val, ',');
+ if (opt) {
+ opt[0] = '\0';
+ opt++;
+ }
+
+ if (!strlen(val))
+ return 0;
+
+ return serial_base_add_con(val, opt);
+}
+
+/*
+ * The "console=" option is handled by console_setup() in printk. We can't use
+ * early_param() as do_early_param() checks for "console" and "earlycon" options
+ * so console_setup() potentially handles console also early. Use parse_args().
+ */
+static int __init serial_base_opts_init(void)
+{
+ char *command_line;
+
+ command_line = kstrdup(boot_command_line, GFP_KERNEL);
+ if (!command_line)
+ return -ENOMEM;
+
+ parse_args("Setting serial core console", command_line,
+ NULL, 0, -1, -1, NULL, serial_base_parse_one);
+
+ kfree(command_line);
+
+ return 0;
+}
+
+arch_initcall(serial_base_opts_init);
@@ -3358,6 +3358,10 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
if (ret)
goto err_unregister_ctrl_dev;
+ ret = serial_base_add_preferred_console(drv, port);
+ if (ret)
+ goto err_unregister_port_dev;
+
ret = serial_core_add_one_port(drv, port);
if (ret)
goto err_unregister_port_dev;