Re: [PATCH 2/7] Input: edt-ft5x06 - move the controller bring-up into a helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Input: edt-ft5x06: edt_ft5x06_ts_init_hw() registers device before IRQ
--

commit 5aab9a2f7e5d9f696c6cdfb8f9baf87581af8689
Author: Luka Panio <lukapanio@gmail.com>

Input: edt-ft5x06 - move the controller bring-up into a helper

This commit moves the controller reset release, hardware identification,
input device registration, and debugfs setup out of edt_ft5x06_probe()
into a new edt_ft5x06_ts_init_hw() helper function.

> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index 543cc62088c7a..536d633d97a78 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -1116,16 +1116,96 @@ static void edt_ft5x06_disable_regulators(void *arg)
>  	regulator_disable(data->iovcc);
>  }
>  
> +/*
> + * Take the controller out of reset, identify it and register the input
> + * device.
> + */
> +static int edt_ft5x06_ts_init_hw(struct edt_ft5x06_ts_data *tsdata)
> +{

[ ... ]

> +	error = input_register_device(input);
> +	if (error)
> +		return error;
> +
> +	edt_ft5x06_ts_prepare_debugfs(tsdata);
> +
> +	return 0;
> +}
> +
>  int edt_ft5x06_probe(struct device *dev, struct regmap *regmap, int irq,
>  		     const struct edt_ft5x06_chip_data *chip_data, u16 bustype,
>  		     struct dentry *debugfs)
>  {

[ ... ]

> @@ -1225,78 +1305,22 @@ int edt_ft5x06_probe(struct device *dev, struct regmap *regmap, int irq,

[ ... ]

> +	error = edt_ft5x06_ts_init_hw(tsdata);
> +	if (error)
>  		return error;
> -	}
>  
>  	irq_flags = irq_get_trigger_type(irq);
>  	if (irq_flags == IRQF_TRIGGER_NONE)
> @@ -1310,12 +1334,6 @@ int edt_ft5x06_probe(struct device *dev, struct regmap *regmap, int irq,
>  		return error;
>  	}
>  
> -	error = input_register_device(input);
> -	if (error)
> -		return error;
> -
> -	edt_ft5x06_ts_prepare_debugfs(tsdata);
> -
>  	dev_dbg(dev,
>  		"EDT FT5x06 initialized: IRQ %d, WAKE pin %d, Reset pin %d.\n",
>  		irq,

[Severity: Medium]
Previously, edt_ft5x06_probe() called devm_request_threaded_irq() before
calling input_register_device() and edt_ft5x06_ts_prepare_debugfs().

With edt_ft5x06_ts_init_hw() now running before devm_request_threaded_irq(),
the input device and debugfs files are published to userspace before the
touchscreen IRQ is requested.

If devm_request_threaded_irq() fails, the input device is announced to
userspace only to be immediately torn down during devres release.

Additionally, because edt_ft5x06_ts_prepare_debugfs() exposes the debugfs
mode file early, writing 1 to it before or during IRQ setup triggers:

edt_ft5x06_debugfs_mode_set()
  -> edt_ft5x06_factory_mode()
       disable_irq(tsdata->irq);
       if (!tsdata->raw_buffer) {
           tsdata->raw_bufsize = tsdata->num_x * tsdata->num_y *
                                 sizeof(u16);
           tsdata->raw_buffer = kzalloc(tsdata->raw_bufsize, GFP_KERNEL);
           ...
       }

Can this cause disable_irq() (or enable_irq() on error or when switching
back in edt_ft5x06_work_mode()) to be called before the IRQ is requested, or
cause irq_startup() inside devm_request_threaded_irq() to re-enable the
interrupt while tsdata->factory_mode is true?

Also, if devm_request_threaded_irq() fails after tsdata->raw_buffer is
allocated, edt_ft5x06_remove() -> edt_ft5x06_ts_teardown_debugfs() is not
run on probe failure. Does this leak tsdata->raw_buffer?

Should devm_request_threaded_irq() be called before edt_ft5x06_ts_init_hw()
(for example, with IRQF_NO_AUTOEN if needed until the hardware is ready), or
should input_register_device() and edt_ft5x06_ts_prepare_debugfs() happen
after the IRQ is requested?

oggler

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