Re: [PATCH v2 11/15] HID: ft260: workaround for TN_189 errata endpoint STALL after enumeration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] HID: ft260: duplicate per-interface resets in ft260_schedule_reset()
- [Medium] HID: ft260: race between ft260_reset_and_rebind() and sibling ft260_probe()
- [Low] HID: ft260: ft260_check_dev_responsive() masks -ENOMEM as -ENODEV
--

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

HID: ft260: workaround for TN_189 errata endpoint STALL after enumeration

This patch adds an in-driver workaround for FTDI errata TN_189 where the
FT260's USB interrupt endpoints are occasionally halted after enumeration.
During probe, the driver checks interrupt endpoint health and device
responsiveness, and on failure schedules deferred work on an ordered
workqueue to reset the USB device and rebind all of its interfaces.

Link: https://ftdichip.com/wp-content/uploads/2026/05/TN_189-FT260-Errata-Technical-Note.pdf

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 65835e13fa2ad..923ce6ee9dcfd 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c

[ ... ]

> @@ -2396,15 +2397,229 @@ static int ft260_uart_probe(struct ft260_device *dev,
>  	return ret;
>  }
>  
> +/*
> + * FT260 errata TN_189 Section 2.1: the USB interrupt endpoints are
> + * occasionally halted right after enumeration. When this happens:
> + *  - Standard Clear-Feature ENDPOINT_HALT does not recover the endpoint
> + *  - Subsequent communication with the device is dead
> + *  - The only known recovery is a USB device reset
> + *
> + * A separate finding from FTDI engineering (confirmed by USB analyzer
> + * trace while testing this workaround) is that the FT260 does NOT
> + * honestly report the halt state via USB_REQ_GET_STATUS: it returns 0
> + * even when the endpoint is STALLed. Detection must therefore observe
> + * the STALL handshake at the host controller level rather than ask
> + * the device.
> + *
> + * Recovery is performed by a deferred work item that resets the USB
> + * device and unbinds/rebinds all interfaces to force usbhid to
> + * destroy stale HID devices and create fresh ones, which triggers a
> + * new ft260_probe() that succeeds.
> + *
> + * https://ftdichip.com/wp-content/uploads/2026/05/TN_189-FT260-Errata-Technical-Note.pdf
> + */
> +struct ft260_reset_work {
> +	struct work_struct work;
> +	struct usb_interface *usbif;
> +};
> +
> +static struct workqueue_struct *ft260_reset_wq;
> +
> +static void ft260_reset_and_rebind(struct work_struct *ws)
> +{
> +	struct ft260_reset_work *rw =
> +		container_of(ws, struct ft260_reset_work, work);
> +	struct usb_interface *usbif = rw->usbif;
> +	struct usb_device *usbdev = interface_to_usbdev(usbif);
> +	struct usb_host_config *actconfig;
> +	int ret, i, attempt;
> +
> +	/*
> +	 * Retry the device lock for up to ~10 seconds. The lock is held
> +	 * by hub_event for the duration of device enumeration; with the
> +	 * fast-fail responsiveness check in probe, both interfaces should
> +	 * abort within ~1-2 seconds, after which the lock becomes free.
> +	 * Each usb_lock_device_for_reset() attempt already polls for up to
> +	 * one second internally.
> +	 */
> +	for (attempt = 0; attempt < 10; attempt++) {
> +		ret = usb_lock_device_for_reset(usbdev, NULL);
> +		if (ret >= 0)
> +			break;

[Severity: Medium]
Can ft260_reset_and_rebind() race with a concurrent ft260_probe() on the
sibling HID interface when the driver is loaded after USB enumeration has
already finished?

When an FT260 device in the TN_189 errata state is enumerated before
hid-ft260 is loaded, usbhid_probe() registers both hid_device instances and
hub_event releases usb_lock_device(usbdev). When udev later loads hid-ft260
(or the driver is bound via sysfs), __hid_register_driver() probes the HID
devices sequentially on hid_bus_type without holding usb_lock_device(usbdev).

When interface 0 fails ft260_check_intr_ep_health() or
ft260_check_dev_responsive() in ft260_probe(), it queues
ft260_reset_and_rebind() on ft260_reset_wq and returns -ENODEV. While the
module init thread moves on to probe interface 1, the worker thread on
another CPU can immediately acquire usb_lock_device_for_reset(usbdev, NULL)
and run usb_reset_device(usbdev) and device_release_driver(&intf->dev)
concurrently with interface 1 running usbhid_start(), USB transfers, and
usbhid_stop() in ft260_probe().

In particular, usb_reset_device() invokes hid_pre_reset() -> hid_cease_io()
-> usb_kill_urb() without holding usbhid->mutex while interface 1's error
path in ft260_probe() -> hid_hw_stop() -> usbhid_stop() calls usb_free_urb(),
and interface 1's failing probe will also queue a redundant reset work item.

> +		if (ret == -ENODEV || ret == -EHOSTUNREACH) {
> +			dev_dbg(&usbif->dev,
> +				"device gone before reset (%d), abort\n", ret);
> +			goto out;
> +		}
> +		/* -EBUSY: someone else holds the lock; retry. */
> +	}
> +	if (ret < 0) {
> +		dev_err(&usbif->dev,
> +			"failed to acquire USB device lock for reset after %d attempts: %d\n",
> +			attempt, ret);
> +		goto out;
> +	}
> +
> +	ret = usb_reset_device(usbdev);
> +	if (ret < 0) {
> +		dev_err(&usbif->dev, "USB reset failed: %d\n", ret);
> +		usb_unlock_device(usbdev);
> +		goto out;
> +	}
> +
> +	/*
> +	 * usb_reset_device() keeps usbhid bound (its pre_reset/post_reset
> +	 * both return 0) and does not re-trigger HID-level driver probing.
> +	 * Unbind and rebind all USB interfaces to force usbhid to destroy
> +	 * stale HID devices and create new ones, which triggers fresh
> +	 * ft260_probe() calls.
> +	 */
> +	actconfig = usbdev->actconfig;
> +	for (i = 0; actconfig && i < actconfig->desc.bNumInterfaces; i++) {
> +		struct usb_interface *intf = actconfig->interface[i];
> +
> +		if (intf && intf->dev.driver)
> +			device_release_driver(&intf->dev);
> +	}
> +	for (i = 0; actconfig && i < actconfig->desc.bNumInterfaces; i++) {
> +		struct usb_interface *intf = actconfig->interface[i];
> +
> +		if (!intf)
> +			continue;
> +		ret = device_attach(&intf->dev);
> +		if (ret < 0)
> +			dev_err(&intf->dev,
> +				"failed to rebind USB interface: %d\n", ret);
> +	}
> +
> +	usb_unlock_device(usbdev);
> +out:
> +	usb_put_intf(usbif);
> +	kfree(rw);
> +}
> +
> +static int ft260_schedule_reset(struct usb_interface *usbif)
> +{
> +	struct ft260_reset_work *rw;
> +
> +	rw = kmalloc_obj(*rw, GFP_KERNEL);
> +	if (!rw)
> +		return -ENOMEM;
> +
> +	usb_get_intf(usbif);
> +	rw->usbif = usbif;
> +	INIT_WORK(&rw->work, ft260_reset_and_rebind);
> +	queue_work(ft260_reset_wq, &rw->work);
> +
> +	return 0;
> +}

[Severity: High]
As reported by Sashiko on v1 [1], since the FT260 in composite modes
(FT260_MODE_ALL and FT260_MODE_BOTH) exposes two USB HID interfaces on the
same struct usb_device, won't scheduling an independent ft260_reset_work
item per failing interface cause a redundant reset that tears down the
already-recovered device and multiply pending work items on repeated
failures?

During enumeration under hub_event, usb_set_configuration() probes both
interface 0 and interface 1 while holding the USB device lock. When the
chip hits the TN_189 state, ft260_probe() fails on both interfaces and calls
ft260_schedule_reset(usbif) twice, queueing two ft260_reset_work items on
ft260_reset_wq for the same usb_device.

Once hub_event releases the USB device lock:

1. The first work item runs ft260_reset_and_rebind(), calls
   usb_reset_device(usbdev), and loops over all interfaces in
   actconfig->desc.bNumInterfaces calling device_release_driver(&intf->dev)
   and device_attach(&intf->dev). When the first reset recovers the chip,
   device_attach() synchronously runs usbhid_probe() and ft260_probe() on
   both interfaces, registering the live I2C adapter, GPIO chip, and
   /dev/ttyFT* port.

2. Immediately after the first work item unlocks usbdev, the second work
   item queued during initial enumeration runs ft260_reset_and_rebind().
   Because ft260_schedule_reset() does not deduplicate per usb_device and
   ft260_reset_and_rebind() does not check whether the interfaces are
   already bound and healthy, it unconditionally resets the healthy USB
   device a second time and calls device_release_driver(&intf->dev) on all
   interfaces, invoking ft260_remove() to tear down the active I2C adapter,
   GPIO chip, and TTY device that were just registered.

3. If a reset does not recover the device on the first attempt (or the
   device is persistently unresponsive), each execution of
   ft260_reset_and_rebind() consumes one work item from ft260_reset_wq
   while its device_attach() loop synchronously triggers failing
   ft260_probe() calls on both interfaces that queue two new
   ft260_reset_work items, causing the workqueue backlog to grow on every
   reset cycle.

[ ... ]

> +static int ft260_check_dev_responsive(struct hid_device *hdev)
> +{
> +	struct usb_interface *usbif = to_usb_interface(hdev->dev.parent);
> +	struct usb_device *usbdev = interface_to_usbdev(usbif);
> +	__le16 *status;
> +	int ret;
> +
> +	status = kmalloc_obj(*status, GFP_KERNEL);
> +	if (!status)
> +		return -ENOMEM;
> +
> +	ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
> +			      USB_REQ_GET_STATUS,
> +			      USB_DIR_IN | USB_RECIP_DEVICE,
> +			      0, 0, status, sizeof(*status), 500);
> +	kfree(status);
> +
> +	if (ret < 0) {
> +		hid_warn(hdev,
> +			 "device unresponsive to GET_STATUS (%d), suspected TN_189 errata, scheduling USB reset and rebind\n",
> +			 ret);
> +		return -ENODEV;
> +	}

[Severity: Low]
In ft260_check_dev_responsive(), usb_control_msg() allocates a struct
usb_ctrlrequest and a struct urb (via usb_internal_control_msg() ->
usb_alloc_urb()) with GFP_NOIO and returns -ENOMEM if either allocation
fails.

Because the check afterward converts any negative return value (ret < 0) to
-ENODEV, an -ENOMEM failure inside usb_control_msg() will log the TN_189
warning and return -ENODEV to ft260_probe(), which then calls
ft260_schedule_reset(usbif). Should ft260_check_dev_responsive() propagate
ret directly when ret == -ENOMEM before converting other negative errors to
-ENODEV?

[1] https://lore.kernel.org/r/20260822213941.98882-13-michael.zaidman@gmail.com

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