Re: [PATCH v2] 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
issues to consider:
- [Medium] Input: synaptics - touchpad LED left on after
  synaptics_disconnect()
- [Medium] Input: synaptics - synaptics_rmi_led_set() ignores brightness
  value
- [Low] Input: synaptics - deprecated system_wq used in
  synaptics_rmi_led_set()
- [Low] Input: synaptics - non-standard error return style in RMI LED
  helpers
--

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

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

This commit adds support for the touchpad LED wired to RMI function $30
over PS/2 on HP laptops such as the HP ZBook 17 G3.

> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index b007e642ea765..4186e4f5373ab 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -589,6 +589,278 @@ static int synaptics_set_advanced_gesture_mode(struct psmouse *psmouse)

[ ... ]

> +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);
> +}

[ ... ]

> +/* 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);
> +}

[Severity: Low]
This isn't a bug, but in input subsystem functions that have multiple
failure points using an error variable, the preferred style for the final
call is to assign to error, check if (error) return error, and explicitly
return 0 on the success path in both synaptics_rmi_write() and
__synaptics_rmi_led_set().

> +
> +/*
> + * Runs with psmouse_mutex held, like the protected psmouse attributes, so
> + * that it does not race with reconnect, resync or attribute changes.
> + * synaptics_disconnect() cancels it with psmouse_mutex held, so it must not
> + * wait for the mutex: when it is busy, try again a little later.
> + */
> +static void synaptics_rmi_led_work(struct work_struct *work)
> +{
> +	struct synaptics_data *priv = container_of(to_delayed_work(work),
> +						   struct synaptics_data,
> +						   rmi_led_work);
> +	struct psmouse *psmouse = priv->psmouse;
> +	int error;
> +
> +	if (!mutex_trylock(&psmouse_mutex)) {
> +		schedule_delayed_work(&priv->rmi_led_work,
> +				      msecs_to_jiffies(SYN_RMI_LED_RETRY_MS));
> +		return;
> +	}
> +
> +	if (psmouse->state == PSMOUSE_RESYNCING) {
> +		schedule_delayed_work(&priv->rmi_led_work,
> +				      msecs_to_jiffies(SYN_RMI_LED_RETRY_MS));
> +	} else if (psmouse->state == PSMOUSE_ACTIVATED) {
> +		/* Otherwise the LED is restored on reconnect */
> +		psmouse_deactivate(psmouse);
> +
> +		error = __synaptics_rmi_led_set(psmouse,
> +						priv->rmi_led.brightness != LED_OFF);
> +		if (error)
> +			psmouse_warn(psmouse,
> +				     "failed to set touchpad LED: %d\n", error);
> +
> +		/* Setting the mode byte again leaves RMI access */
> +		if (synaptics_mode_cmd(psmouse, priv->mode))
> +			psmouse_warn(psmouse, "failed to restore mode\n");
> +
> +		psmouse_activate(psmouse);
> +	}
> +
> +	mutex_unlock(&psmouse_mutex);
> +}
> +
> +static void synaptics_rmi_led_set(struct led_classdev *led,
> +				  enum led_brightness value)
> +{
> +	struct synaptics_data *priv = container_of(led, struct synaptics_data, rmi_led);
> +
> +	mod_delayed_work(system_wq, &priv->rmi_led_work, 0);
> +}

[Severity: Medium]
Can ignoring the value argument in synaptics_rmi_led_set() leave the LED
stuck on when software blinking is disabled?

When led_set_brightness(led_cdev, LED_OFF) is called while software
blinking (LED_BLINK_SW) is active, set_brightness_delayed() stops blinking
and invokes brightness_set(led_cdev, LED_OFF) without updating
led_cdev->brightness:

drivers/leds/led-core.c:set_brightness_delayed() {
	if (test_and_clear_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) {
		led_stop_software_blink(led_cdev);
		set_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags);
	}
	...
	if (test_and_clear_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags))
		set_brightness_delayed_set_brightness(led_cdev, LED_OFF);
}

If the blink timer last left priv->rmi_led.brightness at 1,
synaptics_rmi_led_work() (and synaptics_reconnect()) will read that stale
value instead of LED_OFF and keep the LED lit.

[Severity: Low]
Also in synaptics_rmi_led_set(), system_wq is deprecated and scheduled for
removal; please use system_percpu_wq in mod_delayed_work() instead.

[ ... ]

> +static void synaptics_rmi_led_register(struct psmouse *psmouse)
> +{
> +	struct synaptics_data *priv = psmouse->private;
> +	int error;
> +
> +	if (!IS_REACHABLE(CONFIG_LEDS_CLASS) || !priv->has_rmi_led)
> +		return;
> +
> +	priv->psmouse = psmouse;
> +	INIT_DELAYED_WORK(&priv->rmi_led_work, synaptics_rmi_led_work);
> +	priv->rmi_led.name = "synaptics::touchpad_disabled";
> +	priv->rmi_led.max_brightness = 1;
> +	priv->rmi_led.brightness_set = synaptics_rmi_led_set;
> +	/* synaptics_disconnect() resets the touchpad, which turns the LED off */
> +	priv->rmi_led.flags = LED_RETAIN_AT_SHUTDOWN;
> +	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 void synaptics_rmi_led_unregister(struct psmouse *psmouse)
> +{
> +	struct synaptics_data *priv = psmouse->private;
> +
> +	if (!IS_REACHABLE(CONFIG_LEDS_CLASS) || !priv->has_rmi_led)
> +		return;
> +
> +	led_classdev_unregister(&priv->rmi_led);
> +	cancel_delayed_work_sync(&priv->rmi_led_work);
> +}
> +
>  static int synaptics_set_mode(struct psmouse *psmouse)
>  {
>  	struct synaptics_data *priv = psmouse->private;
> @@ -1444,6 +1716,8 @@ static void synaptics_disconnect(struct psmouse *psmouse)
>  {
>  	struct synaptics_data *priv = psmouse->private;
>  
> +	synaptics_rmi_led_unregister(psmouse);
> +
>  	/*
>  	 * We might have left a breadcrumb when trying to
>  	 * set up SMbus companion.

[Severity: Medium]
Does synaptics_disconnect() leave the touchpad LED lit in hardware if it
was on when the driver disconnects?

Because LED_RETAIN_AT_SHUTDOWN is set, led_classdev_unregister() does not
turn the LED off, and any pending rmi_led_work is canceled before it can
acquire psmouse_mutex.  Unlike synaptics_reconnect(), which calls
psmouse_reset(), synaptics_disconnect() only calls synaptics_reset():

drivers/input/mouse/synaptics.c:synaptics_reset() {
	/* reset touchpad back to relative mode, gestures enabled */
	synaptics_mode_cmd(psmouse, 0);
}

Writing mode byte 0 only leaves RMI register access mode without clearing
the F30 LED control register, so the LED stays on after driver unbind,
module unload, or a protocol switch.  Calling __synaptics_rmi_led_set(psmouse,
false) in synaptics_rmi_led_unregister() after cancel_delayed_work_sync()
(before synaptics_reset() writes mode byte 0) would turn it off.

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