Re: [PATCH v2 07/11] 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 1 potential issue(s) to consider:
- [Medium] HID: hid-lenovo-go-s: stale setup_attempts in cfg_setup() prevents retries
--

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

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

This commit moves sysfs attribute and multicolor LED registration from
hid_gos_cfg_probe() to cfg_setup() so that attributes are only registered
after the MCU has been queried. It also adds retry logic to reschedule
cfg_setup() up to 5 times if cfg_query_device() fails.

> diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
> index 14aeddd65704b..cefb243800b65 100644
> --- a/drivers/hid/hid-lenovo-go-s.c
> +++ b/drivers/hid/hid-lenovo-go-s.c
> @@ -42,6 +42,7 @@ static struct hid_gos_cfg {
>  	bool gp_registered;
>  	bool rgb_registered;
>  	int cmd_status;
> +	int setup_attempts;
>  	u8 gp_auto_sleep_time;
>  	u8 gp_dpad_mode;
>  	u8 gp_mode;
> @@ -1453,57 +1454,127 @@ static struct led_classdev_mc gos_cdev_rgb = {

[ ... ]

> +static void cfg_setup(struct work_struct *work)
> +{
> +	bool gp_registered = false;
> +	bool rgb_registered = false;
> +	int ret;
> +
> +	ret = cfg_query_device();
>  	if (ret) {
> -		dev_err(&drvdata.hdev->dev,
> -			"Failed to retrieve OS Mode: %i\n", ret);
> +		/* The MCU may not be ready yet. Retry a bounded number of
> +		 * times before giving up.
> +		 */
> +		drvdata.setup_attempts++;
> +		if (drvdata.setup_attempts >= 5) {
> +			dev_err(&drvdata.hdev->dev,
> +				"Giving up on device setup after %d attempts\n",
> +				drvdata.setup_attempts);
> +			return;
> +		}
> +
> +		schedule_delayed_work(&drvdata.gos_cfg_setup, msecs_to_jiffies(50));
>  		return;
>  	}
>  
> -	/* Pairs with smp_load_acquire in attribute show/store functions */
> -	smp_store_release(&drvdata.gp_registered, true);
> -	/* Pairs with smp_load_acquire in attribute show/store functions */
> -	smp_store_release(&drvdata.rgb_registered, true);
> +	drvdata.setup_attempts = 0;

[Severity: Medium]
What happens on a subsequent setup run if cfg_query_device() fails 5 times
and cfg_setup() gives up, or if hid_gos_cfg_suspend() cancels gos_cfg_setup
after one or more failed attempts?

Since drvdata.setup_attempts is only reset to 0 when cfg_query_device()
succeeds, giving up leaves drvdata.setup_attempts at 5, and a suspend
cancellation leaves it at a non-zero count:

drivers/hid/hid-lenovo-go-s.c:hid_gos_cfg_suspend() {
	cancel_delayed_work_sync(&drvdata.gos_cfg_setup);
	return 0;
}

When hid_gos_cfg_reset_resume() later runs and sees that registration did
not complete, it schedules gos_cfg_setup again without resetting
drvdata.setup_attempts:

drivers/hid/hid-lenovo-go-s.c:hid_gos_cfg_reset_resume() {
	...
	if (!gp_registered || !rgb_registered) {
		ret = schedule_delayed_work(&drvdata.gos_cfg_setup, msecs_to_jiffies(2));
		...
	}
}

The same applies if the driver is unbound and rebound in hid_gos_cfg_probe()
while drvdata is static.

If the first query on that new run fails because the MCU is not immediately
ready, drvdata.setup_attempts++ increments the stale value (for example from
5 to 6) and cfg_setup() immediately gives up on the first attempt without
retrying. Should drvdata.setup_attempts be reset when giving up or when
scheduling a fresh setup sequence in hid_gos_cfg_probe() and
hid_gos_cfg_reset_resume()?

[ ... ]

> +}
>  

[ ... ]

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