Re: [PATCH v3 2/5] Input: zinitix - do not ignore non-moving fingers

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

New issues:
- [High] The patch incorrectly relaxes the validation of touch contacts, turning an AND condition into an OR condition, which risks processing stale slots as ghost touches.

Pre-existing issues:
- [High] The input device is registered and exposed to the system before its private data is fully initialized, causing it to be opened with uninitialized hardware settings.
--

--- Patch [2]: [PATCH v3 2/5] Input: zinitix - do not ignore non-moving fingers ---
commit 454d687d20badd95d32dd2be0a6d7cb81eb6b592
Author: Kaustabh Chakraborty <[email protected]>

Input: zinitix - do not ignore non-moving fingers

This commit allows the ZT7548 touchscreen driver to report fingers even when
they haven't moved since the last interrupt. It does this by adding
SUB_BIT_EXIST to the valid status bits and relaxing the sub_status check in
the interrupt handler to allow any non-zero sub_status.

> diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
> index 3421b8ffb19b2..a0a89d25e25bb 100644
> --- a/drivers/input/touchscreen/zinitix.c
> +++ b/drivers/input/touchscreen/zinitix.c
> @@ -406,7 +406,7 @@ static void zinitix_report_finger(struct bt541_ts_data *bt541, int slot,
>  	u16 x, y;
>  
>  	if (unlikely(!(p->sub_status &
> -		       (SUB_BIT_UP | SUB_BIT_DOWN | SUB_BIT_MOVE)))) {
> +		       (SUB_BIT_EXIST | SUB_BIT_UP | SUB_BIT_DOWN | SUB_BIT_MOVE)))) {

[Severity: High]
Does this modification allow a contact with SUB_BIT_MOVE, SUB_BIT_UP, or
SUB_BIT_DOWN to bypass validation and report coordinates even if the mandatory
SUB_BIT_EXIST valid bit is missing?

If the hardware clears the SUB_BIT_EXIST flag for a removed finger but leaves
stale status bits (like SUB_BIT_MOVE) in the I2C memory buffer, will this
cause stale slots to be processed as ghost touches?

>  		dev_dbg(&bt541->client->dev, "unknown finger event %#02x\n",
>  			p->sub_status);
>  		return;

[ ... ]

> @@ -473,7 +473,7 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
>  		const struct point_coord *p = &touch_event.point_coord[i];
>  
>  		/* Only process contacts that are actually reported */
> -		if (p->sub_status & SUB_BIT_EXIST)
> +		if (p->sub_status)

[Severity: High]
Will removing the strict SUB_BIT_EXIST check pass any slot with stale status
bits to the processing logic?

Since the driver now relies entirely on status bits to validate active slots,
a stale slot without SUB_BIT_EXIST but with a non-zero sub_status (like
SUB_BIT_MOVE) will now pass both checks.

>  			zinitix_report_finger(bt541, i, p);
>  	}

[Severity: High]
This is a pre-existing issue, but does the driver register the input device
before fully initializing its private data?

In drivers/input/touchscreen/zinitix.c:zinitix_ts_probe():

	error = zinitix_init_input_dev(bt541);
	if (error) {
		...
	}

	error = device_property_read_u32(&client->dev, "zinitix,mode",
					 &bt541->zinitix_mode);

If the kernel's kbd_handler synchronously opens the device during registration
(due to EV_KEY), zinitix_init_touch() will configure the hardware using the
uninitialized zinitix_mode (0) instead of the parsed mode. Does this lead to
malfunctioning touch reporting for devices declaring linux,keycodes in their
device tree?

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