Re: [PATCH v3 3/3] Input: ilitek_ts: ratelimit unexpected report logging
From: sashiko-bot@kernel.org
Date: 2026-07-23 15:49:46
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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