Re: [PATCH v7 1/4] Input: stmfts - wait for controller ready after reset

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unbalanced disable_irq() and regulator_bulk_disable() on consecutive suspend/resume failures.
- [High] System resume fails to restore the touchscreen's active state, leaving the hardware locked in SLEEP_IN mode if it was in use prior to suspend.
- [High] The driver fragments multi-touch frames by calling `input_sync()` after parsing every individual contact event.
- [High] Information leak of uninitialized kernel stack memory via the 'status' sysfs attribute, and functional bug where valid status reads return uninitialized buffer data.
--

commit 1a68dcc09b6690980cb8297285fc328a7de4c568
Author: David Heidelberg <david@ixit.cz>

Input: stmfts - wait for controller ready after reset

After releasing the reset line stmfts_reset() sleeps a fixed 50 ms and
stmfts_power_on() another 50 ms before the first I2C access.  That is
enough for a warm reset, but when both supplies were really cut during
system suspend the controller boots from cold and can need longer.  If
it does, the first read fails, stmfts_resume() returns an error and the
touchscreen is left powered off with its interrupt disabled.

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

[ ... ]

> @@ -598,16 +607,21 @@ static int stmfts_power_on(struct stmfts_data *sdata)
>  	 */
>  	msleep(20);
>  
> -	if (sdata->reset_gpio)
> -		stmfts_reset(sdata);
> +	if (sdata->reset_gpio) {
> +		err = stmfts_reset(sdata);
> +		if (err) {
> +			dev_err(&sdata->client->dev,
> +				"controller not ready after reset: %d\n", err);
> +			goto err_disable_irq;
> +		}
> +	} else {
> +		enable_irq(sdata->client->irq);
> +		msleep(50);
> +	}

[Severity: High]
This is a pre-existing issue, but if system resume fails here in
stmfts_reset(), stmfts_power_on() cleans up and returns an error. On the next
suspend cycle, won't stmfts_suspend() unconditionally call stmfts_power_off()
again?

drivers/input/touchscreen/stmfts.c:stmfts_suspend() {
    ...
	stmfts_power_off(sdata);
    ...
}

And won't this increment the IRQ disable depth to 2 and unbalanced regulator
usage counts, eventually causing the touchscreen to become permanently
unresponsive?


[Severity: High]
This is a pre-existing issue, but stmfts_power_on() ends with a command to put
the controller to sleep:

drivers/input/touchscreen/stmfts.c: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);

	return 0;
    ...
}

If the touchscreen was held open by userspace (sdata->running) prior to
suspend, won't this leave the hardware asleep upon system resume? Since the PM
core does not automatically invoke runtime_resume, does the driver need to
check sdata->running and manually wake the device here?


[Severity: High]
This is a pre-existing issue, but is it intentional to call input_sync() for
every individual contact event here?

drivers/input/touchscreen/stmfts.c:stmfts_report_contact_event() {
    ...
	input_mt_report_slot_state(sdata->input, MT_TOOL_FINGER, true);
	input_report_abs(sdata->input, ABS_MT_POSITION_X, x);
    ...
	input_sync(sdata->input);
}

Since this is called for each finger during multi-finger touch frames, doesn't
this fragment multi-touch frames by emitting a sync after each finger rather
than grouping them at the end of the event parsing loop?


[Severity: High]
This is a pre-existing issue, but is the return value of
i2c_smbus_read_i2c_block_data() handled correctly here?

drivers/input/touchscreen/stmfts.c:stmfts_sysfs_read_status() {
    ...
	err = i2c_smbus_read_i2c_block_data(sdata->client, STMFTS_READ_STATUS,
					    sizeof(status), status);
	if (err)
		return err;

	return sysfs_emit(buf, "%#02x\n", status[0]);
}

Since i2c_smbus_read_i2c_block_data() returns the number of bytes read on
success (e.g. 4), won't "if (err)" evaluate to true and return the byte count
directly, bypassing sysfs_emit()? 

Additionally, if it were to return 0, wouldn't sysfs_emit() leak uninitialized
kernel stack memory (status[0]) to userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-stmfts5-v7-0-4a53381cd038@ixit.cz?part=1