Re: [PATCH 06/10] HID: hid-lenovo-go-s: Add suspend function and gate access under bool.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid: hid-lenovo-go-s: Suspend callback permanently disables setup work, breaking resume
- [Medium] hid: hid-lenovo-go-s: Transient MCU query failures permanently lock out sysfs configuration
--

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

HID: hid-lenovo-go-s: Add suspend function and gate access under bool.

This commit adds a suspend function to cancel pending delayed work during
system sleep. It also introduces boolean flags to prevent sysfs attribute
access prior to the configuration setup completing.

> diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
> index 712823e22beae..6d98fff6d987e 100644
> --- a/drivers/hid/hid-lenovo-go-s.c
> +++ b/drivers/hid/hid-lenovo-go-s.c

[ ... ]

>  static void cfg_setup(struct work_struct *work)
>  {
>  	int ret;
>  
>  	/* MCU */
>  	ret = mcu_property_out(drvdata.hdev, GET_MCU_ID, FEATURE_NONE, NULL, 0);
>  	if (ret) {
>  		dev_err(&drvdata.hdev->dev, "Failed to retrieve MCU ID: %i\n",
>  			ret);
>  		return;

[Severity: Medium]
If one of these initial MCU queries fails due to a transient error or timeout,
does this early return prevent the gp_registered and rgb_registered flags from
ever being set? Since there is no retry mechanism here, this could leave the
sysfs configuration permanently returning -ENODEV until the device is
replugged.

>  	}

[ ... ]

>  static int hid_gos_cfg_reset_resume(struct hid_device *hdev)
>  {
> +	bool gp_registered, rgb_registered;
>  	u8 os_mode = drvdata.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 (!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;
> +	}

[ ... ]

> +static int hid_gos_cfg_suspend(void)
> +{
> +	disable_delayed_work_sync(&drvdata.gos_cfg_setup);

[Severity: High]
Does using disable_delayed_work_sync() here permanently disable the work
queue since there is no matching enable_delayed_work() call?

If the work queue is permanently disabled, schedule_delayed_work() in
hid_gos_cfg_reset_resume() will return false. Would treating a false return
from schedule_delayed_work() as a fatal error cause the resume operation to
abort unconditionally with -ENODEV?

Could cancel_delayed_work_sync() be used instead to cancel the work without
disabling it?

> +
> +	return 0;
> +}

[ ... ]

While looking at this driver, I noticed a pre-existing issue in
hid_gos_probe():

>  static int hid_gos_probe(struct hid_device *hdev,
>  			 const struct hid_device_id *id)
>  {
[ ... ]
>  	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>  	if (ret) {
>  		hid_err(hdev, "Failed to start HID device\n");
>  		return ret;
>  	}
>  
>  	ret = hid_hw_open(hdev);
>  	if (ret) {
>  		hid_err(hdev, "Failed to open HID device\n");
>  		hid_hw_stop(hdev);
>  		return ret;
>  	}
>  
>  	ep = get_endpoint_address(hdev);
>  	if (ep != GO_S_CFG_INTF_IN) {
>  		dev_dbg(&hdev->dev, "Started interface %x as generic HID device.\n", ep);
>  		return 0;
>  	}
>  
>  	ret = hid_gos_cfg_probe(hdev, id);
>  	if (ret)
>  		dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface");
>  
>  	dev_dbg(&hdev->dev, "Started interface %x as Go S configuration interface\n", ep);
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but if hid_gos_cfg_probe() fails,
does this error path return without calling hid_hw_stop() and hid_hw_close()?
This would leave the hardware active and leak resources since the HID core
does not automatically invoke the .remove() callback on a probe failure.

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