Re: [PATCH 2/2] serial: core: Drive the transceiver interface select GPIO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: core: stale rs485_select_gpio access in uart_rs485_config()
- [Medium] serial: core: rs485-select asserted before RTS parked in uart_get_rs485_mode()
--

commit 49736223bf33ff5e8eb201ccbef821265b8748f6
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

serial: core: Drive the transceiver interface select GPIO

This commit adds support for an optional rs485-select-gpios property in
uart_get_rs485_mode() to switch multiprotocol transceivers between RS485
and alternative interfaces such as RS232. The GPIO state is updated during
boot/resume configuration in uart_rs485_config() and during ioctl handling
in uart_set_rs485_config().

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c0deec0e6af41..57b022d0cf01b 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1435,13 +1435,22 @@ static void uart_set_rs485_rx_during_tx(struct uart_port *port,
>  				 !!(rs485->flags & SER_RS485_RX_DURING_TX));
>  }
>  
> +static void uart_set_rs485_select_gpio(struct uart_port *port,
> +				       const struct serial_rs485 *rs485)
> +{
> +	gpiod_set_value_cansleep(port->rs485_select_gpio,
> +				 !!(rs485->flags & SER_RS485_ENABLED));
> +}
> +
>  static int uart_rs485_config(struct uart_port *port)
>  {
>  	struct serial_rs485 *rs485 = &port->rs485;
>  	int ret;
>  
> -	if (!(rs485->flags & SER_RS485_ENABLED))
> +	if (!(rs485->flags & SER_RS485_ENABLED)) {
> +		uart_set_rs485_select_gpio(port, rs485);
>  		return 0;
> +	}

[Severity: High]
Could calling uart_set_rs485_select_gpio() unconditionally here when
SER_RS485_ENABLED is not set access a released port->rs485_select_gpio
descriptor when a static uart_port slot is reused?

In drivers/tty/serial/8250/8250_core.c, __serial8250_register_8250_port()
reuses static entries from serial8250_ports[] and populates port->rs485_select_gpio
via uart_get_rs485_mode():

drivers/tty/serial/8250/8250_core.c:__serial8250_register_8250_port() {
	...
	uart->port.rs485_config	= up->port.rs485_config;
	uart->port.rs485_supported = up->port.rs485_supported;
	uart->port.rs485	= up->port.rs485;
	...
	if (up->port.dev) {
		uart->port.dev = up->port.dev;
		ret = uart_get_rs485_mode(&uart->port);
		if (ret)
			return ret;
	}

Because port->rs485_select_gpio is acquired with devm_gpiod_get_optional(),
its lifetime is bound to port->dev. If registration subsequently fails in
__serial8250_register_8250_port() (for example in mctrl_gpio_init() or
uart_add_one_port()) or the device is later unbound and unregistered via
serial8250_unregister_port(), devres releases the descriptor while leaving
the pointer set in the static serial8250_ports[] entry.

When a subsequent 8250 device claims the same slot without RS485 support or
without a firmware node, uart_get_rs485_mode() returns early without
resetting port->rs485_select_gpio:

drivers/tty/serial/serial_core.c:uart_get_rs485_mode() {
	...
	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
		return 0;

	if (!dev_fwnode(dev))
		return 0;

Previously, uart_rs485_config() returned 0 immediately when
SER_RS485_ENABLED was clear. With this change, uart_configure_port() ->
uart_rs485_config() and uart_resume_port() -> uart_rs485_config() call
gpiod_set_value_cansleep(port->rs485_select_gpio, 0) using the dangling
pointer.

Should uart_get_rs485_mode() clear the RS485 GPIO pointers before its early
returns (or when a port is unregistered), and should uart_rs485_config()
also check port->rs485_supported.flags?

[ ... ]

> @@ -3643,6 +3663,13 @@ int uart_get_rs485_mode(struct uart_port *port)
>  	if (port->rs485_rx_during_tx_gpio)
>  		port->rs485_supported.flags |= SER_RS485_RX_DURING_TX;
>  
> +	dflags = (rs485conf->flags & SER_RS485_ENABLED) ?
> +		 GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
> +	desc = devm_gpiod_get_optional(dev, "rs485-select", dflags);
> +	if (IS_ERR(desc))
> +		return dev_err_probe(dev, PTR_ERR(desc), "Cannot get rs485-select-gpios\n");
> +	port->rs485_select_gpio = desc;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(uart_get_rs485_mode);

[Severity: Medium]
When linux,rs485-enabled-at-boot-time is set, doesn't requesting
rs485-select with GPIOD_OUT_HIGH in uart_get_rs485_mode() switch the
transceiver into RS485 mode before the UART driver has parked the RTS line
at its RS485 idle level?

During probe (for example in __serial8250_register_8250_port(),
imx_uart_probe(), serial_omap_probe_rs485(), and ar933x_uart_probe()),
uart_get_rs485_mode() runs before uart_add_one_port() ->
uart_configure_port() -> uart_rs485_config() invokes port->rs485_config()
to configure RTS and validate the boot-time settings.

In drivers/tty/serial/8250/8250_core.c, uart_get_rs485_mode() also runs
before mctrl_gpio_init() requests the RTS GPIO (which initializes it to
GPIOD_OUT_LOW regardless of rs485-rts-active-low) and before 8250
autoconfig manipulates UART_MCR_RTS:

drivers/tty/serial/8250/8250_core.c:__serial8250_register_8250_port() {
	...
	if (up->port.dev) {
		uart->port.dev = up->port.dev;
		ret = uart_get_rs485_mode(&uart->port);
		if (ret)
			return ret;
	}
	...
	if (uart->port.dev && !has_acpi_companion(uart->port.dev)) {
		struct mctrl_gpios *gpios = mctrl_gpio_init(&uart->port, 0);

Since RTS acts as the transceiver driver enable in RS485 mode, could
asserting rs485-select-gpios here in uart_get_rs485_mode() cause transient
bus contention on probe before uart_rs485_config() runs?

Should uart_get_rs485_mode() instead request rs485-select with
GPIOD_OUT_LOW and leave asserting it to uart_rs485_config() after
port->rs485_config() has parked RTS?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-serial-rs485-mode-gpio-v1-0-80d7b0903d93@vaisala.com?part=2