linux-kernel - Re: [PATCH 30/33] serial: 8250: invert serial8250_register_8250

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aFJTQqVvmLBvrVRA@black.fi.intel.com>
Date: Wed, 18 Jun 2025 08:48:50 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: "Jiri Slaby (SUSE)" <jirislaby@...nel.org>
Cc: gregkh@...uxfoundation.org, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 30/33] serial: 8250: invert
 serial8250_register_8250_port() CIR condition

On Wed, Jun 11, 2025 at 12:03:16PM +0200, 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.

...

> +	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.type != PORT_8250_CIR) {

I admit that there tons of mysterious ways of UART initialisation, but can you
elaborate how this is not a always-true conditional?

> +		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;
> +	}

-- 
With Best Regards,
Andy Shevchenko