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

[sashiko-bot@kernel.org]

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