Re: [PATCH v5 2/3] Input: stmfts - support FTS5

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

New issues:
- [High] Unconditional dereference of `sdata->ops` leads to NULL pointer dereference if probed via I2C board info.
- [High] The FTS5 Y-coordinate is entirely corrupted due to erroneous MSB and LSB bit shifts.
- [Medium] The power-off sequence attempts an I2C write after the chip has already been placed into hardware reset.
- [Medium] Touch Major and Minor axes are corrupted due to logic errors combining bits with the constant Event ID.

Pre-existing issues:
- [High] The touchscreen is left in a sleep state after system resume if it was active prior to suspend.
--

--- Patch [2]: [PATCH v5 2/3] Input: stmfts - support FTS5 ---
commit 8851d4460d3a4355e8954e1dd8b5a724d25fb3be
Author: David Heidelberg <[email protected]>
Subject: Input: stmfts - support FTS5

This commit adds support for FTS5 touchscreens to the stmfts driver. It
introduces a mode-switch GPIO for SLPI and AP modes, currently only handling
full power AP mode.

> diff --git a/drivers/input/touchscreen/stmfts.c b/drivers/input/touchscreen/stmfts.c
> index 972687797f826..1da113d7197e6 100644
> --- a/drivers/input/touchscreen/stmfts.c
> +++ b/drivers/input/touchscreen/stmfts.c

[ ... ]

> @@ -205,6 +246,104 @@ static void stmfts_report_contact_release(struct stmfts_data *sdata,
>  	input_sync(sdata->input);
>  }
>  
> +/* FTS5 event handling functions */
> +static void stmfts5_report_contact_event(struct stmfts_data *sdata,
> +					 const u8 event[])
> +{
[ ... ]
> +	/* Parse coordinates with better precision */
> +	x = (((int)event[3] & STMFTS_MASK_X_MSB) << 8) | event[2];
> +	y = ((int)event[4] << 4) | ((event[3] & STMFTS_MASK_Y_LSB) >> 4);

[Severity: High]
Does this bitwise logic for the Y coordinate scramble the value?

According to the comment above this code, event[4] is the 8-bit Y LSB, and
event[3] contains the 4-bit Y MSB. By shifting event[4] up by 4 and event[3]
down by 4, this appears to treat the 8-bit LSB as the most significant bits
and the 4-bit MSB as the least significant bits.

Should this be (((int)event[3] & STMFTS_MASK_Y_LSB) << 4) | event[4] instead?

> +
> +	/* Parse pressure - ensure non-zero for active touch */
> +	area = event[5];
[ ... ]
> +
> +	/* Parse touch area with improved bit extraction */
> +	maj = (((event[0] & 0x0C) << 2) | ((event[6] & 0xF0) >> 4));
> +	min = (((event[7] & 0xC0) >> 2) | (event[6] & 0x0F));

[Severity: Medium]
Is it intentional to use event[0] to calculate maj?

event[0] holds the event ID (like STMFTS5_EV_MULTI_TOUCH_ENTER which is
0x13). For valid contact events, event[0] & 0x0C will always be zero,
meaning maj only captures the upper 4 bits of event[6].

Does this result in malformed touch area statistics being reported?

[ ... ]

> @@ -601,15 +835,11 @@ static int stmfts_power_on(struct stmfts_data *sdata)
>  	if (sdata->reset_gpio)
>  		stmfts_reset(sdata);
>  
> -	err = stmfts_read_system_info(sdata);
> -	if (err)
> -		goto err_disable_regulators;
> -
>  	enable_irq(sdata->client->irq);
>  
>  	msleep(50);
>  
> -	err = stmfts_configure(sdata);
> +	err = sdata->ops->configure(sdata);
>  	if (err)
>  		goto err_disable_irq;

[Severity: High]
This is a pre-existing issue, but does stmfts_power_on() unintentionally
leave the touchscreen dead after a system resume if it was active before
suspend?

When resuming, the device is powered on and unconditionally put to sleep
with STMFTS_SLEEP_IN at the end of stmfts_power_on():

        /*
         * At this point no one is using the touchscreen
         * and I don't really care about the return value
         */
        (void)i2c_smbus_write_byte(sdata->client, STMFTS_SLEEP_IN);

Because the PM core still considers the device RPM_ACTIVE from before the
suspend, it will not automatically invoke stmfts_runtime_resume(). Thus,
STMFTS_SLEEP_OUT is never sent and the hardware remains unresponsive until
userspace closes and reopens the device.

[ ... ]

> @@ -637,9 +887,61 @@ static void stmfts_power_off(void *data)
>  	if (sdata->reset_gpio)
>  		gpiod_set_value_cansleep(sdata->reset_gpio, 1);
>  
> +	if (sdata->ops->power_off)
> +		sdata->ops->power_off(sdata);

[Severity: Medium]
Will this cause an I2C transaction timeout?

By asserting the reset GPIO to 1 just before calling
sdata->ops->power_off(), the chip is placed into hardware reset.

When stmfts5_chip_power_off() immediately tries to send the
STMFTS_SLEEP_IN I2C command:

static void stmfts5_chip_power_off(struct stmfts_data *sdata)
{
        i2c_smbus_write_byte(sdata->client, STMFTS_SLEEP_IN);
        msleep(20);
}

The unresponsive chip would likely NACK the transfer or cause a bus
timeout since it is held in reset.

[ ... ]

> @@ -685,6 +987,8 @@ static int stmfts_probe(struct i2c_client *client)
>  	mutex_init(&sdata->mutex);
>  	init_completion(&sdata->cmd_done);
>  
> +	sdata->ops = of_device_get_match_data(dev);
> +
[ ... ]
> @@ -703,8 +1007,8 @@ static int stmfts_probe(struct i2c_client *client)
>  
>  	sdata->input->name = STMFTS_DEV_NAME;
>  	sdata->input->id.bustype = BUS_I2C;
> -	sdata->input->open = stmfts_input_open;
> -	sdata->input->close = stmfts_input_close;
> +	sdata->input->open = sdata->ops->input_open;
> +	sdata->input->close = sdata->ops->input_close;

[Severity: High]
What happens if this driver is instantiated via I2C sysfs board info?

Since dev->of_node would be NULL, of_device_get_match_data() would
return NULL. Would this cause a NULL pointer dereference when
accessing sdata->ops->input_open shortly after?

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