Re: [PATCH v2 2/3] Input: ilitek_ts: handle short I2C transfers

[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 suspend and resume functions.
- [High] Incorrect touch packet parsing offset across 64-byte chunks for points > 10.
--

--- Patch [2]: [PATCH v2 2/3] Input: ilitek_ts: handle short I2C transfers ---
commit fea6e8240af99ced183b54d3a3d2c69a63ac6791
Author: Kristian Mide <kristian@mide.dk>

Input: ilitek_ts: handle short I2C transfers

Check that i2c_transfer() returns the expected number of
messages in ilitek_i2c_write_and_read().

> diff --git a/drivers/input/touchscreen/ilitek_ts_i2c.c b/drivers/input/touchscreen/ilitek_ts_i2c.c
> index 54e39721f2473..6146baa2b6062 100644
> --- a/drivers/input/touchscreen/ilitek_ts_i2c.c
> +++ b/drivers/input/touchscreen/ilitek_ts_i2c.c
> @@ -131,11 +131,15 @@ static int ilitek_i2c_write_and_read(struct ilitek_ts_data *ts,
>  		error = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>  		if (error < 0)
>  			return error;
> +		if (error != ARRAY_SIZE(msgs))
> +			return -EIO;
>  	} else {
>  		if (write_len > 0) {
>  			error = i2c_transfer(client->adapter, msgs, 1);
>  			if (error < 0)
>  				return error;
> +			if (error != 1)
> +				return -EIO;
>  		}
>  		if (delay > 0)
>  			fsleep(delay * 1000);
> @@ -144,6 +148,8 @@ static int ilitek_i2c_write_and_read(struct ilitek_ts_data *ts,
>  			error = i2c_transfer(client->adapter, msgs + 1, 1);
>  			if (error < 0)
>  				return error;
> +			if (error != 1)
> +				return -EIO;
>  		}
>  	}

drivers/input/touchscreen/ilitek_ts_i2c.c:ilitek_suspend() {
    ...
    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 does this error path return
early without re-enabling the IRQ that was disabled at the start of the
function?

If api_protocol_set_cmd() fails during system suspend or resume, the PM core
will abort the suspend process. Because enable_irq() is skipped, won't
ilitek_resume() not be called for this device, leaving the IRQ permanently
disabled and the device unresponsive?

drivers/input/touchscreen/ilitek_ts_i2c.c:ilitek_resume() {
    ...
    error = api_protocol_set_cmd(ts, SET_IC_WAKE, NULL, NULL);
    if (error)
        return error;
    ...
}

[Severity: High]
This is a pre-existing issue, but does this error path also return early
before the enable_irq() call is reached?

drivers/input/touchscreen/ilitek_ts_i2c.c:ilitek_process_and_report_v6() {
    ...
    status = buf[i * packet_len + 1] & 0x40;
    if (!status)
        continue;
        
    id = buf[i * packet_len + 1] & 0x3F;
    
    x = get_unaligned_le16(buf + i * packet_len + 2);
    y = get_unaligned_le16(buf + i * packet_len + 4);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this purely linear
index calculation i * packet_len cause out-of-bounds offsets into padding
areas and conflict with metadata offsets for indices >= 10?

The driver reads touch points into buf in 64-byte chunks, each holding up
to 10 touch points, and advances the buffer pointer by 64 bytes for each
chunk.

Because each chunk only uses 50 bytes of data (leaving 14 bytes of padding),
won't the linear formula fail to account for the 64-byte chunk boundaries
instead of correctly tracking them?

For example, for the 11th point (i = 10), the driver should read from
buf[64], but it incorrectly reads from buf[50] (which is inside the first
chunk's padding). For i = 12, the offset buf[12 * 5 + 1] resolves to 61,
colliding exactly with REPORT_COUNT_ADDRESS, resulting in corrupted
coordinates and statuses for points > 10.

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