Re: [PATCH 10/13] HID: ft260: uart: fix active-low RTS/CTS/DTR/DSR polarity

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Read-modify-write race condition in ft260_uart_tiocmset() due to reading shared state outside of a mutex.
- [High] ft260_uart_tiocmget() fails to check if ft260_gpio_get_all() returns an error code, silently returning a corrupted modem status bitmask to userspace.
--

--- Patch [10]: [PATCH 10/13] HID: ft260: uart: fix active-low RTS/CTS/DTR/DSR polarity ---
commit 1c4e1ea308d880a16bd5a589b4e5e279a4dec5e8
Author: Rio Liu <[email protected]>

HID: ft260: uart: fix active-low RTS/CTS/DTR/DSR polarity

This commit inverts the TIOCMGET sense and the TIOCMSET drive levels for
the active-low FT260 modem control GPIOs (RTS, CTS, DTR, and DSR). This
aligns the userspace ioctl state with the electrical lines, leaving DCD
and RI unchanged.

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 0a1875a1a7eca..a2f4b6e5a16bb 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -1985,10 +1985,10 @@ static int ft260_uart_tiocmget(struct tty_struct *tty)
>  
>  	gpios = ft260_gpio_get_all(port->gc, FT260_GPIO_VALUE);
>  

[Severity: High]
This isn't a bug introduced by this patch, but ft260_gpio_get_all() can return
a negative error code (such as -EIO) if a hardware or transport error occurs.

Does evaluating this negative error code as a bitmask in the return statement
below risk masking the hardware error and returning a corrupted modem status
to userspace instead of propagating the error?

> -	return (((gpios & FT260_GPIO_B) ? TIOCM_RTS : 0) |
> -		((gpios & FT260_GPIO_E) ? TIOCM_CTS : 0) |
> -		((gpios & FT260_GPIO_F) ? TIOCM_DTR : 0) |
> -		((gpios & FT260_GPIO_H) ? TIOCM_DSR : 0) |
> +	return ((!(gpios & FT260_GPIO_B) ? TIOCM_RTS : 0) |
> +		(!(gpios & FT260_GPIO_E) ? TIOCM_CTS : 0) |
> +		(!(gpios & FT260_GPIO_F) ? TIOCM_DTR : 0) |
> +		(!(gpios & FT260_GPIO_H) ? TIOCM_DSR : 0) |
>  		((gpios & FT260_GPIO_4) ? TIOCM_CAR : 0) |
>  		((gpios & FT260_GPIO_5) ? TIOCM_RNG : 0));
>  }
> @@ -2013,13 +2013,13 @@ static int ft260_uart_tiocmset(struct tty_struct *tty,
>  	mutex_lock(&port->lock);

[Severity: High]
This is also a pre-existing issue, but in ft260_uart_tiocmset(), the local
variable rep.gpio is populated from the shared state port->gpio before
acquiring port->lock:

	rep.gpio = port->gpio;
	mutex_lock(&port->lock);

Could this result in a read-modify-write race where a concurrent GPIO
modification (e.g., via sysfs) is silently reverted by the TTY ioctl?

Should rep.gpio be populated inside the critical section instead?

>  
>  	if (set & TIOCM_RTS)
> -		ft260_gpio_output_cfg(&rep.gpio, FT260_GPIO_UART_RTS, 1);
> +		ft260_gpio_output_cfg(&rep.gpio, FT260_GPIO_UART_RTS, 0);
>  	if (set & TIOCM_DTR)
> -		ft260_gpio_output_cfg(&rep.gpio, FT260_GPIO_UART_DTR, 1);
> +		ft260_gpio_output_cfg(&rep.gpio, FT260_GPIO_UART_DTR, 0);
>  	if (clear & TIOCM_RTS)
> -		ft260_gpio_output_cfg(&rep.gpio, FT260_GPIO_UART_RTS, 0);
> +		ft260_gpio_output_cfg(&rep.gpio, FT260_GPIO_UART_RTS, 1);
>  	if (clear & TIOCM_DTR)
> -		ft260_gpio_output_cfg(&rep.gpio, FT260_GPIO_UART_DTR, 0);
> +		ft260_gpio_output_cfg(&rep.gpio, FT260_GPIO_UART_DTR, 1);
>  
>  	ft260_dbg("dirs %#02x vals %#02x ex_dirs %#02x ex_vals %#02x\n",
>  		  rep.gpio.dirs, rep.gpio.vals,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10