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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] Input: synaptics - LED left on in
  synaptics_rmi_led_unregister()
--

commit 8aa89954a3d89d6d5994f705dc23ae6d58848db8
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 on Synaptics
touchpads that expose RMI function $30 over PS/2, registering an LED class
device and updating the hardware registers via a delayed work item under
psmouse_mutex.

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

[ ... ]

> +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);
> +
> +	WRITE_ONCE(priv->rmi_led_on, value != LED_OFF);
> +	mod_delayed_work(system_percpu_wq, &priv->rmi_led_work, 0);
> +}

[ ... ]

> +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);
> +
> +	/*
> +	 * The touchpad is in command mode here. Mode byte 0, which
> +	 * synaptics_reset() sets next, leaves RMI access but keeps the LED lit.
> +	 */
> +	if (priv->rmi_led_on)
> +		__synaptics_rmi_led_set(psmouse, false);
> +}

[Severity: Medium]
Can synaptics_rmi_led_unregister() leave the physical touchpad LED lit on
disconnect?

synaptics_rmi_led_set() clears priv->rmi_led_on before
synaptics_rmi_led_work() takes psmouse_mutex and updates the hardware
register.  Because synaptics_disconnect() runs with psmouse_mutex held,
any LED_OFF request queued while the LED is lit cannot run its work item.
For example, if a trigger is attached to the LED,
led_classdev_unregister() detaches it before checking
LED_RETAIN_AT_SHUTDOWN:

drivers/leds/led-class.c:led_classdev_unregister() {
    ...
#ifdef CONFIG_LEDS_TRIGGERS
	down_write(&led_cdev->trigger_lock);
	if (led_cdev->trigger)
		led_trigger_set(led_cdev, NULL);
	up_write(&led_cdev->trigger_lock);
#endif
    ...
}

led_trigger_set(led_cdev, NULL) calls led_set_brightness(led_cdev,
LED_OFF), which invokes synaptics_rmi_led_set() and sets priv->rmi_led_on
to false.  cancel_delayed_work_sync() then cancels the pending work before
it can write to the touchpad, and the check on priv->rmi_led_on is false,
so __synaptics_rmi_led_set(psmouse, false) is skipped and the hardware LED
stays on.

Calling __synaptics_rmi_led_set(psmouse, false) unconditionally in
synaptics_rmi_led_unregister() (as synaptics_rmi_led_init() does), or
tracking the committed hardware state separately from the requested state,
would avoid this.

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