Re: [PATCH v2 06/11] 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 1 potential issue(s) to consider:
- [Medium] HID: hid-lenovo-go-s: missing resume callback after hid_gos_suspend()
--

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

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

This patch adds a suspend callback to cancel the initial configuration
delayed work if the device suspends before setup finishes, and gates
sysfs and LED brightness access behind registration flags set when
cfg_setup() completes.

> diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
> index ed08200e3571a..14aeddd65704b 100644
> --- a/drivers/hid/hid-lenovo-go-s.c
> +++ b/drivers/hid/hid-lenovo-go-s.c
[ ... ]
> @@ -1455,9 +1564,24 @@ static void hid_gos_cfg_remove(struct hid_device *hdev)
>  
>  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;
> +	}
> +
>  	ret = mcu_property_out(drvdata.hdev, SET_GAMEPAD_CFG,
>  			       FEATURE_OS_MODE, &os_mode, 1);
>  	if (ret < 0)
> @@ -1546,6 +1670,30 @@ static int hid_gos_reset_resume(struct hid_device *hdev)
>  	return 0;
>  }
>  
> +static int hid_gos_cfg_suspend(void)
> +{
> +	cancel_delayed_work_sync(&drvdata.gos_cfg_setup);
> +
> +	return 0;
> +}
> +
> +static int hid_gos_suspend(struct hid_device *hdev, pm_message_t msg)
> +{
> +	int ret;
> +	u8 ep;
> +
> +	/* Safe assumption. SET_INTERFACE ioctl can't be used while driver is bound */
> +	ret = get_endpoint_address(hdev);
> +	if (ret <= 0)
> +		return 0;
> +
> +	ep = ret;
> +	if (ep == GO_S_CFG_INTF_IN)
> +		return hid_gos_cfg_suspend();
> +
> +	return 0;
> +}
> +
>  static const struct hid_device_id hid_gos_devices[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_QHE,
>  			 USB_DEVICE_ID_LENOVO_LEGION_GO_S_XINPUT) },
> @@ -1562,6 +1710,7 @@ static struct hid_driver hid_lenovo_go_s = {
>  	.remove = hid_gos_remove,
>  	.raw_event = hid_gos_raw_event,
>  	.reset_resume = pm_ptr(hid_gos_reset_resume),
> +	.suspend = pm_ptr(hid_gos_suspend),
>  };
>  module_hid_driver(hid_lenovo_go_s);

[Severity: Medium]
What happens if the device suspends before cfg_setup() finishes and then
resumes via a normal non-reset resume?

In hid_gos_cfg_probe(), drvdata.gos_cfg_setup is scheduled with a 2ms
delay. If a system suspend where USB bus power is maintained, runtime
autosuspend, or an aborted suspend rollback in usbhid occurs before
cfg_setup() sets drvdata.gp_registered and drvdata.rgb_registered,
hid_suspend() calls:

  hid_driver_suspend()
    -> hid_gos_suspend()
      -> hid_gos_cfg_suspend()
        -> cancel_delayed_work_sync(&drvdata.gos_cfg_setup)

When the device subsequently resumes without a USB port reset (or rolls
back from a failed suspend), usbhid calls:

  hid_resume()
    -> hid_resume_common()
      -> hid_driver_resume()

which invokes hdev->driver->resume rather than hdev->driver->reset_resume.

Because hid_lenovo_go_s only sets .reset_resume and leaves .resume NULL,
drvdata.gos_cfg_setup is never rescheduled on that path. Would that leave
drvdata.gp_registered and drvdata.rgb_registered permanently false (and in
the later patch "HID: hid-lenovo-go-s: Move sysfs group creation into
delayed work", prevent the sysfs groups and LED device from ever being
registered)?

Should hid_lenovo_go_s also wire up a .resume callback to reschedule
drvdata.gos_cfg_setup when gp_registered or rgb_registered is false?

титься

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