Re: [PATCH 2/3] Input: mms114 - fix endianness portability in I2C packet layout

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Race condition in suspend causes stuck touches.
- [High] Unsafe DMA buffer allocation (stack-allocated memory passed to I2C transfers).
--

commit 2e96699ad380d6d47c8e998bc41268e9b77bac90
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: mms114 - fix endianness portability in I2C packet layout

The driver defines the I2C packet layout using C bitfields in struct
mms114_touch. This is not portable as the layout of bitfields within a
byte is compiler-dependent and varies with endianness. On Big Endian
systems, the fields will be parsed incorrectly.

> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index 23e0283bc6b8b..84afdadb3bccc 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c

[ ... ]

> @@ -325,8 +338,9 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
>  
>  	for (index = 0; index < touch_size; index++) {
>  		t = (struct mms114_touch *)((u8 *)touch + index * event_size);

[Severity: High]
This isn't a bug introduced by this patch, but does passing stack-allocated
buffers to I2C transfer functions risk memory corruption if the I2C adapter
uses DMA?

Since kernel stacks are often allocated in vmalloc space (when
CONFIG_VMAP_STACK is enabled), they cannot be safely mapped for DMA.

In mms114_interrupt(), the touch array is stack-allocated and used as a
receive buffer:

	struct mms114_touch touch[MMS114_MAX_TOUCH];
	// ...
	error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size, touch);

Similarly, __mms114_read_reg() and mms114_write_reg() pass local stack
variables (u8 buf and u8 buf[2]) to I2C transfers.

> +		unsigned int type = FIELD_GET(MMS114_FLAGS_TYPE_MASK, t->flags);
>  
> -		switch (t->type) {
> +		switch (type) {
>  		case MMS114_TYPE_TOUCHSCREEN:
>  			mms114_process_mt(data, t);
>  			break;

[ ... ]

[Severity: High]
This is a pre-existing issue, but could the sequence in mms114_suspend()
have a race condition that results in stuck touches?

In mms114_suspend(), the driver injects artificial release events before
disabling the IRQ:

	/* 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_sync(input_dev);

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

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

If a touch interrupt fires and its threaded handler (mms114_interrupt)
executes after these release events are injected but before mms114_stop()
disables the IRQ, might it report the touch as active again and leave it
stuck indefinitely after suspend?

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