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

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

[Lukas Wunner]

Commit 44b27aec9d96 ("serial: core, 8250: set RS485 termination GPIO in
serial core") enabled support for RS485 termination GPIOs behind i2c
expanders by setting the GPIO outside of the critical section protected
by the port spinlock.  Access to the i2c expander may sleep, which
caused a splat with the port spinlock held.

Commit 7c7f9bc986e6 ("serial: Deassert Transmit Enable on probe in
driver-specific way") erroneously regressed that by spinlocking the
GPIO manipulation again.

Fix by moving uart_rs485_config() (the function manipulating the GPIO)
outside of the spinlocked section and acquiring the spinlock inside of
uart_rs485_config() for the invocation of ->rs485_config() only.

This gets us one step closer to pushing the spinlock down into the
->rs485_config() callbacks which actually need it.  (Some callbacks
do not want to be spinlocked because they perform sleepable register
accesses, see e.g. sc16is7xx_config_rs485().)

Stack trace for posterity:

 Voluntary context switch within RCU read-side critical section!
 WARNING: CPU: 0 PID: 56 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch
 Call trace:
 rcu_note_context_switch
 __schedule
 schedule
 schedule_timeout
 wait_for_completion_timeout
 bcm2835_i2c_xfer
 __i2c_transfer
 i2c_transfer
 i2c_transfer_buffer_flags
 regmap_i2c_write
 _regmap_raw_write_impl
 _regmap_bus_raw_write
 _regmap_write
 _regmap_update_bits
 regmap_update_bits_base
 pca953x_gpio_set_value
 gpiod_set_raw_value_commit
 gpiod_set_value_nocheck
 gpiod_set_value_cansleep
 uart_rs485_config
 uart_add_one_port
 pl011_register_port
 pl011_probe

Fixes: 7c7f9bc986e6 ("serial: Deassert Transmit Enable on probe in driver-specific way")
Suggested-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v6.1+
---
 drivers/tty/serial/serial_core.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

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));
 
@@ -2474,11 +2480,10 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
 			if (ret == 0) {
 				if (tty)
 					uart_change_line_settings(tty, state, NULL);
+				uart_rs485_config(uport);
 				spin_lock_irq(&uport->lock);
 				if (!(uport->rs485.flags & SER_RS485_ENABLED))
 					ops->set_mctrl(uport, uport->mctrl);
-				else
-					uart_rs485_config(uport);
 				ops->start_tx(uport);
 				spin_unlock_irq(&uport->lock);
 				tty_port_set_initialized(port, true);
@@ -2587,10 +2592,10 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 		port->mctrl &= TIOCM_DTR;
 		if (!(port->rs485.flags & SER_RS485_ENABLED))
 			port->ops->set_mctrl(port, port->mctrl);
-		else
-			uart_rs485_config(port);
 		spin_unlock_irqrestore(&port->lock, flags);
 
+		uart_rs485_config(port);
+
 		/*
 		 * If this driver supports console, and it hasn't been
 		 * successfully registered yet, try to re-register it.
-- 
2.40.1

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

[Ilpo Järvinen]

On Thu, 21 Sep 2023, Lukas Wunner wrote:

> Commit 44b27aec9d96 ("serial: core, 8250: set RS485 termination GPIO in
> serial core") enabled support for RS485 termination GPIOs behind i2c
> expanders by setting the GPIO outside of the critical section protected
> by the port spinlock.  Access to the i2c expander may sleep, which
> caused a splat with the port spinlock held.
> 
> Commit 7c7f9bc986e6 ("serial: Deassert Transmit Enable on probe in
> driver-specific way") erroneously regressed that by spinlocking the
> GPIO manipulation again.
> 
> Fix by moving uart_rs485_config() (the function manipulating the GPIO)
> outside of the spinlocked section and acquiring the spinlock inside of
> uart_rs485_config() for the invocation of ->rs485_config() only.
> 
> This gets us one step closer to pushing the spinlock down into the
> ->rs485_config() callbacks which actually need it.  (Some callbacks
> do not want to be spinlocked because they perform sleepable register
> accesses, see e.g. sc16is7xx_config_rs485().)
> 
> Stack trace for posterity:
> 
>  Voluntary context switch within RCU read-side critical section!
>  WARNING: CPU: 0 PID: 56 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch
>  Call trace:
>  rcu_note_context_switch
>  __schedule
>  schedule
>  schedule_timeout
>  wait_for_completion_timeout
>  bcm2835_i2c_xfer
>  __i2c_transfer
>  i2c_transfer
>  i2c_transfer_buffer_flags
>  regmap_i2c_write
>  _regmap_raw_write_impl
>  _regmap_bus_raw_write
>  _regmap_write
>  _regmap_update_bits
>  regmap_update_bits_base
>  pca953x_gpio_set_value
>  gpiod_set_raw_value_commit
>  gpiod_set_value_nocheck
>  gpiod_set_value_cansleep
>  uart_rs485_config
>  uart_add_one_port
>  pl011_register_port
>  pl011_probe
> 
> Fixes: 7c7f9bc986e6 ("serial: Deassert Transmit Enable on probe in driver-specific way")
> Suggested-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v6.1+
> ---
>  drivers/tty/serial/serial_core.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> 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);

There's a subtle change in behavior here, uart_sanitize_serial_rs485() 
memset()s rs485 if RS485 is not enabled but the early return above does 
not.

Was that an oversight or intentional change? (I'm not sure it matters but 
it seems this fix doesn't realy depend on changing the zeroing behavior)

-- 
 i.

>  	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));
>  
> @@ -2474,11 +2480,10 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
>  			if (ret == 0) {
>  				if (tty)
>  					uart_change_line_settings(tty, state, NULL);
> +				uart_rs485_config(uport);
>  				spin_lock_irq(&uport->lock);
>  				if (!(uport->rs485.flags & SER_RS485_ENABLED))
>  					ops->set_mctrl(uport, uport->mctrl);
> -				else
> -					uart_rs485_config(uport);
>  				ops->start_tx(uport);
>  				spin_unlock_irq(&uport->lock);
>  				tty_port_set_initialized(port, true);
> @@ -2587,10 +2592,10 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
>  		port->mctrl &= TIOCM_DTR;
>  		if (!(port->rs485.flags & SER_RS485_ENABLED))
>  			port->ops->set_mctrl(port, port->mctrl);
> -		else
> -			uart_rs485_config(port);
>  		spin_unlock_irqrestore(&port->lock, flags);
>  
> +		uart_rs485_config(port);
> +
>  		/*
>  		 * If this driver supports console, and it hasn't been
>  		 * successfully registered yet, try to re-register it.
> 

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

[John Ogness]

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.

John Ogness

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

[Ilpo Järvinen]

On Thu, 21 Sep 2023, 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.

Both callers seem to be inside tty port's mutex.

-- 
 i.

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

[Lukas Wunner]

On Thu, Sep 21, 2023 at 06:01:41PM +0300, Ilpo Järvinen wrote:
> On Thu, 21 Sep 2023, Lukas Wunner wrote:
> > --- 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);
> 
> There's a subtle change in behavior here, uart_sanitize_serial_rs485() 
> memset()s rs485 if RS485 is not enabled but the early return above does 
> not.

The two callers of uart_rs485_config() only call it if
(!(uport->rs485.flags & SER_RS485_ENABLED)).

Adding that early return ensures that the behavior doesn't change.

So I don't quite see why you think there's a change in behavior?
Am I missing something?

Thanks,

Lukas

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

[Lukas Wunner]

On Fri, Sep 22, 2023 at 06:02:28AM +0200, Lukas Wunner wrote:
> On Thu, Sep 21, 2023 at 06:01:41PM +0300, Ilpo Järvinen wrote:
> > On Thu, 21 Sep 2023, Lukas Wunner wrote:
> > > --- 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);
> > 
> > There's a subtle change in behavior here, uart_sanitize_serial_rs485() 
> > memset()s rs485 if RS485 is not enabled but the early return above does 
> > not.
> 
> The two callers of uart_rs485_config() only call it if
> (!(uport->rs485.flags & SER_RS485_ENABLED)).

I meant to say "if (uport->rs485.flags & SER_RS485_ENABLED)"
(i.e. without negation).

Otherwise my point still stands. :)

> Adding that early return ensures that the behavior doesn't change.
> 
> So I don't quite see why you think there's a change in behavior?
> Am I missing something?

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

[Lukas Wunner]

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

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

[Ilpo Järvinen]

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

On Fri, 22 Sep 2023, Lukas Wunner wrote:

> On Fri, Sep 22, 2023 at 06:02:28AM +0200, Lukas Wunner wrote:
> > On Thu, Sep 21, 2023 at 06:01:41PM +0300, Ilpo Järvinen wrote:
> > > On Thu, 21 Sep 2023, Lukas Wunner wrote:
> > > > --- 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);
> > > 
> > > There's a subtle change in behavior here, uart_sanitize_serial_rs485() 
> > > memset()s rs485 if RS485 is not enabled but the early return above does 
> > > not.
> > 
> > The two callers of uart_rs485_config() only call it if
> > (!(uport->rs485.flags & SER_RS485_ENABLED)).
> 
> I meant to say "if (uport->rs485.flags & SER_RS485_ENABLED)"
> (i.e. without negation).
> 
> Otherwise my point still stands. :)

Okay yeah, it's fine. I did not take the behavior under the old code into 
account well enough.

-- 
 i.