Re: [PATCH 30/33] serial: 8250: invert serial8250_register_8250_port() CIR condition

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 10226 bytes --]

On Wed, 11 Jun 2025, Jiri Slaby (SUSE) wrote:

> There is no point in a long 'if' in serial8250_register_8250_port() to
> just return ENOSPC for PORT_8250_CIR ports. Invert the condition and
> return immediately.
> 
> 'gpios' variable was moved to its set location.
> 
> And return ENODEV instead of ENOSPC. The latter is a leftover from the
> previous find-uart 'if'. The former makes a lot more sense in this case.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> ---
>  drivers/tty/serial/8250/8250_core.c | 253 ++++++++++++++--------------
>  1 file changed, 127 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 2bac9c7827de..10f25bae9f46 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -725,139 +725,140 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
>  		nr_uarts++;
>  	}
>  
> -	if (uart->port.type != PORT_8250_CIR) {
> -		struct mctrl_gpios *gpios;
> -
> -		if (uart->port.dev)
> -			uart_remove_one_port(&serial8250_reg, &uart->port);
> -
> -		uart->port.ctrl_id	= up->port.ctrl_id;
> -		uart->port.port_id	= up->port.port_id;
> -		uart->port.iobase       = up->port.iobase;
> -		uart->port.membase      = up->port.membase;
> -		uart->port.irq          = up->port.irq;
> -		uart->port.irqflags     = up->port.irqflags;
> -		uart->port.uartclk      = up->port.uartclk;
> -		uart->port.fifosize     = up->port.fifosize;
> -		uart->port.regshift     = up->port.regshift;
> -		uart->port.iotype       = up->port.iotype;
> -		uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;
> -		uart->bugs		= up->bugs;
> -		uart->port.mapbase      = up->port.mapbase;
> -		uart->port.mapsize      = up->port.mapsize;
> -		uart->port.private_data = up->port.private_data;
> -		uart->tx_loadsz		= up->tx_loadsz;
> -		uart->capabilities	= up->capabilities;
> -		uart->port.throttle	= up->port.throttle;
> -		uart->port.unthrottle	= up->port.unthrottle;
> -		uart->port.rs485_config	= up->port.rs485_config;
> -		uart->port.rs485_supported = up->port.rs485_supported;
> -		uart->port.rs485	= up->port.rs485;
> -		uart->rs485_start_tx	= up->rs485_start_tx;
> -		uart->rs485_stop_tx	= up->rs485_stop_tx;
> -		uart->lsr_save_mask	= up->lsr_save_mask;
> -		uart->dma		= up->dma;
> -
> -		/* Take tx_loadsz from fifosize if it wasn't set separately */
> -		if (uart->port.fifosize && !uart->tx_loadsz)
> -			uart->tx_loadsz = uart->port.fifosize;
> -
> -		if (up->port.dev) {
> -			uart->port.dev = up->port.dev;
> -			ret = uart_get_rs485_mode(&uart->port);
> -			if (ret)
> -				goto err;
> -		}
> +	if (uart->port.type == PORT_8250_CIR) {
> +		ret = -ENODEV;
> +		goto unlock;
> +	}
>  
> -		if (up->port.flags & UPF_FIXED_TYPE)
> -			uart->port.type = up->port.type;
> +	if (uart->port.dev)
> +		uart_remove_one_port(&serial8250_reg, &uart->port);
> +
> +	uart->port.ctrl_id	= up->port.ctrl_id;
> +	uart->port.port_id	= up->port.port_id;
> +	uart->port.iobase       = up->port.iobase;
> +	uart->port.membase      = up->port.membase;
> +	uart->port.irq          = up->port.irq;
> +	uart->port.irqflags     = up->port.irqflags;
> +	uart->port.uartclk      = up->port.uartclk;
> +	uart->port.fifosize     = up->port.fifosize;
> +	uart->port.regshift     = up->port.regshift;
> +	uart->port.iotype       = up->port.iotype;
> +	uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;
> +	uart->bugs		= up->bugs;
> +	uart->port.mapbase      = up->port.mapbase;
> +	uart->port.mapsize      = up->port.mapsize;
> +	uart->port.private_data = up->port.private_data;
> +	uart->tx_loadsz		= up->tx_loadsz;
> +	uart->capabilities	= up->capabilities;
> +	uart->port.throttle	= up->port.throttle;
> +	uart->port.unthrottle	= up->port.unthrottle;
> +	uart->port.rs485_config	= up->port.rs485_config;
> +	uart->port.rs485_supported = up->port.rs485_supported;
> +	uart->port.rs485	= up->port.rs485;
> +	uart->rs485_start_tx	= up->rs485_start_tx;
> +	uart->rs485_stop_tx	= up->rs485_stop_tx;
> +	uart->lsr_save_mask	= up->lsr_save_mask;
> +	uart->dma		= up->dma;
> +
> +	/* Take tx_loadsz from fifosize if it wasn't set separately */
> +	if (uart->port.fifosize && !uart->tx_loadsz)
> +		uart->tx_loadsz = uart->port.fifosize;
> +
> +	if (up->port.dev) {
> +		uart->port.dev = up->port.dev;
> +		ret = uart_get_rs485_mode(&uart->port);
> +		if (ret)
> +			goto err;
> +	}
>  
> -		/*
> -		 * Only call mctrl_gpio_init(), if the device has no ACPI
> -		 * companion device
> -		 */
> -		if (!has_acpi_companion(uart->port.dev)) {
> -			gpios = mctrl_gpio_init(&uart->port, 0);
> -			if (IS_ERR(gpios)) {
> -				ret = PTR_ERR(gpios);
> -				goto err;
> -			} else {
> -				uart->gpios = gpios;
> -			}
> -		}
> +	if (up->port.flags & UPF_FIXED_TYPE)
> +		uart->port.type = up->port.type;
>  
> -		serial8250_set_defaults(uart);
> -
> -		/* Possibly override default I/O functions.  */
> -		if (up->port.serial_in)
> -			uart->port.serial_in = up->port.serial_in;
> -		if (up->port.serial_out)
> -			uart->port.serial_out = up->port.serial_out;
> -		if (up->port.handle_irq)
> -			uart->port.handle_irq = up->port.handle_irq;
> -		/*  Possibly override set_termios call */
> -		if (up->port.set_termios)
> -			uart->port.set_termios = up->port.set_termios;
> -		if (up->port.set_ldisc)
> -			uart->port.set_ldisc = up->port.set_ldisc;
> -		if (up->port.get_mctrl)
> -			uart->port.get_mctrl = up->port.get_mctrl;
> -		if (up->port.set_mctrl)
> -			uart->port.set_mctrl = up->port.set_mctrl;
> -		if (up->port.get_divisor)
> -			uart->port.get_divisor = up->port.get_divisor;
> -		if (up->port.set_divisor)
> -			uart->port.set_divisor = up->port.set_divisor;
> -		if (up->port.startup)
> -			uart->port.startup = up->port.startup;
> -		if (up->port.shutdown)
> -			uart->port.shutdown = up->port.shutdown;
> -		if (up->port.pm)
> -			uart->port.pm = up->port.pm;
> -		if (up->port.handle_break)
> -			uart->port.handle_break = up->port.handle_break;
> -		if (up->dl_read)
> -			uart->dl_read = up->dl_read;
> -		if (up->dl_write)
> -			uart->dl_write = up->dl_write;
> -
> -		if (uart->port.type != PORT_8250_CIR) {
> -			if (uart_console_registered(&uart->port))
> -				pm_runtime_get_sync(uart->port.dev);
> -
> -			if (serial8250_isa_config != NULL)
> -				serial8250_isa_config(0, &uart->port,
> -						&uart->capabilities);
> -
> -			serial8250_apply_quirks(uart);
> -			ret = uart_add_one_port(&serial8250_reg,
> -						&uart->port);
> -			if (ret)
> -				goto err;
> -
> -			ret = uart->port.line;
> +	/*
> +	 * Only call mctrl_gpio_init(), if the device has no ACPI
> +	 * companion device
> +	 */
> +	if (!has_acpi_companion(uart->port.dev)) {
> +		struct mctrl_gpios *gpios = mctrl_gpio_init(&uart->port, 0);
> +		if (IS_ERR(gpios)) {
> +			ret = PTR_ERR(gpios);
> +			goto err;
>  		} else {
> -			dev_info(uart->port.dev,
> -				"skipping CIR port at 0x%lx / 0x%llx, IRQ %d\n",
> -				uart->port.iobase,
> -				(unsigned long long)uart->port.mapbase,
> -				uart->port.irq);
> -
> -			ret = 0;
> +			uart->gpios = gpios;
>  		}
> +	}
>  
> -		if (!uart->lsr_save_mask)
> -			uart->lsr_save_mask = LSR_SAVE_FLAGS;	/* Use default LSR mask */
> +	serial8250_set_defaults(uart);
> +
> +	/* Possibly override default I/O functions.  */
> +	if (up->port.serial_in)
> +		uart->port.serial_in = up->port.serial_in;
> +	if (up->port.serial_out)
> +		uart->port.serial_out = up->port.serial_out;
> +	if (up->port.handle_irq)
> +		uart->port.handle_irq = up->port.handle_irq;
> +	/*  Possibly override set_termios call */
> +	if (up->port.set_termios)
> +		uart->port.set_termios = up->port.set_termios;
> +	if (up->port.set_ldisc)
> +		uart->port.set_ldisc = up->port.set_ldisc;
> +	if (up->port.get_mctrl)
> +		uart->port.get_mctrl = up->port.get_mctrl;
> +	if (up->port.set_mctrl)
> +		uart->port.set_mctrl = up->port.set_mctrl;
> +	if (up->port.get_divisor)
> +		uart->port.get_divisor = up->port.get_divisor;
> +	if (up->port.set_divisor)
> +		uart->port.set_divisor = up->port.set_divisor;
> +	if (up->port.startup)
> +		uart->port.startup = up->port.startup;
> +	if (up->port.shutdown)
> +		uart->port.shutdown = up->port.shutdown;
> +	if (up->port.pm)
> +		uart->port.pm = up->port.pm;
> +	if (up->port.handle_break)
> +		uart->port.handle_break = up->port.handle_break;
> +	if (up->dl_read)
> +		uart->dl_read = up->dl_read;
> +	if (up->dl_write)
> +		uart->dl_write = up->dl_write;
>  
> -		/* Initialise interrupt backoff work if required */
> -		if (up->overrun_backoff_time_ms > 0) {
> -			uart->overrun_backoff_time_ms =
> -				up->overrun_backoff_time_ms;
> -			INIT_DELAYED_WORK(&uart->overrun_backoff,
> -					serial_8250_overrun_backoff_work);
> -		} else {
> -			uart->overrun_backoff_time_ms = 0;
> -		}
> +	if (uart->port.type != PORT_8250_CIR) {
> +		if (uart_console_registered(&uart->port))
> +			pm_runtime_get_sync(uart->port.dev);
> +
> +		if (serial8250_isa_config != NULL)
> +			serial8250_isa_config(0, &uart->port,
> +					&uart->capabilities);
> +
> +		serial8250_apply_quirks(uart);
> +		ret = uart_add_one_port(&serial8250_reg,
> +					&uart->port);
> +		if (ret)
> +			goto err;
> +
> +		ret = uart->port.line;
> +	} else {
> +		dev_info(uart->port.dev,
> +			"skipping CIR port at 0x%lx / 0x%llx, IRQ %d\n",
> +			uart->port.iobase,
> +			(unsigned long long)uart->port.mapbase,
> +			uart->port.irq);
> +
> +		ret = 0;
> +	}
> +
> +	if (!uart->lsr_save_mask)
> +		uart->lsr_save_mask = LSR_SAVE_FLAGS;	/* Use default LSR mask */
> +
> +	/* Initialise interrupt backoff work if required */
> +	if (up->overrun_backoff_time_ms > 0) {
> +		uart->overrun_backoff_time_ms =
> +			up->overrun_backoff_time_ms;
> +		INIT_DELAYED_WORK(&uart->overrun_backoff,
> +				serial_8250_overrun_backoff_work);
> +	} else {
> +		uart->overrun_backoff_time_ms = 0;
>  	}
>  
>  unlock:
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.