Re: [PATCH v3 3/3] Input: ilitek_ts: ratelimit unexpected report logging

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `enable_irq()` on error paths in PM suspend and resume callbacks, leaving the touchscreen interrupt permanently disabled.
- [High] The driver unconditionally disables the device IRQ during suspend, breaking system wakeup.
--

--- Patch [3]: [PATCH v3 3/3] Input: ilitek_ts: ratelimit unexpected report logging ---
commit d857848ba27cfb246552c60fc75946ecead183d4
Author: Kristian Mide <kristian@mide.dk>

Input: ilitek_ts: ratelimit unexpected report logging

This commit replaces dev_err() with dev_err_ratelimited() when logging
unexpected report IDs in the ilitek touchscreen driver. It adds the first
16 bytes of the raw packet to the log for better debugging context. It also
returns success after logging to prevent duplicate errors from being emitted
by the IRQ handler.

> diff --git a/drivers/input/touchscreen/ilitek_ts_i2c.c b/drivers/input/touchscreen/ilitek_ts_i2c.c
> index a184d024d6c4f..2f1741d77e6a9 100644
> --- a/drivers/input/touchscreen/ilitek_ts_i2c.c
> +++ b/drivers/input/touchscreen/ilitek_ts_i2c.c
> @@ -238,8 +238,10 @@ static int ilitek_process_and_report_v6(struct ilitek_ts_data *ts)
>  		return ilitek_process_pen_report(ts, buf);
>  
>  	if (buf[0] != ILITEK_TP_I2C_REPORT_ID) {
> -		dev_err(dev, "get touch info failed. Wrong id: 0x%02X\n", buf[0]);
> -		return -EINVAL;
> +		dev_err_ratelimited(dev,
> +				    "get touch info failed. Wrong id: 0x%02X raw[0:16]=%*ph\n",
> +				    buf[0], 16, buf);
> +		return 0;
>  	}
>  
>  	report_max_point = buf[REPORT_COUNT_ADDRESS];

[ ... ]

While looking at this driver, I noticed a few pre-existing issues in the
power management callbacks:

> static int ilitek_suspend(struct device *dev)
> {
> 	struct i2c_client *client = to_i2c_client(dev);
> 	struct ilitek_ts_data *ts = i2c_get_clientdata(client);
> 	int error;
> 
> 	disable_irq(client->irq);
> 
> 	if (!device_may_wakeup(dev)) {

[Severity: High]
This is a pre-existing issue, but does unconditionally masking the IRQ here
break system wakeup?

By calling disable_irq(), the interrupt controller is prevented from seeing
the event and waking the CPU. Even though the sleep command is skipped when
device_may_wakeup(dev) is true, the unconditionally disabled IRQ line ensures
that any touch events generated by the hardware while suspended are dropped.

> 		error = api_protocol_set_cmd(ts, SET_IC_SLEEP, NULL, NULL);
> 		if (error)
> 			return error;

[Severity: High]
This isn't a bug introduced by this patch, but if api_protocol_set_cmd()
fails, does this error path leave the touchscreen interrupt permanently
disabled?

If this returns early without calling enable_irq(), the PM core aborts the
suspend process. Since the suspend failed, the resume callback is never
invoked for this device, leaving the IRQ depth unbalanced and the touchscreen
unresponsive until a reboot or module reload.

> 	}
> 
> 	return 0;
> }
> 
> static int ilitek_resume(struct device *dev)
> {
> 	struct i2c_client *client = to_i2c_client(dev);
> 	struct ilitek_ts_data *ts = i2c_get_clientdata(client);
> 	int error;
> 
> 	if (!device_may_wakeup(dev)) {
> 		error = api_protocol_set_cmd(ts, SET_IC_WAKE, NULL, NULL);
> 		if (error)
> 			return error;

[Severity: High]
This is a pre-existing issue, but could this error path also leave the IRQ
disabled?

Similar to the suspend callback, if SET_IC_WAKE fails here, the function
returns an error before reaching enable_irq(client->irq). This would also
leave the interrupt permanently disabled.

> 
> 		ilitek_reset(ts, ts->reset_time);
> 	}
> 
> 	enable_irq(client->irq);
> 
> 	return 0;
> }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723152244.11252-1-kristian@mide.dk?part=3