Re: [PATCH v2 5/5] arm64: dts: exynos7870-j6lte: set mode 0 as default for zinitix touchscreen

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] NULL pointer dereference in IRQ handler due to race condition with input device registration.
- [Medium] Commit message and code logic mismatch regarding finger event filtering.
--

--- Patch [5]: [PATCH v2 5/5] arm64: dts: exynos7870-j6lte: set mode 0 as default for zinitix touchscreen ---
commit 8857c310eb5f5f4f8a5e742e862931240c3227fa
Author: Kaustabh Chakraborty <[email protected]>

arm64: dts: exynos7870-j6lte: set mode 0 as default for zinitix touchscreen

This commit sets the default mode for the ZT7548 touchscreen chip to 0 in the
Galaxy J6 device tree. It ensures that fake aftermarket digitizers, which only
implement mode 0, function correctly since the downstream kernel driver also
utilizes mode 0.

> arm64: dts: exynos7870-j6lte: set mode 0 as default for zinitix touchscreen

[Severity: Medium]
Is there a mismatch between the commit message and code logic regarding finger
event filtering?

The commit "Input: zinitix - do not ignore non-moving fingers" states in its
message: "...under the following condition: if (p->sub_status & SUB_BIT_EXIST)
zinitix_report_finger(bt541, i, p); This implies and ensures that every valid
finger must have the SUB_BIT_EXIST flag."

However, the condition simply checks if (p->sub_status):

drivers/input/touchscreen/zinitix.c:zinitix_ts_irq_handler() {
    ...
		/* Only process contacts that are actually reported */
		if (p->sub_status)
			zinitix_report_finger(bt541, i, p);
    ...
}

Does this regression allow events without SUB_BIT_EXIST to bypass the filter,
invalidating the stated premise?

> diff --git a/arch/arm64/boot/dts/exynos/exynos7870-j6lte.dts b/arch/arm64/boot/dts/exynos/exynos7870-j6lte.dts
> index de30d0970336f..bd8f798c9810b 100644
> --- a/arch/arm64/boot/dts/exynos/exynos7870-j6lte.dts
> +++ b/arch/arm64/boot/dts/exynos/exynos7870-j6lte.dts
> @@ -424,6 +424,8 @@ touchscreen@20 {
>  		touchscreen-size-y = <1480>;
>  
>  		vdd-supply = <&vdd_ldo34>;
> +
> +		zinitix,mode = <0>;

[Severity: High]
Could setting the mode here introduce a regression where a NULL pointer
dereference occurs in the IRQ handler due to a race condition with input
device registration?

Looking at drivers/input/touchscreen/zinitix.c:zinitix_ts_probe(), it calls
zinitix_init_input_dev() which registers the input device:

drivers/input/touchscreen/zinitix.c:zinitix_init_input_dev() {
    ...
	error = input_register_device(input_dev);
    ...
}

This immediately exposes the evdev node to userspace. If userspace (e.g., udev)
opens the device instantly, zinitix_input_open() invokes zinitix_start(),
which enables the hardware IRQ.

If an interrupt fires at this exact moment, zinitix_ts_irq_handler() runs and
dereferences the callback:

drivers/input/touchscreen/zinitix.c:zinitix_ts_irq_handler() {
    ...
	error = bt541->read_point_status(bt541, &touch_event);
    ...
}

However, the probe thread has not yet assigned this function pointer, because
it happens later via the mode switch:

drivers/input/touchscreen/zinitix.c:zinitix_ts_probe() {
    ...
	switch (bt541->zinitix_mode) {
	case 0:
		bt541->read_point_status = zinitix_read_point_status_mode0;
    ...
}

Since the function pointer is uninitialized when the IRQ fires, would this
regression cause a kernel panic?

>  	};
>  };

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