Re: [PATCH RFC 2/3] Input: edt-ft5x06 - Add wakeup gesture support for FT5452

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Input: edt-ft5x06 - in_gesture_mode desync across non-wakeup suspend/resume
- [Medium] Input: edt-ft5x06 - IRQ race in edt_ft5x06_ts_set_gesture_mode()
- [Medium] Input: edt-ft5x06: missing input->mutex serialization in suspend and resume
- [Low] Input: edt-ft5x06 - use __free(fwnode_handle) and explicit error returns
--

commit 3e641160614ae8cff0e8f98270a632d8f526b6a7
Author: David Heidelberg <david@ixit.cz>

Input: edt-ft5x06 - Add wakeup gesture support for FT5452

This commit adds wakeup gesture support for the FocalTech FT5452 touchscreen
controller by parsing event child nodes from the device tree and reporting
detected gestures as key events on a separate input device. Gesture mode is
entered when the touchscreen input device is closed or during system suspend
when wakeup is enabled.

> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index e69d64c3db2dd..fc810d68409cf 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
[ ... ]
> @@ -303,6 +362,11 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
>  	int i, type, x, y, id;
>  	int error;
>  
> +	if (tsdata->in_gesture_mode) {
> +		edt_ft5x06_ts_report_gesture(tsdata);
> +		return IRQ_HANDLED;
> +	}
> +
[ ... ]
> @@ -1113,6 +1177,110 @@ static void edt_ft5x06_disable_regulators(void *arg)
>  	regulator_disable(data->iovcc);
>  }
>  
> +static int edt_ft5x06_ts_set_gesture_mode(struct edt_ft5x06_ts_data *tsdata,
> +					  bool enable)
> +{
> +	int err;
> +
> +	if (enable) {\
> +		err = regmap_write(tsdata->regmap, GESTURE_REGISTER_MASK,
> +				   tsdata->gesture_mask);
> +		if (err)
> +			return err;
> +	} else {
> +		/* let the IRQ thread read the gesture that woke us first */
> +		synchronize_irq(tsdata->client->irq);
> +	}
> +
> +	err = regmap_write(tsdata->regmap, GESTURE_REGISTER_EN, enable);
> +	if (err)
> +		return err;
> +
> +	tsdata->in_gesture_mode = enable;
> +
> +	return 0;
> +}

[Severity: Medium]
Can edt_ft5x06_ts_set_gesture_mode() race with the threaded interrupt handler
edt_ft5x06_ts_isr() while client->irq remains enabled?

When enable is true, synchronize_irq() is not called before switching modes.
An already-triggered touch interrupt in edt_ft5x06_ts_isr() can observe
tsdata->in_gesture_mode as true after GESTURE_REGISTER_EN is written and read
GESTURE_REGISTER_OUTPUT instead of touch coordinates. Conversely, if a
gesture interrupt fires right after the sleeping regmap_write() to
GESTURE_REGISTER_EN completes on the I2C bus, edt_ft5x06_ts_isr() can run
before tsdata->in_gesture_mode = enable is stored and read normal touch data
instead of GESTURE_REGISTER_OUTPUT.

When enable is false, synchronize_irq() is called before writing 0 to
GESTURE_REGISTER_EN while the interrupt line is still enabled. Once the
controller receives GESTURE_REGISTER_EN = 0 over I2C, it resumes normal touch
scanning before regmap_write() returns to clear tsdata->in_gesture_mode, so a
touch interrupt in that window will see tsdata->in_gesture_mode as true in
edt_ft5x06_ts_isr() and call edt_ft5x06_ts_report_gesture().

> +
> +/*
> + * The touchscreen is opened and closed by userspace, either by opening and
> + * closing its event device or through the "inhibited" attribute. Closing it
> + * is the moment to start looking for the wakeup gestures.
> + */
> +static int edt_ft5x06_ts_open(struct input_dev *input)
> +{
> +	struct edt_ft5x06_ts_data *tsdata = input_get_drvdata(input);
> +
> +	if (!tsdata->in_gesture_mode)
> +		return 0;
> +
> +	return edt_ft5x06_ts_set_gesture_mode(tsdata, false);
> +}
> +
> +static void edt_ft5x06_ts_close(struct input_dev *input)
> +{
> +	struct edt_ft5x06_ts_data *tsdata = input_get_drvdata(input);
> +	struct device *dev = &tsdata->client->dev;
> +	int err;
> +
> +	if (!device_may_wakeup(dev))
> +		return;
> +
> +	err = edt_ft5x06_ts_set_gesture_mode(tsdata, true);
> +	if (err)
> +		dev_warn(dev, "Failed to enter gesture mode: %d\n", err);
> +}
> +
> +/*
> + * The gestures detected in gesture mode are reported on their own input
> + * device, so that they still reach userspace when the touchscreen input
> + * device has been closed.
> + */
> +static int edt_ft5x06_ts_setup_gestures(struct edt_ft5x06_ts_data *tsdata)
> +{
> +	struct device *dev = &tsdata->client->dev;
> +	struct input_dev *input = NULL;
> +	struct fwnode_handle *child;
> +	int err;
> +
> +	for (int i = 0; i < ARRAY_SIZE(edt_ft5x06_gestures); i++) {
> +		const struct edt_ft5x06_gesture *gesture = &edt_ft5x06_gestures[i];
> +		unsigned int code;
> +
> +		child = device_get_named_child_node(dev, gesture->node);
> +		if (!child)
> +			continue;
> +
> +		err = fwnode_property_read_u32(child, "linux,code", &code);
> +		fwnode_handle_put(child);
> +		if (err)
> +			return dev_err_probe(dev, err, "%s: failed to read linux,code\n",
> +					     gesture->node);
> +
> +		if (code == KEY_RESERVED)
> +			continue;
> +
> +		if (!input) {
> +			input = devm_input_allocate_device(dev);
> +			if (!input)
> +				return -ENOMEM;
> +
> +			input->name = EDT_GESTURE_DEV_NAME;
> +			input->id.bustype = BUS_I2C;
> +		}
> +
> +		input_set_capability(input, EV_KEY, code);
> +		tsdata->gesture_code[i] = code;
> +		tsdata->gesture_mask |= BIT(gesture->id - GESTURE_ID_FIRST);
> +	}
> +
> +	if (!input)
> +		return 0;
> +
> +	tsdata->gesture_input = input;
> +
> +	return input_register_device(input);
> +}

[Severity: Low]
This isn't a bug, but in edt_ft5x06_ts_setup_gestures(), could child be
declared inside the loop using __free(fwnode_handle):

	struct fwnode_handle *child __free(fwnode_handle) =
		device_get_named_child_node(dev, gesture->node);

instead of calling fwnode_handle_put(child) manually?

Also, in both edt_ft5x06_ts_setup_gestures() and edt_ft5x06_ts_resume(),
which have multiple failure points, could the final calls to
input_register_device(input) and edt_ft5x06_ts_set_gesture_mode(tsdata, false)
check err and explicitly return 0 on success to match input subsystem style
conventions?

[ ... ]
> @@ -1349,8 +1530,13 @@ static int edt_ft5x06_ts_suspend(struct device *dev)
>  	struct gpio_desc *reset_gpio = tsdata->reset_gpio;
>  	int ret;
>  
> -	if (device_may_wakeup(dev))
> -		return 0;
> +	if (device_may_wakeup(dev)) {
> +		/* the IRQ stays enabled, the PM core arms it as the wake source */
> +		if (!tsdata->gesture_input || tsdata->in_gesture_mode)
> +			return 0;
> +
> +		return edt_ft5x06_ts_set_gesture_mode(tsdata, true);
> +	}
>  
>  	if (tsdata->suspend_mode == EDT_PMODE_NOT_SUPPORTED)
>  		return 0;
> @@ -1391,8 +1577,18 @@ static int edt_ft5x06_ts_resume(struct device *dev)
>  	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
>  	int ret = 0;
>  
> -	if (device_may_wakeup(dev))
> -		return 0;
> +	if (device_may_wakeup(dev)) {
> +		if (!tsdata->in_gesture_mode)
> +			return 0;
> +
> +		guard(mutex)(&tsdata->input->mutex);
> +
> +		/* stays armed until the input device is opened again */
> +		if (!input_device_enabled(tsdata->input))
> +			return 0;
> +
> +		return edt_ft5x06_ts_set_gesture_mode(tsdata, false);
> +	}

[Severity: Medium]
What happens if the touchscreen input device is closed while wakeup is
enabled, and userspace then disables wakeup via sysfs before suspending?

1. edt_ft5x06_ts_close() calls edt_ft5x06_ts_set_gesture_mode(tsdata, true),
   which sets tsdata->in_gesture_mode = true.
2. Userspace disables wakeup via power/wakeup while the input device remains
   closed, so device_may_wakeup(dev) becomes false.
3. On system suspend, edt_ft5x06_ts_suspend() takes the non-wakeup path and
   hibernates or powers off the controller (asserting reset_gpio and disabling
   vcc/iovcc) without clearing tsdata->in_gesture_mode.
4. On resume, edt_ft5x06_ts_resume() takes the non-wakeup path and resets or
   wakes the controller back into normal touch mode while
   tsdata->in_gesture_mode remains true.

While the input device remains closed, any touch interrupt will be handled by
edt_ft5x06_ts_isr() as a gesture via edt_ft5x06_ts_report_gesture(). In
addition, if wakeup is later re-enabled before a subsequent suspend while the
device is still closed, edt_ft5x06_ts_suspend() will see
tsdata->in_gesture_mode == true and return 0 without programming
GESTURE_REGISTER_MASK or GESTURE_REGISTER_EN.

[Severity: Medium]
Should edt_ft5x06_ts_suspend() and the initial tsdata->in_gesture_mode check
in edt_ft5x06_ts_resume() also be serialized under tsdata->input->mutex?

edt_ft5x06_ts_open() and edt_ft5x06_ts_close() are called under
tsdata->input->mutex from input_open_device(), input_close_device(),
input_inhibit_device(), and input_uninhibit_device(). However,
edt_ft5x06_ts_suspend() reads tsdata->in_gesture_mode and calls
edt_ft5x06_ts_set_gesture_mode(tsdata, true) without holding
tsdata->input->mutex, and edt_ft5x06_ts_resume() checks
!tsdata->in_gesture_mode before acquiring guard(mutex)(&tsdata->input->mutex).

If a deferred file release via delayed_fput_work (evdev_release() ->
evdev_close_device() -> input_close_device()) or an in-kernel input handler
runs concurrently with edt_ft5x06_ts_suspend() or edt_ft5x06_ts_resume(), can
concurrent calls to edt_ft5x06_ts_set_gesture_mode() interleave I2C writes to
GESTURE_REGISTER_MASK and GESTURE_REGISTER_EN and leave
tsdata->in_gesture_mode out of sync with the hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-ft5x06-double-tap-v1-0-5a1bd80449cc@ixit.cz?part=2