Re: [PATCH] serial: 8250_of: set UART_CAP_NOTEMT for rts-gpios RS485 direction

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added UART_CAP_NOTEMT flag is either silently overwritten or causes the port to lose all of its default hardware capabilities.
--

commit 1d8dc615b4080d6f9bde37f54eafa34f59649ede
Author: Nicolas Thibert <nithibert@gmail.com>

serial: 8250_of: set UART_CAP_NOTEMT for rts-gpios RS485 direction

This patch sets UART_CAP_NOTEMT for RS485 ports using rts-gpios to prevent
the direction GPIO from getting stuck due to missing TEMT interrupts.

> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
> index b1561c499acda..600a77e1e6143 100644
> --- a/drivers/tty/serial/8250/8250_of.c
> +++ b/drivers/tty/serial/8250/8250_of.c
> @@ -212,6 +212,22 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
>  	up->rs485_start_tx = serial8250_em485_start_tx;
>  	up->rs485_stop_tx = serial8250_em485_stop_tx;
>  
> +	/*
> +	 * This generic driver never enables a dedicated line-status
> +	 * interrupt on TEMT, so for ports whose RS485 direction is
> +	 * controlled via a GPIO (rts-gpios) rather than the native RTS
> +	 * pin, __stop_tx() (8250_port.c) can see THRE without TEMT on the
> +	 * last byte and bail out without ever retrying -- leaving the
> +	 * direction GPIO stuck asserted after the last byte sent, on
> +	 * hardware whose shift register doesn't reliably surface TEMT.
> +	 * UART_CAP_NOTEMT makes it fall back to a frame-time-based timer
> +	 * instead of waiting on that interrupt. Scoped to rts-gpios users
> +	 * only, to avoid changing timing for ports relying on the native
> +	 * RTS pin, which this has not been observed to affect.
> +	 */
> +	if (of_property_present(np, "rts-gpios"))
> +		up->capabilities |= UART_CAP_NOTEMT;
> +
>  	switch (type) {

[Severity: High]
If the device explicitly configures a FIFO size, does the caller overwrite
this newly added capability?

When of_platform_serial_probe() calls of_platform_serial_setup(), it does
this right after:

drivers/tty/serial/8250/8250_of.c:of_platform_serial_probe() {
	...
	ret = of_platform_serial_setup(ofdev, port_type, &port8250, info);
	if (ret)
		goto err_free;

	if (port8250.port.fifosize)
		port8250.capabilities = UART_CAP_FIFO;
	...
}

Will this direct assignment completely erase the UART_CAP_NOTEMT flag?

Conversely, if the device does not specify a fifosize, port8250.capabilities
will be non-zero because it holds the UART_CAP_NOTEMT flag. Does this
cause the port to miss all its default hardware capabilities?

Later during registration, serial8250_set_defaults() checks if capabilities
is zero before assigning defaults:

drivers/tty/serial/8250/8250_port.c:serial8250_set_defaults() {
	...
	if (!up->capabilities)
		up->capabilities = uart_config[port->type].flags;
	...
}

Since up->capabilities is non-zero, this assignment is skipped entirely,
stripping the port of default features like UART_CAP_FIFO and UART_CAP_SLEEP.

Could we change the assignment in of_platform_serial_probe() to use |=,
and find a different way to merge UART_CAP_NOTEMT with the default flags?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907152744.1084359-1-nithibert@gmail.com?part=1