Re: [PATCH v5 3/6] HID: himax: Add DRM panel follower support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] input: unbounded resource leaks and permanent initialization failures on DRM panel prepare retries
- [High] hid: destroying and recreating the HID device on panel suspend/resume breaks userspace input continuity
- [High] input: masking of -EPROBE_DEFER breaks DRM panel follower probe deferral
- [Medium] hid: missing hardware boot delay after reset pin deassertion in himax_chip_resume()
--

commit eb8f38c8024df3c3e0a8931cb1c98f90a31716d2
Author: Allen_Lin <allencl_lin@hotmail.com>

    HID: himax: Add DRM panel follower support
    
    The touch function of the HX83102J is part of a TDDI controller and has
    to follow the power sequence of the panel. Register as a DRM panel
    follower, and suspend and resume the touch function when the panel is
    unprepared and prepared.
    
    Link: https://lore.kernel.org/all/20230727171750.633410-1-dianders@chromium.org

> diff --git a/drivers/hid/hid-himax.c b/drivers/hid/hid-himax.c
> index 1cdb310dec642..653c92829ec7f 100644
> --- a/drivers/hid/hid-himax.c
> +++ b/drivers/hid/hid-himax.c

[ ... ]

> +static int himax_chip_suspend(struct himax_ts_data *ts)
> +{
> +	himax_int_enable(ts, false);
> +	gpiod_set_value(ts->pdata.gpiod_rst, 1);
> +	himax_power_set(ts, false);
> +	himax_hid_remove(ts);

[Severity: High]
Does tearing down the HID device here break userspace input continuity?

When the panel unblanks and blanks, destroying and recreating the HID device
causes userspace compositors (like X11 or Wayland) to permanently lose track
of the touch device or drop existing input grabs.

> +
> +	return 0;
> +}
> +
> +/**
> + * himax_chip_resume() - Setup flags, I/O and resume
> + * @ts: Himax touch screen data
> + *
> + * This function is used to resume the touch screen. It will set the resume
> + * success flag to false, and disable reset pin. Then call the himax_resume_proc()
> + * to process detailed resume procedure.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int himax_chip_resume(struct himax_ts_data *ts)
> +{
> +	if (himax_power_set(ts, true))
> +		return -EIO;
> +	gpiod_set_value(ts->pdata.gpiod_rst, 0);
> +	himax_resume_proc(ts);

[Severity: Medium]
Is there a missing hardware boot delay after deasserting the reset pin?

The hardware seems to require a 20ms delay after the reset pin is pulled low
(as seen with usleep_range(20000, 20100) in himax_pin_reset()), but here
gpiod_set_value(..., 0) is immediately followed by SPI communication in
himax_resume_proc(). Could this cause intermittent hardware failures, SPI
timeouts, or ignored commands on resume?

> +	himax_hid_probe(ts);

[Severity: High]
This recreates the HID device on panel resume, which as mentioned above,
breaks userspace input continuity.

> +	himax_int_enable(ts, true);
> +
> +	return 0;
> +}

[ ... ]

> +static int __himax_initial_power_up(struct himax_ts_data *ts)
> +{
> +	int ret;
> +
> +	ret = himax_platform_init(ts);
> +	if (ret) {
> +		dev_err(ts->dev, "%s: platform init failed\n", __func__);
> +		return ret;
> +	}
> +
> +	ret = himax_chip_detect(ts);
> +	if (ret) {
> +		dev_err(ts->dev, "%s: IC detect failed\n", __func__);
> +		return ret;
> +	}

[Severity: High]
Does this error path leak resources?

If __himax_initial_power_up() fails midway (e.g., himax_chip_detect() fails
due to IC boot time), it returns without cleaning up the resources allocated
in himax_platform_init() (like the kzalloc'd ts->xfer_buf).

Since the panel follower mechanism will retry asynchronously when the panel
is toggled via himax_panel_prepared(), the retry will overwrite the
ts->xfer_buf pointer and permanently leak memory.

Additionally, since himax_ts_register_interrupt() uses
devm_request_threaded_irq() and the probe function already completed, devm
allocations are not rolled back on asynchronous errors. A subsequent retry
will fail permanently with -EBUSY because the IRQ is already devres-claimed.

Also, if the driver unbinds, himax_spi_drv_remove() checks if
(ts->probe_finish) and skips himax_platform_deinit(), leaving ts->xfer_buf
permanently leaked.

[ ... ]

> @@ -1660,25 +1932,13 @@ static int himax_spi_drv_probe(struct spi_device *spi)
>  	ts->initialized = false;
>  	ts->ic_boot_done = false;
>  
> -	ret = himax_platform_init(ts);
> -	if (ret) {
> -		dev_err(ts->dev, "%s: platform init failed\n", __func__);
> -		return ret;
> -	}
> -
> -	ret = himax_chip_detect(ts);
> +	ret = himax_initial_power_up(ts);
>  	if (ret) {
> -		dev_err(ts->dev, "%s: IC detect failed\n", __func__);
> -		return ret;
> +		dev_err(ts->dev, "%s: initial power up failed\n", __func__);
> +		return -ENODEV;

[Severity: High]
Does returning -ENODEV here break DRM panel follower probe deferral?

If the display panel driver is not yet ready, drm_panel_add_follower() (called
via himax_initial_power_up()) correctly returns -EPROBE_DEFER. Overwriting
this return value with -ENODEV permanently breaks standard probe deferral
logic.

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003142741.48634-1-michal@nozomi.space?part=3