[PATCH v5 0/7] Fixes and improvements for RS485

[PATCH v5 0/7] Fixes and improvements for RS485

[Lino Sanfilippo]

The following series includes some fixes and improvements around RS485 in
the serial core and UART drivers:

Patch 1: Do not hold the port lock when setting rx-during-tx GPIO
Patch 2: set missing supported flag for RX during TX GPIO
Patch 3: fix sanitizing check for RTS settings
Patch 4: make sure RS485 is cannot be enabled when it is not supported
Patch 5: imx: do not set RS485 enabled if it is not supported
Patch 6: omap: do not override settings for rs485 support
Patch 7: exar: set missing RS485 supported flag

Changes in v5:

- do not combine the functions that the RS484 GPIOs (as Hugo originally
  suggested)

Changes in v4:

- add comment for function uart_set_rs485_gpios after hint from Hugo
- correct commit message as pointed out by Hugo
- rephrase commit messages
- add patch 7 after discussion with Ilpo

Changes in v3
- Drop patch "Get rid of useless wrapper pl011_get_rs485_mode()" as
  requested by Greg

Changes in v2:
- add missing 'Fixes' tags as requested by Greg
- corrected a typo as pointed out by Hugo
- fix issue in imx driver in the serial core as suggested by Uwe
- partly rephrase some commit messages
- add patch 7


Lino Sanfilippo (7):
  serial: Do not hold the port lock when setting rx-during-tx GPIO
  serial: core: set missing supported flag for RX during TX GPIO
  serial: core: fix sanitizing check for RTS settings
  serial: core: make sure RS485 cannot be enabled when it is not
    supported
  serial: core, imx: do not set RS485 enabled if it is not supported
  serial: omap: do not override settings for RS485 support
  serial: 8250_exar: Set missing rs485_supported flag

 drivers/tty/serial/8250/8250_exar.c |  5 +--
 drivers/tty/serial/imx.c            |  8 -----
 drivers/tty/serial/omap-serial.c    |  8 ++---
 drivers/tty/serial/serial_core.c    | 50 ++++++++++++++++++++++-------
 drivers/tty/serial/stm32-usart.c    |  5 +--
 5 files changed, 47 insertions(+), 29 deletions(-)


base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a
-- 
2.42.0

[PATCH v5 1/7] serial: Do not hold the port lock when setting rx-during-tx GPIO

[Lino Sanfilippo]

Both the imx and stm32 driver set the rx-during-tx GPIO in rs485_config().
Since this function is called with the port lock held, this can be an
problem in case that setting the GPIO line can sleep (e.g. if a GPIO
expander is used which is connected via SPI or I2C).

Avoid this issue by moving the GPIO setting outside of the port lock into
the serial core and thus making it a generic feature.

Fixes: c54d48543689 ("serial: stm32: Add support for rs485 RX_DURING_TX output GPIO")
Fixes: ca530cfa968c ("serial: imx: Add support for RS485 RX_DURING_TX output GPIO")
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: stable@vger.kernel.org
Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/tty/serial/imx.c         |  4 ----
 drivers/tty/serial/serial_core.c | 12 ++++++++++++
 drivers/tty/serial/stm32-usart.c |  5 +----
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 708b9852a575..9cffeb23112b 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1943,10 +1943,6 @@ static int imx_uart_rs485_config(struct uart_port *port, struct ktermios *termio
 	    rs485conf->flags & SER_RS485_RX_DURING_TX)
 		imx_uart_start_rx(port);
 
-	if (port->rs485_rx_during_tx_gpio)
-		gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
-					 !!(rs485conf->flags & SER_RS485_RX_DURING_TX));
-
 	return 0;
 }
 
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f1348a509552..a0290a5fe8b3 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1402,6 +1402,16 @@ static void uart_set_rs485_termination(struct uart_port *port,
 				 !!(rs485->flags & SER_RS485_TERMINATE_BUS));
 }
 
+static void uart_set_rs485_rx_during_tx(struct uart_port *port,
+					const struct serial_rs485 *rs485)
+{
+	if (!(rs485->flags & SER_RS485_ENABLED))
+		return;
+
+	gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
+				 !!(rs485->flags & SER_RS485_RX_DURING_TX));
+}
+
 static int uart_rs485_config(struct uart_port *port)
 {
 	struct serial_rs485 *rs485 = &port->rs485;
@@ -1413,6 +1423,7 @@ static int uart_rs485_config(struct uart_port *port)
 
 	uart_sanitize_serial_rs485(port, rs485);
 	uart_set_rs485_termination(port, rs485);
+	uart_set_rs485_rx_during_tx(port, rs485);
 
 	uart_port_lock_irqsave(port, &flags);
 	ret = port->rs485_config(port, NULL, rs485);
@@ -1457,6 +1468,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port,
 		return ret;
 	uart_sanitize_serial_rs485(port, &rs485);
 	uart_set_rs485_termination(port, &rs485);
+	uart_set_rs485_rx_during_tx(port, &rs485);
 
 	uart_port_lock_irqsave(port, &flags);
 	ret = port->rs485_config(port, &tty->termios, &rs485);
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 3048620315d6..ec9a72a5bea9 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -226,10 +226,7 @@ static int stm32_usart_config_rs485(struct uart_port *port, struct ktermios *ter
 
 	stm32_usart_clr_bits(port, ofs->cr1, BIT(cfg->uart_enable_bit));
 
-	if (port->rs485_rx_during_tx_gpio)
-		gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
-					 !!(rs485conf->flags & SER_RS485_RX_DURING_TX));
-	else
+	if (!port->rs485_rx_during_tx_gpio)
 		rs485conf->flags |= SER_RS485_RX_DURING_TX;
 
 	if (rs485conf->flags & SER_RS485_ENABLED) {
-- 
2.42.0

Re: [PATCH v5 1/7] serial: Do not hold the port lock when setting rx-during-tx GPIO

[Ilpo Järvinen]

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

On Sat, 9 Dec 2023, Lino Sanfilippo wrote:

> Both the imx and stm32 driver set the rx-during-tx GPIO in rs485_config().
> Since this function is called with the port lock held, this can be an
> problem in case that setting the GPIO line can sleep (e.g. if a GPIO
> expander is used which is connected via SPI or I2C).
> 
> Avoid this issue by moving the GPIO setting outside of the port lock into
> the serial core and thus making it a generic feature.
> 
> Fixes: c54d48543689 ("serial: stm32: Add support for rs485 RX_DURING_TX output GPIO")
> Fixes: ca530cfa968c ("serial: imx: Add support for RS485 RX_DURING_TX output GPIO")
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: stable@vger.kernel.org
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/tty/serial/imx.c         |  4 ----
>  drivers/tty/serial/serial_core.c | 12 ++++++++++++
>  drivers/tty/serial/stm32-usart.c |  5 +----
>  3 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 708b9852a575..9cffeb23112b 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1943,10 +1943,6 @@ static int imx_uart_rs485_config(struct uart_port *port, struct ktermios *termio
>  	    rs485conf->flags & SER_RS485_RX_DURING_TX)
>  		imx_uart_start_rx(port);
>  
> -	if (port->rs485_rx_during_tx_gpio)
> -		gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
> -					 !!(rs485conf->flags & SER_RS485_RX_DURING_TX));
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index f1348a509552..a0290a5fe8b3 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1402,6 +1402,16 @@ static void uart_set_rs485_termination(struct uart_port *port,
>  				 !!(rs485->flags & SER_RS485_TERMINATE_BUS));
>  }
>  
> +static void uart_set_rs485_rx_during_tx(struct uart_port *port,
> +					const struct serial_rs485 *rs485)
> +{
> +	if (!(rs485->flags & SER_RS485_ENABLED))
> +		return;
> +
> +	gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
> +				 !!(rs485->flags & SER_RS485_RX_DURING_TX));
> +}
> +
>  static int uart_rs485_config(struct uart_port *port)
>  {
>  	struct serial_rs485 *rs485 = &port->rs485;
> @@ -1413,6 +1423,7 @@ static int uart_rs485_config(struct uart_port *port)
>  
>  	uart_sanitize_serial_rs485(port, rs485);
>  	uart_set_rs485_termination(port, rs485);
> +	uart_set_rs485_rx_during_tx(port, rs485);
>  
>  	uart_port_lock_irqsave(port, &flags);
>  	ret = port->rs485_config(port, NULL, rs485);
> @@ -1457,6 +1468,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port,
>  		return ret;
>  	uart_sanitize_serial_rs485(port, &rs485);
>  	uart_set_rs485_termination(port, &rs485);
> +	uart_set_rs485_rx_during_tx(port, &rs485);
>
>  	uart_port_lock_irqsave(port, &flags);
>  	ret = port->rs485_config(port, &tty->termios, &rs485);

Also a nice simplification of driver-side code.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Just noting since this is now in core that if ->rs485_config() fails,
I suppose it's just normal to not rollback gpiod_set_value_cansleep() 
(skimming through existing users in tree, it looks it's practically
never touched on the error rollback paths so I guess it's the normal
practice)?

Anyway, since neither of the users currently don't fail in their 
->rs485_config() so it doesn't seem a critical issue.


-- 
 i.

Re: [PATCH v5 1/7] serial: Do not hold the port lock when setting rx-during-tx GPIO

[Lino Sanfilippo]

Hi,

On 11.12.23 11:35, Ilpo Järvinen wrote:
> On Sat, 9 Dec 2023, Lino Sanfilippo wrote:
>
>> Both the imx and stm32 driver set the rx-during-tx GPIO in rs485_config().
>> Since this function is called with the port lock held, this can be an
>> problem in case that setting the GPIO line can sleep (e.g. if a GPIO
>> expander is used which is connected via SPI or I2C).
>>
>> Avoid this issue by moving the GPIO setting outside of the port lock into
>> the serial core and thus making it a generic feature.
>>
>> Fixes: c54d48543689 ("serial: stm32: Add support for rs485 RX_DURING_TX output GPIO")
>> Fixes: ca530cfa968c ("serial: imx: Add support for RS485 RX_DURING_TX output GPIO")
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>> ---
>>  drivers/tty/serial/imx.c         |  4 ----
>>  drivers/tty/serial/serial_core.c | 12 ++++++++++++
>>  drivers/tty/serial/stm32-usart.c |  5 +----
>>  3 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 708b9852a575..9cffeb23112b 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -1943,10 +1943,6 @@ static int imx_uart_rs485_config(struct uart_port *port, struct ktermios *termio
>>  	    rs485conf->flags & SER_RS485_RX_DURING_TX)
>>  		imx_uart_start_rx(port);
>>
>> -	if (port->rs485_rx_during_tx_gpio)
>> -		gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
>> -					 !!(rs485conf->flags & SER_RS485_RX_DURING_TX));
>> -
>>  	return 0;
>>  }
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index f1348a509552..a0290a5fe8b3 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -1402,6 +1402,16 @@ static void uart_set_rs485_termination(struct uart_port *port,
>>  				 !!(rs485->flags & SER_RS485_TERMINATE_BUS));
>>  }
>>
>> +static void uart_set_rs485_rx_during_tx(struct uart_port *port,
>> +					const struct serial_rs485 *rs485)
>> +{
>> +	if (!(rs485->flags & SER_RS485_ENABLED))
>> +		return;
>> +
>> +	gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
>> +				 !!(rs485->flags & SER_RS485_RX_DURING_TX));
>> +}
>> +
>>  static int uart_rs485_config(struct uart_port *port)
>>  {
>>  	struct serial_rs485 *rs485 = &port->rs485;
>> @@ -1413,6 +1423,7 @@ static int uart_rs485_config(struct uart_port *port)
>>
>>  	uart_sanitize_serial_rs485(port, rs485);
>>  	uart_set_rs485_termination(port, rs485);
>> +	uart_set_rs485_rx_during_tx(port, rs485);
>>
>>  	uart_port_lock_irqsave(port, &flags);
>>  	ret = port->rs485_config(port, NULL, rs485);
>> @@ -1457,6 +1468,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port,
>>  		return ret;
>>  	uart_sanitize_serial_rs485(port, &rs485);
>>  	uart_set_rs485_termination(port, &rs485);
>> +	uart_set_rs485_rx_during_tx(port, &rs485);
>>
>>  	uart_port_lock_irqsave(port, &flags);
>>  	ret = port->rs485_config(port, &tty->termios, &rs485);
>
> Also a nice simplification of driver-side code.
>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>
> Just noting since this is now in core that if ->rs485_config() fails,
> I suppose it's just normal to not rollback gpiod_set_value_cansleep()
> (skimming through existing users in tree, it looks it's practically
> never touched on the error rollback paths so I guess it's the normal
> practice)?
>
> Anyway, since neither of the users currently don't fail in their
> ->rs485_config() so it doesn't seem a critical issue.
>

Thats a good point actually. Rolling back is not hard to implement and
although it may not matter right now since currently no driver returns an error
code, this can change very soon.
So I will rework this patch for the next version, thanks!

Regards,
Lino

Re: [PATCH v5 1/7] serial: Do not hold the port lock when setting rx-during-tx GPIO

[Hugo Villeneuve]

On Sat,  9 Dec 2023 13:58:30 +0100
Lino Sanfilippo <l.sanfilippo@kunbus.com> wrote:

> Both the imx and stm32 driver set the rx-during-tx GPIO in rs485_config().
> Since this function is called with the port lock held, this can be an
> problem in case that setting the GPIO line can sleep (e.g. if a GPIO
> expander is used which is connected via SPI or I2C).
> 
> Avoid this issue by moving the GPIO setting outside of the port lock into
> the serial core and thus making it a generic feature.
> 
> Fixes: c54d48543689 ("serial: stm32: Add support for rs485 RX_DURING_TX output GPIO")
> Fixes: ca530cfa968c ("serial: imx: Add support for RS485 RX_DURING_TX output GPIO")
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: stable@vger.kernel.org
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/tty/serial/imx.c         |  4 ----
>  drivers/tty/serial/serial_core.c | 12 ++++++++++++
>  drivers/tty/serial/stm32-usart.c |  5 +----
>  3 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 708b9852a575..9cffeb23112b 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1943,10 +1943,6 @@ static int imx_uart_rs485_config(struct uart_port *port, struct ktermios *termio
>  	    rs485conf->flags & SER_RS485_RX_DURING_TX)
>  		imx_uart_start_rx(port);
>  
> -	if (port->rs485_rx_during_tx_gpio)
> -		gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
> -					 !!(rs485conf->flags & SER_RS485_RX_DURING_TX));
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index f1348a509552..a0290a5fe8b3 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1402,6 +1402,16 @@ static void uart_set_rs485_termination(struct uart_port *port,
>  				 !!(rs485->flags & SER_RS485_TERMINATE_BUS));
>  }
>  
> +static void uart_set_rs485_rx_during_tx(struct uart_port *port,
> +					const struct serial_rs485 *rs485)
> +{
> +	if (!(rs485->flags & SER_RS485_ENABLED))
> +		return;
> +
> +	gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
> +				 !!(rs485->flags & SER_RS485_RX_DURING_TX));
> +}
> +
>  static int uart_rs485_config(struct uart_port *port)
>  {
>  	struct serial_rs485 *rs485 = &port->rs485;
> @@ -1413,6 +1423,7 @@ static int uart_rs485_config(struct uart_port *port)
>  
>  	uart_sanitize_serial_rs485(port, rs485);
>  	uart_set_rs485_termination(port, rs485);
> +	uart_set_rs485_rx_during_tx(port, rs485);
>  
>  	uart_port_lock_irqsave(port, &flags);
>  	ret = port->rs485_config(port, NULL, rs485);
> @@ -1457,6 +1468,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port,
>  		return ret;
>  	uart_sanitize_serial_rs485(port, &rs485);
>  	uart_set_rs485_termination(port, &rs485);
> +	uart_set_rs485_rx_during_tx(port, &rs485);
>  
>  	uart_port_lock_irqsave(port, &flags);
>  	ret = port->rs485_config(port, &tty->termios, &rs485);
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index 3048620315d6..ec9a72a5bea9 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -226,10 +226,7 @@ static int stm32_usart_config_rs485(struct uart_port *port, struct ktermios *ter
>  
>  	stm32_usart_clr_bits(port, ofs->cr1, BIT(cfg->uart_enable_bit));
>  
> -	if (port->rs485_rx_during_tx_gpio)
> -		gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
> -					 !!(rs485conf->flags & SER_RS485_RX_DURING_TX));
> -	else
> +	if (!port->rs485_rx_during_tx_gpio)
>  		rs485conf->flags |= SER_RS485_RX_DURING_TX;
>  
>  	if (rs485conf->flags & SER_RS485_ENABLED) {
> -- 
> 2.42.0
> 

Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Hugo

[PATCH v5 2/7] serial: core: set missing supported flag for RX during TX GPIO

[Lino Sanfilippo]

If the RS485 feature RX-during-TX is supported by means of a GPIO set the
according supported flag. Otherwise setting this feature from userspace may
not be possible, since in uart_sanitize_serial_rs485() the passed RS485
configuration is matched against the supported features and unsupported
settings are thereby removed and thus take no effect.

Cc: stable@vger.kernel.org
Fixes: 163f080eb717 ("serial: core: Add option to output RS485 RX_DURING_TX state via GPIO")
Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/tty/serial/serial_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a0290a5fe8b3..c254e88c8452 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3632,6 +3632,8 @@ int uart_get_rs485_mode(struct uart_port *port)
 	if (IS_ERR(desc))
 		return dev_err_probe(dev, PTR_ERR(desc), "Cannot get rs485-rx-during-tx-gpios\n");
 	port->rs485_rx_during_tx_gpio = desc;
+	if (port->rs485_rx_during_tx_gpio)
+		port->rs485_supported.flags |= SER_RS485_RX_DURING_TX;
 
 	return 0;
 }
-- 
2.42.0

Re: [PATCH v5 2/7] serial: core: set missing supported flag for RX during TX GPIO

[Ilpo Järvinen]

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

On Sat, 9 Dec 2023, Lino Sanfilippo wrote:

> If the RS485 feature RX-during-TX is supported by means of a GPIO set the
> according supported flag. Otherwise setting this feature from userspace may
> not be possible, since in uart_sanitize_serial_rs485() the passed RS485
> configuration is matched against the supported features and unsupported
> settings are thereby removed and thus take no effect.
> 
> Cc: stable@vger.kernel.org
> Fixes: 163f080eb717 ("serial: core: Add option to output RS485 RX_DURING_TX state via GPIO")
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/tty/serial/serial_core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index a0290a5fe8b3..c254e88c8452 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3632,6 +3632,8 @@ int uart_get_rs485_mode(struct uart_port *port)
>  	if (IS_ERR(desc))
>  		return dev_err_probe(dev, PTR_ERR(desc), "Cannot get rs485-rx-during-tx-gpios\n");
>  	port->rs485_rx_during_tx_gpio = desc;
> +	if (port->rs485_rx_during_tx_gpio)
> +		port->rs485_supported.flags |= SER_RS485_RX_DURING_TX;
>  
>  	return 0;
>  }

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v5 3/7] serial: core: fix sanitizing check for RTS settings

[Lino Sanfilippo]

Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS
settings in a RS485 configuration that has been passed by userspace.
If RTS-on-send and RTS-after-send are both set or unset the configuration
is adjusted and RTS-after-send is disabled and RTS-on-send enabled.

This however makes only sense if both RTS modes are actually supported by
the driver.

With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does
take the driver support into account but only checks if one of both RTS
modes are supported. This may lead to the errorneous result of RTS-on-send
being set even if only RTS-after-send is supported.

Fix this by changing the implemented logic: First clear all unsupported
flags in the RS485 configuration, then adjust an invalid RTS setting by
taking into account which RTS mode is supported.

Cc: stable@vger.kernel.org
Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct")
Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c254e88c8452..4eae1406cb6c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1371,19 +1371,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
 		return;
 	}
 
+	rs485->flags &= supported_flags;
+
 	/* Pick sane settings if the user hasn't */
-	if ((supported_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)) {
-		dev_warn_ratelimited(port->dev,
-			"%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
-			port->name, port->line);
-		rs485->flags |= SER_RS485_RTS_ON_SEND;
-		rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
-		supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
-	}
+		if (supported_flags & SER_RS485_RTS_ON_SEND) {
+			rs485->flags |= SER_RS485_RTS_ON_SEND;
+			rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
 
-	rs485->flags &= supported_flags;
+			dev_warn_ratelimited(port->dev,
+				"%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
+				port->name, port->line);
+		} else {
+			rs485->flags |= SER_RS485_RTS_AFTER_SEND;
+			rs485->flags &= ~SER_RS485_RTS_ON_SEND;
+
+			dev_warn_ratelimited(port->dev,
+				"%s (%d): invalid RTS setting, using RTS_AFTER_SEND instead\n",
+				port->name, port->line);
+		}
+	}
 
 	uart_sanitize_serial_rs485_delays(port, rs485);
 
-- 
2.42.0

Re: [PATCH v5 3/7] serial: core: fix sanitizing check for RTS settings

[Ilpo Järvinen]

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

On Sat, 9 Dec 2023, Lino Sanfilippo wrote:

> Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS
> settings in a RS485 configuration that has been passed by userspace.
> If RTS-on-send and RTS-after-send are both set or unset the configuration
> is adjusted and RTS-after-send is disabled and RTS-on-send enabled.
> 
> This however makes only sense if both RTS modes are actually supported by
> the driver.
> 
> With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does
> take the driver support into account but only checks if one of both RTS
> modes are supported. This may lead to the errorneous result of RTS-on-send
> being set even if only RTS-after-send is supported.
> 
> Fix this by changing the implemented logic: First clear all unsupported
> flags in the RS485 configuration, then adjust an invalid RTS setting by
> taking into account which RTS mode is supported.
> 
> Cc: stable@vger.kernel.org
> Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct")
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c254e88c8452..4eae1406cb6c 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1371,19 +1371,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
>  		return;
>  	}
>  
> +	rs485->flags &= supported_flags;
> +
>  	/* Pick sane settings if the user hasn't */
> -	if ((supported_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)) {
> -		dev_warn_ratelimited(port->dev,
> -			"%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
> -			port->name, port->line);
> -		rs485->flags |= SER_RS485_RTS_ON_SEND;
> -		rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> -		supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
> -	}
> +		if (supported_flags & SER_RS485_RTS_ON_SEND) {
> +			rs485->flags |= SER_RS485_RTS_ON_SEND;
> +			rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
>  
> -	rs485->flags &= supported_flags;
> +			dev_warn_ratelimited(port->dev,
> +				"%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
> +				port->name, port->line);
> +		} else {
> +			rs485->flags |= SER_RS485_RTS_AFTER_SEND;
> +			rs485->flags &= ~SER_RS485_RTS_ON_SEND;
> +
> +			dev_warn_ratelimited(port->dev,
> +				"%s (%d): invalid RTS setting, using RTS_AFTER_SEND instead\n",
> +				port->name, port->line);
> +		}
> +	}

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v5 4/7] serial: core: make sure RS485 cannot be enabled when it is not supported

[Lino Sanfilippo]

Some uart drivers specify a rs485_config() function and then decide later
to disable RS485 support for some reason (e.g. imx and ar933).

In these cases userspace may be able to activate RS485 via TIOCSRS485
nevertheless, since in uart_set_rs485_config() an existing rs485_config()
function indicates that RS485 is supported.

Make sure that this is not longer possible by checking the uarts
rs485_supported.flags instead and bailing out if SER_RS485_ENABLED is not
set.

Furthermore instead of returning an empty structure return -ENOTTY if the
RS485 configuration is requested via TIOCGRS485 but RS485 is not supported.
This has a small impact on userspace visibility but it is consistent with
the -ENOTTY error for TIOCGRS485.

Fixes: e849145e1fdd ("serial: ar933x: Fill in rs485_supported")
Fixes: 55e18c6b6d42 ("serial: imx: Remove serial_rs485 sanitization")
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: stable@vger.kernel.org
Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/tty/serial/serial_core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 4eae1406cb6c..661074ab8edb 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1448,6 +1448,9 @@ static int uart_get_rs485_config(struct uart_port *port,
 	unsigned long flags;
 	struct serial_rs485 aux;
 
+	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
+		return -ENOTTY;
+
 	uart_port_lock_irqsave(port, &flags);
 	aux = port->rs485;
 	uart_port_unlock_irqrestore(port, flags);
@@ -1465,7 +1468,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port,
 	int ret;
 	unsigned long flags;
 
-	if (!port->rs485_config)
+	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
 		return -ENOTTY;
 
 	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
-- 
2.42.0

Re: [PATCH v5 4/7] serial: core: make sure RS485 cannot be enabled when it is not supported

[Ilpo Järvinen]

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

On Sat, 9 Dec 2023, Lino Sanfilippo wrote:

> Some uart drivers specify a rs485_config() function and then decide later
> to disable RS485 support for some reason (e.g. imx and ar933).
> 
> In these cases userspace may be able to activate RS485 via TIOCSRS485
> nevertheless, since in uart_set_rs485_config() an existing rs485_config()
> function indicates that RS485 is supported.
> 
> Make sure that this is not longer possible by checking the uarts
> rs485_supported.flags instead and bailing out if SER_RS485_ENABLED is not
> set.
> 
> Furthermore instead of returning an empty structure return -ENOTTY if the
> RS485 configuration is requested via TIOCGRS485 but RS485 is not supported.
> This has a small impact on userspace visibility but it is consistent with
> the -ENOTTY error for TIOCGRS485.
> 
> Fixes: e849145e1fdd ("serial: ar933x: Fill in rs485_supported")
> Fixes: 55e18c6b6d42 ("serial: imx: Remove serial_rs485 sanitization")
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: stable@vger.kernel.org
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/tty/serial/serial_core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 4eae1406cb6c..661074ab8edb 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1448,6 +1448,9 @@ static int uart_get_rs485_config(struct uart_port *port,
>  	unsigned long flags;
>  	struct serial_rs485 aux;
>  
> +	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
> +		return -ENOTTY;
> +
>  	uart_port_lock_irqsave(port, &flags);
>  	aux = port->rs485;
>  	uart_port_unlock_irqrestore(port, flags);
> @@ -1465,7 +1468,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port,
>  	int ret;
>  	unsigned long flags;
>  
> -	if (!port->rs485_config)
> +	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
>  		return -ENOTTY;
>  
>  	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))

Looking through debian code search entries for TIOCGRS485, this might 
actually fly... I'd suggest splitting this into two patches though.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH v5 4/7] serial: core: make sure RS485 cannot be enabled when it is not supported

[Lino Sanfilippo]

On 11.12.23 11:53, Ilpo Järvinen wrote:
> On Sat, 9 Dec 2023, Lino Sanfilippo wrote:
>
>> Some uart drivers specify a rs485_config() function and then decide later
>> to disable RS485 support for some reason (e.g. imx and ar933).
>>
>> In these cases userspace may be able to activate RS485 via TIOCSRS485
>> nevertheless, since in uart_set_rs485_config() an existing rs485_config()
>> function indicates that RS485 is supported.
>>
>> Make sure that this is not longer possible by checking the uarts
>> rs485_supported.flags instead and bailing out if SER_RS485_ENABLED is not
>> set.
>>
>> Furthermore instead of returning an empty structure return -ENOTTY if the
>> RS485 configuration is requested via TIOCGRS485 but RS485 is not supported.
>> This has a small impact on userspace visibility but it is consistent with
>> the -ENOTTY error for TIOCGRS485.
>>
>> Fixes: e849145e1fdd ("serial: ar933x: Fill in rs485_supported")
>> Fixes: 55e18c6b6d42 ("serial: imx: Remove serial_rs485 sanitization")
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>> ---
>>  drivers/tty/serial/serial_core.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 4eae1406cb6c..661074ab8edb 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -1448,6 +1448,9 @@ static int uart_get_rs485_config(struct uart_port *port,
>>  	unsigned long flags;
>>  	struct serial_rs485 aux;
>>
>> +	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
>> +		return -ENOTTY;
>> +
>>  	uart_port_lock_irqsave(port, &flags);
>>  	aux = port->rs485;
>>  	uart_port_unlock_irqrestore(port, flags);
>> @@ -1465,7 +1468,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port,
>>  	int ret;
>>  	unsigned long flags;
>>
>> -	if (!port->rs485_config)
>> +	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
>>  		return -ENOTTY;
>>
>>  	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
>
> Looking through debian code search entries for TIOCGRS485, this might
> actually fly... I'd suggest splitting this into two patches though.

Ok. I will split this into two patches or maybe even leave the change for uart_get_rs485_config()
completely out of this series (which is only about bug fixes) for now. Thanks!

[PATCH v5 5/7] serial: core, imx: do not set RS485 enabled if it is not supported

[Lino Sanfilippo]

If the imx driver cannot support RS485 it sets the ports rs485_supported
structure to NULL. But it still calls uart_get_rs485_mode() which may set
the RS485_ENABLED flag nevertheless.

This may lead to an attempt to configure RS485 even if it is not supported
when the flag is evaluated in uart_configure_port() at port startup.

Avoid this by bailing out of uart_get_rs485_mode() if the RS485_ENABLED
flag is not supported by the caller.

With this fix a check for RTS availability is now obsolete in the imx
driver, since it can not evaluate to true any more. Remove this check, too.

Fixes: 00d7a00e2a6f ("serial: imx: Fill in rs485_supported")
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: stable@vger.kernel.org
Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/tty/serial/imx.c         | 4 ----
 drivers/tty/serial/serial_core.c | 3 +++
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 9cffeb23112b..98b78d360a74 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2328,10 +2328,6 @@ static int imx_uart_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	if (sport->port.rs485.flags & SER_RS485_ENABLED &&
-	    (!sport->have_rtscts && !sport->have_rtsgpio))
-		dev_err(&pdev->dev, "no RTS control, disabling rs485\n");
-
 	/*
 	 * If using the i.MX UART RTS/CTS control then the RTS (CTS_B)
 	 * signal cannot be set low during transmission in case the
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 661074ab8edb..b418952c03df 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3593,6 +3593,9 @@ int uart_get_rs485_mode(struct uart_port *port)
 	u32 rs485_delay[2];
 	int ret;
 
+	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
+		return 0;
+
 	ret = device_property_read_u32_array(dev, "rs485-rts-delay",
 					     rs485_delay, 2);
 	if (!ret) {
-- 
2.42.0

Re: [PATCH v5 5/7] serial: core, imx: do not set RS485 enabled if it is not supported

[Ilpo Järvinen]

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

On Sat, 9 Dec 2023, Lino Sanfilippo wrote:

> If the imx driver cannot support RS485 it sets the ports rs485_supported
> structure to NULL.

No, an embedded struct inside struct uart_port cannot be set to NULL, 
it's always there.

Looking into the code, that setting of rs485_supported from imx_no_rs485 
is actually superfluous as it should be already cleared to zeros on alloc.

> But it still calls uart_get_rs485_mode() which may set
> the RS485_ENABLED flag nevertheless.
> 
> This may lead to an attempt to configure RS485 even if it is not supported
> when the flag is evaluated in uart_configure_port() at port startup.
> 
> Avoid this by bailing out of uart_get_rs485_mode() if the RS485_ENABLED
> flag is not supported by the caller.
> 
> With this fix a check for RTS availability is now obsolete in the imx
> driver, since it can not evaluate to true any more. Remove this check, too.
> 
> Fixes: 00d7a00e2a6f ("serial: imx: Fill in rs485_supported")
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: stable@vger.kernel.org
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>

-- 
 i.

Re: [PATCH v5 5/7] serial: core, imx: do not set RS485 enabled if it is not supported

[Lino Sanfilippo]

On 11.12.23 12:00, Ilpo Järvinen wrote:
> On Sat, 9 Dec 2023, Lino Sanfilippo wrote:
>
>> If the imx driver cannot support RS485 it sets the ports rs485_supported
>> structure to NULL.
>
> No, an embedded struct inside struct uart_port cannot be set to NULL,
> it's always there.
>

Hmm, ok. What I meant was that the structure is nullified. "set to NULL" is maybe a bit
misleading. I will correct this.

> Looking into the code, that setting of rs485_supported from imx_no_rs485
> is actually superfluous as it should be already cleared to zeros on alloc.
>

Yes. BTW: Another "no_rs485" configuration setting can be found in the ar933x driver.
If we do not want to keep those assignments I can remove the one for the imx
driver with the next version of this patch...

>> But it still calls uart_get_rs485_mode() which may set
>> the RS485_ENABLED flag nevertheless.
>>
>> This may lead to an attempt to configure RS485 even if it is not supported
>> when the flag is evaluated in uart_configure_port() at port startup.
>>
>> Avoid this by bailing out of uart_get_rs485_mode() if the RS485_ENABLED
>> flag is not supported by the caller.
>>
>> With this fix a check for RTS availability is now obsolete in the imx
>> driver, since it can not evaluate to true any more. Remove this check, too.
>>
>> Fixes: 00d7a00e2a6f ("serial: imx: Fill in rs485_supported")
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: stable@vger.kernel.org
>> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>

Re: [PATCH v5 5/7] serial: core, imx: do not set RS485 enabled if it is not supported

[Ilpo Järvinen]

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

On Wed, 13 Dec 2023, Lino Sanfilippo wrote:
> On 11.12.23 12:00, Ilpo Järvinen wrote:
> > On Sat, 9 Dec 2023, Lino Sanfilippo wrote:
> 
> > Looking into the code, that setting of rs485_supported from imx_no_rs485
> > is actually superfluous as it should be already cleared to zeros on alloc.
> >
> 
> Yes. BTW: Another "no_rs485" configuration setting can be found in the ar933x driver.
> If we do not want to keep those assignments I can remove the one for the imx
> driver with the next version of this patch...

I think they can just be dropped as it's normal in Linux code to assume 
that things are zeroed by default. Those "no"-variants originate from the 
time when supported_rs485 was not yet embedded but just a pointer to a 
const struct and I didn't realize I could have removed them when I ended 
up embedding the struct so it can be altered per port.

-- 
 i.

[PATCH v5 6/7] serial: omap: do not override settings for RS485 support

[Lino Sanfilippo]

In serial_omap_rs485() RS485 support may be deactivated due to a missing
RTS GPIO. This is done by nullifying the ports rs485_supported struct.
After that however the serial_omap_rs485_supported struct is assigned to
the same structure unconditionally, which results in an unintended
reactivation of RS485 support.
Fix this by callling serial_omap_rs485() after the assignment of
rs485_supported.

Fixes: e2752ae3cfc9 ("serial: omap: Disallow RS-485 if rts-gpio is not specified")
Cc: stable@vger.kernel.org
Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/tty/serial/omap-serial.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index ad4c1c5d0a7f..d9b2936308c4 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1604,10 +1604,6 @@ static int serial_omap_probe(struct platform_device *pdev)
 		dev_info(up->port.dev, "no wakeirq for uart%d\n",
 			 up->port.line);
 
-	ret = serial_omap_probe_rs485(up, &pdev->dev);
-	if (ret < 0)
-		goto err_rs485;
-
 	sprintf(up->name, "OMAP UART%d", up->port.line);
 	up->port.mapbase = mem->start;
 	up->port.membase = base;
@@ -1622,6 +1618,10 @@ static int serial_omap_probe(struct platform_device *pdev)
 			 DEFAULT_CLK_SPEED);
 	}
 
+	ret = serial_omap_probe_rs485(up, &pdev->dev);
+	if (ret < 0)
+		goto err_rs485;
+
 	up->latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
 	up->calc_latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
 	cpu_latency_qos_add_request(&up->pm_qos_request, up->latency);
-- 
2.42.0

Re: [PATCH v5 6/7] serial: omap: do not override settings for RS485 support

[Ilpo Järvinen]

On Sat, 9 Dec 2023, Lino Sanfilippo wrote:

> In serial_omap_rs485() RS485 support may be deactivated due to a missing

There's no serial_omap_rs485() function. I assume/know you meant 
serial_omap_probe_rs485() but please correct.

> RTS GPIO. This is done by nullifying the ports rs485_supported struct.
> After that however the serial_omap_rs485_supported struct is assigned to
> the same structure unconditionally, which results in an unintended
> reactivation of RS485 support.
>
> Fix this by callling serial_omap_rs485() after the assignment of

callling -> calling.

Again, the function name is incorrect.

> rs485_supported.

Wouldn't it be better if all rs485 init/setups would occur in the same 
place rather than being spread around? That is, move the rs485_config and 
rs485_supported setup into serial_omap_probe_rs485()?

-- 
 i.

> Fixes: e2752ae3cfc9 ("serial: omap: Disallow RS-485 if rts-gpio is not specified")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/tty/serial/omap-serial.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index ad4c1c5d0a7f..d9b2936308c4 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -1604,10 +1604,6 @@ static int serial_omap_probe(struct platform_device *pdev)
>  		dev_info(up->port.dev, "no wakeirq for uart%d\n",
>  			 up->port.line);
>  
> -	ret = serial_omap_probe_rs485(up, &pdev->dev);
> -	if (ret < 0)
> -		goto err_rs485;
> -
>  	sprintf(up->name, "OMAP UART%d", up->port.line);
>  	up->port.mapbase = mem->start;
>  	up->port.membase = base;
> @@ -1622,6 +1618,10 @@ static int serial_omap_probe(struct platform_device *pdev)
>  			 DEFAULT_CLK_SPEED);
>  	}
>  
> +	ret = serial_omap_probe_rs485(up, &pdev->dev);
> +	if (ret < 0)
> +		goto err_rs485;
> +
>  	up->latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
>  	up->calc_latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
>  	cpu_latency_qos_add_request(&up->pm_qos_request, up->latency);
> -- 
> 2.42.0
> 
> 

Re: [PATCH v5 6/7] serial: omap: do not override settings for RS485 support

[Lino Sanfilippo]

On 13.12.23 11:26, Ilpo Järvinen wrote:
> On Sat, 9 Dec 2023, Lino Sanfilippo wrote:
>
>> In serial_omap_rs485() RS485 support may be deactivated due to a missing
>
> There's no serial_omap_rs485() function. I assume/know you meant
> serial_omap_probe_rs485() but please correct.
>

Right, I meant serial_omap_probe_rs485(). Will fix the misnaming as well as
the typos below.

>> RTS GPIO. This is done by nullifying the ports rs485_supported struct.
>> After that however the serial_omap_rs485_supported struct is assigned to
>> the same structure unconditionally, which results in an unintended
>> reactivation of RS485 support.
>>
>> Fix this by callling serial_omap_rs485() after the assignment of
>
> callling -> calling.
>
> Again, the function name is incorrect.
>

>> rs485_supported.
>
> Wouldn't it be better if all rs485 init/setups would occur in the same
> place rather than being spread around? That is, move the rs485_config and
> rs485_supported setup into serial_omap_probe_rs485()?
>

No problem, I can do that. Thanks for the review(s)!

Regards,
Lino

[PATCH v5 7/7] serial: 8250_exar: Set missing rs485_supported flag

[Lino Sanfilippo]

The UART supports an auto-RTS mode in which the RTS pin is automatically
activated during transmission. So mark this mode as being supported even
if RTS is not controlled by the driver but the UART.

Also the serial core expects now at least one of both modes rts-on-send or
rts-after-send to be supported. This is since during sanitization
unsupported flags are deleted from a RS485 configuration set by userspace.
However if the configuration ends up with both flags unset, the core prints
a warning since it considers such a configuration invalid (see
uart_sanitize_serial_rs485()).

Cc: stable@vger.kernel.org
Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/tty/serial/8250/8250_exar.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 6085d356ad86..23366f868ae3 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -480,7 +480,7 @@ static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termio
 }
 
 static const struct serial_rs485 generic_rs485_supported = {
-	.flags = SER_RS485_ENABLED,
+	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
 };
 
 static const struct exar8250_platform exar8250_default_platform = {
@@ -524,7 +524,8 @@ static int iot2040_rs485_config(struct uart_port *port, struct ktermios *termios
 }
 
 static const struct serial_rs485 iot2040_rs485_supported = {
-	.flags = SER_RS485_ENABLED | SER_RS485_RX_DURING_TX | SER_RS485_TERMINATE_BUS,
+	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
+		 SER_RS485_RX_DURING_TX | SER_RS485_TERMINATE_BUS,
 };
 
 static const struct property_entry iot2040_gpio_properties[] = {
-- 
2.42.0

Re: [PATCH v5 7/7] serial: 8250_exar: Set missing rs485_supported flag

[Ilpo Järvinen]

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

On Sat, 9 Dec 2023, Lino Sanfilippo wrote:

> The UART supports an auto-RTS mode in which the RTS pin is automatically
> activated during transmission. So mark this mode as being supported even
> if RTS is not controlled by the driver but the UART.
> 
> Also the serial core expects now at least one of both modes rts-on-send or
> rts-after-send to be supported. This is since during sanitization
> unsupported flags are deleted from a RS485 configuration set by userspace.
> However if the configuration ends up with both flags unset, the core prints
> a warning since it considers such a configuration invalid (see
> uart_sanitize_serial_rs485()).
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 6085d356ad86..23366f868ae3 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -480,7 +480,7 @@ static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termio
>  }
>  
>  static const struct serial_rs485 generic_rs485_supported = {
> -	.flags = SER_RS485_ENABLED,
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
>  };
>  
>  static const struct exar8250_platform exar8250_default_platform = {
> @@ -524,7 +524,8 @@ static int iot2040_rs485_config(struct uart_port *port, struct ktermios *termios
>  }
>  
>  static const struct serial_rs485 iot2040_rs485_supported = {
> -	.flags = SER_RS485_ENABLED | SER_RS485_RX_DURING_TX | SER_RS485_TERMINATE_BUS,
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> +		 SER_RS485_RX_DURING_TX | SER_RS485_TERMINATE_BUS,
>  };
>  
>  static const struct property_entry iot2040_gpio_properties[] = {

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

(I assume you picked the correct flag among the two alternatives).

-- 
 i.