Re: [PATCH 07/10] HID: hid-lenovo-go-s: Move attribute init to after device query

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] hid: hid-lenovo-go-s: double register of LED device corrupts leds_list
- [High] hid: hid-lenovo-go-s: permanently disabled workqueue breaks resume
- [Medium] sysfs: hid-lenovo-go-s: attributes return -ENODEV immediately after creation
--

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

HID: hid-lenovo-go-s: Move attribute init to after device query

Currently this driver registers attributes before the MCU is ready to
accept commands, which can lead to attribute access that blocks the MCU
in some rare cases. Move attribute construction to gos_cfg_setup, after
the device has been queried.

> diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
> index 6d98fff6d987e..f1a0bcfcdb71a 100644
> --- a/drivers/hid/hid-lenovo-go-s.c
> +++ b/drivers/hid/hid-lenovo-go-s.c
[ ... ]
> @@ -1497,10 +1498,53 @@ static void cfg_setup(struct work_struct *work)
>  		return;
>  	}
>  
> +	/* Pairs with smp_store_release from below */
> +	gp_registered = smp_load_acquire(&drvdata.gp_registered);
> +	if (gp_registered)
> +		goto try_rgb;
> +
> +	ret = sysfs_create_groups(&drvdata.hdev->dev.kobj, top_level_attr_groups);
> +	if (ret) {
> +		dev_err(&drvdata.hdev->dev,
> +			"Failed to create gamepad configuration attributes: %i\n", ret);
> +		goto try_rgb;
> +	}
> +
>  	/* Pairs with smp_load_acquire in attribute show/store functions */
>  	smp_store_release(&drvdata.gp_registered, true);

[Severity: Medium]
After sysfs_create_groups() exposes the attributes to userspace, could a
fast userspace process read them and invoke gamepad_property_show() before
smp_store_release(&drvdata.gp_registered, true) executes?

If so, would this cause spurious -ENODEV errors for userspace, violating
the requirement that all private data needed by userspace callbacks must be
complete before publishing the interface?

> +	gp_registered = true;
> +
> +try_rgb:
> +	/* Pairs with smp_store_release from below */
> +	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
> +	if (rgb_registered)
> +		goto update_kobjects;
> +
> +	ret = devm_led_classdev_multicolor_register(&drvdata.hdev->dev, &gos_cdev_rgb);
> +	if (ret) {
> +		dev_err(&drvdata.hdev->dev,
> +			"Failed to create RGB device: %i\n", ret);
> +		goto update_kobjects;
> +	}
> +
> +	ret = devm_device_add_group(gos_cdev_rgb.led_cdev.dev, &rgb_attr_group);
> +	if (ret) {
> +		dev_err(&drvdata.hdev->dev,
> +			"Failed to create RGB configuration attributes: %i\n", ret);
> +		goto update_kobjects;
> +	}

[Severity: High]
If devm_device_add_group() fails, cfg_setup() jumps to update_kobjects without
setting rgb_registered = true, and without unregistering the LED device
successfully created by devm_led_classdev_multicolor_register().

If a USB reset occurs, hid_gos_cfg_reset_resume() sees !rgb_registered and
reschedules cfg_setup(). When it runs again, it will call
devm_led_classdev_multicolor_register() on gos_cdev_rgb which is already
actively registered.

Does this execute list_add_tail() on a node already in the subsystem list,
corrupting it?

> +
> +	drvdata.led_cdev = &gos_cdev_rgb.led_cdev;
> +
>  	/* Pairs with smp_load_acquire in attribute show/store functions */
>  	smp_store_release(&drvdata.rgb_registered, true);
> +	rgb_registered = true;
> +
> +update_kobjects:
> +	if (gp_registered)
> +		kobject_uevent(&drvdata.hdev->dev.kobj, KOBJ_CHANGE);
> +	if (rgb_registered)
> +		kobject_uevent(&drvdata.led_cdev->dev->kobj, KOBJ_CHANGE);
>  }

[Severity: High]
The commit message mentions that a resume before setup has run shouldn't
prevent attribute creation. However, can the device fail to resume and
become unusable after a suspend/resume cycle?

If the system suspends, hid_gos_cfg_suspend() permanently disables the work
item:

hid_gos_cfg_suspend() {
    disable_delayed_work_sync(&drvdata.gos_cfg_setup);
    ...
}

When the system wakes, if cfg_setup() hadn't finished (e.g. within the 2ms
delay) or failed, hid_gos_cfg_reset_resume() will retry scheduling the work:

hid_gos_cfg_reset_resume() {
    ...
    if (!gp_registered || !rgb_registered) {
        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;
        }
        return 0;
    }
    ...
}

Because the work is permanently disabled and never re-enabled with
enable_delayed_work(), schedule_delayed_work() will return 0. Will this
cause hid_gos_cfg_reset_resume() to return -ENODEV, failing the resume
sequence entirely?

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