[PATCH] Input: synaptics - support the touchpad LED on RMI function $30

[PATCH] Input: synaptics - support the touchpad LED on RMI function $30

[ahmad baghereslami]

Some HP laptops have an LED in the top left corner of the Synaptics
touchpad that HP's Windows driver lights while the touchpad is disabled
(double tap on that corner). On the HP ZBook 17 G3 (SYN3040, TM2706-002)
the LED is not driven through the PS/2 deluxe LED command: the Windows
driver (SynTP.sys 19.0.19.68) switches the touchpad to RMI register
access over PS/2 with mode byte 0x7f, looks up function $30 (GPIO/LED)
in the page description table and sets the LED bit in its control
register:

  register write:  SetScale 2:1, SetRate <reg>, SetRate <value>
  register read:   SetScale 2:1, SetRate <reg>, GetInfo (first byte)

For touchpads in a new rmi_led_pnp_ids list, look up the F30 LED this
way when the touchpad is set up and register a
synaptics::touchpad_disabled LED class device. Setting it deactivates
the touchpad, writes the F30 registers and restores the mode byte, which
leaves RMI access. The LED is restored after reconnect.

On this touchpad F30 is on page 1 with control base 0x55 and four
GPIO/LED pins; the LED is bit 3 of control register 0x59 and its
brightness register is 0x5f.

Tested on an HP ZBook 17 G3 with Ubuntu's 7.0.0 kernel: the LED follows
writes to its brightness file, the touchpad and the pointing stick keep
working, and a userspace helper that turns the touchpad off on a double
tap in the corner keeps the LED in step with GNOME's touchpad setting.

Reading HP's Windows driver (Ghidra) and writing this patch were done
with the help of an AI coding assistant (Claude); I tested the LED and
the touchpad on the machine.

Signed-off-by: ahmad baghereslami <ahmad.b1995@gmail.com>
---
 drivers/input/mouse/synaptics.c | 241 ++++++++++++++++++++++++++++++++
 drivers/input/mouse/synaptics.h |  13 ++
 2 files changed, 254 insertions(+)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index b007e642e..bf1c6c738 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -589,6 +589,236 @@ static int synaptics_set_advanced_gesture_mode(struct psmouse *psmouse)
 	return 0;
 }
 
+/*
+ * Some HP touchpads have an LED in the top left corner that HP's Windows
+ * driver lights while the touchpad is disabled. It is wired to RMI function
+ * $30 (GPIO/LED), which these touchpads also expose over PS/2: mode byte 0x7f
+ * switches them to RMI register access, where SetScale 2:1 followed by two
+ * SetRate commands writes a register, and SetScale 2:1, SetRate and GetInfo
+ * read one. Setting the normal mode byte again leaves RMI access.
+ */
+static const char * const rmi_led_pnp_ids[] = {
+	"SYN3040", /* HP ZBook 17 G3 */
+	NULL
+};
+
+#define SYN_RMI_MODE			0x7f
+#define SYN_RMI_PAGE_SELECT		0xff
+#define SYN_RMI_PDT_START		0xee
+#define SYN_RMI_PDT_END			0x0b
+#define SYN_RMI_PDT_ENTRY_SIZE		6
+#define SYN_RMI_NUM_PAGES		4
+#define SYN_RMI_F30			0x30
+#define SYN_RMI_F30_HAS_LED		BIT(2)
+#define SYN_RMI_F30_HAS_GPIO		BIT(3)
+#define SYN_RMI_LED_BRIGHTNESS		0x0b	/* as set by HP's Windows driver */
+
+static int synaptics_rmi_write(struct psmouse *psmouse, u8 reg, u8 val)
+{
+	struct ps2dev *ps2dev = &psmouse->ps2dev;
+	int error;
+
+	error = ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE21);
+	if (error)
+		return error;
+
+	error = ps2_command(ps2dev, &reg, PSMOUSE_CMD_SETRATE);
+	if (error)
+		return error;
+
+	return ps2_command(ps2dev, &val, PSMOUSE_CMD_SETRATE);
+}
+
+static int synaptics_rmi_read(struct psmouse *psmouse, u8 reg, u8 *val)
+{
+	struct ps2dev *ps2dev = &psmouse->ps2dev;
+	u8 param[3];
+	int error;
+
+	error = ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE21);
+	if (error)
+		return error;
+
+	error = ps2_command(ps2dev, &reg, PSMOUSE_CMD_SETRATE);
+	if (error)
+		return error;
+
+	error = ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO);
+	if (error)
+		return error;
+
+	*val = param[0];
+	return 0;
+}
+
+/* Finds the LED of RMI function $30. Leaves the touchpad in RMI mode. */
+static int synaptics_rmi_led_probe(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+	u8 fn, query, ctrl, caps, count, gpios;
+	unsigned int page, zeros, i;
+	int addr, error;
+
+	error = synaptics_mode_cmd(psmouse, SYN_RMI_MODE);
+	if (error)
+		return error;
+
+	for (page = 0; page < SYN_RMI_NUM_PAGES; page++) {
+		error = synaptics_rmi_write(psmouse, SYN_RMI_PAGE_SELECT, page);
+		if (error)
+			return error;
+
+		for (addr = SYN_RMI_PDT_START; addr >= SYN_RMI_PDT_END;
+		     addr -= SYN_RMI_PDT_ENTRY_SIZE) {
+			error = synaptics_rmi_read(psmouse, addr, &fn);
+			if (error)
+				return error;
+			if (!fn)
+				break;
+			if (fn != SYN_RMI_F30)
+				continue;
+
+			error = synaptics_rmi_read(psmouse, addr - 5, &query);
+			if (!error)
+				error = synaptics_rmi_read(psmouse, addr - 3, &ctrl);
+			if (!error)
+				error = synaptics_rmi_read(psmouse, query, &caps);
+			if (!error)
+				error = synaptics_rmi_read(psmouse, query + 1, &count);
+			if (error)
+				return error;
+
+			if (!(caps & SYN_RMI_F30_HAS_LED))
+				return -ENODEV;
+
+			priv->rmi_led_page = page;
+			if (!(caps & SYN_RMI_F30_HAS_GPIO)) {
+				priv->rmi_led_mask = BIT(0);
+				priv->rmi_led_ctrl = ctrl + 1;
+				priv->rmi_led_brightness = ctrl + 4;
+				return 0;
+			}
+
+			/* GPIO/LED select: the LED is the first set bit */
+			error = synaptics_rmi_read(psmouse, ctrl, &gpios);
+			if (error)
+				return error;
+
+			priv->rmi_led_mask = 0;
+			for (i = 0, zeros = 0; i < min(count, 8); i++) {
+				if (!(gpios & BIT(i)))
+					zeros++;
+				else if (!priv->rmi_led_mask)
+					priv->rmi_led_mask = BIT(i);
+			}
+			if (!priv->rmi_led_mask)
+				return -ENODEV;
+
+			priv->rmi_led_ctrl = ctrl + 4;
+			priv->rmi_led_brightness = ctrl + 7 + zeros;
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
+/* Sets the LED. Leaves the touchpad in RMI mode. */
+static int __synaptics_rmi_led_set(struct psmouse *psmouse, bool on)
+{
+	struct synaptics_data *priv = psmouse->private;
+	u8 val;
+	int error;
+
+	error = synaptics_mode_cmd(psmouse, SYN_RMI_MODE);
+	if (error)
+		return error;
+
+	error = synaptics_rmi_write(psmouse, SYN_RMI_PAGE_SELECT, priv->rmi_led_page);
+	if (error)
+		return error;
+
+	if (on) {
+		error = synaptics_rmi_write(psmouse, priv->rmi_led_brightness,
+					    SYN_RMI_LED_BRIGHTNESS);
+		if (error)
+			return error;
+	}
+
+	error = synaptics_rmi_read(psmouse, priv->rmi_led_ctrl, &val);
+	if (error)
+		return error;
+
+	val = on ? val | priv->rmi_led_mask : val & ~priv->rmi_led_mask;
+	return synaptics_rmi_write(psmouse, priv->rmi_led_ctrl, val);
+}
+
+static int synaptics_set_mode(struct psmouse *psmouse);
+
+static int synaptics_rmi_led_set(struct led_classdev *led,
+				 enum led_brightness value)
+{
+	struct synaptics_data *priv = container_of(led, struct synaptics_data, rmi_led);
+	struct psmouse *psmouse = priv->psmouse;
+	int error, ret;
+
+	guard(mutex)(&priv->rmi_led_lock);
+
+	if (psmouse->state != PSMOUSE_ACTIVATED)
+		return -EBUSY;
+
+	error = psmouse_deactivate(psmouse);
+	if (error)
+		return error;
+
+	ret = __synaptics_rmi_led_set(psmouse, value != LED_OFF);
+
+	/* Setting the mode byte again leaves RMI access */
+	error = synaptics_set_mode(psmouse);
+	if (!ret)
+		ret = error;
+
+	error = psmouse_activate(psmouse);
+	return ret ?: error;
+}
+
+static void synaptics_rmi_led_init(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+
+	if (!IS_REACHABLE(CONFIG_LEDS_CLASS) ||
+	    !psmouse_matches_pnp_id(psmouse, rmi_led_pnp_ids))
+		return;
+
+	if (synaptics_rmi_led_probe(psmouse))
+		return;
+
+	psmouse_info(psmouse, "touchpad LED on RMI F30 (page %u, ctrl %#x, mask %#x)\n",
+		     priv->rmi_led_page, priv->rmi_led_ctrl, priv->rmi_led_mask);
+	__synaptics_rmi_led_set(psmouse, false);
+	priv->has_rmi_led = true;
+}
+
+static void synaptics_rmi_led_register(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+	int error;
+
+	if (!priv->has_rmi_led)
+		return;
+
+	mutex_init(&priv->rmi_led_lock);
+	priv->psmouse = psmouse;
+	priv->rmi_led.name = "synaptics::touchpad_disabled";
+	priv->rmi_led.max_brightness = 1;
+	priv->rmi_led.brightness_set_blocking = synaptics_rmi_led_set;
+	error = led_classdev_register(&psmouse->ps2dev.serio->dev, &priv->rmi_led);
+	if (error) {
+		psmouse_warn(psmouse, "failed to register touchpad LED: %d\n", error);
+		priv->has_rmi_led = false;
+	}
+}
+
 static int synaptics_set_mode(struct psmouse *psmouse)
 {
 	struct synaptics_data *priv = psmouse->private;
@@ -1444,6 +1674,9 @@ static void synaptics_disconnect(struct psmouse *psmouse)
 {
 	struct synaptics_data *priv = psmouse->private;
 
+	if (priv->has_rmi_led)
+		led_classdev_unregister(&priv->rmi_led);
+
 	/*
 	 * We might have left a breadcrumb when trying to
 	 * set up SMbus companion.
@@ -1496,6 +1729,10 @@ static int synaptics_reconnect(struct psmouse *psmouse)
 		return error;
 	}
 
+	/* The reset turned the LED off; the mode byte below leaves RMI access. */
+	if (priv->has_rmi_led && priv->rmi_led.brightness)
+		__synaptics_rmi_led_set(psmouse, true);
+
 	error = synaptics_set_mode(psmouse);
 	if (error) {
 		psmouse_err(psmouse, "Unable to initialize device.\n");
@@ -1616,6 +1853,8 @@ static int synaptics_init_ps2(struct psmouse *psmouse,
 	 */
 	priv->is_forcepad = psmouse_matches_pnp_id(psmouse, forcepad_pnp_ids);
 
+	synaptics_rmi_led_init(psmouse);
+
 	err = synaptics_set_mode(psmouse);
 	if (err) {
 		psmouse_err(psmouse, "Unable to initialize device.\n");
@@ -1693,6 +1932,8 @@ static int synaptics_init_ps2(struct psmouse *psmouse,
 		}
 	}
 
+	synaptics_rmi_led_register(psmouse);
+
 	return 0;
 
  init_fail:
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 3853165b6..1d36a207e 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -6,6 +6,9 @@
 #ifndef _SYNAPTICS_H
 #define _SYNAPTICS_H
 
+#include <linux/leds.h>
+#include <linux/mutex.h>
+
 /* synaptics queries */
 #define SYN_QUE_IDENTIFY		0x00
 #define SYN_QUE_MODES			0x01
@@ -202,6 +205,16 @@ struct synaptics_data {
 	bool					press;
 	bool					report_press;
 	bool					is_forcepad;
+
+	/* Touchpad LED on RMI function $30, reached over PS/2 */
+	struct psmouse				*psmouse;
+	struct led_classdev			rmi_led;
+	struct mutex				rmi_led_lock;
+	bool					has_rmi_led;
+	u8					rmi_led_page;
+	u8					rmi_led_ctrl;
+	u8					rmi_led_mask;
+	u8					rmi_led_brightness;
 };
 
 void synaptics_module_init(void);
-- 
2.53.0

Re: [PATCH] Input: synaptics - support the touchpad LED on RMI function $30

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Input: synaptics - missing psmouse_mutex locking in synaptics_rmi_led_set()
- [Medium] Input: synaptics - synaptics_rmi_led_set() fails with -EBUSY on disconnect
- [Medium] Input: synaptics - synaptics_rmi_led_set() clears SYN_BIT_FOUR_BYTE_CLIENT
- [Medium] Input: synaptics - unmasked F30 pin count in synaptics_rmi_led_probe()
- [Low] Input: synaptics - link failure when CONFIG_LEDS_CLASS is not reachable
--

commit ba11d4207158064745617f3cd241e591354af8aa
Author: ahmad baghereslami <ahmad.b1995@gmail.com>

Input: synaptics - support the touchpad LED on RMI function $30

This commit adds support for the touchpad disabled LED wired to RMI function
$30 over PS/2 on HP laptops matching rmi_led_pnp_ids, probing the F30
registers during setup and registering a synaptics::touchpad_disabled LED
class device.

> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index b007e642ea765..bf1c6c738295c 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c

[ ... ]

> +/* Finds the LED of RMI function $30. Leaves the touchpad in RMI mode. */
> +static int synaptics_rmi_led_probe(struct psmouse *psmouse)
> +{
> +	struct synaptics_data *priv = psmouse->private;
> +	u8 fn, query, ctrl, caps, count, gpios;
> +	unsigned int page, zeros, i;
> +	int addr, error;
> +
> +	error = synaptics_mode_cmd(psmouse, SYN_RMI_MODE);
> +	if (error)
> +		return error;
> +
> +	for (page = 0; page < SYN_RMI_NUM_PAGES; page++) {
> +		error = synaptics_rmi_write(psmouse, SYN_RMI_PAGE_SELECT, page);
> +		if (error)
> +			return error;
> +
> +		for (addr = SYN_RMI_PDT_START; addr >= SYN_RMI_PDT_END;
> +		     addr -= SYN_RMI_PDT_ENTRY_SIZE) {
> +			error = synaptics_rmi_read(psmouse, addr, &fn);
> +			if (error)
> +				return error;
> +			if (!fn)
> +				break;
> +			if (fn != SYN_RMI_F30)
> +				continue;
> +
> +			error = synaptics_rmi_read(psmouse, addr - 5, &query);
> +			if (!error)
> +				error = synaptics_rmi_read(psmouse, addr - 3, &ctrl);
> +			if (!error)
> +				error = synaptics_rmi_read(psmouse, query, &caps);
> +			if (!error)
> +				error = synaptics_rmi_read(psmouse, query + 1, &count);
> +			if (error)
> +				return error;
> +
> +			if (!(caps & SYN_RMI_F30_HAS_LED))
> +				return -ENODEV;
> +
> +			priv->rmi_led_page = page;
> +			if (!(caps & SYN_RMI_F30_HAS_GPIO)) {
> +				priv->rmi_led_mask = BIT(0);
> +				priv->rmi_led_ctrl = ctrl + 1;
> +				priv->rmi_led_brightness = ctrl + 4;
> +				return 0;
> +			}
> +
> +			/* GPIO/LED select: the LED is the first set bit */
> +			error = synaptics_rmi_read(psmouse, ctrl, &gpios);
> +			if (error)
> +				return error;
> +
> +			priv->rmi_led_mask = 0;
> +			for (i = 0, zeros = 0; i < min(count, 8); i++) {
> +				if (!(gpios & BIT(i)))
> +					zeros++;
> +				else if (!priv->rmi_led_mask)
> +					priv->rmi_led_mask = BIT(i);
> +			}
> +			if (!priv->rmi_led_mask)
> +				return -ENODEV;
> +
> +			priv->rmi_led_ctrl = ctrl + 4;
> +			priv->rmi_led_brightness = ctrl + 7 + zeros;

[Severity: Medium]
In RMI4 Function $30, Query 1 encodes the GPIO/LED pin count in bits 4:0,
which rmi_f30_initialize() in drivers/input/rmi4/rmi_f30.c extracts using
RMI_F30_GPIO_LED_COUNT (0x1f):

    f30->gpioled_count = buf[1] & RMI_F30_GPIO_LED_COUNT;

Here in synaptics_rmi_led_probe(), count is read from query + 1 without
masking with 0x1f, and the loop continues incrementing zeros for clear bits
in gpios even after latching the first set bit into priv->rmi_led_mask.

If any of bits 7:5 in Query 1 are set when the 5-bit pin count is less than
8, or if a GPIO pin is present at a bit index above the first LED bit, could
zeros include extra clear bits and offset priv->rmi_led_brightness to the
wrong F30 control register?

[ ... ]

> +static int synaptics_rmi_led_set(struct led_classdev *led,
> +				 enum led_brightness value)
> +{
> +	struct synaptics_data *priv = container_of(led, struct synaptics_data, rmi_led);
> +	struct psmouse *psmouse = priv->psmouse;
> +	int error, ret;
> +
> +	guard(mutex)(&priv->rmi_led_lock);
> +
> +	if (psmouse->state != PSMOUSE_ACTIVATED)
> +		return -EBUSY;
> +
> +	error = psmouse_deactivate(psmouse);
> +	if (error)
> +		return error;
> +
> +	ret = __synaptics_rmi_led_set(psmouse, value != LED_OFF);

[Severity: High]
When brightness is updated via sysfs, the LED core queues set_brightness_work
to invoke synaptics_rmi_led_set() asynchronously. In psmouse-base.c,
psmouse_mutex serializes operations that inspect or change psmouse->state,
modify priv->mode, or run multi-command sequences on the PS/2 device and its
pass-through port, such as psmouse_reconnect(), psmouse_resync(),
psmouse_disconnect(), psmouse_cleanup(), and psmouse_attr_set_helper().

Here, synaptics_rmi_led_set() only takes priv->rmi_led_lock, which is not
acquired anywhere else (including in synaptics_reconnect() when reading
priv->rmi_led.brightness and calling __synaptics_rmi_led_set()).

Without holding psmouse_mutex, can synaptics_rmi_led_set() race with a
concurrent reconnect, resync, sysfs attribute write, or pass-through port
operation after checking psmouse->state != PSMOUSE_ACTIVATED, interleaving
PS/2 commands while the touchpad is in SYN_RMI_MODE (0x7f) and prematurely
setting psmouse->state back to PSMOUSE_ACTIVATED?

> +
> +	/* Setting the mode byte again leaves RMI access */
> +	error = synaptics_set_mode(psmouse);

[Severity: Medium]
When a 4-byte protocol child device is attached to the pass-through port,
synaptics_pt_activate() sets SYN_BIT_FOUR_BYTE_CLIENT in priv->mode:

    if (child) {
        if (child->pktsize == 4)
            priv->mode |= SYN_BIT_FOUR_BYTE_CLIENT;
        else
            priv->mode &= ~SYN_BIT_FOUR_BYTE_CLIENT;

        if (synaptics_mode_cmd(psmouse, priv->mode))
            psmouse_warn(psmouse,
                         "failed to switch guest protocol\n");
    }

Unlike synaptics_set_rate() and synaptics_set_disable_gesture(), which update
their respective bit in priv->mode and call synaptics_mode_cmd(psmouse,
priv->mode), calling synaptics_set_mode() here resets priv->mode = 0 and
rebuilds it without SYN_BIT_FOUR_BYTE_CLIENT.

Will toggling the LED at runtime clear SYN_BIT_FOUR_BYTE_CLIENT in both
priv->mode and the hardware mode register when a 4-byte pass-through child is
active?

> +	if (!ret)
> +		ret = error;
> +
> +	error = psmouse_activate(psmouse);
> +	return ret ?: error;
> +}
> +
> +static void synaptics_rmi_led_init(struct psmouse *psmouse)
> +{
> +	struct synaptics_data *priv = psmouse->private;
> +
> +	if (!IS_REACHABLE(CONFIG_LEDS_CLASS) ||
> +	    !psmouse_matches_pnp_id(psmouse, rmi_led_pnp_ids))
> +		return;
> +
> +	if (synaptics_rmi_led_probe(psmouse))
> +		return;
> +
> +	psmouse_info(psmouse, "touchpad LED on RMI F30 (page %u, ctrl %#x, mask %#x)\n",
> +		     priv->rmi_led_page, priv->rmi_led_ctrl, priv->rmi_led_mask);
> +	__synaptics_rmi_led_set(psmouse, false);
> +	priv->has_rmi_led = true;
> +}
> +
> +static void synaptics_rmi_led_register(struct psmouse *psmouse)
> +{
> +	struct synaptics_data *priv = psmouse->private;
> +	int error;
> +
> +	if (!priv->has_rmi_led)
> +		return;
> +
> +	mutex_init(&priv->rmi_led_lock);
> +	priv->psmouse = psmouse;
> +	priv->rmi_led.name = "synaptics::touchpad_disabled";
> +	priv->rmi_led.max_brightness = 1;
> +	priv->rmi_led.brightness_set_blocking = synaptics_rmi_led_set;
> +	error = led_classdev_register(&psmouse->ps2dev.serio->dev, &priv->rmi_led);
> +	if (error) {
> +		psmouse_warn(psmouse, "failed to register touchpad LED: %d\n", error);
> +		priv->has_rmi_led = false;
> +	}
> +}

[Severity: Low]
In drivers/input/mouse/Kconfig, MOUSE_PS2 and MOUSE_PS2_SYNAPTICS do not
depend on or select LEDS_CLASS, and include/linux/leds.h declares
led_classdev_register_ext() and led_classdev_unregister() without inline
stubs when CONFIG_LEDS_CLASS is disabled.

While synaptics_rmi_led_init() checks IS_REACHABLE(CONFIG_LEDS_CLASS),
synaptics_rmi_led_register() and synaptics_disconnect() only guard the calls
to led_classdev_register() and led_classdev_unregister() with the runtime
field priv->has_rmi_led. Because priv escapes through psmouse->private across
external calls before synaptics_rmi_led_register() and synaptics_disconnect()
is an address-taken callback, the compiler cannot eliminate those calls.

Will building with CONFIG_MOUSE_PS2=y and CONFIG_LEDS_CLASS=n (or
CONFIG_LEDS_CLASS=m) fail at link time with undefined references to
led_classdev_register_ext() and led_classdev_unregister()?

[ ... ]

> @@ -1444,6 +1674,9 @@ static void synaptics_disconnect(struct psmouse *psmouse)
>  {
>  	struct synaptics_data *priv = psmouse->private;
>  
> +	if (priv->has_rmi_led)
> +		led_classdev_unregister(&priv->rmi_led);

[Severity: Medium]
Every caller of psmouse->disconnect() in drivers/input/mouse/psmouse-base.c
(psmouse_disconnect(), psmouse_connect(), and psmouse_attr_set_helper() ->
psmouse_attr_set_protocol()) transitions psmouse->state to PSMOUSE_CMD_MODE
before invoking synaptics_disconnect():

    psmouse_set_state(psmouse, PSMOUSE_CMD_MODE);
    ...
    if (psmouse->disconnect)
        psmouse->disconnect(psmouse);

When synaptics_disconnect() calls led_classdev_unregister(&priv->rmi_led),
led_classdev_unregister() sets the brightness to LED_OFF and flushes
set_brightness_work, which calls synaptics_rmi_led_set(). Because
psmouse->state is already PSMOUSE_CMD_MODE rather than PSMOUSE_ACTIVATED,
synaptics_rmi_led_set() returns -EBUSY, causing the LED core to log an error
("Setting an LED's brightness failed (-16)") and leaving the hardware LED on.

Similarly, synaptics_init_ps2() calls synaptics_rmi_led_register() while
psmouse->state is still PSMOUSE_INITIALIZING (before psmouse_connect()
activates the device or while the pass-through serio child port probes and
deactivates the parent), so any brightness write in that window also fails
with -EBUSY and leaves priv->rmi_led.brightness out of sync with the
hardware.

Should synaptics_rmi_led_set() handle being called when the touchpad is not
in PSMOUSE_ACTIVATED, or how should LED updates during init and disconnect be
handled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030065240.1706377-1-ahmad.b1995@gmail.com?part=1

[PATCH] Input: synaptics - support the touchpad LED on RMI function $30

[ahmad baghereslami]

Some HP laptops have an LED in the top left corner of the Synaptics
touchpad that HP's Windows driver lights while the touchpad is disabled
(double tap on that corner). On the HP ZBook 17 G3 (SYN3040, TM2706-002)
the LED is not driven through the PS/2 deluxe LED command: the Windows
driver (SynTP.sys 19.0.19.68) switches the touchpad to RMI register
access over PS/2 with mode byte 0x7f, looks up function $30 (GPIO/LED)
in the page description table and sets the LED bit in its control
register:

  register write:  SetScale 2:1, SetRate <reg>, SetRate <value>
  register read:   SetScale 2:1, SetRate <reg>, GetInfo (first byte)

For touchpads in a new rmi_led_pnp_ids list, look up the F30 LED this
way when the touchpad is set up and register a
synaptics::touchpad_disabled LED class device. Setting it deactivates
the touchpad, writes the F30 registers and restores the mode byte, which
leaves RMI access. The LED is restored after reconnect.

On this touchpad F30 is on page 1 with control base 0x55 and four
GPIO/LED pins; the LED is bit 3 of control register 0x59 and its
brightness register is 0x5f.

Tested on an HP ZBook 17 G3 with Ubuntu's 7.0.0 kernel: the LED follows
writes to its brightness file, the touchpad and the pointing stick keep
working, and a userspace helper that turns the touchpad off on a double
tap in the corner keeps the LED in step with GNOME's touchpad setting.

Reading HP's Windows driver (Ghidra) and writing this patch were done
with the help of an AI coding assistant (Claude); I tested the LED and
the touchpad on the machine.

Signed-off-by: ahmad baghereslami <ahmad.b1995@gmail.com>
---
 drivers/input/mouse/synaptics.c | 241 ++++++++++++++++++++++++++++++++
 drivers/input/mouse/synaptics.h |  13 ++
 2 files changed, 254 insertions(+)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index b007e642e..bf1c6c738 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -589,6 +589,236 @@ static int synaptics_set_advanced_gesture_mode(struct psmouse *psmouse)
 	return 0;
 }
 
+/*
+ * Some HP touchpads have an LED in the top left corner that HP's Windows
+ * driver lights while the touchpad is disabled. It is wired to RMI function
+ * $30 (GPIO/LED), which these touchpads also expose over PS/2: mode byte 0x7f
+ * switches them to RMI register access, where SetScale 2:1 followed by two
+ * SetRate commands writes a register, and SetScale 2:1, SetRate and GetInfo
+ * read one. Setting the normal mode byte again leaves RMI access.
+ */
+static const char * const rmi_led_pnp_ids[] = {
+	"SYN3040", /* HP ZBook 17 G3 */
+	NULL
+};
+
+#define SYN_RMI_MODE			0x7f
+#define SYN_RMI_PAGE_SELECT		0xff
+#define SYN_RMI_PDT_START		0xee
+#define SYN_RMI_PDT_END			0x0b
+#define SYN_RMI_PDT_ENTRY_SIZE		6
+#define SYN_RMI_NUM_PAGES		4
+#define SYN_RMI_F30			0x30
+#define SYN_RMI_F30_HAS_LED		BIT(2)
+#define SYN_RMI_F30_HAS_GPIO		BIT(3)
+#define SYN_RMI_LED_BRIGHTNESS		0x0b	/* as set by HP's Windows driver */
+
+static int synaptics_rmi_write(struct psmouse *psmouse, u8 reg, u8 val)
+{
+	struct ps2dev *ps2dev = &psmouse->ps2dev;
+	int error;
+
+	error = ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE21);
+	if (error)
+		return error;
+
+	error = ps2_command(ps2dev, &reg, PSMOUSE_CMD_SETRATE);
+	if (error)
+		return error;
+
+	return ps2_command(ps2dev, &val, PSMOUSE_CMD_SETRATE);
+}
+
+static int synaptics_rmi_read(struct psmouse *psmouse, u8 reg, u8 *val)
+{
+	struct ps2dev *ps2dev = &psmouse->ps2dev;
+	u8 param[3];
+	int error;
+
+	error = ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE21);
+	if (error)
+		return error;
+
+	error = ps2_command(ps2dev, &reg, PSMOUSE_CMD_SETRATE);
+	if (error)
+		return error;
+
+	error = ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO);
+	if (error)
+		return error;
+
+	*val = param[0];
+	return 0;
+}
+
+/* Finds the LED of RMI function $30. Leaves the touchpad in RMI mode. */
+static int synaptics_rmi_led_probe(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+	u8 fn, query, ctrl, caps, count, gpios;
+	unsigned int page, zeros, i;
+	int addr, error;
+
+	error = synaptics_mode_cmd(psmouse, SYN_RMI_MODE);
+	if (error)
+		return error;
+
+	for (page = 0; page < SYN_RMI_NUM_PAGES; page++) {
+		error = synaptics_rmi_write(psmouse, SYN_RMI_PAGE_SELECT, page);
+		if (error)
+			return error;
+
+		for (addr = SYN_RMI_PDT_START; addr >= SYN_RMI_PDT_END;
+		     addr -= SYN_RMI_PDT_ENTRY_SIZE) {
+			error = synaptics_rmi_read(psmouse, addr, &fn);
+			if (error)
+				return error;
+			if (!fn)
+				break;
+			if (fn != SYN_RMI_F30)
+				continue;
+
+			error = synaptics_rmi_read(psmouse, addr - 5, &query);
+			if (!error)
+				error = synaptics_rmi_read(psmouse, addr - 3, &ctrl);
+			if (!error)
+				error = synaptics_rmi_read(psmouse, query, &caps);
+			if (!error)
+				error = synaptics_rmi_read(psmouse, query + 1, &count);
+			if (error)
+				return error;
+
+			if (!(caps & SYN_RMI_F30_HAS_LED))
+				return -ENODEV;
+
+			priv->rmi_led_page = page;
+			if (!(caps & SYN_RMI_F30_HAS_GPIO)) {
+				priv->rmi_led_mask = BIT(0);
+				priv->rmi_led_ctrl = ctrl + 1;
+				priv->rmi_led_brightness = ctrl + 4;
+				return 0;
+			}
+
+			/* GPIO/LED select: the LED is the first set bit */
+			error = synaptics_rmi_read(psmouse, ctrl, &gpios);
+			if (error)
+				return error;
+
+			priv->rmi_led_mask = 0;
+			for (i = 0, zeros = 0; i < min(count, 8); i++) {
+				if (!(gpios & BIT(i)))
+					zeros++;
+				else if (!priv->rmi_led_mask)
+					priv->rmi_led_mask = BIT(i);
+			}
+			if (!priv->rmi_led_mask)
+				return -ENODEV;
+
+			priv->rmi_led_ctrl = ctrl + 4;
+			priv->rmi_led_brightness = ctrl + 7 + zeros;
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
+/* Sets the LED. Leaves the touchpad in RMI mode. */
+static int __synaptics_rmi_led_set(struct psmouse *psmouse, bool on)
+{
+	struct synaptics_data *priv = psmouse->private;
+	u8 val;
+	int error;
+
+	error = synaptics_mode_cmd(psmouse, SYN_RMI_MODE);
+	if (error)
+		return error;
+
+	error = synaptics_rmi_write(psmouse, SYN_RMI_PAGE_SELECT, priv->rmi_led_page);
+	if (error)
+		return error;
+
+	if (on) {
+		error = synaptics_rmi_write(psmouse, priv->rmi_led_brightness,
+					    SYN_RMI_LED_BRIGHTNESS);
+		if (error)
+			return error;
+	}
+
+	error = synaptics_rmi_read(psmouse, priv->rmi_led_ctrl, &val);
+	if (error)
+		return error;
+
+	val = on ? val | priv->rmi_led_mask : val & ~priv->rmi_led_mask;
+	return synaptics_rmi_write(psmouse, priv->rmi_led_ctrl, val);
+}
+
+static int synaptics_set_mode(struct psmouse *psmouse);
+
+static int synaptics_rmi_led_set(struct led_classdev *led,
+				 enum led_brightness value)
+{
+	struct synaptics_data *priv = container_of(led, struct synaptics_data, rmi_led);
+	struct psmouse *psmouse = priv->psmouse;
+	int error, ret;
+
+	guard(mutex)(&priv->rmi_led_lock);
+
+	if (psmouse->state != PSMOUSE_ACTIVATED)
+		return -EBUSY;
+
+	error = psmouse_deactivate(psmouse);
+	if (error)
+		return error;
+
+	ret = __synaptics_rmi_led_set(psmouse, value != LED_OFF);
+
+	/* Setting the mode byte again leaves RMI access */
+	error = synaptics_set_mode(psmouse);
+	if (!ret)
+		ret = error;
+
+	error = psmouse_activate(psmouse);
+	return ret ?: error;
+}
+
+static void synaptics_rmi_led_init(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+
+	if (!IS_REACHABLE(CONFIG_LEDS_CLASS) ||
+	    !psmouse_matches_pnp_id(psmouse, rmi_led_pnp_ids))
+		return;
+
+	if (synaptics_rmi_led_probe(psmouse))
+		return;
+
+	psmouse_info(psmouse, "touchpad LED on RMI F30 (page %u, ctrl %#x, mask %#x)\n",
+		     priv->rmi_led_page, priv->rmi_led_ctrl, priv->rmi_led_mask);
+	__synaptics_rmi_led_set(psmouse, false);
+	priv->has_rmi_led = true;
+}
+
+static void synaptics_rmi_led_register(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+	int error;
+
+	if (!priv->has_rmi_led)
+		return;
+
+	mutex_init(&priv->rmi_led_lock);
+	priv->psmouse = psmouse;
+	priv->rmi_led.name = "synaptics::touchpad_disabled";
+	priv->rmi_led.max_brightness = 1;
+	priv->rmi_led.brightness_set_blocking = synaptics_rmi_led_set;
+	error = led_classdev_register(&psmouse->ps2dev.serio->dev, &priv->rmi_led);
+	if (error) {
+		psmouse_warn(psmouse, "failed to register touchpad LED: %d\n", error);
+		priv->has_rmi_led = false;
+	}
+}
+
 static int synaptics_set_mode(struct psmouse *psmouse)
 {
 	struct synaptics_data *priv = psmouse->private;
@@ -1444,6 +1674,9 @@ static void synaptics_disconnect(struct psmouse *psmouse)
 {
 	struct synaptics_data *priv = psmouse->private;
 
+	if (priv->has_rmi_led)
+		led_classdev_unregister(&priv->rmi_led);
+
 	/*
 	 * We might have left a breadcrumb when trying to
 	 * set up SMbus companion.
@@ -1496,6 +1729,10 @@ static int synaptics_reconnect(struct psmouse *psmouse)
 		return error;
 	}
 
+	/* The reset turned the LED off; the mode byte below leaves RMI access. */
+	if (priv->has_rmi_led && priv->rmi_led.brightness)
+		__synaptics_rmi_led_set(psmouse, true);
+
 	error = synaptics_set_mode(psmouse);
 	if (error) {
 		psmouse_err(psmouse, "Unable to initialize device.\n");
@@ -1616,6 +1853,8 @@ static int synaptics_init_ps2(struct psmouse *psmouse,
 	 */
 	priv->is_forcepad = psmouse_matches_pnp_id(psmouse, forcepad_pnp_ids);
 
+	synaptics_rmi_led_init(psmouse);
+
 	err = synaptics_set_mode(psmouse);
 	if (err) {
 		psmouse_err(psmouse, "Unable to initialize device.\n");
@@ -1693,6 +1932,8 @@ static int synaptics_init_ps2(struct psmouse *psmouse,
 		}
 	}
 
+	synaptics_rmi_led_register(psmouse);
+
 	return 0;
 
  init_fail:
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 3853165b6..1d36a207e 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -6,6 +6,9 @@
 #ifndef _SYNAPTICS_H
 #define _SYNAPTICS_H
 
+#include <linux/leds.h>
+#include <linux/mutex.h>
+
 /* synaptics queries */
 #define SYN_QUE_IDENTIFY		0x00
 #define SYN_QUE_MODES			0x01
@@ -202,6 +205,16 @@ struct synaptics_data {
 	bool					press;
 	bool					report_press;
 	bool					is_forcepad;
+
+	/* Touchpad LED on RMI function $30, reached over PS/2 */
+	struct psmouse				*psmouse;
+	struct led_classdev			rmi_led;
+	struct mutex				rmi_led_lock;
+	bool					has_rmi_led;
+	u8					rmi_led_page;
+	u8					rmi_led_ctrl;
+	u8					rmi_led_mask;
+	u8					rmi_led_brightness;
 };
 
 void synaptics_module_init(void);
-- 
2.53.0

Re: [PATCH] Input: synaptics - support the touchpad LED on RMI function $30

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Input: synaptics: LED unregister in synaptics_disconnect() fails with -EBUSY
- [Medium] Input: synaptics: missing psmouse_mutex locking in synaptics_rmi_led_set()
- [Medium] Input: synaptics: synaptics_rmi_led_set() clears SYN_BIT_FOUR_BYTE_CLIENT
- [Low] Input: synaptics: link failure when CONFIG_LEDS_CLASS is not reachable
--

commit 2ccb85e9eccb48086da73fe8acd02267ab779ff1
Author: ahmad baghereslami <ahmad.b1995@gmail.com>

Input: synaptics - support the touchpad LED on RMI function $30

This commit adds support for the touchpad disabled LED wired to RMI function
$30 on certain HP laptops (such as the HP ZBook 17 G3) by accessing RMI
registers over PS/2 using mode byte 0x7f. It probes F30 during PS/2
initialization, registers an LED class device, and restores the LED state on
reconnect.

> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index b007e642ea765..bf1c6c738295c 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c

[ ... ]

> +static int synaptics_rmi_led_set(struct led_classdev *led,
> +				 enum led_brightness value)
> +{
> +	struct synaptics_data *priv = container_of(led, struct synaptics_data, rmi_led);
> +	struct psmouse *psmouse = priv->psmouse;
> +	int error, ret;
> +
> +	guard(mutex)(&priv->rmi_led_lock);
> +
> +	if (psmouse->state != PSMOUSE_ACTIVATED)
> +		return -EBUSY;
> +
> +	error = psmouse_deactivate(psmouse);
> +	if (error)
> +		return error;
> +
> +	ret = __synaptics_rmi_led_set(psmouse, value != LED_OFF);

[Severity: Medium]
Should synaptics_rmi_led_set() synchronize with psmouse_mutex rather than a
private rmi_led_lock?

In drivers/input/mouse/psmouse-base.c, psmouse_mutex serializes all paths
that inspect or transition psmouse->state, activate or deactivate a device or
pass-through parent port, or run multi-command PS/2 sequences (such as
psmouse_reconnect(), psmouse_resync(), psmouse_cleanup(),
psmouse_disconnect(), and psmouse_attr_set_helper()). Meanwhile,
ps2_command() only holds ps2dev->cmd_mutex for one PS/2 command at a time,
and nothing else in synaptics.c (including synaptics_reconnect()) or
psmouse-base.c takes priv->rmi_led_lock.

When synaptics_rmi_led_set() runs asynchronously from set_brightness_work
without holding psmouse_mutex:

1. While __synaptics_rmi_led_set() has the touchpad in mode 0x7f across
   multiple SetScale 2:1, SetRate, and GetInfo commands, concurrent PS/2
   commands from reconnect, resync, sysfs attribute writes (such as
   synaptics_set_rate() or synaptics_set_disable_gesture()), or pass-through
   port operations can interleave and corrupt both the RMI register sequence
   and the concurrent operation.

2. At the end of synaptics_rmi_led_set(), psmouse_activate(psmouse)
   unconditionally sends the enable command and sets psmouse->state to
   activated, even if a concurrent thread holding psmouse_mutex (such as
   pass-through child probing, reconnect, cleanup, or disconnect) has
   deactivated the touchpad and expects it to stay in command or
   initializing mode.

3. If set_brightness_work runs while a sysfs write or pass-through operation
   temporarily places psmouse->state into command mode, or right after
   synaptics_rmi_led_register() before psmouse_connect() activates the
   device, synaptics_rmi_led_set() immediately returns -EBUSY instead of
   waiting for the state transition to finish.

> +
> +	/* Setting the mode byte again leaves RMI access */
> +	error = synaptics_set_mode(psmouse);
> +	if (!ret)
> +		ret = error;
> +
> +	error = psmouse_activate(psmouse);
> +	return ret ?: error;
> +}

[Severity: Medium]
Will calling synaptics_set_mode() here clobber the 4-byte client bit in
priv->mode when a pass-through guest device is attached?

When a 4-byte protocol child is activated on the pass-through port,
synaptics_pt_activate() updates priv->mode:

drivers/input/mouse/synaptics.c:synaptics_pt_activate() {
	...
	if (child) {
		if (child->pktsize == 4)
			priv->mode |= SYN_BIT_FOUR_BYTE_CLIENT;
		else
			priv->mode &= ~SYN_BIT_FOUR_BYTE_CLIENT;

		if (synaptics_mode_cmd(psmouse, priv->mode))
			psmouse_warn(psmouse,
				     "failed to switch guest protocol\n");
	}
}

Other runtime mode updates such as synaptics_set_rate() and
synaptics_set_disable_gesture() modify priv->mode in place and call
synaptics_mode_cmd(psmouse, priv->mode). By contrast, synaptics_set_mode()
resets priv->mode = 0 and rebuilds it without SYN_BIT_FOUR_BYTE_CLIENT,
which will clear that bit in hardware whenever the LED brightness is changed.

> +
> +static void synaptics_rmi_led_init(struct psmouse *psmouse)
> +{
> +	struct synaptics_data *priv = psmouse->private;
> +
> +	if (!IS_REACHABLE(CONFIG_LEDS_CLASS) ||
> +	    !psmouse_matches_pnp_id(psmouse, rmi_led_pnp_ids))
> +		return;
> +
> +	if (synaptics_rmi_led_probe(psmouse))
> +		return;
> +
> +	psmouse_info(psmouse, "touchpad LED on RMI F30 (page %u, ctrl %#x, mask %#x)\n",
> +		     priv->rmi_led_page, priv->rmi_led_ctrl, priv->rmi_led_mask);
> +	__synaptics_rmi_led_set(psmouse, false);
> +	priv->has_rmi_led = true;
> +}
> +
> +static void synaptics_rmi_led_register(struct psmouse *psmouse)
> +{
> +	struct synaptics_data *priv = psmouse->private;
> +	int error;
> +
> +	if (!priv->has_rmi_led)
> +		return;
> +
> +	mutex_init(&priv->rmi_led_lock);
> +	priv->psmouse = psmouse;
> +	priv->rmi_led.name = "synaptics::touchpad_disabled";
> +	priv->rmi_led.max_brightness = 1;
> +	priv->rmi_led.brightness_set_blocking = synaptics_rmi_led_set;
> +	error = led_classdev_register(&psmouse->ps2dev.serio->dev, &priv->rmi_led);
> +	if (error) {
> +		psmouse_warn(psmouse, "failed to register touchpad LED: %d\n", error);
> +		priv->has_rmi_led = false;
> +	}
> +}

[Severity: Low]
Can this cause a link failure when CONFIG_LEDS_CLASS is disabled, or when
psmouse is built-in and CONFIG_LEDS_CLASS=m?

In drivers/input/mouse/Kconfig, MOUSE_PS2_SYNAPTICS does not depend on or
select LEDS_CLASS, and include/linux/leds.h declares
led_classdev_register_ext() and led_classdev_unregister() unconditionally
without inline stubs when CONFIG_LEDS_CLASS is not enabled.

Because synaptics_rmi_led_init() checks IS_REACHABLE(CONFIG_LEDS_CLASS),
whereas synaptics_rmi_led_register() and synaptics_disconnect() only check
the runtime struct field priv->has_rmi_led after priv is loaded from
psmouse->private across function calls, the compiler cannot fold
priv->has_rmi_led to a compile-time constant false and eliminate the calls
to led_classdev_register() and led_classdev_unregister().

> +
>  static int synaptics_set_mode(struct psmouse *psmouse)
>  {
>  	struct synaptics_data *priv = psmouse->private;
> @@ -1444,6 +1674,9 @@ static void synaptics_disconnect(struct psmouse *psmouse)
>  {
>  	struct synaptics_data *priv = psmouse->private;
>  
> +	if (priv->has_rmi_led)
> +		led_classdev_unregister(&priv->rmi_led);

[Severity: Medium]
When synaptics_disconnect() calls led_classdev_unregister(&priv->rmi_led),
will the shutdown brightness update always fail with -EBUSY?

Every caller of psmouse->disconnect(psmouse) in
drivers/input/mouse/psmouse-base.c transitions psmouse->state to command mode
before invoking the disconnect callback:

drivers/input/mouse/psmouse-base.c:psmouse_disconnect() {
	...
	psmouse_set_state(psmouse, PSMOUSE_CMD_MODE);
	...
	if (psmouse->disconnect)
		psmouse->disconnect(psmouse);
}

Because LED_RETAIN_AT_SHUTDOWN is not set on priv->rmi_led.flags,
led_classdev_unregister() calls led_set_brightness(&priv->rmi_led, LED_OFF)
and flushes set_brightness_work:

led_classdev_unregister()
  -> led_set_brightness(&priv->rmi_led, LED_OFF)
  -> flush_work(&priv->rmi_led.set_brightness_work)
       -> set_brightness_delayed()
            -> synaptics_rmi_led_set()

Inside synaptics_rmi_led_set(), the check:

	if (psmouse->state != PSMOUSE_ACTIVATED)
		return -EBUSY;

sees PSMOUSE_CMD_MODE and returns -EBUSY, causing
set_brightness_delayed_set_brightness() to log a kernel error and leaving the
hardware LED lit if it was on before disconnect.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030070821.1716595-1-ahmad.b1995@gmail.com?part=1