Received-SPF: pass (google.com: domain of linux-kernel+bounces-122463-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 28 Mar 2024 12:01:52 +0530
From: Dhruva Gole <d-gole@ti.com>
To: Tony Lindgren <tony@atomide.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby
	<jirislaby@kernel.org>, Jonathan Corbet <corbet@lwn.net>,
        Petr Mladek
	<pmladek@suse.com>, Steven Rostedt <rostedt@goodmis.org>,
        John Ogness
	<john.ogness@linutronix.de>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        "David S . Miller" <davem@davemloft.net>,
        Andy Shevchenko
	<andriy.shevchenko@linux.intel.com>,
        Ilpo =?utf-8?B?SsOkcnZpbmVu?=
	<ilpo.jarvinen@linux.intel.com>,
        Johan Hovold <johan@kernel.org>,
        Sebastian
 Andrzej Siewior <bigeasy@linutronix.de>,
        Vignesh Raghavendra
	<vigneshr@ti.com>, <linux-kernel@vger.kernel.org>,
        <linux-serial@vger.kernel.org>, Sebastian Reichel <sre@kernel.org>,
        <linux-doc@vger.kernel.org>
Subject: Re: [PATCH v7 4/7] serial: core: Add support for DEVNAME:0.0 style
 naming for kernel console
Message-ID: <20240328063152.bjkdtdsu42cqbgf3@dhruva>
References: <20240327110021.59793-1-tony@atomide.com>
 <20240327110021.59793-5-tony@atomide.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240327110021.59793-5-tony@atomide.com>

Hi,

On Mar 27, 2024 at 12:59:38 +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

This is GOOD!

>    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 name.
> 
> Note that when using console=DEVNAME:0.0 style kernel command line, the
> 8250 serial console gets enabled later compared to using console=ttyS
> naming for ISA ports. This is because the serial port DEVNAME to character
> device mapping is not known until the serial driver probe time. If used
> together with earlycon, this issue is avoided.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/tty/serial/serial_base.h     | 16 +++++++
>  drivers/tty/serial/serial_base_bus.c | 66 ++++++++++++++++++++++++++++
>  drivers/tty/serial/serial_core.c     |  4 ++
>  3 files changed, 86 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,19 @@ 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
> @@ -8,6 +8,7 @@
>   * The serial core bus manages the serial core controller instances.
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/container_of.h>
>  #include <linux/device.h>
>  #include <linux/idr.h>
> @@ -204,6 +205,71 @@ void serial_base_port_device_remove(struct serial_port_device *port_dev)
>  	put_device(&port_dev->dev);
>  }
>  
> +#ifdef CONFIG_SERIAL_CORE_CONSOLE
> +
> +static int serial_base_add_one_prefcon(const char *match, const char *dev_name,
> +				       int port_id)
> +{
> +	int ret;
> +
> +	ret = add_preferred_console_match(match, dev_name, port_id);
> +	if (ret == -ENOENT)
> +		return 0;
> +
> +	return ret;

Can we do this instead?
return (ret == -ENOENT ? 0 : ret);

> +}
> +
> +static int serial_base_add_prefcon(const char *name, int idx)
> +{
> +	const char *char_match __free(kfree) = NULL;
> +
> +	/* Handle the traditional character device name style console=ttyS0 */
> +	char_match = kasprintf(GFP_KERNEL, "%s%i", name, idx);
> +	if (!char_match)
> +		return -ENOMEM;
> +
> +	return serial_base_add_one_prefcon(char_match, name, idx);
> +}
> +
> +/**
> + * 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.
> + * Cannot be called early for ISA ports, depends on struct device.
> + *
> + * Note that duplicates are ignored by add_preferred_console().
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int serial_base_add_preferred_console(struct uart_driver *drv,
> +				      struct uart_port *port)
> +{
> +	const char *port_match __free(kfree) = NULL;
> +	int ret;
> +
> +	ret = serial_base_add_prefcon(drv->dev_name, port->line);
> +	if (ret)
> +		return 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 */
> +	return serial_base_add_one_prefcon(port_match, drv->dev_name, port->line);
> +}
> +
> +#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
> @@ -3407,6 +3407,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;
> +

Looks okay otherwise,
Reviewed-by: Dhruva Gole <d-gole@ti.com>

-- 
Best regards,
Dhruva