Re: [PATCH v2 15/15] HID: ft260: i2c: recover a bus held by a slave

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] HID: ft260: unsynchronized FT260_GPIO access across interfaces in recovery
--

commit 1e0d001c06a246dc407bfd48dc94ce59eae220c5
Author: Michael Zaidman <michael.zaidman@gmail.com>

HID: ft260: i2c: recover a bus held by a slave

This commit adds I2C bus recovery using i2c_generic_scl_recovery() when the
I2C bus remains busy after resetting the controller in ft260_i2c_abort(). It
temporarily disables the I2C controller so GPIO0 and GPIO1 can be driven as
GPIOs via ft260_gpio_pin_set() and ft260_gpio_read_all().

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index cd1117c170e1d..faa1e12cd2d54 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c

[ ... ]

> @@ -1237,34 +1291,23 @@ static int ft260_gpio_set(struct gpio_chip *gc, u32 offset, int value)
>  	return ret;
>  }
>  
> -static int ft260_gpio_direction_set(struct gpio_chip *gc, u32 offset,
> -				    int value, int direction)
> +/*
> + * Set the direction and the value of one pin. Takes no lock and does
> + * not look at gpio_en, so the I2C bus recovery can drive SCL and SDA
> + * while it holds dev->lock and while the pins are not GPIOs.
> + */
> +static int ft260_gpio_pin_set(struct ft260_device *dev, u32 offset,
> +			      int value, int direction)
>  {
>  	int ret;
>  	struct ft260_gpio_read_request_report buf;
>  	struct ft260_gpio_write_request_report *rep;
> -	struct ft260_device *dev = gpiochip_get_data(gc);
>  	struct hid_device *hdev = dev->hdev;
>  
> -	if (offset >= FT260_GPIO_TOTAL) {
> -		hid_err(hdev, "%s: invalid offset %d\n", __func__, offset);
> -		return -EINVAL;
> -	}
> -
> -	ft260_dbg("offset %d val %d direction %d\n", offset, value, direction);
> -
> -	mutex_lock(&dev->lock);
> -
> -	if (!(dev->gpio_en & BIT(offset))) {
> -		hid_err(hdev, "%s: wrong pin function %d\n", __func__, offset);
> -		ret = -EIO;
> -		goto exit;
> -	}
> -
>  	ret = ft260_hid_feature_report_get(hdev, FT260_GPIO, (u8 *)&buf, sizeof(buf));
>  	if (unlikely(ret < 0)) {
>  		hid_err(hdev, "%s: cannot get GPIO: %d\n", __func__, ret);
> -		goto exit;
> +		return ret;
>  	}
>  
>  	rep = (struct ft260_gpio_write_request_report *)&buf;
> @@ -1298,12 +1341,36 @@ static int ft260_gpio_direction_set(struct gpio_chip *gc, u32 offset,
>  	ret = ft260_hid_feature_report_set(hdev, (u8 *)rep, sizeof(*rep));
>  	if (unlikely(ret < 0)) {
>  		hid_err(hdev, "%s: cannot set GPIO: %d\n", __func__, ret);
> -		goto exit;
> +		return ret;
>  	}
>  
>  	dev->gpio = rep->gpio;
> -	ret = 0;
> -exit:
> +	return 0;
> +}

[ ... ]

> @@ -1360,6 +1431,65 @@ static int ft260_gpio_get(struct gpio_chip *gc, u32 offset)
>  	return (ret >> offset) & 1;
>  }
>  
> +/*
> + * I2C bus recovery. Turning the I2C controller off gives GPIO0 and
> + * GPIO1 back their GPIO function, so i2c_generic_scl_recovery() can
> + * clock the slave out of the byte it holds and put a STOP on the bus.
> + * A pin is released by making it an input, the bus pull-ups then drive
> + * it high.
> + */
> +static void ft260_i2c_set_pin(struct i2c_adapter *adap, u32 offset, int val)
> +{
> +	struct ft260_device *dev = i2c_get_adapdata(adap);
> +
> +	ft260_gpio_pin_set(dev, offset, 0,
> +			   val ? FT260_GPIO_DIR_INPUT : FT260_GPIO_DIR_OUTPUT);
> +}

[Severity: Medium]
When the chip is in dual-interface mode (FT260_MODE_ALL or FT260_MODE_BOTH),
ft260_probe() allocates separate ft260_device instances for the I2C and UART
USB interfaces, each with its own dev->lock mutex and dev->gpio shadow state.

In those modes, ft260_uart_probe() registers the gpio_chip on the UART
interface's ft260_device:

drivers/hid/hid-ft260.c:ft260_uart_probe()
	if (dev->chip_mode & FT260_MODE_UART ||
	    dev->chip_mode == FT260_MODE_ALL ||
	    dev->chip_mode == FT260_MODE_BOTH) {
		ret = ft260_gpio_init(dev, cfg);

Meanwhile, ft260_i2c_abort() -> i2c_recover_bus() -> ft260_i2c_set_pin() ->
ft260_gpio_pin_set() runs on the I2C interface's ft260_device while holding
only the I2C interface's dev->lock.

Because both interfaces access the same chip-wide FT260_GPIO feature report,
can ft260_gpio_pin_set() race with concurrent FT260_GPIO operations on the
UART interface?

1. ft260_gpio_set() and ft260_gpio_direction_set() perform a non-atomic
   ft260_hid_feature_report_get() and ft260_hid_feature_report_set() cycle on
   FT260_GPIO while holding the UART interface's dev->lock. If this interleaves
   with ft260_gpio_pin_set() during bus recovery, one side's pin updates can
   be overwritten, or the UART interface's dev->gpio can record the transient
   output direction of GPIO0 (SCL) or GPIO1 (SDA).

2. ft260_uart_tiocmset() on the UART interface does not read FT260_GPIO from
   the chip before writing; it takes port->lock, copies the cached port->gpio,
   modifies RTS/DTR, and writes the full FT260_GPIO report:

drivers/hid/hid-ft260.c:ft260_uart_tiocmset()
	mutex_lock(&port->lock);
	rep.gpio = port->gpio;
	...
	ret = ft260_hid_feature_report_set(hdev, (u8 *)&rep, sizeof(rep));

   If called during recovery (or after the UART interface's port->gpio cached
   a transient SCL/SDA output direction), wouldn't ft260_uart_tiocmset()
   overwrite the SCL and SDA directions in FT260_GPIO?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030223637.86907-1-michael.zaidman@gmail.com?part=15