Re: [PATCH v2 10/11] HID: hid-lenovo-go-s: Use devm_kzalloc for drvdata

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] HID: hid-lenovo-go-s: sleeping in atomic context in hid_gos_brightness_set()
- [Medium] HID: hid-lenovo-go-s: missing resume callback leaves gos_cfg_setup cancelled
- [Medium] HID: hid-lenovo-go-s: rgb_registered not cleared in hid_gos_cfg_remove()
- [Low] HID: hid-lenovo-go-s: clear rgb_registered before unregister in cfg_setup()
--

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

HID: hid-lenovo-go-s: Use devm_kzalloc for drvdata

This commit converts drvdata in hid-lenovo-go-s from a static global
structure to a per-device devm-allocated structure and updates the
setup work function name in barrier comments.

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

[ ... ]

> @@ -1189,13 +1261,14 @@ static ssize_t rgb_profile_range_show(struct device *dev,
>  static void hid_gos_brightness_set(struct led_classdev *led_cdev,
>  				   enum led_brightness brightness)
>  {
> -	struct led_classdev_mc *led_mc = &drvdata.led_mc;
> +	struct led_classdev_mc *led_mc = lcdev_to_mccdev(led_cdev);
> +	struct gos_cfg_drvdata *drvdata = container_of(led_mc, struct gos_cfg_drvdata, led_mc);
>  	enum rgb_config_index index;
>  	bool rgb_registered;
>  	int ret;
>  
> -	/* Pairs with smp_store_release from gos_cfg_setup */
> -	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
> +	/* Pairs with smp_store_release from cfg_setup */
> +	rgb_registered = smp_load_acquire(&drvdata->rgb_registered);
>  	if (!rgb_registered)
>  		return;
>  
> @@ -1204,15 +1277,15 @@ static void hid_gos_brightness_set(struct led_classdev *led_cdev,
>  		return;
>  	}
>  
> -	index = drvdata.rgb_profile + 2;
> -	u8 rgb_profile[6] = { drvdata.rgb_effect,
> +	index = drvdata->rgb_profile + 2;
> +	u8 rgb_profile[6] = { drvdata->rgb_effect,
>  			      led_mc->subled_info[0].intensity,
>  			      led_mc->subled_info[1].intensity,
>  			      led_mc->subled_info[2].intensity,
>  			      brightness,
> -			      drvdata.rgb_speed };
> +			      drvdata->rgb_speed };
>  
> -	ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, index, rgb_profile, 6);
> +	ret = rgb_cfg_call(drvdata->hdev, SET_RGB_CFG, index, rgb_profile, 6);

[Severity: High]
Can hid_gos_brightness_set() sleep in atomic context here?

In hid_gos_cfg_probe(), hid_gos_brightness_set() is wired to
drvdata->led_mc.led_cdev.brightness_set rather than brightness_set_blocking.
In the LED core, setting brightness_set tells led_set_brightness_nopm() that
the callback does not sleep, so __led_set_brightness() invokes it directly in
the caller's context without deferring to set_brightness_work.

When software blinking (via led_timer_function() in timer softirq context) or
an atomic LED trigger calls led_set_brightness_nosleep(), the call chain is:

  hid_gos_brightness_set()
    rgb_cfg_call()
      mcu_property_out()

Inside mcu_property_out(), the code acquires a sleeping mutex
(drvdata->cfg_mutex), allocates memory with GFP_KERNEL, and blocks on
wait_for_completion_timeout() and wait_for_completion_interruptible_timeout().
Should this use brightness_set_blocking instead?

[ ... ]

> @@ -1536,67 +1610,75 @@ static void cfg_setup(struct work_struct *work)
>  		}
>  	}
>  
> -	if (!drvdata.rgb_registered) {
> -		/* Pairs with smp_load_acquire in attribute show/store functions */
> -		smp_store_release(&drvdata.rgb_registered, true);
> +	if (!drvdata->rgb_registered) {
> +		/* Pairs with smp_load_acquire in attribute show/store */
> +		smp_store_release(&drvdata->rgb_registered, true);
>  		rgb_registered = true;
>  
> -		ret = led_classdev_multicolor_register(&drvdata.hdev->dev,
> -						       &drvdata.led_mc);
> +		ret = led_classdev_multicolor_register(&drvdata->hdev->dev,
> +						       &drvdata->led_mc);
>  		if (ret) {
> -			dev_err(&drvdata.hdev->dev, "Failed to create RGB device: %i\n",
> +			dev_err(&drvdata->hdev->dev, "Failed to create RGB device: %i\n",
>  				ret);
>  
> -			/* Pairs with smp_load_acquire in attribute show/store functions */
> -			smp_store_release(&drvdata.rgb_registered, false);
> +			/* Pairs with smp_load_acquire in attribute show/store */
> +			smp_store_release(&drvdata->rgb_registered, false);
>  			rgb_registered = false;
>  		} else {
> -			ret = devm_device_add_group(drvdata.led_mc.led_cdev.dev,
> +			ret = devm_device_add_group(drvdata->led_mc.led_cdev.dev,
>  						    &rgb_attr_group);
>  			if (ret) {
> -				dev_err(&drvdata.hdev->dev,
> +				dev_err(&drvdata->hdev->dev,
>  					"Failed to create RGB configuration attributes: %i\n",
>  					ret);
>  
> -				led_classdev_multicolor_unregister(&drvdata.led_mc);
> +				led_classdev_multicolor_unregister(&drvdata->led_mc);
>  
> -				/* Pairs with smp_load_acquire in attribute show/store functions */
> -				smp_store_release(&drvdata.rgb_registered, false);
> +				/* Pairs with smp_load_acquire in attribute show/store */
> +				smp_store_release(&drvdata->rgb_registered, false);
>  				rgb_registered = false;
>  			}
>  		}
>  	}

[Severity: Low]
In cfg_setup(), if devm_device_add_group() fails, should
drvdata->rgb_registered be set to false before calling
led_classdev_multicolor_unregister(&drvdata->led_mc)?

Because LED_RETAIN_AT_SHUTDOWN is not set on drvdata->led_mc.led_cdev.flags,
led_classdev_unregister() calls led_set_brightness(led_cdev, LED_OFF), which
synchronously invokes hid_gos_brightness_set(). Since drvdata->rgb_registered
is still true at that point and drvdata->rgb_profile is still 0,
hid_gos_brightness_set() sends an unintended SET_RGB_CFG command with index
LIGHT_PROFILE_SEL (0 + 2) and a 6-byte payload to the MCU during error
rollback.

[ ... ]

> @@ -1607,15 +1691,19 @@ static int hid_gos_cfg_probe(struct hid_device *hdev,
>  
>  static void hid_gos_cfg_remove(struct hid_device *hdev)
>  {
> +	struct gos_cfg_drvdata *drvdata = hid_get_drvdata(hdev);
>  	bool rgb_registered;
>  
> -	disable_delayed_work_sync(&drvdata.gos_cfg_setup);
> +	if (!drvdata)
> +		return;
> +
> +	disable_delayed_work_sync(&drvdata->gos_cfg_setup);
>  	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);
>  
> -	/* Pairs with smp_store_release in cfg_setup */
> -	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
> +	/* Pairs with smp_store_release from cfg_setup */
> +	rgb_registered = smp_load_acquire(&drvdata->rgb_registered);
>  	if (rgb_registered)
> -		led_classdev_multicolor_unregister(&drvdata.led_mc);
> +		led_classdev_multicolor_unregister(&drvdata->led_mc);
>  
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);

[Severity: Medium]
Does calling led_classdev_multicolor_unregister(&drvdata->led_mc) in
hid_gos_cfg_remove() without clearing drvdata->rgb_registered first cause a
timeout and error log on every driver unbind or module unload?

When hid_device_remove() runs, it holds hdev->driver_input_lock, causing
__hid_input_report() to drop any incoming HID reports. In
hid_gos_cfg_remove(), led_classdev_multicolor_unregister() is called while
drvdata->rgb_registered is still true and before hid_hw_stop():

  hid_device_remove()
    hid_gos_remove()
      hid_gos_cfg_remove()
        led_classdev_multicolor_unregister()
          led_classdev_unregister()
            led_set_brightness(led_cdev, LED_OFF)
              hid_gos_brightness_set()
                rgb_cfg_call()
                  mcu_property_out()

Because drvdata->rgb_registered is still true and hid_hw_stop() has not been
called yet, mcu_property_out() sends SET_RGB_CFG to the MCU and blocks in
wait_for_completion_interruptible_timeout(). Since driver_input_lock blocks
hid_gos_raw_event(), the wait always times out with -EBUSY and logs
dev_err("Failed to write RGB profile: -16").

> @@ -1624,16 +1712,21 @@ static void hid_gos_cfg_remove(struct hid_device *hdev)
>  
>  static int hid_gos_cfg_reset_resume(struct hid_device *hdev)
>  {
> +	struct gos_cfg_drvdata *drvdata = hid_get_drvdata(hdev);
>  	bool gp_registered, rgb_registered;
> -	u8 os_mode = drvdata.os_mode;
> +	u8 os_mode;
>  	int ret;
>  
> -	/* Pairs with smp_store_release from gos_cfg_setup */
> -	gp_registered = smp_load_acquire(&drvdata.gp_registered);
> -	/* Pairs with smp_store_release from gos_cfg_setup */
> -	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
> +	if (!drvdata)
> +		return -ENODEV;
> +
> +	os_mode = drvdata->os_mode;
> +	/* Pairs with smp_store_release from cfg_setup */
> +	gp_registered = smp_load_acquire(&drvdata->gp_registered);
> +	/* Pairs with smp_store_release from cfg_setup */
> +	rgb_registered = smp_load_acquire(&drvdata->rgb_registered);
>  	if (!gp_registered || !rgb_registered) {
> -		ret = schedule_delayed_work(&drvdata.gos_cfg_setup, msecs_to_jiffies(2));
> +		ret = schedule_delayed_work(&drvdata->gos_cfg_setup, msecs_to_jiffies(2));
>  		if (!ret) {
>  			dev_err(&hdev->dev, "Failed to schedule startup delayed work\n");
>  			return -ENODEV;

[ ... ]

> @@ -1730,9 +1823,14 @@ static int hid_gos_reset_resume(struct hid_device *hdev)
>  	return 0;
>  }
>  
> -static int hid_gos_cfg_suspend(void)
> +static int hid_gos_cfg_suspend(struct hid_device *hdev)
>  {
> -	cancel_delayed_work_sync(&drvdata.gos_cfg_setup);
> +	struct gos_cfg_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (!drvdata)
> +		return -ENODEV;
> +
> +	cancel_delayed_work_sync(&drvdata->gos_cfg_setup);
>  
>  	return 0;
>  }
> @@ -1749,7 +1847,7 @@ static int hid_gos_suspend(struct hid_device *hdev, pm_message_t msg)
>  
>  	ep = ret;
>  	if (ep == GO_S_CFG_INTF_IN)
> -		return hid_gos_cfg_suspend();
> +		return hid_gos_cfg_suspend(hdev);
>  
>  	return 0;
>  }

[Severity: Medium]
What happens if the device suspends before drvdata->gos_cfg_setup completes
and then resumes without a USB reset?

In hid_gos_cfg_probe(), drvdata->gos_cfg_setup is scheduled with a 2ms delay
and retries up to 5 times at 50ms intervals in cfg_setup(). On any suspend
(such as s2idle or runtime autosuspend), hid_gos_suspend() calls
hid_gos_cfg_suspend(), which cancels drvdata->gos_cfg_setup via
cancel_delayed_work_sync().

However, struct hid_driver hid_lenovo_go_s only defines .reset_resume and
omits .resume:

  static struct hid_driver hid_lenovo_go_s = {
          .name = "hid-lenovo-go-s",
          .id_table = hid_gos_devices,
          .probe = hid_gos_probe,
          .remove = hid_gos_remove,
          .raw_event = hid_gos_raw_event,
          .reset_resume = pm_ptr(hid_gos_reset_resume),
          .suspend = pm_ptr(hid_gos_suspend),
  };

When usbhid resumes without a port reset, hid_driver_resume() checks
hdev->driver->resume and does not fall back to .reset_resume. Without a
.resume callback to reschedule drvdata->gos_cfg_setup when !gp_registered ||
!rgb_registered, won't the gamepad sysfs groups and RGB LED device remain
permanently unregistered?

I hope this is helpful.

Lawall-Bot

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