Re: [PATCH v7 4/7] serial: core: Add support for DEVNAME:0.0 style naming for kernel console

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Mar 28, 2024 at 12:01:52PM +0530, Dhruva Gole wrote:
> 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);

No, please no.

Just spell it out, like was done here, dealing with ? : is a pain to
read and follow and the generated code should be identical.

Only use ? : in places where it's the only way to do it (i.e. as
function parameters or in printk-like lines.)

Write for people first, compilers second.

thanks,

greg k-h