Re: [PATCH tty-linus] serial: Reduce spinlocked portion of uart_rs485_config()

[Lukas Wunner <lukas@wunner.de>]

On Thu, Sep 21, 2023 at 05:24:04PM +0206, John Ogness wrote:
> On 2023-09-21, Lukas Wunner <lukas@wunner.de> wrote:
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index 7bdc21d5e13b..ca26a8aef2cb 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -1404,12 +1404,18 @@ static void uart_set_rs485_termination(struct uart_port *port,
> >  static int uart_rs485_config(struct uart_port *port)
> >  {
> >  	struct serial_rs485 *rs485 = &port->rs485;
> > +	unsigned long flags;
> >  	int ret;
> >  
> > +	if (!(rs485->flags & SER_RS485_ENABLED))
> > +		return 0;
> > +
> >  	uart_sanitize_serial_rs485(port, rs485);
> >  	uart_set_rs485_termination(port, rs485);
> >  
> > +	spin_lock_irqsave(&port->lock, flags);
> >  	ret = port->rs485_config(port, NULL, rs485);
> > +	spin_unlock_irqrestore(&port->lock, flags);
> >  	if (ret)
> >  		memset(rs485, 0, sizeof(*rs485));
> 
> Is there some kind of guarantee that 2 CPUs cannot go into
> uart_rs485_config() simultaneously? Otherwise it seems dangerous to be
> using and clearing @port->rs485 outside of the spin_lock.

uart_rs485_config() is only called on probe and resume.
I don't quite see how 2 CPUs can probe or resume the same UART
simultaneously?

Granted, it *is* possible to access or modify port->rs485 from
user space through uart_{get,set}_rs485_config() in parallel to
uart_resume_port(), but as Ilpo has correctly stated, that's
serialized using port->mutex.

To provide some background information:

The majority of products *hardwire* the UART port to the RS485
transceiver and thus the UART should be switched to RS485 mode
on probe (through a "linux,rs485-enabled-at-boot-time" devicetree
property).  User space generally has no business fiddling with
RS485 properties in those cases, save for enabling/disabling
termination etc.

A minority of products has serial ports which can be switched
between RS232 and RS485 mode.  For those products, user space
may indeed have the need to enable/disable RS485 at runtime.
The Siemens IOT2040 is a case in point:

https://assets.new.siemens.com/siemens/assets/api/uuid:7b2c5580-7436-4081-9ae6-7c4eba29ddd1/version:1562856379/simaticiot2040.pdf

Thanks,

Lukas