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

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 [off-list ref]

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.

[ ... ]

[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