Re: [PATCH 7/7] Input: edt-ft5x06 - support following a DRM panel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Input: edt-ft5x06 - unbind race between panel_work and remove/devres teardown
- [High] Input: edt-ft5x06 - non-idempotent edt_ft5x06_ts_init_hw() retried in panel_work
- [Medium] Input: edt-ft5x06 - reset_gpio left deasserted on boot failure in panel_work
- [Medium] Input: edt-ft5x06 - missing powered_off check and factory_mode restore for panel follower
- [Medium] Input: edt-ft5x06 - unlocked sysfs reads in model_show() and fw_version_show()
--

commit 8d0f54b6a0e4f2aafd88896b2e7280382657e313
Author: Luka Panio <lukapanio@gmail.com>

Input: edt-ft5x06 - support following a DRM panel

This commit adds DRM panel follower support to the edt-ft5x06 touchscreen
driver for TDDI controllers that share power and reset lines with a display
panel. When following a panel, initial hardware probing and subsequent power
transitions are deferred to a workqueue triggered by the panel follower
callbacks.

> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index 7bcc48e07d629..b2fca6c372052 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c

[ ... ]

> @@ -1206,9 +1217,117 @@ static int edt_ft5x06_ts_init_hw(struct edt_ft5x06_ts_data *tsdata)
>  
>  	edt_ft5x06_ts_prepare_debugfs(tsdata);
>  
> +	tsdata->hw_initialized = true;
> +
> +	return 0;
> +}
> +
> +/*
> + * TDDI controllers share their power and reset with the display panel. When
> + * following a panel, power the touch controller down before the panel goes
> + * off and bring it back once the panel is up again.
> + */
> +static void edt_ft5x06_panel_work(struct work_struct *work)
> +{
> +	struct edt_ft5x06_ts_data *tsdata =
> +		container_of(work, struct edt_ft5x06_ts_data, panel_work);
> +	int error;
> +
> +	guard(mutex)(&tsdata->mutex);
> +
> +	if (!tsdata->powered_off)
> +		return;
> +
> +	if (!tsdata->hw_initialized) {
> +		error = edt_ft5x06_ts_init_hw(tsdata);
> +		if (error) {
> +			/* try again the next time the panel comes up */
> +			gpiod_set_value_cansleep(tsdata->reset_gpio, 1);
> +			dev_err(tsdata->dev,
> +				"Failed to initialize the controller: %d\n",
> +				error);
> +			return;
> +		}
> +		goto out;
> +	}

[Severity: High]
Is it safe to retry edt_ft5x06_ts_init_hw() on a subsequent panel prepare
cycle if the first attempt fails?

edt_ft5x06_ts_init_hw() performs several one-shot actions that do not roll
back on error:

1. In edt_ft5x06_ts_init_hw() -> edt_ft5x06_ts_identify(), if an EDT_M06
   chip is detected, it destroys tsdata->regmap and replaces it:

	regmap_exit(tsdata->regmap);
	tsdata->regmap = regmap_init_i2c(client,
					 &edt_M06_i2c_regmap_config);
	if (IS_ERR(tsdata->regmap)) {
		dev_err(tsdata->dev, "regmap allocation failed\n");
		return PTR_ERR(tsdata->regmap);
	}

   If regmap_init_i2c() returns an error pointer, the next retry will pass
   that error pointer into regmap_bulk_read() and dereference it. Even if
   regmap_init_i2c() succeeds and a later step in edt_ft5x06_ts_init_hw()
   fails, the next retry will call edt_ft5x06_ts_identify() using
   edt_M06_i2c_regmap_config, which masks the 0xBB identification register
   to 0x3B and only reads a single byte, breaking M06 identification.

2. edt_ft5x06_ts_init_hw() calls input_register_device(tsdata->input) on the
   input_dev allocated once in edt_ft5x06_probe(). If input_register_device()
   fails, struct input_dev cannot be passed to input_register_device() a
   second time on the next retry.

3. In edt_ft5x06_ts_init_hw(), if tsdata->boot() fails, it calls
   dev_err_probe() from the background panel_work workqueue after
   edt_ft5x06_probe() has already returned.

> +
> +	gpiod_set_value_cansleep(tsdata->reset_gpio, 0);
> +
> +	if (tsdata->boot) {
> +		error = tsdata->boot(tsdata->dev, tsdata->reset_gpio);
> +		if (error) {
> +			dev_err(tsdata->dev,
> +				"Failed to boot the controller: %d\n", error);
> +			return;
> +		}
> +	} else {
> +		msleep(300);
> +	}

[Severity: Medium]
Should reset_gpio be re-asserted to 1 if tsdata->boot() fails here, just as
is done in the !tsdata->hw_initialized error path above?

Because tsdata->powered_off remains true on this early return, a subsequent
call to edt_ft5x06_panel_unpreparing() will see tsdata->powered_off == true
and return early without asserting reset_gpio to 1:

	if (tsdata->powered_off)
		return 0;

	disable_irq(tsdata->irq);
	gpiod_set_value_cansleep(tsdata->reset_gpio, 1);
	tsdata->powered_off = true;

That leaves reset_gpio deasserted (0) while the shared TDDI panel power goes
off, and also prevents a 1 -> 0 reset transition on the next panel wake-up.

> +
> +	edt_ft5x06_restore_reg_parameters(tsdata);
> +out:
> +	tsdata->powered_off = false;
> +	enable_irq(tsdata->irq);
> +}

[Severity: Medium]
What happens if tsdata->factory_mode is active across a panel power cycle, or
if the debugfs files are accessed while tsdata->powered_off is true?

In edt_ft5x06_debugfs_mode_set() and edt_ft5x06_debugfs_raw_data_read(),
tsdata->mutex is acquired, but unlike edt_ft5x06_setting_show() and
edt_ft5x06_setting_store(), neither function checks tsdata->powered_off
before accessing the controller over regmap or toggling IRQ state.

In addition, if factory mode was enabled before the panel turned off
(calling disable_irq(tsdata->irq) once and setting tsdata->factory_mode =
true), edt_ft5x06_panel_unpreparing() will call disable_irq(tsdata->irq) a
second time and assert reset_gpio. When edt_ft5x06_panel_work() runs on
resume, the controller comes out of reset in work mode while
tsdata->factory_mode is still true, causing edt_ft5x06_restore_reg_parameters()
on M06 to format writes as factory-mode requests, and leaving tsdata->irq
disabled after the single enable_irq(tsdata->irq) call because
edt_ft5x06_factory_mode() is not called as in edt_ft5x06_ts_resume():

	edt_ft5x06_restore_reg_parameters(tsdata);
	enable_irq(tsdata->irq);

	if (tsdata->factory_mode)
		ret = edt_ft5x06_factory_mode(tsdata);

[ ... ]

> +static void edt_ft5x06_cancel_panel_work(void *data)
> +{
> +	struct edt_ft5x06_ts_data *tsdata = data;
> +
> +	cancel_work_sync(&tsdata->panel_work);
> +}
> +
> +static int edt_ft5x06_register_panel_follower(struct edt_ft5x06_ts_data *tsdata)
> +{
> +	struct device *dev = tsdata->dev;
> +	int error;
> +
> +	INIT_WORK(&tsdata->panel_work, edt_ft5x06_panel_work);
> +	error = devm_add_action_or_reset(dev, edt_ft5x06_cancel_panel_work,
> +					 tsdata);
> +	if (error)
> +		return error;
> +
> +	tsdata->panel_follower.funcs = &edt_ft5x06_panel_follower_funcs;
> +
> +	return devm_drm_panel_add_follower(dev, &tsdata->panel_follower);
> +}

[Severity: High]
Can deferring panel follower removal and cancel_work_sync(&tsdata->panel_work)
to devres actions race with edt_ft5x06_remove() and devres teardown when the
driver is unbound while edt_ft5x06_panel_work() is running?

When unbinding the driver:

1. edt_ft5x06_remove() runs before devres release:

	void edt_ft5x06_remove(struct device *dev)
	{
		struct edt_ft5x06_ts_data *tsdata = dev_get_drvdata(dev);

		edt_ft5x06_ts_teardown_debugfs(tsdata);
	}

   For I2C devices, i2c_device_remove() then immediately calls
   debugfs_remove_recursive(client->debugfs) before devres_release_group().
   Because panel_work has not been cancelled yet, a concurrent execution of
   edt_ft5x06_panel_work() -> edt_ft5x06_ts_init_hw() ->
   edt_ft5x06_ts_prepare_debugfs() can dereference the freed tsdata->debugfs
   dentry and allocate tsdata->raw_buffer after teardown has already run.

2. During devres teardown, remove_nodes() drains dev->devres_head into a
   local todo list before release_nodes() invokes the devm actions for
   drm_panel_remove_follower and edt_ft5x06_cancel_panel_work. If
   edt_ft5x06_panel_work() is mid-flight inside edt_ft5x06_ts_init_hw()
   waiting to finish while edt_ft5x06_cancel_panel_work() blocks in
   cancel_work_sync(), input_register_device(tsdata->input) will append
   devm_input_device_unregister to dev->devres_head after dev->devres_head
   was already drained into todo. release_nodes() will then execute
   devm_input_device_release from todo without having run
   devm_input_device_unregister.

> @@ -1331,14 +1450,24 @@ int edt_ft5x06_probe(struct device *dev, struct regmap *regmap, int irq,
>  	tsdata->factory_mode = false;
>  	dev_set_drvdata(dev, tsdata);
>  
> -	error = edt_ft5x06_ts_init_hw(tsdata);
> -	if (error)
> -		return error;
> +	/*
> +	 * A controller sharing its power with a panel can only be accessed
> +	 * once the panel is up, so leave it in reset and do the rest of the
> +	 * initialization from the panel follower callback.
> +	 */
> +	tsdata->is_panel_follower = drm_is_panel_follower(dev);
> +	if (!tsdata->is_panel_follower) {
> +		error = edt_ft5x06_ts_init_hw(tsdata);
> +		if (error)
> +			return error;
> +	}

[Severity: Medium]
When tsdata->is_panel_follower is true and edt_ft5x06_ts_init_hw() is
deferred to edt_ft5x06_panel_work(), should model_show() and
fw_version_show() hold tsdata->mutex when reading tsdata->name and
tsdata->fw_version?

	static ssize_t model_show(struct device *dev,
				  struct device_attribute *attr, char *buf)
	{
		struct edt_ft5x06_ts_data *tsdata = dev_get_drvdata(dev);

		return sysfs_emit(buf, "%s\n", tsdata->name);
	}

	static ssize_t fw_version_show(struct device *dev,
				       struct device_attribute *attr, char *buf)
	{
		struct edt_ft5x06_ts_data *tsdata = dev_get_drvdata(dev);

		return sysfs_emit(buf, "%s\n", tsdata->fw_version);
	}

Previously, tsdata->name and tsdata->fw_version were populated synchronously
in edt_ft5x06_probe() before the sysfs attributes in edt_ft5x06_groups were
registered. With hardware initialization deferred to edt_ft5x06_panel_work(),
edt_ft5x06_ts_identify() now writes to tsdata->name and tsdata->fw_version
under tsdata->mutex after the sysfs files are already visible to userspace,
so unlocked reads in model_show() and fw_version_show() can race with
edt_ft5x06_ts_identify().

[ ... ]

>
>
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017140827.290666-1-lukapanio@gmail.com?part=7