linux-kernel - Re: [PATCH next v2 2/4] serial: 8250: Split out IER from rs485_stop

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZuqjCN18dSUDEa0d@pathway.suse.cz>
Date: Wed, 18 Sep 2024 11:53:12 +0200
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Esben Haabendal <esben@...nix.com>, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Sunil V L <sunilvl@...tanamicro.com>, Arnd Bergmann <arnd@...db.de>,
	Tony Lindgren <tony@...mide.com>, Udit Kumar <u-kumar1@...com>,
	Ronald Wahl <ronald.wahl@...itan.com>,
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	Thomas Richard <thomas.richard@...tlin.com>,
	Griffin Kroah-Hartman <griffin@...ah.com>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Rengarajan S <rengarajan.s@...rochip.com>,
	Serge Semin <fancer.lancer@...il.com>
Subject: Re: [PATCH next v2 2/4] serial: 8250: Split out IER from
 rs485_stop_tx()

On Fri 2024-09-13 16:11:36, John Ogness wrote:
> Move IER handling out of rs485_stop_tx() callback and into a new
> wrapper serial8250_rs485_stop_tx(). Replace all callback call sites
> with wrapper, except for the console write() callback, where it is
> inappropriate to modify IER.

It would be great to provide more details:

  + why it is done (IER modification requires port lock?)

  + why it is suddenly safe to call serial8250_em485_handle_stop_tx()
    without holding &p->port.lock


> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -558,7 +558,7 @@ static int serial8250_em485_init(struct uart_8250_port *p)
>  
>  deassert_rts:
>  	if (p->em485->tx_stopped)
> -		p->rs485_stop_tx(p);
> +		serial8250_rs485_stop_tx(p);

This would keep the same functionality only when

	p->rs485_stop_tx == serial8250_em485_stop_tx

Is it always the case?
Is it OK when it is not the case?

For example, serial8250_em485_init() is involved in bcm2835aux driver
probe which uses another rs485_stop_tx() callback, see below.

>  
>  	return 0;
>  }
> @@ -1397,16 +1396,29 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
>  	/*
>  	 * Empty the RX FIFO, we are not interested in anything
>  	 * received during the half-duplex transmission.
> -	 * Enable previously disabled RX interrupts.
>  	 */
> -	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> +	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>  		serial8250_clear_and_reinit_fifos(p);
> +}
> +EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
> +
> +/**
> + * serial8250_rs485_stop_tx() - stop rs485 transmission, restore RX interrupts
> + * @p: uart 8250 port
> + */
> +void serial8250_rs485_stop_tx(struct uart_8250_port *p)
> +{
> +	/* Port locked to synchronize UART_IER access against the console. */
> +	lockdep_assert_held_once(&p->port.lock);
> +
> +	p->rs485_stop_tx(p);
>  
> +	/* Enable previously disabled RX interrupts. */
> +	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
>  		p->ier |= UART_IER_RLSI | UART_IER_RDI;
>  		serial_port_out(&p->port, UART_IER, p->ier);
>  	}
>  }
> -EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
>  
>  static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
>  {
> @@ -1418,7 +1430,7 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
>  	serial8250_rpm_get(p);
>  	uart_port_lock_irqsave(&p->port, &flags);
>  	if (em485->active_timer == &em485->stop_tx_timer) {
> -		p->rs485_stop_tx(p);
> +		serial8250_rs485_stop_tx(p);

This causes that UART_IER is manipulated for all p->rs485_stop_tx()
callbacks. Is that correct, please?

For example, it seems serial8250_em485_handle_stop_tx() might be used
also by bcm2835aux driver. It set by:

static int serial8250_em485_init(struct uart_8250_port *p)
{
	[...]
	p->em485->stop_tx_timer.function = &serial8250_em485_handle_stop_tx;
	[...]
}

which is called via

int serial8250_em485_config(struct uart_port *port, struct ktermios *termios,
			    struct serial_rs485 *rs485)
{
	[...]
	if (rs485->flags & SER_RS485_ENABLED)
		return serial8250_em485_init(up);
	[...]
}

which is set by:

static int bcm2835aux_serial_probe(struct platform_device *pdev)
{
	[...]
	up.port.rs485_config = serial8250_em485_config;		<--------
	[...]
	up.rs485_stop_tx = bcm2835aux_rs485_stop_tx;
	[...]
}

But this same _probe() call sets

	up.rs485_stop_tx = bcm2835aux_rs485_stop_tx;

which does not manipulate UART_IER.

>  		em485->active_timer = NULL;
>  		em485->tx_stopped = true;
>  	}
> @@ -1450,7 +1462,7 @@ static void __stop_tx_rs485(struct uart_8250_port *p, u64 stop_delay)
>  		em485->active_timer = &em485->stop_tx_timer;
>  		hrtimer_start(&em485->stop_tx_timer, ns_to_ktime(stop_delay), HRTIMER_MODE_REL);
>  	} else {
> -		p->rs485_stop_tx(p);
> +		serial8250_rs485_stop_tx(p);

I can't find easily whether serial8250_em485_stop_tx() is always set
as p->rs485_stop_tx callback here. I would expect that it might be
another callback. It is a callback after all.

Is it always safe?

>  		em485->active_timer = NULL;
>  		em485->tx_stopped = true;
>  	}

Best Regards,
Petr