Re: [PATCH v3 1/5] Input: zinitix - check all available fingers for every touch event

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

New issues:
- [High] Unconditionally iterating all slots without a mask reinstates a known phantom fingers bug on certain devices.

Pre-existing issues:
- [High] The input device is registered and exposed to userspace before its operational properties are fully parsed and initialized.
- [High] Failing to read icon events discards already successfully read touch events, bypassing synchronization.
- [High] Stack-allocated variables are used directly as data buffers for I2C transfers, violating DMA safety requirements.
--

--- Patch [1]: [PATCH v3 1/5] Input: zinitix - check all available fingers for every touch event ---
commit 324dca54b2be4e95e88ba6bec7c1bec867e0e687
Author: Kaustabh Chakraborty <[email protected]>
Input: zinitix - check all available fingers for every touch event

This patch changes the touch reporting to iterate unconditionally over all
slots rather than relying on the finger_mask field. It relies on the
SUB_BIT_EXIST bit in the finger status to filter valid touches.

Link: https://android.googlesource.com/kernel/bcm/+/23d376ef33aa4c500a5ea24a290f029d5f8e2de3/drivers/input/touchscreen/zinitix_touch.c#1942 [1]

> diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
> index 0c36765bd79f8..3421b8ffb19b2 100644
> --- a/drivers/input/touchscreen/zinitix.c
> +++ b/drivers/input/touchscreen/zinitix.c
> @@ -445,7 +445,6 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
>  	struct bt541_ts_data *bt541 = bt541_handler;
>  	struct i2c_client *client = bt541->client;
>  	struct touch_event touch_event;

[Severity: High]
This isn't a bug introduced by this patch, but touch_event is allocated on
the stack and then passed to zinitix_read_data(), which feeds it directly
to the I2C subsystem. Since this buffer might be used for DMA transfers on
certain I2C host controllers, can this cause memory corruption or a kernel
panic when CONFIG_VMAP_STACK is enabled?

Similarly, in zinitix_read_data():

zinitix_read_data() {
    __le16 reg_le = cpu_to_le16(reg);
    ...
    ret = i2c_master_send(client, (u8 *)&reg_le, sizeof(reg_le));
    ...
}

Are there similar DMA safety concerns when using the stack-allocated reg_le
buffer for I2C sending?

> -	unsigned long finger_mask;
>  	__le16 icon_events;
>  	int error;
>  	int i;
> @@ -470,8 +469,7 @@ static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
>  		zinitix_report_keys(bt541, le16_to_cpu(icon_events));
>  	}

[Severity: High]
This is a pre-existing issue, but I noticed something about the error handling
just above this block:

zinitix_ts_irq_handler() {
    ...
    if (le16_to_cpu(touch_event.status) & BIT_ICON_EVENT) {
        error = zinitix_read_data(bt541->client, bt541->icon_status_reg,
                                  &icon_events, sizeof(icon_events));
        if (error) {
            dev_err(&client->dev, "Failed to read icon events\n");
            goto out;
        }
        ...
    }
    ...
out:
    zinitix_write_cmd(bt541->client, ZINITIX_CLEAR_INT_STATUS_CMD);
    return IRQ_HANDLED;
}

If zinitix_read_data() for the icon events fails, the code jumps directly to
out. Will this discard the successfully read touch_event data and skip the
input_mt_sync_frame() and input_sync() calls, potentially leaving stuck touches
in the input core?

>  
> -	finger_mask = touch_event.finger_mask;
> -	for_each_set_bit(i, &finger_mask, MAX_SUPPORTED_FINGER_NUM) {
> +	for (i = 0; i < MAX_SUPPORTED_FINGER_NUM; i++) {
>  		const struct point_coord *p = &touch_event.point_coord[i];
>  
>  		/* Only process contacts that are actually reported */

[Severity: High]
The commit message mentions that phantom fingers will be ignored because
zinitix_report_finger() checks the SUB_BIT_EXIST bit. However, the
finger_mask check was originally introduced (in commit e941dc13fd37) because
some hardware erroneously sets SUB_BIT_EXIST for phantom fingers. 

By removing the finger_mask iteration, is it possible this reinstates the
known phantom touches bug on affected hardware (like the BT404) where a
single physical touch causes multiple active slots?

[Severity: High]
This isn't a bug introduced by this patch, but looking at the device
initialization in zinitix_ts_probe():

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

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

Since zinitix_init_input_dev() calls input_register_device(), the input
device becomes immediately visible to userspace. If userspace opens the device
instantly, zinitix_input_open() -> zinitix_start() will run before the
zinitix,mode property is read.

Could this cause the hardware to be configured with an incorrect mode
(0 instead of the requested or default 2)?

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