On Tue, Sep 12, 2023 at 02:03:44PM +0300, Tony Lindgren wrote:
> We can now add hardware based addressing to serial ports. Starting with
> commit 84a9582fd203 ("serial: core: Start managing serial controllers to
> enable runtime PM"), and all the related fixes to this commit, the serial
> core now knows to which serial port controller the ports are connected.
>
> The serial ports can be addressed with DEVNAME:0.0 style naming. The names
> are something like 00:04:0.0 for a serial port on qemu, and something like
> 2800000.serial:0.0 on platform device using systems like ARM64 for example.
>
> The DEVNAME is the unique serial port hardware controller device name, AKA
> the name for port->dev. The 0.0 are the serial core controller id and port
> id.
>
> Typically 0.0 are used for each controller and port instance unless the
> serial port hardware controller has multiple controllers or ports.
>
> Using DEVNAME:0.0 style naming actually solves two long term issues for
> addressing the serial ports:
>
> 1. According to Andy Shevchenko, using DEVNAME:0.0 style naming fixes an
> issue where depending on the BIOS settings, the kernel serial port ttyS
> instance number may change if HSUART is enabled
>
> 2. Device tree using architectures no longer necessarily need to specify
> aliases to find a specific serial port, and we can just allocate the
> ttyS instance numbers dynamically in whatever probe order
>
> To do this, we need a custom init time parser for the console= command
> line option as printk already handles parsing it with console_setup().
> Also early_param() gets handled by console_setup() if "console" and
> "earlycon" are used.
...
> +#ifdef CONFIG_SERIAL_CORE_CONSOLE
> +int serial_base_add_preferred_console(struct uart_driver *drv,
> + struct uart_port *port);
> +#else
> +static inline int serial_base_add_preferred_console(struct uart_driver *drv,
> + struct uart_port *port)
Maybe
static inline
int serial_base_add_preferred_console(struct uart_driver *drv,
struct uart_port *port)
for being aligned with the above?
> +{
> + return 0;
> +}
> +#endif
...
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/kernel.h>
Hmm... Can we use better header(s) instead?
types.h, etc?
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
...
> +static LIST_HEAD(serial_base_consoles);
Don't you need a locking to access this list?
If not, perhaps a comment why it's okay?
...
> +int serial_base_add_preferred_console(struct uart_driver *drv,
> + struct uart_port *port)
> +{
> + struct serial_base_console *entry;
> + char *port_match;
...
> + port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
> + port->ctrl_id, port->port_id);
What about starting using cleanup.h?
> + if (!port_match)
> + return -ENOMEM;
> +
> + list_for_each_entry(entry, &serial_base_consoles, node) {
> + if (!strcmp(port_match, entry->name)) {
> + add_preferred_console(drv->dev_name, port->line,
> + entry->opt);
> + break;
> + }
> + }
> +
> + kfree(port_match);
Also (with the above) this can be written as
list_for_each_entry(entry, &serial_base_consoles, node) {
if (!strcmp(port_match, entry->name))
break;
}
if (list_entry_is_head(entry, &serial_base_consoles, node)
return 0; // Hmm... it maybe -ENOENT, but okay.
add_preferred_console(drv->dev_name, port->line, entry->opt);
> + return 0;
> +}
...
> +EXPORT_SYMBOL_GPL(serial_base_add_preferred_console);
Can we use (start using) namespaced exports?
...
> +static int __init serial_base_add_con(char *name, char *opt)
const name
const opt
?
> +{
> + struct serial_base_console *con;
> +
> + con = kzalloc(sizeof(*con), GFP_KERNEL);
> + if (!con)
> + return -ENOMEM;
> +
> + con->name = kstrdup(name, GFP_KERNEL);
> + if (!con->name)
> + goto free_con;
> +
> + if (opt) {
> + con->opt = kstrdup(opt, GFP_KERNEL);
> + if (!con->name)
Are you sure? I think it's c&p typo here.
> + goto free_name;
> + }
> +
> + list_add_tail(&con->node, &serial_base_consoles);
> +
> + return 0;
> +
> +free_name:
> + kfree(con->name);
> +
> +free_con:
> + kfree(con);
With cleanup.h this will look much better.
> + return -ENOMEM;
> +}
...
> +static int __init serial_base_parse_one(char *param, char *val,
> + const char *unused, void *arg)
> +{
> + char *opt;
> +
> + if (strcmp(param, "console"))
> + return 0;
> +
> + if (!val)
> + return 0;
> + opt = strchr(val, ',');
> + if (opt) {
> + opt[0] = '\0';
> + opt++;
> + }
strsep() ?
Actually param_array() uses strcspn() in similar situation.
> + if (!strlen(val))
> + return 0;
Btw, have you seen lib/cmdline.c? Can it be helpful here?
> + return serial_base_add_con(val, opt);
> +}
--
With Best Regards,
Andy Shevchenko
