Re: [PATCH v7 3/3] HID: lamparray: blank lamps across suspend and restore on resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] HID: hid-generic: missing PMSG_IS_AUTO() check in hid_generic_suspend()
- [Medium] HID: lamparray: missing LED_SUSPENDED and work flush in lamparray_suspend()
- [Medium] HID: lamparray: race on use_leds_uapi in suspend and resume paths
--

commit eae25839ebd677bf288de15dd8a2d0952e978e6d
Author: Cristian Mazzotta <cmmazzo@icloud.com>

HID: lamparray: blank lamps across suspend and restore on resume

This commit adds lamparray_suspend() and lamparray_resume() helpers to blank
LampArray lighting before system sleep and restore the cached RGB and
brightness state on resume, wiring them up in hid-generic.

> diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
> index fde772c5014bb..f78c62f796fbb 100644
> --- a/drivers/hid/hid-generic.c
> +++ b/drivers/hid/hid-generic.c
> @@ -102,9 +102,34 @@ static int hid_generic_probe(struct hid_device *hdev,
>  
>  static int hid_generic_reset_resume(struct hid_device *hdev)
>  {
> +	struct lamparray *la = hid_get_drvdata(hdev);
> +
>  	if (hdev->claimed & HID_CLAIMED_INPUT)
>  		hidinput_reset_resume(hdev);
>  
> +	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
> +		lamparray_resume(la);
> +
> +	return 0;
> +}
> +
> +static int hid_generic_suspend(struct hid_device *hdev, pm_message_t message)
> +{
> +	struct lamparray *la = hid_get_drvdata(hdev);
> +
> +	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
> +		lamparray_suspend(la);
> +
> +	return 0;
> +}
> +
> +static int hid_generic_resume(struct hid_device *hdev)
> +{
> +	struct lamparray *la = hid_get_drvdata(hdev);
> +
> +	if (IS_ENABLED(CONFIG_HID_LAMPARRAY) && la)
> +		lamparray_resume(la);
> +
>  	return 0;
>  }

[Severity: High]
How does this behave with USB runtime autosuspend when PMSG_IS_AUTO(message)
is true?

In drivers/hid/usbhid/hid-core.c, hid_suspend() counts only EV_LED usages in
HID_OUTPUT_REPORT via hidinput_count_leds(), not LampArray HID_FEATURE_REPORT
LEDs. When a USB LampArray interface enters runtime autosuspend in S0,
hid_suspend() sets HID_SUSPENDED on usbhid->iofl before calling
hid_driver_suspend():

drivers/hid/usbhid/hid-core.c:hid_suspend() {
	if (PMSG_IS_AUTO(message)) {
		ledcount = hidinput_count_leds(hid);
		spin_lock_irq(&usbhid->lock);	/* Sync with error handler */
		if (!test_bit(HID_RESET_PENDING, &usbhid->iofl)
		    && !test_bit(HID_CLEAR_HALT, &usbhid->iofl)
		    && !test_bit(HID_OUT_RUNNING, &usbhid->iofl)
		    && !test_bit(HID_CTRL_RUNNING, &usbhid->iofl)
		    && !test_bit(HID_KEYS_PRESSED, &usbhid->iofl)
		    && (!ledcount || ignoreled))
		{
			set_bit(HID_SUSPENDED, &usbhid->iofl);
			spin_unlock_irq(&usbhid->lock);
			status = hid_driver_suspend(hid, message);
}

Because hid_generic_suspend() ignores message and calls lamparray_suspend(),
lamparray_hw_set_state() -> hid_hw_request() -> __usbhid_submit_report()
queues the (0, 0, 0, 0) report in usbhid->ctrl[] and calls
usbhid_restart_ctrl_queue(), which returns early because HID_SUSPENDED is
already set:

drivers/hid/usbhid/hid-core.c:usbhid_restart_ctrl_queue() {
	if (!hid || test_bit(HID_RESET_PENDING, &usbhid->iofl) ||
			test_bit(HID_SUSPENDED, &usbhid->iofl))
		return 0;
}

hid_hw_wait() then sees HID_CTRL_RUNNING == 0 and returns immediately while
the blanking report remains unsent in usbhid->ctrl[].

If the system subsequently enters s2idle or S3 while the USB device is already
runtime-suspended, usb_suspend_both() skips usb_suspend_interface() and the
blanking report is never transmitted to the hardware before system sleep. In
S0, when the device runtime-resumes via hid_resume(), hid_restart_io()
transmits the stale (0, 0, 0, 0) report and hid_generic_resume() calls
lamparray_resume(), cycling AutonomousMode (1 -> 0) and restoring state on
every runtime wakeup.

Should hid_generic_suspend() and hid_generic_resume() skip LampArray
blanking/restoration during runtime PM (or should runtime autosuspend be
prevented while LampArray LEDs are active)?

[ ... ]

> diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
> index fb3082f6d19a0..5c97cafbd25d2 100644
> --- a/drivers/hid/hid-lamparray.c
> +++ b/drivers/hid/hid-lamparray.c
> @@ -852,6 +852,60 @@ void lamparray_unregister(struct lamparray *la)
>  }
>  EXPORT_SYMBOL_GPL(lamparray_unregister);
>  
> +/*
> + * Blank all lamps on suspend rather than handing control back to the firmware,
> + * which may not turn them off in low power states. On an Acer Predator PT14-52T,
> + * system power draw during s2idle was ~12.35W with lamps lit, and ~3.14W with
> + * them blanked; the lighting accounted for ~75% of the power draw during suspend.
> + * Since writing zeroes is well defined on all lamparray devices, always do it.
> + * This is ignored if use_leds_uapi is 0; let userspace keep full control.
> + */
> +void lamparray_suspend(struct lamparray *la)
> +{
> +	struct lamparray_device *ldev;
> +
> +	if (!la)
> +		return;
> +
> +	ldev = &la->ldev;
> +
> +	if (!ldev->use_leds_uapi)
> +		return;
> +
> +	mutex_lock(&ldev->dev_lock);
> +	lamparray_hw_set_state(ldev, 0, 0, 0, 0);
> +	mutex_unlock(&ldev->dev_lock);
> +
> +	hid_hw_wait(ldev->hdev);
> +}
> +EXPORT_SYMBOL_GPL(lamparray_suspend);

[Severity: Medium]
Could pending LED work or software blink timers run after lamparray_suspend()
blanks the hardware, un-blanking the lamps and overwriting ldev->last_*?

lamparray_register_led() registers ldev->mc_cdev.led_cdev with
brightness_set_blocking = lamparray_led_brightness_set and without
LED_CORE_SUSPENDRESUME. All non-sleeping brightness updates (sysfs writes,
led_timer_function() software blink, and in-kernel LED triggers) go through
led_set_brightness_nosleep() -> led_set_brightness_nopm(), which queues
cdev->set_brightness_work onto leds_wq (a non-freezable workqueue) unless
LED_SUSPENDED is set on cdev->flags.

Because lamparray_suspend() does not set LED_SUSPENDED on
ldev->mc_cdev.led_cdev.flags or call
flush_work(&ldev->mc_cdev.led_cdev.set_brightness_work), a queued
set_brightness_work or a blink/trigger timer firing during dpm_suspend() or
s2idle can run lamparray_led_brightness_set() after lamparray_suspend() drops
ldev->dev_lock:

drivers/hid/hid-lamparray.c:lamparray_led_brightness_set() {
	...
	guard(mutex)(&ldev->dev_lock);
	...
	if (brightness)
		ret = lamparray_hw_set_state(ldev, r, g, b, brightness);
	else
		ret = lamparray_hw_set_state(ldev, 0, 0, 0, 0);
	...
	ldev->last_r = r;
	ldev->last_g = g;
	ldev->last_b = b;
	ldev->last_brightness = brightness;
}

This sends a new HID_REQ_SET_REPORT during suspend and clobbers the cached
ldev->last_r, ldev->last_g, ldev->last_b, and ldev->last_brightness values.
Similarly, during lamparray_resume(), set_brightness_work can interleave
between lamparray_hw_set_autonomous() and lamparray_restore_state() while
ldev->dev_lock is dropped.

> +
> +void lamparray_resume(struct lamparray *la)
> +{
> +	struct lamparray_device *ldev;
> +
> +	if (!la)
> +		return;
> +
> +	ldev = &la->ldev;
> +
> +	if (!ldev->use_leds_uapi)
> +		return;
> +
> +	/*
> +	 * After a S4 transition, some devices report
> +	 * AutonomousMode = 0 while still ignoring host lamp updates.
> +	 * Writing 0 again does nothing; forcing a 1 -> 0
> +	 * will guarantee the device will update.
> +	 */
> +	lamparray_hw_set_autonomous(ldev, true);
> +	hid_hw_wait(ldev->hdev);
> +	lamparray_hw_set_autonomous(ldev, false);
> +
> +	lamparray_restore_state(ldev);
> +}
> +EXPORT_SYMBOL_GPL(lamparray_resume);

[Severity: Medium]
Can lamparray_suspend() and lamparray_resume() race with concurrent writes to
use_leds_uapi_store()?

In use_leds_uapi_store(), transitions of ldev->use_leds_uapi, LED
registration/unregistration, and AutonomousMode updates are serialized under
ldev->sysfs_lock:

drivers/hid/hid-lamparray.c:use_leds_uapi_store() {
	guard(mutex)(&ldev->sysfs_lock);
	old_val = ldev->use_leds_uapi;

	if (val == old_val)
		return count;

	ldev->use_leds_uapi = val;
	...
}

Neither lamparray_suspend() nor lamparray_resume() acquires ldev->sysfs_lock,
and lamparray_resume() drops ldev->dev_lock between
lamparray_hw_set_autonomous(ldev, true), hid_hw_wait(),
lamparray_hw_set_autonomous(ldev, false), and lamparray_restore_state().

If use_leds_uapi_store() transitions from 1 to 0 while lamparray_resume() is
sleeping in hid_hw_wait() (for example, during USB runtime resume),
use_leds_uapi_store() unregisters the LED and sets AutonomousMode = 1, after
which lamparray_resume() calls lamparray_hw_set_autonomous(ldev, false)
without re-checking use_leds_uapi, leaving AutonomousMode disabled while
use_leds_uapi is 0.

Conversely, during a 0 to 1 transition in use_leds_uapi_store(),
ldev->use_leds_uapi = val is written before lamparray_register_led()
completes. If initial LED registration failed during lamparray_register() and
lamparray_resume() observes ldev->use_leds_uapi == true before
lamparray_register_led() finishes initializing ldev->mc_cdev.subled_info,
lamparray_restore_state() can dereference a NULL ldev->mc_cdev.subled_info
pointer.

Should lamparray_suspend() and lamparray_resume() serialize against
use_leds_uapi_store() with ldev->sysfs_lock?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-hid-lamparray-v7-0-47418b667b47@tuxedocomputers.com?part=3