Re: [PATCH 5/5] Input: imagis - add support for IST4050

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

New issues:
- [High] The IMAGIS_PROTOCOL_TOUCH_EVENTS protocol handling drops active contacts due to incompatibility with INPUT_MT_DROP_UNUSED.

Pre-existing issues:
- [High] Missing i2c_set_clientdata() in probe causes guaranteed NULL pointer dereference during system suspend/resume.
- [High] Stack-allocated buffers are passed to I2C transfers, violating DMA constraints and potentially failing device probe.
--

--- Patch [5]: [PATCH 5/5] Input: imagis - add support for IST4050 ---
commit cea4b151bd39f3f451e6ebe48a6b0a29508b291a
Author: Markuss Broks <[email protected]>

Input: imagis - add support for IST4050

The Imagis IST4050 shares the register interface and the power-up
sequence with IST3038C, and reports touches using the IST40xx touch
reporting format. Its chip ID register reads back 0x4050.

Also mention the IST40xx family in the Kconfig help text, and refer to
the older chips as IST30xx, since the driver supports more than the C
variants.

> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
> index 5254cccdd653e..9bf5afd438488 100644
> --- a/drivers/input/touchscreen/imagis.c
> +++ b/drivers/input/touchscreen/imagis.c
[ ... ]
> @@ -491,12 +492,21 @@ static const struct imagis_properties imagis_3038h_data = {
>  	.protocol = IMAGIS_PROTOCOL_SHARED_REGISTER,
>  };
>  
> +static const struct imagis_properties imagis_4050_data = {
> +	.interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE,
> +	.touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
> +	.whoami_cmd = IST3038C_REG_CHIPID,
> +	.whoami_val = IST4050_WHOAMI,
> +	.protocol = IMAGIS_PROTOCOL_TOUCH_EVENTS,

[Severity: High]
Does the IMAGIS_PROTOCOL_TOUCH_EVENTS protocol drop active contacts?

In imagis_interrupt(), when processing the IST4050 event stream, the driver
extracts an event type. If the hardware sends an event type other than
PRESS or RELEASE (such as a MOVE event), the switch statement hits the
default case:

drivers/input/touchscreen/imagis.c:imagis_interrupt() {
    ...
			switch (FIELD_GET(IST40XX_TOUCH_EVENT_MASK,
					  finger_status)) {
			case IST40XX_TOUCH_EVENT_PRESS:
				pressed = true;
				break;
			case IST40XX_TOUCH_EVENT_RELEASE:
				pressed = false;
				break;
			default:
				continue;
			}
    ...
}

This silently skips the input_mt_slot() update. Also, if the hardware
only reports contacts with changes, unchanged stationary contacts will
not appear in the loop.

Because input_mt_init_slots() is configured with INPUT_MT_DROP_UNUSED:

drivers/input/touchscreen/imagis.c:imagis_init_input_dev() {
    ...
	error = input_mt_init_slots(input_dev,
				    IST3038C_MAX_FINGER_NUM,
				    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
    ...
}

input_mt_sync_frame() will unconditionally drop any previously active
contact that is not explicitly updated in the current frame, which would
break touch tracking for moving or stationary contacts.

[Severity: High]
This is a pre-existing issue, but I noticed imagis_probe() allocates the
context but never assigns it to the device client data:

drivers/input/touchscreen/imagis.c:imagis_probe() {
    ...
	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
	if (!ts)
		return -ENOMEM;

	ts->client = i2c;
    ...
}

When the system suspends, imagis_suspend() retrieves an uninitialized
NULL pointer and dereferences it:

drivers/input/touchscreen/imagis.c:imagis_suspend() {
    ...
	struct i2c_client *client = to_i2c_client(dev);
	struct imagis_ts *ts = i2c_get_clientdata(client);
	int error;

	guard(mutex)(&ts->input_dev->mutex);
    ...
}

Can this lead to a guaranteed kernel panic during system suspend and
resume cycles?

[Severity: High]
This is a pre-existing issue, but I noticed stack-allocated buffers are
passed to I2C transfers in imagis_i2c_read_reg():

drivers/input/touchscreen/imagis.c:imagis_i2c_read_reg() {
    ...
	__be32 ret_be;
	__be32 reg_be = cpu_to_be32(reg);
	struct i2c_msg msg[] = {
		{
			.addr = ts->client->addr,
			.flags = 0,
			.buf = (unsigned char *)&reg_be,
			.len = sizeof(reg_be),
		},
    ...
}

If the system's I2C host controller relies on DMA and CONFIG_VMAP_STACK
is enabled, the kernel stack is not physically contiguous. Could this
cause the DMA mapping to fail and the device probe to abort?

> +};
> +
>  static const struct of_device_id imagis_of_match[] = {
>  	{ .compatible = "imagis,ist3032c", .data = &imagis_3032c_data },
>  	{ .compatible = "imagis,ist3038", .data = &imagis_3038_data },

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5