Re: [PATCH 4/6] Input: mms114 - replace udelay with usleep_range

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Race condition in `mms114_suspend` releases touches before disabling the hardware IRQ, potentially leaving touches stuck active.
- [High] Failure in `mms114_resume()` leads to unbalanced `disable_irq()` and `regulator_disable()`, causing kernel warnings and state corruption.
- [High] Copy-paste error in `mms114_setup_regs` writes the X resolution to the Y resolution register, breaking touchscreen coordinates mapping.
- [High] Hardware data bytes are read directly into a struct containing C bitfields, leading to endian-dependent memory layout corruption.
--

commit 4b0151507f5071f8f96d0af3ccffe9ae30a17c92
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: mms114 - replace udelay with usleep_range

The driver currently uses udelay(MMS114_I2C_DELAY) (50us) to ensure a
mandatory delay between I2C transfers in __mms114_read_reg() and
mms114_write_reg().

Both functions invoke underlying I2C core operations (i2c_transfer,
i2c_master_send) which acquire mutexes and sleep. Furthermore, the
interrupt handler mms114_interrupt() is registered as a threaded IRQ
handler. Since the entire execution path is fully sleepable,
busy-waiting with udelay() for 50us unnecessarily wastes CPU cycles.

Replace udelay() with usleep_range() to allow the CPU to enter low-power
states or execute other tasks during the delay.

> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index c2e006ac11960..c59aec8f2febe 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -116,7 +116,7 @@ static int __mms114_read_reg(struct mms114_data *data, u8 reg,
>  			"%s: i2c transfer failed (%d)\n", __func__, error);
>  		return error < 0 ? error : -EIO;
>  	}
> -	udelay(MMS114_I2C_DELAY);
> +	usleep_range(MMS114_I2C_DELAY, MMS114_I2C_DELAY + 50);
>  
>  	return 0;
>  }

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition in the
suspend path.

drivers/input/touchscreen/mms114.c:mms114_suspend() {
...
	/* Release all touch */
	for (id = 0; id < MMS114_MAX_TOUCH; id++) {
		input_mt_slot(input_dev, id);
		input_mt_report_slot_inactive(input_dev);
	}

	input_mt_report_pointer_emulation(input_dev, true);
	input_sync(input_dev);

	guard(mutex)(&input_dev->mutex);

	if (input_device_enabled(input_dev))
		mms114_stop(data);
...
}

Because mms114_stop() disables the hardware IRQ after input_sync() has reported
the release events, is it possible for a hardware interrupt to fire between
the synthetic release and the actual IRQ disable?

Since the interrupt handler runs as a concurrent kernel thread and does not
share a lock with this early part of the suspend path, the touch could be
reported as active again right before the system sleeps. Would this leave the
touch stuck active indefinitely upon resume?

[Severity: High]
This isn't a bug introduced by this patch, but the resume error handling
might lead to unbalanced resource disabling.

drivers/input/touchscreen/mms114.c:mms114_resume() {
...
	if (input_device_enabled(input_dev)) {
		error = mms114_start(data);
		if (error)
			return error;
	}
...
}

If mms114_start() fails (e.g. due to I2C communication failure), it disables
the regulators and returns an error without enabling the IRQ. However, the
input device remains logically open.

If userspace subsequently closes the device, or another suspend cycle occurs,
mms114_stop() will be called unconditionally:

drivers/input/touchscreen/mms114.c:mms114_stop() {
...
	disable_irq(client->irq);

	error = regulator_disable(data->io_reg);
...
}

Will this unconditionally call disable_irq() and regulator_disable() on
resources that are already disabled, triggering warnings and corrupting
regulator reference counts?

[Severity: High]
This is a pre-existing issue, but I spotted a potential copy-paste error
during resolution configuration.

drivers/input/touchscreen/mms114.c:mms114_setup_regs() {
...
	val = props->max_x & 0xff;
	error = mms114_write_reg(data, MMS114_X_RESOLUTION, val);
	if (error < 0)
		return error;

	val = props->max_x & 0xff;
	error = mms114_write_reg(data, MMS114_Y_RESOLUTION, val);
...
}

Should the second register write use props->max_y instead of props->max_x?

Writing the X resolution to MMS114_Y_RESOLUTION might truncate the hardware's
internal Y resolution, breaking touch tracking and coordinate mapping on the
Y axis for screens where the dimensions differ.

[Severity: High]
This isn't a bug introduced by this patch, but there might be an endianness
problem with the touch data structure.

drivers/input/touchscreen/mms114.c:
...
struct mms114_touch {
	u8 id:4, reserved_bit4:1, type:2, pressed:1;
	u8 x_hi:4, y_hi:4;
	u8 x_lo;
	u8 y_lo;
	u8 width;
	u8 strength;
	u8 reserved[2];
} __packed;
...

This struct is used to map raw byte stream data from the I2C hardware. Since
the memory layout of C bitfields is compiler and endian-dependent, will these
bitfields be reversed on big-endian architectures?

This could result in incorrect parsing of touch coordinates, IDs, and event
types.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616050912.1531241-1-dmitry.torokhov@gmail.com?part=4