Re: [PATCH] Input: novatek-nvt-ts - support being panel follower

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Unconditional dereference of `data->input` in `on_novatek_panel_unpreparing` causes a kernel crash if initialization failed.
- [High] Devres ordering inversion causes a Use-After-Free of the input device and invalid IRQ access during driver unbind.
- [High] Late `devm_*` allocations in a retryable panel callback cause permanent `-EBUSY` errors on initialization retries.
- [High] Independent userspace `open()` defeats the panel follower power sequencing, unconditionally powering the touchscreen.
- [Medium] Premature assignment of `data->input` without rollback on error leaves a dangling pointer that breaks initialization retries.
--

--- Patch [1]: [PATCH] Input: novatek-nvt-ts - support being panel follower ---
commit 13c26acf74fd7e860a0a8efe083b34d9bd7e6937
Author: Alexey Minnekhanov <alexeymin@minlexx.ru>

    Input: novatek-nvt-ts - support being panel follower

This commit adds support for the novatek-nvt-ts driver to operate in panel
follower mode, where the touchscreen and panel are powered together in sync.
It splits the input device registration out of the probe function, deferring
it until the panel prepares via callbacks.

> diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c
> index 0f771f6819528..8b290f75a8e48 100644
> --- a/drivers/input/touchscreen/novatek-nvt-ts.c
> +++ b/drivers/input/touchscreen/novatek-nvt-ts.c

[ ... ]

> @@ -145,6 +156,9 @@ static int nvt_ts_start(struct input_dev *dev)
>  	struct nvt_ts_data *data = input_get_drvdata(dev);
>  	int error;
>  
> +	if (data->running)
> +		return 0;
> +
>  	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators), data->regulators);

[Severity: High]
Does this bypass the panel follower power sequencing if userspace opens the
device independently?

Once the panel is prepared and the input device is registered, userspace can
open the input node (e.g. /dev/input/eventX) even if the panel has since been
powered off.

When that happens, input_dev->open() calls nvt_ts_start(), which will
unconditionally enable the regulators and IRQ without checking if
data->is_panel_follower restricts power control.

Should this check if the panel is currently prepared before forcing the
hardware on?

[ ... ]

> @@ -292,10 +305,11 @@ static int nvt_ts_probe(struct i2c_client *client)
>  	data->input = input;
>  	input_set_drvdata(input, data);
>  
> -	error = devm_request_threaded_irq(dev, client->irq, NULL, nvt_ts_irq,
> +	error = devm_request_threaded_irq(dev, data->client->irq, NULL,
> +					  nvt_ts_irq,
>  					  IRQF_ONESHOT | IRQF_NO_AUTOEN |
>  						nvt_ts_irq_type[irq_type],
> -					  client->name, data);
> +					  data->client->name, data);
>  	if (error) {
>  		dev_err(dev, "failed to request irq: %d\n", error);
>  		return error;
>  	}

[Severity: Medium]
Does an error here leave data->input as a dangling pointer?

Because data->input = input was already assigned above, if
devm_request_threaded_irq() or input_register_device() fails, the function
returns an error but leaves data->input populated.

When the panel retries initialization in on_novatek_panel_prepared(), it checks
if (!data->input). Since it's no longer NULL, it will skip initialization and
attempt to interact with an unregistered or broken input device.

Should data->input be set back to NULL in these error paths, or assigned
only after everything succeeds?

[ ... ]

> @@ -310,6 +324,76 @@ static int nvt_ts_probe(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static int on_novatek_panel_prepared(struct drm_panel_follower *follower)
> +{
> +	struct nvt_ts_data *data = container_of(follower, struct nvt_ts_data, panel_follower);
> +	int ret;
> +
> +	/* Is this the first power on? */
> +	if (!data->input) {
> +		ret = nvt_ts_initial_power_on_and_register_inputdev(data);
> +		if (ret)
> +			return ret;

[Severity: High]
Does calling devm allocation functions from a retryable callback cause permanent
errors on failure?

If nvt_ts_initial_power_on_and_register_inputdev() fails partway through
(e.g., an I2C timeout), it will return an error, but any devm resources it
already acquired (like devm_gpiod_get) will remain attached to the device.

When the panel power-on is retried later, it will execute again and attempt
to acquire the same GPIO and regulators, which will fail with -EBUSY.

Should these devm allocations be moved back to the probe function, or cleaned
up manually on error?

> +	}
> +
> +	guard(mutex)(&data->input->mutex);
> +
> +	if (input_device_enabled(data->input))
> +		nvt_ts_start(data->input);
> +
> +	return 0;
> +}
> +
> +static int on_novatek_panel_unpreparing(struct drm_panel_follower *follower)
> +{
> +	struct nvt_ts_data *data = container_of(follower, struct nvt_ts_data, panel_follower);
> +
> +	guard(mutex)(&data->input->mutex);

[Severity: Critical]
Is it possible for data->input to be NULL here?

If nvt_ts_initial_power_on_and_register_inputdev() fails during
on_novatek_panel_prepared() (for instance, due to an I2C error), data->input
will remain NULL.

Since the follower is still registered, the DRM framework will eventually call
on_novatek_panel_unpreparing(), and this guard(mutex) will dereference the NULL
pointer.

Should this check if data->input is valid before attempting to lock its mutex?

[Severity: High]
Could the devm lifecycle ordering cause a use-after-free here during unbind?

When nvt_ts_probe() calls devm_drm_panel_add_follower(), it registers the
follower cleanup action. Later, on_novatek_panel_prepared() calls
nvt_ts_initial_power_on_and_register_inputdev(), which allocates the input
device via devm_input_allocate_device() and the IRQ via
devm_request_threaded_irq().

Because devres cleans up in LIFO order, on unbind:
1. The IRQ and input device are freed first.
2. The follower is removed, which invokes on_novatek_panel_unpreparing().

This means on_novatek_panel_unpreparing() will try to lock the mutex of an
already freed input device, and nvt_ts_stop() will attempt to disable an
already freed IRQ.

> +
> +	if (input_device_enabled(data->input))
> +		nvt_ts_stop(data->input);
> +
> +	return 0;
> +}

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