Move RS485 implementation from drivers to serial core

Move RS485 implementation from drivers to serial core

[Lino Sanfilippo]

This patch series is an attempt to simplify rs485 implementation in drivers
by moving the following tasks out of the drivers into the serial core:

- ensure sane RTS settings: in case of an invalid configuration (both RTS
  after send and RTS on send set or both unset) enable RTS on send and
  disable RTS after send

- nullify the padding field of the serial_rs485 struct before it is
  returned to userspace

- copy the configuration stored in the serial_rs485 struct to the port
  configuration if setting the configuration in the driver was successfull

- limit the RTS delays to 100ms


Redundant code has been removed from the following drivers for now:

- atmel
- fsl_lpuart
- amba
- imx
- max310x
- omap-serial
- sc16is7xx
- stm32-usart

The code has been tested with the amba pl011 driver.

Changes in v2:
- use a makro for max RTS delays and comment it (as requested by Jiri)
- add a comment concerning the memset of a structures padding field
- correct typos in the commit message (found by Uwe) 
- rephrase all commit messages to make more clear that function 
  uart_set_rs485_config() has been extended by checks and other
  functionalities (as requested by Uwe)

[PATCH 2 1/9] serial: core: move RS485 configuration tasks from drivers into core

[Lino Sanfilippo]

Several drivers that support setting the RS485 configuration via userspace
implement one or more of the following tasks:

- in case of an invalid RTS configuration (both RTS after send and RTS on
  send set or both unset) fall back to enable RTS on send and disable RTS
  after send

- nullify the padding field of the returned serial_rs485 struct

- copy the configuration into the uart port struct

- limit RTS delays to 100 ms

Move these tasks into the serial core to make them generic and to provide
a consistent behaviour among all drivers.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++
 include/uapi/linux/serial.h      |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 846192a7b4bf..a4f7e847d414 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1282,8 +1282,26 @@ static int uart_set_rs485_config(struct uart_port *port,
 	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
 		return -EFAULT;
 
+	/* pick sane settings if the user hasn't */
+	if (!(rs485.flags & SER_RS485_RTS_ON_SEND) ==
+	    !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) {
+		rs485.flags |= SER_RS485_RTS_ON_SEND;
+		rs485.flags &= ~SER_RS485_RTS_AFTER_SEND;
+	}
+
+	rs485.delay_rts_before_send = min_t(unsigned int,
+					    rs485.delay_rts_before_send,
+					    SER_RS485_MAX_RTS_DELAY);
+	rs485.delay_rts_after_send = min_t(unsigned int,
+					   rs485.delay_rts_after_send,
+					   SER_RS485_MAX_RTS_DELAY);
+	/* Return clean padding area to userspace */
+	memset(rs485.padding, 0, sizeof(rs485.padding));
+
 	spin_lock_irqsave(&port->lock, flags);
 	ret = port->rs485_config(port, &rs485);
+	if (!ret)
+		port->rs485 = rs485;
 	spin_unlock_irqrestore(&port->lock, flags);
 	if (ret)
 		return ret;
diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
index fa6b16e5fdd8..859045a53231 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -128,6 +128,9 @@ struct serial_rs485 {
 							   (if supported) */
 	__u32	delay_rts_before_send;	/* Delay before send (milliseconds) */
 	__u32	delay_rts_after_send;	/* Delay after send (milliseconds) */
+#define SER_RS485_MAX_RTS_DELAY		100		/* Max time with active
+							   RTS before/after
+							   data sent (msecs) */
 	__u32	padding[5];		/* Memory is cheap, new structs
 					   are a royal PITA .. */
 };

base-commit: 802d00bd774b77fe132e9e83462b96fb9919411c
-- 
2.34.1

[PATCH 2 2/9] serial: amba-pl011: remove redundant code in rs485_config

[Lino Sanfilippo]

In uart_set_rs485_config() the serial core already

- ensures that only one of both options RTS on send or RTS after send is
  set

- nullifies the padding field of the passed serial_rs485 struct

- clamps the RTS delays

- assigns the passed serial_rs485 struct to the uart port

So remove these tasks from the code of the drivers rs485_config() function
to avoid redundancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/amba-pl011.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index ba053a68529f..35c633739975 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2170,25 +2170,11 @@ static int pl011_rs485_config(struct uart_port *port,
 	struct uart_amba_port *uap =
 		container_of(port, struct uart_amba_port, port);
 
-	/* pick sane settings if the user hasn't */
-	if (!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
-	    !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
-		rs485->flags |= SER_RS485_RTS_ON_SEND;
-		rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
-	}
-	/* clamp the delays to [0, 100ms] */
-	rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
-	rs485->delay_rts_after_send = min(rs485->delay_rts_after_send, 100U);
-	memset(rs485->padding, 0, sizeof(rs485->padding));
-
 	if (port->rs485.flags & SER_RS485_ENABLED)
 		pl011_rs485_tx_stop(uap);
 
-	/* Set new configuration */
-	port->rs485 = *rs485;
-
 	/* Make sure auto RTS is disabled */
-	if (port->rs485.flags & SER_RS485_ENABLED) {
+	if (rs485->flags & SER_RS485_ENABLED) {
 		u32 cr = pl011_read(uap, REG_CR);
 
 		cr &= ~UART011_CR_RTSEN;
-- 
2.34.1

[PATCH 2 3/9] serial: stm32: remove redundant code in rs485_config

[Lino Sanfilippo]

In uart_set_rs485_config() the serial core already ensures that only one of
both options RTS on send or RTS after send is set. It also assigns the
passed serial_rs485 struct to the uart port.

So remove the check and the assignment from the drivers rs485_config()
function to avoid redundancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/stm32-usart.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 1b3a611ac39e..6a014168102c 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -107,8 +107,6 @@ static int stm32_usart_config_rs485(struct uart_port *port,
 
 	stm32_usart_clr_bits(port, ofs->cr1, BIT(cfg->uart_enable_bit));
 
-	port->rs485 = *rs485conf;
-
 	rs485conf->flags |= SER_RS485_RX_DURING_TX;
 
 	if (rs485conf->flags & SER_RS485_ENABLED) {
@@ -128,13 +126,10 @@ static int stm32_usart_config_rs485(struct uart_port *port,
 					     rs485conf->delay_rts_after_send,
 					     baud);
 
-		if (rs485conf->flags & SER_RS485_RTS_ON_SEND) {
+		if (rs485conf->flags & SER_RS485_RTS_ON_SEND)
 			cr3 &= ~USART_CR3_DEP;
-			rs485conf->flags &= ~SER_RS485_RTS_AFTER_SEND;
-		} else {
+		else
 			cr3 |= USART_CR3_DEP;
-			rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
-		}
 
 		writel_relaxed(cr3, port->membase + ofs->cr3);
 		writel_relaxed(cr1, port->membase + ofs->cr1);
-- 
2.34.1

[PATCH 2 4/9] serial: sc16is7xx: remove redundant check in rs485_config

[Lino Sanfilippo]

In uart_set_rs485_config() the serial core already ensures that only one of
both options RTS on send or RTS after send is set.

So remove this check from the drivers rs485_config() function to avoid
redundancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/sc16is7xx.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 64e7e6c8145f..730f697bb517 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -959,16 +959,6 @@ static int sc16is7xx_config_rs485(struct uart_port *port,
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 
 	if (rs485->flags & SER_RS485_ENABLED) {
-		bool rts_during_rx, rts_during_tx;
-
-		rts_during_rx = rs485->flags & SER_RS485_RTS_AFTER_SEND;
-		rts_during_tx = rs485->flags & SER_RS485_RTS_ON_SEND;
-
-		if (rts_during_rx == rts_during_tx)
-			dev_err(port->dev,
-				"unsupported RTS signalling on_send:%d after_send:%d - exactly one of RS485 RTS flags should be set\n",
-				rts_during_tx, rts_during_rx);
-
 		/*
 		 * RTS signal is handled by HW, it's timing can't be influenced.
 		 * However, it's sometimes useful to delay TX even without RTS
-- 
2.34.1

[PATCH 2 5/9] serial: omap: remove redundant code in rs485_config

[Lino Sanfilippo]

In uart_set_rs485_config() the serial core already clamps the RTS delays.
It also assigns the passed serial_rs485 struct to the uart port.

So remove these tasks from the drivers rs485_config() function to avoid
redundancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/omap-serial.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 0862941862c8..a3afcccfbd96 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1350,18 +1350,11 @@ serial_omap_config_rs485(struct uart_port *port, struct serial_rs485 *rs485)
 	up->ier = 0;
 	serial_out(up, UART_IER, 0);
 
-	/* Clamp the delays to [0, 100ms] */
-	rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
-	rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send, 100U);
-
-	/* store new config */
-	port->rs485 = *rs485;
-
 	if (up->rts_gpiod) {
 		/* enable / disable rts */
-		val = (port->rs485.flags & SER_RS485_ENABLED) ?
+		val = (rs485->flags & SER_RS485_ENABLED) ?
 			SER_RS485_RTS_AFTER_SEND : SER_RS485_RTS_ON_SEND;
-		val = (port->rs485.flags & val) ? 1 : 0;
+		val = (rs485->flags & val) ? 1 : 0;
 		gpiod_set_value(up->rts_gpiod, val);
 	}
 
@@ -1372,7 +1365,7 @@ serial_omap_config_rs485(struct uart_port *port, struct serial_rs485 *rs485)
 	/* If RS-485 is disabled, make sure the THR interrupt is fired when
 	 * TX FIFO is below the trigger level.
 	 */
-	if (!(port->rs485.flags & SER_RS485_ENABLED) &&
+	if (!(rs485->flags & SER_RS485_ENABLED) &&
 	    (up->scr & OMAP_UART_SCR_TX_EMPTY)) {
 		up->scr &= ~OMAP_UART_SCR_TX_EMPTY;
 		serial_out(up, UART_OMAP_SCR, up->scr);
-- 
2.34.1

[PATCH 2 6/9] serial: max310: remove redundant memset in rs485_config

[Lino Sanfilippo]

In uart_set_rs485_config() the serial core already nullifies the padding
field of the passed serial_rs485 struct before returning it to userspace.

Doing the same in the drivers rs485_config() function is redundant, so
remove the concerning memset in this function.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/max310x.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index dde0824b2fa5..2ecc5f66deaf 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1037,7 +1037,6 @@ static int max310x_rs485_config(struct uart_port *port,
 
 	rs485->flags &= SER_RS485_RTS_ON_SEND | SER_RS485_RX_DURING_TX |
 			SER_RS485_ENABLED;
-	memset(rs485->padding, 0, sizeof(rs485->padding));
 	port->rs485 = *rs485;
 
 	schedule_work(&one->rs_work);
-- 
2.34.1

[PATCH 2 7/9] serial: imx: remove redundant assignment in rs485_config

[Lino Sanfilippo]

In uart_set_rs485_config() the serial core already assigns the passed
serial_rs485 struct to the uart port.

So remove the assignment in the drivers rs485_config() function to avoid
reduncancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/imx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 0b467ce8d737..ab56ff23e8a9 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1937,8 +1937,6 @@ static int imx_uart_rs485_config(struct uart_port *port,
 	    rs485conf->flags & SER_RS485_RX_DURING_TX)
 		imx_uart_start_rx(port);
 
-	port->rs485 = *rs485conf;
-
 	return 0;
 }
 
-- 
2.34.1

[PATCH 2 8/9] serial: fsl_lpuart: remove redundant code in rs485_config functions

[Lino Sanfilippo]

In uart_set_rs485_config() the serial core already ensures that only one of
both options RTS on send or RTS after send is set. It also assigns the
passed serial_rs485 struct to the uart port.

So remove the check and the assignment from the drivers rs485_config()
function to avoid redundancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/fsl_lpuart.c | 32 --------------------------------
 1 file changed, 32 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 7d90c5a530ee..a201be44d68a 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1377,19 +1377,6 @@ static int lpuart_config_rs485(struct uart_port *port,
 		/* Enable auto RS-485 RTS mode */
 		modem |= UARTMODEM_TXRTSE;
 
-		/*
-		 * RTS needs to be logic HIGH either during transfer _or_ after
-		 * transfer, other variants are not supported by the hardware.
-		 */
-
-		if (!(rs485->flags & (SER_RS485_RTS_ON_SEND |
-				SER_RS485_RTS_AFTER_SEND)))
-			rs485->flags |= SER_RS485_RTS_ON_SEND;
-
-		if (rs485->flags & SER_RS485_RTS_ON_SEND &&
-				rs485->flags & SER_RS485_RTS_AFTER_SEND)
-			rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
-
 		/*
 		 * The hardware defaults to RTS logic HIGH while transfer.
 		 * Switch polarity in case RTS shall be logic HIGH
@@ -1402,9 +1389,6 @@ static int lpuart_config_rs485(struct uart_port *port,
 			modem |= UARTMODEM_TXRTSPOL;
 	}
 
-	/* Store the new configuration */
-	sport->port.rs485 = *rs485;
-
 	writeb(modem, sport->port.membase + UARTMODEM);
 	return 0;
 }
@@ -1428,19 +1412,6 @@ static int lpuart32_config_rs485(struct uart_port *port,
 		/* Enable auto RS-485 RTS mode */
 		modem |= UARTMODEM_TXRTSE;
 
-		/*
-		 * RTS needs to be logic HIGH either during transfer _or_ after
-		 * transfer, other variants are not supported by the hardware.
-		 */
-
-		if (!(rs485->flags & (SER_RS485_RTS_ON_SEND |
-				SER_RS485_RTS_AFTER_SEND)))
-			rs485->flags |= SER_RS485_RTS_ON_SEND;
-
-		if (rs485->flags & SER_RS485_RTS_ON_SEND &&
-				rs485->flags & SER_RS485_RTS_AFTER_SEND)
-			rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
-
 		/*
 		 * The hardware defaults to RTS logic HIGH while transfer.
 		 * Switch polarity in case RTS shall be logic HIGH
@@ -1453,9 +1424,6 @@ static int lpuart32_config_rs485(struct uart_port *port,
 			modem |= UARTMODEM_TXRTSPOL;
 	}
 
-	/* Store the new configuration */
-	sport->port.rs485 = *rs485;
-
 	lpuart32_write(&sport->port, modem, UARTMODIR);
 	return 0;
 }
-- 
2.34.1

[PATCH 2 9/9] serial: atmel: remove redundant assignment in rs485_config

[Lino Sanfilippo]

In uart_set_rs485_config() the serial core already assigns the passed
serial_rs485 struct to the uart port.

So remove the assignment from the drivers rs485_config() function to avoid
redundancy.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/tty/serial/atmel_serial.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 2d09a89974a2..2ab589a3d86c 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -299,11 +299,9 @@ static int atmel_config_rs485(struct uart_port *port,
 	/* Resetting serial mode to RS232 (0x0) */
 	mode &= ~ATMEL_US_USMODE;
 
-	port->rs485 = *rs485conf;
-
 	if (rs485conf->flags & SER_RS485_ENABLED) {
 		dev_dbg(port->dev, "Setting UART to RS485\n");
-		if (port->rs485.flags & SER_RS485_RX_DURING_TX)
+		if (rs485conf->flags & SER_RS485_RX_DURING_TX)
 			atmel_port->tx_done_mask = ATMEL_US_TXRDY;
 		else
 			atmel_port->tx_done_mask = ATMEL_US_TXEMPTY;
-- 
2.34.1

Re: [PATCH 2 7/9] serial: imx: remove redundant assignment in rs485_config

[Uwe Kleine-König]

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

On Wed, Feb 16, 2022 at 01:18:01AM +0100, Lino Sanfilippo wrote:
> In uart_set_rs485_config() the serial core already assigns the passed
> serial_rs485 struct to the uart port.
> 
> So remove the assignment in the drivers rs485_config() function to avoid
> reduncancy.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2 9/9] serial: atmel: remove redundant assignment in rs485_config

[Richard Genoud]

Le 16/02/2022 à 01:18, Lino Sanfilippo a écrit :
> In uart_set_rs485_config() the serial core already assigns the passed
> serial_rs485 struct to the uart port.
> 
> So remove the assignment from the drivers rs485_config() function to avoid
> redundancy.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Acked-by: Richard Genoud <richard.genoud@gmail.com>

> ---
>   drivers/tty/serial/atmel_serial.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 2d09a89974a2..2ab589a3d86c 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -299,11 +299,9 @@ static int atmel_config_rs485(struct uart_port *port,
>   	/* Resetting serial mode to RS232 (0x0) */
>   	mode &= ~ATMEL_US_USMODE;
>   
> -	port->rs485 = *rs485conf;
> -
>   	if (rs485conf->flags & SER_RS485_ENABLED) {
>   		dev_dbg(port->dev, "Setting UART to RS485\n");
> -		if (port->rs485.flags & SER_RS485_RX_DURING_TX)
> +		if (rs485conf->flags & SER_RS485_RX_DURING_TX)
>   			atmel_port->tx_done_mask = ATMEL_US_TXRDY;
>   		else
>   			atmel_port->tx_done_mask = ATMEL_US_TXEMPTY;

Thanks !

Re: [PATCH 2 1/9] serial: core: move RS485 configuration tasks from drivers into core

[Lukas Wunner]

On Wed, Feb 16, 2022 at 01:17:55AM +0100, Lino Sanfilippo wrote:
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1282,8 +1282,26 @@ static int uart_set_rs485_config(struct uart_port *port,
>  	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
>  		return -EFAULT;
>  
> +	/* pick sane settings if the user hasn't */
> +	if (!(rs485.flags & SER_RS485_RTS_ON_SEND) ==
> +	    !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) {
> +		rs485.flags |= SER_RS485_RTS_ON_SEND;
> +		rs485.flags &= ~SER_RS485_RTS_AFTER_SEND;
> +	}

The policy you're enforcing here is:  If settings are nonsensical,
always default to active-high polarity.

However some drivers currently enforce a completely different policy:
E.g. with 8250_lpc18xx.c, if SER_RS485_RTS_ON_SEND is set, use active-high
(and fix up SER_RS485_RTS_AFTER_SEND), else use active-low polarity.
This yields a different result compared to your policy in case both bits
are cleared.

Similarly, sc16is7xx.c defaults to active-low if SER_RS485_RTS_AFTER_SEND
is set, else active-high polarity.  This yields a different result compared
to your policy in case both bits are set.

You risk breaking existing user space applications with this change
if those applications specify nonsensical polarity settings.


I happen to have created a similar commit to this one a month ago
and I came to the conclusion that all one can do is offer a library
function uart_sanitize_rs485_mode() which drivers may elect to call
if that helper's policy is identical to what they're doing now:

https://github.com/l1k/linux/commit/637984111e42


> +
> +	rs485.delay_rts_before_send = min_t(unsigned int,
> +					    rs485.delay_rts_before_send,
> +					    SER_RS485_MAX_RTS_DELAY);
> +	rs485.delay_rts_after_send = min_t(unsigned int,
> +					   rs485.delay_rts_after_send,
> +					   SER_RS485_MAX_RTS_DELAY);

Nonsensical delays may not only be passed in from user space via ioctl(),
but through the device tree.  That's another reason to use a library
function:  It can be called both from the ioctl() as well as after (or in)
uart_get_rs485_mode().


> +	/* Return clean padding area to userspace */
> +	memset(rs485.padding, 0, sizeof(rs485.padding));

Unlike the polarity and delay handling, this one makes sense.

Thanks,

Lukas

Re: [PATCH 2 4/9] serial: sc16is7xx: remove redundant check in rs485_config

[Lukas Wunner]

On Wed, Feb 16, 2022 at 01:17:58AM +0100, Lino Sanfilippo wrote:
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -959,16 +959,6 @@ static int sc16is7xx_config_rs485(struct uart_port *port,
>  	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
>  
>  	if (rs485->flags & SER_RS485_ENABLED) {
> -		bool rts_during_rx, rts_during_tx;
> -
> -		rts_during_rx = rs485->flags & SER_RS485_RTS_AFTER_SEND;
> -		rts_during_tx = rs485->flags & SER_RS485_RTS_ON_SEND;
> -
> -		if (rts_during_rx == rts_during_tx)
> -			dev_err(port->dev,
> -				"unsupported RTS signalling on_send:%d after_send:%d - exactly one of RS485 RTS flags should be set\n",
> -				rts_during_tx, rts_during_rx);
> -

Hm, patch 1 in this series doesn't emit such a message, so unlike now,
users will no longer be warned that they passed in nonsensical settings...

Re: [PATCH 2 1/9] serial: core: move RS485 configuration tasks from drivers into core

[Lino Sanfilippo]

Hi,

On 17.02.22 at 12:33, Lukas Wunner wrote:
> On Wed, Feb 16, 2022 at 01:17:55AM +0100, Lino Sanfilippo wrote:
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -1282,8 +1282,26 @@ static int uart_set_rs485_config(struct uart_port *port,
>>  	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
>>  		return -EFAULT;
>>
>> +	/* pick sane settings if the user hasn't */
>> +	if (!(rs485.flags & SER_RS485_RTS_ON_SEND) ==
>> +	    !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) {
>> +		rs485.flags |= SER_RS485_RTS_ON_SEND;
>> +		rs485.flags &= ~SER_RS485_RTS_AFTER_SEND;
>> +	}
>
> The policy you're enforcing here is:  If settings are nonsensical,
> always default to active-high polarity.
>
> However some drivers currently enforce a completely different policy:
> E.g. with 8250_lpc18xx.c, if SER_RS485_RTS_ON_SEND is set, use active-high
> (and fix up SER_RS485_RTS_AFTER_SEND), else use active-low polarity.
> This yields a different result compared to your policy in case both bits
> are cleared.
>> Similarly, sc16is7xx.c defaults to active-low if SER_RS485_RTS_AFTER_SEND
> is set, else active-high polarity.  This yields a different result compared
> to your policy in case both bits are set.
>
> You risk breaking existing user space applications with this change
> if those applications specify nonsensical polarity settings.
>

Ok, but IMHO this is a very small risk. I cannot imagine that there
are many (or any at all) userspace applications that do not
specify any RTS setting and then rely on a driver specific fallback
implementation. I would not like to remove the RTS check from
uart_set_rs485_config() only because of that.

>
> I happen to have created a similar commit to this one a month ago
> and I came to the conclusion that all one can do is offer a library
> function uart_sanitize_rs485_mode() which drivers may elect to call
> if that helper's policy is identical to what they're doing now:
>
> https://github.com/l1k/linux/commit/637984111e42
>>
>> +
>> +	rs485.delay_rts_before_send = min_t(unsigned int,
>> +					    rs485.delay_rts_before_send,
>> +					    SER_RS485_MAX_RTS_DELAY);
>> +	rs485.delay_rts_after_send = min_t(unsigned int,
>> +					   rs485.delay_rts_after_send,
>> +					   SER_RS485_MAX_RTS_DELAY);
>
> Nonsensical delays may not only be passed in from user space via ioctl(),
> but through the device tree.  That's another reason to use a library
> function:  It can be called both from the ioctl() as well as after (or in)
> uart_get_rs485_mode().
>

The idea of my patch set is actually to provide a common behavior for the
RS485 configuration by userspace similar to what uart_get_rs485_mode()
provides for the configuration by device tree.

However with the solution you propose sanity checks for userspace
configuration are still up to each single driver and thus can vary
from driver to driver or not be implemented at all. I dont think that
this is the better approach in the long term.


>
>> +	/* Return clean padding area to userspace */
>> +	memset(rs485.padding, 0, sizeof(rs485.padding));
>
> Unlike the polarity and delay handling, this one makes sense.
>
> Thanks,
>
> Lukas
>

Regards,
Lino

Re: [PATCH 2 4/9] serial: sc16is7xx: remove redundant check in rs485_config

[Lino Sanfilippo]

Hi,

On 17.02.22 at 12:47, Lukas Wunner wrote:
> On Wed, Feb 16, 2022 at 01:17:58AM +0100, Lino Sanfilippo wrote:
>> --- a/drivers/tty/serial/sc16is7xx.c
>> +++ b/drivers/tty/serial/sc16is7xx.c
>> @@ -959,16 +959,6 @@ static int sc16is7xx_config_rs485(struct uart_port *port,
>>  	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
>>
>>  	if (rs485->flags & SER_RS485_ENABLED) {
>> -		bool rts_during_rx, rts_during_tx;
>> -
>> -		rts_during_rx = rs485->flags & SER_RS485_RTS_AFTER_SEND;
>> -		rts_during_tx = rs485->flags & SER_RS485_RTS_ON_SEND;
>> -
>> -		if (rts_during_rx == rts_during_tx)
>> -			dev_err(port->dev,
>> -				"unsupported RTS signalling on_send:%d after_send:%d - exactly one of RS485 RTS flags should be set\n",
>> -				rts_during_tx, rts_during_rx);
>> -
>
> Hm, patch 1 in this series doesn't emit such a message, so unlike now,
> users will no longer be warned that they passed in nonsensical settings...
>

what about logging a warning for both RTS polarity and delay correction?

Regards,
Lino

Re: [PATCH 2 1/9] serial: core: move RS485 configuration tasks from drivers into core

[Greg KH]

On Wed, Feb 16, 2022 at 01:17:55AM +0100, Lino Sanfilippo wrote:
> Several drivers that support setting the RS485 configuration via userspace
> implement one or more of the following tasks:
> 
> - in case of an invalid RTS configuration (both RTS after send and RTS on
>   send set or both unset) fall back to enable RTS on send and disable RTS
>   after send
> 
> - nullify the padding field of the returned serial_rs485 struct
> 
> - copy the configuration into the uart port struct
> 
> - limit RTS delays to 100 ms
> 
> Move these tasks into the serial core to make them generic and to provide
> a consistent behaviour among all drivers.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---
>  drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++
>  include/uapi/linux/serial.h      |  3 +++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 846192a7b4bf..a4f7e847d414 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1282,8 +1282,26 @@ static int uart_set_rs485_config(struct uart_port *port,
>  	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
>  		return -EFAULT;
>  
> +	/* pick sane settings if the user hasn't */
> +	if (!(rs485.flags & SER_RS485_RTS_ON_SEND) ==
> +	    !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) {
> +		rs485.flags |= SER_RS485_RTS_ON_SEND;
> +		rs485.flags &= ~SER_RS485_RTS_AFTER_SEND;
> +	}
> +
> +	rs485.delay_rts_before_send = min_t(unsigned int,
> +					    rs485.delay_rts_before_send,
> +					    SER_RS485_MAX_RTS_DELAY);
> +	rs485.delay_rts_after_send = min_t(unsigned int,
> +					   rs485.delay_rts_after_send,
> +					   SER_RS485_MAX_RTS_DELAY);
> +	/* Return clean padding area to userspace */
> +	memset(rs485.padding, 0, sizeof(rs485.padding));
> +
>  	spin_lock_irqsave(&port->lock, flags);
>  	ret = port->rs485_config(port, &rs485);
> +	if (!ret)
> +		port->rs485 = rs485;
>  	spin_unlock_irqrestore(&port->lock, flags);
>  	if (ret)
>  		return ret;
> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> index fa6b16e5fdd8..859045a53231 100644
> --- a/include/uapi/linux/serial.h
> +++ b/include/uapi/linux/serial.h
> @@ -128,6 +128,9 @@ struct serial_rs485 {
>  							   (if supported) */
>  	__u32	delay_rts_before_send;	/* Delay before send (milliseconds) */
>  	__u32	delay_rts_after_send;	/* Delay after send (milliseconds) */
> +#define SER_RS485_MAX_RTS_DELAY		100		/* Max time with active
> +							   RTS before/after
> +							   data sent (msecs) */

Why is this a userspace value now?  What can userspace do with this
number?  Once we add this, it's fixed for forever.

thanks,

greg k-h

Re: [PATCH 2 1/9] serial: core: move RS485 configuration tasks from drivers into core

[Lino Sanfilippo]

Hi,

On 21.02.22 at 19:39, Greg KH wrote:

>> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
>> index fa6b16e5fdd8..859045a53231 100644
>> --- a/include/uapi/linux/serial.h
>> +++ b/include/uapi/linux/serial.h
>> @@ -128,6 +128,9 @@ struct serial_rs485 {
>>  							   (if supported) */
>>  	__u32	delay_rts_before_send;	/* Delay before send (milliseconds) */
>>  	__u32	delay_rts_after_send;	/* Delay after send (milliseconds) */
>> +#define SER_RS485_MAX_RTS_DELAY		100		/* Max time with active
>> +							   RTS before/after
>> +							   data sent (msecs) */
>
> Why is this a userspace value now?  What can userspace do with this
> number?  Once we add this, it's fixed for forever.
>
> thanks,
>
> greg k-h
>


Ok, I think we do not really need to expose it to userspace, since we
clamp the delay anyway if it is too big. I will correct this in the next
patch version.

Regards,
Lino