Re: [PATCH 4/5] Input: imagis - add support for the IST40xx touch reporting format

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

New issues:
- [High] Unconditional use of INPUT_MT_DROP_UNUSED breaks multitouch tracking for the new event-based protocol.

Pre-existing issues:
- [High] Missing i2c_set_clientdata() in probe leads to a NULL pointer dereference during suspend/resume.
--

--- Patch [4]: [PATCH 4/5] Input: imagis - add support for the IST40xx touch reporting format ---
commit 5561fc562e0ffc99ddfa7c21852a4e2ad13ae0e3
Author: Markuss Broks <[email protected]>

Input: imagis - add support for the IST40xx touch reporting format

This commit adds support for the IST40xx family to the Imagis touchscreen
driver. It introduces the IMAGIS_PROTOCOL_TOUCH_EVENTS protocol which 
derives contact slots from hardware-assigned touch IDs and translates
press/release events into contact state.

> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
> index 6552d97efe32e..5254cccdd653e 100644
> --- a/drivers/input/touchscreen/imagis.c
> +++ b/drivers/input/touchscreen/imagis.c
[ ... ]
> @@ -150,9 +162,31 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
>  			goto out;
>  		}
>  
> -		pressed = finger_pressed & BIT(i);
> +		if (ts->tdata->protocol == IMAGIS_PROTOCOL_TOUCH_EVENTS) {
> +			unsigned int id = FIELD_GET(IST40XX_TOUCH_ID_MASK,
> +						    finger_status);
> +
> +			if (!id || id > IST3038C_MAX_FINGER_NUM)
> +				continue;
> +
> +			slot = id - 1;
> +
> +			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;

[Severity: High]
Will this continue cause active, stationary contacts to be erroneously dropped?

When using the new IMAGIS_PROTOCOL_TOUCH_EVENTS protocol, records with an
unrecognized event type are skipped here. Additionally, if the hardware omits
stationary contacts from the event stream entirely, they will not be processed
in this loop.

Because imagis_init_input_dev() unconditionally initializes MT slots with
INPUT_MT_DROP_UNUSED:

	error = input_mt_init_slots(input_dev, IST3038C_MAX_FINGER_NUM,
				    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);

If a slot is not explicitly updated with input_mt_report_slot_state() during
the frame, won't input_mt_sync_frame() incorrectly release these valid,
skipped contacts and break multitouch tracking?

> +			}
> +		} else {
> +			pressed = finger_pressed & BIT(i);
> +		}
>  
> -		input_mt_slot(ts->input_dev, i);
> +		input_mt_slot(ts->input_dev, slot);
>  		input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, pressed);
>  		if (!pressed)
>  			continue;

[Severity: High]
This is a pre-existing issue, but does the driver fail to set the I2C client
data during probe?

Looking at imagis_probe(), the private ts structure is allocated but never
attached to the I2C client device via i2c_set_clientdata().

Later, during suspend/resume:

imagis_suspend() {
	struct imagis_ts *ts = i2c_get_clientdata(client);
	...
	guard(mutex)(&ts->input_dev->mutex);
}

Since i2c_get_clientdata() will return NULL because it was never set, won't
this cause a NULL pointer dereference panic on suspend or resume?

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