Re: [PATCH 4/4] Input: edt-ft5x06 - poll while a contact is down to recover dropped releases

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The polling worker can be erroneously re-armed during device suspend, leading to invalid I2C accesses while the device is powered off.
- [Medium] A single transient I2C read failure after an idle period immediately triggers a spurious controller reset.
- [Medium] The controller reset recovery logic breaks permanently after ~24.8 days of uptime on 32-bit systems due to jiffies wrap-around.
--

--- Patch [4]: [PATCH 4/4] Input: edt-ft5x06 - poll while a contact is down to recover dropped releases ---
commit 601140d3fc50fbfeaa9e78699c5e29c00098c932
Author: Alexandre Hamamdjian <azkali.limited@gmail.com>

Input: edt-ft5x06 - poll while a contact is down to recover dropped releases

This commit adds a polling mechanism to recover from dropped release events
on unreliable I2C buses. It tracks active contacts and polls the panel while
any are down, synthesizing releases if contacts disappear. It also adds a
bounded retry for transient bus errors and a controller reset mechanism if
reads fail for over a second.

> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index ac61ac44fd649..794c0650f5cb3 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
[ ... ]
> @@ -321,26 +333,83 @@ static int edt_ft5x06_bulk_read(struct regmap *map, unsigned int start,
[ ... ]
> +static void edt_ft5x06_fetch_and_report(struct edt_ft5x06_ts_data *tsdata)
>  {
[ ... ]
> +	for (tries = 0; tries < 4; tries++) {
[ ... ]
> +	}
>  	if (error) {
>  		dev_err_ratelimited(dev, "Unable to fetch data, error: %d\n",
>  				    error);
> -		goto out;
> +		/*
> +		 * A run of failed reads with no success for over a second means
> +		 * the controller is wedged rather than just glitching; pulse the
> +		 * reset line to recover it and drop any held contacts, since the
> +		 * post-reset finger state is unknown.
> +		 */
> +		if (tsdata->reset_gpio &&
> +		    time_after(jiffies, tsdata->last_success + HZ) &&
> +		    time_after(jiffies, tsdata->last_reset + 2 * HZ)) {

[Severity: Medium]
Will a single transient I2C read failure after an idle period immediately
trigger a spurious controller reset?

If the device sits idle for > 1 second, tsdata->last_success becomes stale.
When a new touch arrives, if the very first I2C read fails due to a transient
bus error, the time_after(jiffies, tsdata->last_success + HZ) check evaluates
to true.

This bypasses the intended 1-second delay of continuous failures, and
immediately pulses the reset line, causing a 300ms stall and dropping the
initial touch.

[Severity: Medium]
Does the controller reset recovery logic break permanently after ~24.8 days of
uptime on 32-bit systems due to jiffies wrap-around?

If the system runs for more than 24.8 days (exceeding MAX_JIFFY_OFFSET)
without the touchscreen wedging, tsdata->last_reset remains at its
initialization value.

A subsequent I2C bus error triggering the recovery path would cause the
time_after(jiffies, tsdata->last_reset + 2 * HZ) check to evaluate to false
due to signed integer overflow, bypassing the reset entirely until jiffies
wraps around the rest of the 32-bit space.

> +			tsdata->last_reset = jiffies;
> +			gpiod_set_value_cansleep(tsdata->reset_gpio, 1);
[ ... ]
> +static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
> +{
> +	struct edt_ft5x06_ts_data *tsdata = dev_id;
> +
> +	guard(mutex)(&tsdata->io_lock);
> +	edt_ft5x06_fetch_and_report(tsdata);
> +	if (tsdata->down_mask)
> +		mod_delayed_work(system_wq, &tsdata->poll_work,
> +				 msecs_to_jiffies(EDT_POLL_INTERVAL_MS));
>  
> -out:
>  	return IRQ_HANDLED;
>  }
[ ... ]
> @@ -1383,6 +1521,9 @@ static int edt_ft5x06_ts_suspend(struct device *dev)
>  	struct gpio_desc *reset_gpio = tsdata->reset_gpio;
>  	int ret;
>  
> +	/* stop the poll worker so it cannot touch i2c after power-down */
> +	cancel_delayed_work_sync(&tsdata->poll_work);
> +

[Severity: High]
Can the polling worker be erroneously re-armed during device suspend?

In edt_ft5x06_ts_suspend(), cancel_delayed_work_sync() is called to stop
the poll worker, but the device's interrupt is not disabled until several
operations later (or never disabled in HIBERNATE mode).

If an interrupt occurs in this window, the ISR edt_ft5x06_ts_isr() runs,
observes a non-zero down_mask, and unconditionally calls mod_delayed_work().

This could cause the worker to execute after the suspend function has disabled
regulators and suspended the I2C adapter, potentially leading to I2C access
timeouts or hardware hangs.

>  	if (device_may_wakeup(dev))
>  		return 0;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-b4-ft5426-v1-0-d4b4e32be042@gmail.com?part=4