Re: [PATCH v2 09/11] HID: hid-lenovo-go-s: Move static led_classdev_mc to drvdata struct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] HID: hid-lenovo-go-s: sleeping in atomic context in hid_gos_brightness_set()
--

commit ce257492c69717cfbc87ce4b7e8049536c911d19
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-lenovo-go-s: Move static led_classdev_mc to drvdata struct

This commit moves the static gos_cdev_rgb multicolor LED structure into
struct gos_cfg_drvdata and initializes it in hid_gos_cfg_probe() in
preparation for dynamically allocating drvdata.

> diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
> index 136372eb6e6bf..2e938dd5e77c5 100644
> --- a/drivers/hid/hid-lenovo-go-s.c
> +++ b/drivers/hid/hid-lenovo-go-s.c

[ ... ]

> @@ -1593,6 +1580,18 @@ static int hid_gos_cfg_probe(struct hid_device *hdev,
>  	mutex_init(&drvdata.cfg_mutex);
>  	init_completion(&drvdata.send_cmd_complete);
>  
> +	/* Device is hardwired and name is guaranteed to be unique */
> +	drvdata.led_mc.led_cdev.name = "go_s:rgb:joystick_rings";
> +	drvdata.led_mc.led_cdev.brightness = 0x50;
> +	drvdata.led_mc.led_cdev.max_brightness = 0x64;
> +	drvdata.led_mc.led_cdev.color = LED_COLOR_ID_RGB;
> +	drvdata.led_mc.led_cdev.brightness_set = hid_gos_brightness_set;
> +	drvdata.led_mc.num_colors = 3;
> +	drvdata.led_mc.subled_info = devm_kmemdup(&hdev->dev, gos_rgb_subled_info,
> +						  sizeof(gos_rgb_subled_info), GFP_KERNEL);
> +	if (!drvdata.led_mc.subled_info)
> +		return -ENOMEM;

[Severity: High]
Can registering hid_gos_brightness_set() as the non-blocking
brightness_set callback sleep in atomic context?

In the LED core, brightness_set is invoked directly from atomic contexts
such as software blink timers and LED triggers via:

  led_timer_function()
    -> led_set_brightness_nosleep()
      -> led_set_brightness_nopm()
        -> __led_set_brightness()
          -> led_cdev->brightness_set()

However, hid_gos_brightness_set() calls rgb_cfg_call() ->
mcu_property_out(), which sleeps:

  drivers/hid/hid-lenovo-go-s.c:mcu_property_out() {
      ...
      guard(mutex)(&drvdata.cfg_mutex);
      ...
      if (drvdata.orphan_ack_pending) {
          wait_for_completion_timeout(&drvdata.send_cmd_complete, msecs_to_jiffies(25));
      ...
      dmabuf = kzalloc(GO_S_PACKET_SIZE, GFP_KERNEL);
      ...
      ret = hid_hw_output_report(hdev, dmabuf, GO_S_PACKET_SIZE);
      ...
      t_remaining = wait_for_completion_interruptible_timeout(&drvdata.send_cmd_complete,
                                                              msecs_to_jiffies(timeout));
      ...
  }

Should hid_gos_brightness_set() be wired up to brightness_set_blocking
instead of brightness_set so that the LED core defers atomic updates to
set_brightness_work?

> +
>  	/* Executing calls prior to returning from probe will lock the MCU. Schedule
>  	 * initial data call after probe has completed and MCU can accept calls.
>  	 */

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017013956.2440130-1-derekjohn.clark@gmail.com?part=9