Re: [PATCH 12/13] HID: ft260: workaround for TN_189 errata endpoint STALL after enumeration

[sashiko-bot@kernel.org]

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

New issues:
- [High] Detached reset work item causes a kernel Oops (Use-After-Free) if the driver module is unloaded before the work executes.
- [High] A persistently broken FT260 device will trigger an exponential fork bomb of reset work items, causing unbounded workqueue growth.
- [Medium] Memory allocation failures unintentionally trigger a full USB device reset.
- [Low] Passing endpoint address (with direction bit) to `usb_rcvintpipe` instead of the endpoint number.

Pre-existing issues:
- [High] `hid_hw_open()` is called before driver spinlocks are initialized, allowing incoming HID reports to access uninitialized locks.
--

--- Patch [12]: [PATCH 12/13] HID: ft260: workaround for TN_189 errata endpoint STALL after enumeration ---
commit 8891d1670b0f7ce6179bd30f8809f4b07e8b46dc
Author: Michael Zaidman <michael.zaidman@gmail.com>

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

This commit introduces a workaround for the FT260 TN_189 errata where USB
interrupt endpoints stall after enumeration. It schedules a delayed work item
to reset the USB device and rebind interfaces if a STALL or unresponsiveness
is detected during probe.

Closes: https://github.com/MichaelZaidman/hid-ft260/issues/40
Link: https://ftdichip.com/wp-content/uploads/2026/05/TN_189-FT260-Errata-Technical-Note.pdf

> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -2359,15 +2359,227 @@ static int ft260_uart_probe(struct ft260_device *dev,

[ ... ]

> +static void ft260_reset_and_rebind(struct work_struct *ws)
> +{

[ ... ]

> +	for (i = 0; actconfig && i < actconfig->desc.bNumInterfaces; i++) {
> +		struct usb_interface *intf = actconfig->interface[i];
> +
> +		if (!intf)
> +			continue;
> +		ret = device_attach(&intf->dev);

[Severity: High]
If the device is persistently wedged and continues to fail probe, will this
device_attach() synchronously trigger fresh ft260_probe() calls that fail
and schedule even more work items? 

For a 2-interface device, failing probe on both interfaces could initially
schedule two reset work items. If each work item unbinds and rebinds all
interfaces, does this risk creating an unbounded exponential loop (a fork
bomb) of work items being queued?

> +		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);
> +	schedule_work(&rw->work);

[Severity: High]
When returning -ENODEV from ft260_probe() on error, the driver binding
aborts and the module reference count stays at 0. 

If an administrator unloads the hid-ft260 module while this scheduled work
item is pending or executing its 10-second retry loop, could the workqueue
execute unmapped memory and trigger a use-after-free panic?

Would it be safer to pin the module with try_module_get()/module_put() or
ensure the work is correctly cancelled on failure?

> +
> +	return 0;
> +}
> +

[ ... ]

> +static int ft260_check_intr_ep_health(struct hid_device *hdev)
> +{

[ ... ]

> +	buf = kmalloc(FT260_REPORT_MAX_LEN, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	pipe = usb_rcvintpipe(usbdev, ep->bEndpointAddress);

[Severity: Low]
Does usb_rcvintpipe() expect just the endpoint number instead of the raw
endpoint address?

Passing ep->bEndpointAddress includes the direction bit, which gets shifted
incorrectly by the pipe creation macro. Could we use
usb_endpoint_num(&ep->desc) here to ensure only the endpoint number is
passed?

> +	ret = usb_interrupt_msg(usbdev, pipe, buf, FT260_REPORT_MAX_LEN,
> +				&actual_length, 100);
> +	kfree(buf);

[ ... ]

> @@ -2392,6 +2604,23 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		goto hid_fail;
>  	}
>  
> +	/*
> +	 * TN_189 errata workaround: bail out fast on a broken device so
> +	 * that hub_event releases the device lock quickly, allowing the
> +	 * scheduled reset work to acquire it and recover the device.
> +	 */
> +	ret = ft260_check_intr_ep_health(hdev);
> +	if (ret) {
> +		ft260_schedule_reset(usbif);

[Severity: Medium]
If ft260_check_intr_ep_health() fails due to a memory allocation error
(-ENOMEM), this broad check will catch it. Could this erroneously schedule
a full USB device reset during system memory pressure rather than just
propagating the -ENOMEM failure?

> +		goto err_hid_stop;
> +	}
> +
> +	ret = ft260_check_dev_responsive(hdev);
> +	if (ret) {
> +		ft260_schedule_reset(usbif);
> +		goto err_hid_stop;
> +	}
> +
>  	ret = hid_hw_open(hdev);

[Severity: High]
This is a pre-existing issue, but calling hid_hw_open() here starts the
hardware and allows incoming USB events to trigger the driver's report
callbacks immediately.

If an incoming report causes ft260_raw_event() to attempt to acquire
dev->read_lock before it is initialized further down in ft260_probe()
via spin_lock_init(), could this lead to an uninitialized spinlock splat
or kernel crash?

>  	if (ret) {
>  		hid_err(hdev, "failed to open HID HW\n");
>  		goto err_hid_stop;
>  	}

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