linux-kernel - Re: [PATCH] serial: 8250: don't lost port's default capabilities

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: <20240706140524.GYA4122589.dlan.gentoo>
Date: Sat, 6 Jul 2024 14:05:24 +0000
From: Yixun Lan <dlan@...too.org>
To: Jisheng Zhang <jszhang@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Arnd Bergmann <arnd@...db.de>, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH] serial: 8250: don't lost port's default capabilities

Hi

On 16:29 Sat 06 Jul     , Jisheng Zhang wrote:
> Commit b0b8c84cf58d ("serial: of_serial: Handle auto-flow-control
> property") added support for fifo-size and hw-flow-control properties
> to avoid adding new types to 8250.c for UARTs that are compatible with
> the standard types but that have different size fifo or support 16750
> compatible auto flow control. We avoided many new 8250 port types with
> this nice feature, but there's a problem, if the code detects fifo-size
> or auto-flow-control property, up->capabilities will be set
> accordingly, then serial8250_set_defaults() will ignore the default
> port's capabilities:

> 
> |if (!up->capabilities)
> |	up->capabilities = uart_config[type].flags;
> 
so the previous old logic is trying to override the config of 'type' uart,
while this patch try to extend capabilities with default config of 'type' uart as base

I tend to agree this is right direction (but wasn't 100% sure..)

btw, Jisheng, can you also check serial8250_do_startup()? which has similar logic

> If the port's default capabilities contains other bits such as
> UART_CAP_SLEEP, UART_CAP_EFR and so on, they are lost.
> 
> Fixes: b0b8c84cf58d ("serial: of_serial: Handle auto-flow-control property")

I believe the commit just reveal the problem, make it more visible, but not the root cause,
'git blame' lead to b6830f6df8914f ("serial: 8250: Split base port operations from universal driver")
as the original commit introduce this logic which seems exist long time ago..


> Signed-off-by: Jisheng Zhang <jszhang@...nel.org>
> ---
>  drivers/tty/serial/8250/8250_port.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 893bc493f662..e20614241229 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3245,8 +3245,7 @@ void serial8250_set_defaults(struct uart_8250_port *up)
>  			up->port.fifosize = uart_config[type].fifo_size;
>  		if (!up->tx_loadsz)
>  			up->tx_loadsz = uart_config[type].tx_loadsz;
> -		if (!up->capabilities)
> -			up->capabilities = uart_config[type].flags;
> +		up->capabilities |= uart_config[type].flags;
>  	}
>  
>  	set_io_from_upio(port);
> -- 
> 2.43.0
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55