2022-02-16 02:55:30

by Lino Sanfilippo

[permalink] [raw]
Subject: Move RS485 implementation from drivers to serial core

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)




2022-02-16 06:14:14

by Lino Sanfilippo

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

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 <[email protected]>
---
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

2022-02-16 06:30:33

by Lino Sanfilippo

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

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 <[email protected]>
---
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

2022-02-16 07:07:59

by Lino Sanfilippo

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

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 <[email protected]>
---
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

2022-02-16 07:26:51

by Lino Sanfilippo

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

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 <[email protected]>
---
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

2022-02-16 07:28:59

by Lino Sanfilippo

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

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 <[email protected]>
---
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

2022-02-17 12:15:08

by Lukas Wunner

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

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

2022-02-17 13:46:26

by Richard Genoud

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


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 <[email protected]>
Acked-by: Richard Genoud <[email protected]>

> ---
> 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 !

2022-02-17 15:50:42

by Lukas Wunner

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

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...

2022-02-17 23:23:33

by Lino Sanfilippo

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

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

2022-02-18 00:05:18

by Lino Sanfilippo

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


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

2022-02-22 04:48:49

by Greg Kroah-Hartman

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

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 <[email protected]>
> ---
> 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

2022-02-22 05:23:55

by Lino Sanfilippo

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


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