[PATCH 0/2] Input: synaptics - enable SMBus for ThinkPad P14s Gen 2i (LEN2072)

[PATCH 0/2] Input: synaptics - enable SMBus for ThinkPad P14s Gen 2i (LEN2072)

[Dmitry Mikushin]

The ThinkPad P14s Gen 2i touchpad (Synaptics TM3471-030, LEN2072) is
not on the SMBus passlist and runs in PS/2 mode. Forcing RMI4 alone
does not help: the F12 attention handler reads the whole 79-byte data
packet over SMBus on every frame, which caps the report rate at about
43 Hz and loses fast swipes.

Patch 1 makes F12 read only the object data it consumes, raising the
report rate to about 65 Hz on this machine. Patch 2 then adds LEN2072
to smbus_pnp_ids. Patch 2 should not be applied without patch 1.

Both were tested on a ThinkPad P14s Gen 2i (20VX) on 7.2.9, including
s2idle suspend/resume and the TrackPoint on the RMI4 pass-through port.

Dmitry Mikushin (2):
  Input: synaptics-rmi4 - f12: read only the object data on attention
  Input: synaptics - add LEN2072 to smbus_pnp_ids for ThinkPad P14s Gen
    2i

 drivers/input/mouse/synaptics.c |  1 +
 drivers/input/rmi4/rmi_f12.c    | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

-- 
2.56.0

[PATCH 1/2] Input: synaptics-rmi4 - f12: read only the object data on attention

[Dmitry Mikushin]

When F12 data is not supplied by the transport as attention data, the
attention handler reads the whole F12 data packet on every interrupt,
although only Data1 (the object reports) is consumed. The remaining
data registers are never used.

On SMBus this transfer is the dominant per-frame cost. On a ThinkPad
P14s Gen 2i with a Synaptics TM3471-030 (i801 SMBus) the F12 data
packet is 79 bytes, of which Data1 is the first 32. Reading 79 bytes
takes about 17.4 ms and reading 32 bytes about 7.3 ms. With the full
read the touchpad reports at about 43 Hz (median 23.0 ms between
frames), and fast swipes yield only a handful of frames, so they are
mostly lost to pointer acceleration and smoothing.
Reading only Data1 raises the report rate to about 65 Hz (median
15.3 ms between frames), measured with a finger on the same machine.

Compute the size of the leading registers up to and including Data1 at
probe time and read only that much. The attention data path (hid-rmi)
is unchanged.

Signed-off-by: Dmitry Mikushin <dmitry@kernelgen.org>
---
 drivers/input/rmi4/rmi_f12.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
index 841884d967..ad210464b2 100644
--- a/drivers/input/rmi4/rmi_f12.c
+++ b/drivers/input/rmi4/rmi_f12.c
@@ -58,6 +58,8 @@ struct f12_data {
 	const struct rmi_register_desc_item *data15;
 	u16 data15_offset;
 
+	u32 read_size;
+
 	unsigned long irq_mask[];
 };
 
@@ -269,8 +271,9 @@ static irqreturn_t rmi_f12_attention(int irq, void *ctx)
 		drvdata->attn_data.data += valid_bytes;
 		drvdata->attn_data.size -= valid_bytes;
 	} else {
+		valid_bytes = f12->read_size;
 		retval = rmi_read_block(rmi_dev, f12->data_addr,
-					sensor->data_pkt, sensor->pkt_size);
+					sensor->data_pkt, valid_bytes);
 		if (retval < 0) {
 			dev_err(&fn->dev, "Failed to read object data. Code: %d.\n",
 				retval);
@@ -529,6 +532,17 @@ static int rmi_f12_probe(struct rmi_function *fn)
 		data_offset += item->reg_size;
 	}
 
+	/*
+	 * Only Data1 (object reports) is consumed when the data registers
+	 * are read from the device, so do not read the remaining registers
+	 * on every attention. On SMBus this transfer dominates the per-frame
+	 * cost and therefore limits the report rate.
+	 */
+	f12->read_size = sensor->pkt_size;
+	if (f12->data1)
+		f12->read_size = min_t(u32, sensor->pkt_size,
+				       f12->data1_offset + f12->data1->reg_size);
+
 	/* allocate the in-kernel tracking buffers */
 	sensor->tracking_pos = devm_kcalloc(&fn->dev, sensor->nbr_fingers,
 					    sizeof(*sensor->tracking_pos),
-- 
2.56.0

[PATCH 2/2] Input: synaptics - add LEN2072 to smbus_pnp_ids for ThinkPad P14s Gen 2i

[Dmitry Mikushin]

The ThinkPad P14s Gen 2i (20VX) has a Synaptics TM3471-030 touchpad
(PNP ID LEN2072, firmware 3324237) that supports SMBus/RMI4, but it is
not in smbus_pnp_ids[], so psmouse drives it in legacy PS/2 mode. There
it is limited to two tracked contacts and shares the i8042 channel with
the TrackPoint.

With psmouse.synaptics_intertouch=1 the touchpad binds to rmi4 at SMBus
address 0x2c, reports four contacts, and the TrackPoint keeps working
through the RMI4 pass-through port. Suspend/resume (s2idle) works.

Add LEN2072 to the smbus_pnp_ids[] passlist.

Signed-off-by: Dmitry Mikushin <dmitry@kernelgen.org>
---
 drivers/input/mouse/synaptics.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 1ed9261bff..b007e642ea 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -194,6 +194,7 @@ static const char * const smbus_pnp_ids[] = {
 	"LEN2058", /* E490 */
 	"LEN205b", /* T490 */
 	"LEN2068", /* T14 Gen 1 */
+	"LEN2072", /* P14s Gen 2i */
 	"SYN1221", /* TUXEDO InfinityBook Pro 14 v5 */
 	"SYN3003", /* HP EliteBook 850 G1 */
 	"SYN3015", /* HP EliteBook 840 G2 */
-- 
2.56.0

Re: [PATCH 1/2] Input: synaptics-rmi4 - f12: read only the object data on attention

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Input: rmi4: unadjusted valid_bytes in rmi_f12_attention() reads past data1
--

commit c4b3a9b641a3a2e611195ada64fab67096a67d47
Author: Dmitry Mikushin <dmitry@kernelgen.org>

Input: synaptics-rmi4 - f12: read only the object data on attention

This commit optimizes F12 attention handling when attention data is not
supplied by the transport by computing the size of the leading registers
up to and including Data1 at probe time and reading only that prefix
instead of the full F12 data packet.

> diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
> index 841884d967a3d..ad210464b2f34 100644
> --- a/drivers/input/rmi4/rmi_f12.c
> +++ b/drivers/input/rmi4/rmi_f12.c
> @@ -58,6 +58,8 @@ struct f12_data {
>  	const struct rmi_register_desc_item *data15;
>  	u16 data15_offset;
>  
> +	u32 read_size;
> +
>  	unsigned long irq_mask[];
>  };
>  
> @@ -269,8 +271,9 @@ static irqreturn_t rmi_f12_attention(int irq, void *ctx)
>  		drvdata->attn_data.data += valid_bytes;
>  		drvdata->attn_data.size -= valid_bytes;
>  	} else {
> +		valid_bytes = f12->read_size;
>  		retval = rmi_read_block(rmi_dev, f12->data_addr,
> -					sensor->data_pkt, sensor->pkt_size);
> +					sensor->data_pkt, valid_bytes);
>  		if (retval < 0) {
>  			dev_err(&fn->dev, "Failed to read object data. Code: %d.\n",
>  				retval);
> @@ -529,6 +532,17 @@ static int rmi_f12_probe(struct rmi_function *fn)
>  		data_offset += item->reg_size;
>  	}
>  
> +	/*
> +	 * Only Data1 (object reports) is consumed when the data registers
> +	 * are read from the device, so do not read the remaining registers
> +	 * on every attention. On SMBus this transfer dominates the per-frame
> +	 * cost and therefore limits the report rate.
> +	 */
> +	f12->read_size = sensor->pkt_size;
> +	if (f12->data1)
> +		f12->read_size = min_t(u32, sensor->pkt_size,
> +				       f12->data1_offset + f12->data1->reg_size);

[Severity: High]
Can this read past the end of Data1 into uninitialized memory in
sensor->data_pkt[] or out-of-bounds past sensor->data_pkt[] when Data0 is
present in the F12 data register descriptor (so f12->data1_offset > 0)?

In rmi_f12_probe(), sensor->data_pkt is allocated with devm_kmalloc()
without zero-initialization:

drivers/input/rmi4/rmi_f12.c:rmi_f12_probe() {
	...
	sensor->data_pkt = devm_kmalloc(&fn->dev, sensor->pkt_size, GFP_KERNEL);
	...
}

In rmi_f12_attention(), rmi_read_block() now populates only the first
valid_bytes (f12->read_size = f12->data1_offset + f12->data1->reg_size)
bytes of sensor->data_pkt, leaving any trailing bytes of sensor->data_pkt
uninitialized, and then passes &sensor->data_pkt[f12->data1_offset] with
the unadjusted valid_bytes to rmi_f12_process_objects():

drivers/input/rmi4/rmi_f12.c:rmi_f12_attention() {
	...
	if (f12->data1)
		rmi_f12_process_objects(f12, &sensor->data_pkt[f12->data1_offset],
					valid_bytes);
	...
}

Inside rmi_f12_process_objects(), size is treated as the number of valid
bytes starting at data1 (&sensor->data_pkt[f12->data1_offset]):

drivers/input/rmi4/rmi_f12.c:rmi_f12_process_objects() {
	struct rmi_2d_sensor *sensor = &f12->sensor;
	u32 objects = min(f12->data1->num_subpackets, size / F12_DATA1_BYTES_PER_OBJ);
	...
}

Because rmi_parse_register_desc_item() parses item->reg_size and
item->num_subpackets independently, if f12->data1->reg_size is smaller
than f12->data1->num_subpackets * F12_DATA1_BYTES_PER_OBJ (or if
valid_bytes on the attn_data path is smaller than f12->data1_offset +
f12->data1->num_subpackets * F12_DATA1_BYTES_PER_OBJ),
rmi_f12_process_objects() can read up to f12->data1_offset bytes past
sensor->data_pkt[valid_bytes - 1], reporting uninitialized kernel heap
memory to userspace via input events or reading past the end of
sensor->data_pkt[].

Should rmi_f12_attention() check that valid_bytes > f12->data1_offset and
pass valid_bytes - f12->data1_offset to rmi_f12_process_objects()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026093118.2312302-1-dmitry@kernelgen.org?part=1