[PATCH v2 00/11] HID: hid-lenovo-go-s: Fix various bugs

[PATCH v2 00/11] HID: hid-lenovo-go-s: Fix various bugs

[Derek J. Clark]

This series fixes various bugs in the hid-lenovo-go-s driver

Patch 1 fixes a dropped error on return from mcu_property_out().

Patch 2 creates a mechanism to ignore stale responses if they timed out
in the previous command.

Patch 3 fixes a deadlock if the device is removed while cfg_setup() is
running.

Patch 4 adds missing pm_ptr to the reset_resume function pointer.

Patch 5 adds a kobject_uevent notify during reset-resume to ensure the
os_mode return is caught by udev.

Patch 6 adds a suspend callback to cancel delayed work and re-queue the
cfg_setup if the device was suspended prior to or during the work.

Patch 7 moves attribute init to the end of cfg_setup, ensuring the
attributes can only be used once the MCU is ready.

Patch 8 reorganizes hid_gos_cfg and renames it to gos_cfg_drvdata in
preparation for later fixes.

Patch 9 moves the static led_classdev to become a member of the drvdata
struct.

Patch 10 moves drvdata from a static global to a device allocated
struct.

Patch 11 stops and closes the HID device when probe fails.

Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>

---
v2:
 - Patch 2: Only mark the ack as orphaned when no reply was consumed,
   i.e. the wait timed out or was interrupted. An MCU error reply is
   fully consumed and does not need draining.
 - Patch 3: Re-take cfg_mutex in the remove path after stopping the
   setup work; the commit body documents why the re-take is safe.
 - Patch 6: Cancel instead of disable the delayed work on suspend so
   the work can be rescheduled again on reset-resume.
 - Patch 7: Bound the device query retry to 5 attempts at 50 ms. Register
   the LED classdev manually and unregister it on failure and in the
   remove path. The registered flags are set before the matching steps
   and cleared on any failure, so a failed setup run can be retried.
 - Patch 10: Fix stale work function name in the memory barrier
   pairing comments.
 - Add patch 11: Stop and close the HID device on probe failure.

v1: https://lore.kernel.org/linux-input/20260914225303.868569-1-derekjohn.clark@gmail.com/

Derek J. Clark (11):
  HID: hid-lenovo-go-s: Return ret instead of 0 in mcu_property_out()
  HID: hid-lenovo-go-s: Bound stale reply window before reusing
    send_cmd_complete
  HID: hid-lenovo-go-s: Prevent deadlock if device removed during setup
  HID: hid-lenovo-go-s: Use pm_ptr for reset_resume callback
  HID: hid-lenovo-go-s: Add kobject_uevent notify during reset-resume
  HID: hid-lenovo-go-s: Add suspend function and gate access under bool.
  HID: hid-lenovo-go-s: Move attribute init to after device query
  HID: hid-lenovo-go-s: Reorganize and rename hid_gos_cfg
  HID: hid-lenovo-go-s: Move static led_classdev_mc to drvdata struct
  HID: hid-lenovo-go-s: Use devm_kzalloc for drvdata
  HID: hid-lenovo-go-s: Stop and close device on probe failure

 drivers/hid/hid-lenovo-go-s.c | 720 +++++++++++++++++++++++++---------
 1 file changed, 526 insertions(+), 194 deletions(-)

-- 
2.55.0

[PATCH v2 01/11] HID: hid-lenovo-go-s: Return ret instead of 0 in mcu_property_out()

[Derek J. Clark]

Currently mcu_property_out() blanket returns 0, discarding the ret
value and any errors with it. Wait completion returns a positive value
when it is not timed out, which would error on all successes, so if it
is positive return the ret value from the raw event handler. Only if
it is 0 set it to -EBUSY, otherwise pass the actual error.

Fixes: a23f3497bf208c59ad ("HID: hid-lenovo-go-s: Add Lenovo Legion Go S Series HID Driver")
Cc: stable@vger.kernel.org
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index a72f7f748cb5..68301d4c762a 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -38,6 +38,7 @@ static struct hid_gos_cfg {
 	struct led_classdev *led_cdev;
 	struct hid_device *hdev;
 	struct mutex cfg_mutex; /*ensure single synchronous output report*/
+	int cmd_status;
 	u8 gp_auto_sleep_time;
 	u8 gp_dpad_mode;
 	u8 gp_mode;
@@ -435,7 +436,9 @@ static int hid_gos_raw_event(struct hid_device *hdev, struct hid_report *report,
 	dev_dbg(&hdev->dev, "Rx data as raw input report: [%*ph]\n",
 		GO_S_PACKET_SIZE, data);
 
+	drvdata.cmd_status = ret;
 	complete(&drvdata.send_cmd_complete);
+
 	return ret;
 }
 
@@ -474,12 +477,10 @@ static int mcu_property_out(struct hid_device *hdev, u8 command, u8 index,
 	timeout = (command == GET_PL_TEST) ? 200 : 5;
 	ret = wait_for_completion_interruptible_timeout(&drvdata.send_cmd_complete,
 							msecs_to_jiffies(timeout));
-
-	if (ret == 0) /* timeout occurred */
-		ret = -EBUSY;
+	ret = ret > 0 ? drvdata.cmd_status : ret ?: -EBUSY;
 
 	reinit_completion(&drvdata.send_cmd_complete);
-	return 0;
+	return ret;
 }
 
 static ssize_t gamepad_property_store(struct device *dev,
-- 
2.55.0

[PATCH v2 02/11] HID: hid-lenovo-go-s: Bound stale reply window before reusing send_cmd_complete

[Derek J. Clark]

mcu_property_out() reinits send_cmd_complete immediately after a
timeout is detected, before the next command that reuses it is sent.
If the MCU's reply to the timed-out command arrives after this reinit
but before the next command's wait begins, it silently satisfies the
next, unrelated command's wait instead of the one it actually answers,
handing that caller stale data with no way to detect the mismatch.

Track when no reply was consumed, i.e. the wait timed out or was
interrupted, via orphan_ack_pending. Before the next command reuses
the completion, wait a bounded 25ms for a stale reply to arrive and be
consumed, then unconditionally clear the flag and reinit the
completion. This does not fully eliminate the window in which an
unrelated reply could still be received, but bounds it to a short
interval right before a new command is sent. Behavior matches the
solution to the same problem in hid-msi.

Fixes: a23f3497bf208c59ad ("HID: hid-lenovo-go-s: Add Lenovo Legion Go S Series HID Driver")
Cc: stable@vger.kernel.org
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index 68301d4c762a..e979527ece8e 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -37,6 +37,7 @@ static struct hid_gos_cfg {
 	struct completion send_cmd_complete;
 	struct led_classdev *led_cdev;
 	struct hid_device *hdev;
+	bool orphan_ack_pending;
 	struct mutex cfg_mutex; /*ensure single synchronous output report*/
 	int cmd_status;
 	u8 gp_auto_sleep_time;
@@ -448,12 +449,26 @@ static int mcu_property_out(struct hid_device *hdev, u8 command, u8 index,
 	unsigned char *dmabuf __free(kfree) = NULL;
 	u8 header[] = { command, index };
 	size_t header_size = ARRAY_SIZE(header);
+	long t_remaining;
 	int timeout, ret;
 
 	if (header_size + len > GO_S_PACKET_SIZE)
 		return -EINVAL;
 
 	guard(mutex)(&drvdata.cfg_mutex);
+
+	/*
+	 * A reply to the previous command may still be in flight. Give it a
+	 * short window to arrive and be consumed before this call reinits the
+	 * completion, so a late reply can't be mistaken for this command's.
+	 */
+	if (drvdata.orphan_ack_pending) {
+		wait_for_completion_timeout(&drvdata.send_cmd_complete, msecs_to_jiffies(25));
+		drvdata.orphan_ack_pending = false;
+		drvdata.cmd_status = -ETIMEDOUT;
+	}
+	reinit_completion(&drvdata.send_cmd_complete);
+
 	/* We can't use a devm_alloc reusable buffer without side effects during suspend */
 	dmabuf = kzalloc(GO_S_PACKET_SIZE, GFP_KERNEL);
 	if (!dmabuf)
@@ -475,11 +490,15 @@ static int mcu_property_out(struct hid_device *hdev, u8 command, u8 index,
 
 	/* PL_TEST commands can take longer because they go out to another device */
 	timeout = (command == GET_PL_TEST) ? 200 : 5;
-	ret = wait_for_completion_interruptible_timeout(&drvdata.send_cmd_complete,
-							msecs_to_jiffies(timeout));
-	ret = ret > 0 ? drvdata.cmd_status : ret ?: -EBUSY;
+	t_remaining = wait_for_completion_interruptible_timeout(&drvdata.send_cmd_complete,
+								msecs_to_jiffies(timeout));
+	if (t_remaining <= 0) {
+		/* A reply to this command may still be in flight. */
+		drvdata.orphan_ack_pending = true;
+	}
+
+	ret = t_remaining > 0 ? drvdata.cmd_status : t_remaining ?: -EBUSY;
 
-	reinit_completion(&drvdata.send_cmd_complete);
 	return ret;
 }
 
-- 
2.55.0

[PATCH v2 03/11] HID: hid-lenovo-go-s: Prevent deadlock if device removed during setup

[Derek J. Clark]

hid_gos_cfg_remove() calls a mutex before canceling delayed work. That
delayed work function calls mcu_property_out(), which holds the same mutex.
Prevent deadlock by removing the mutex. Additionally, since
cancel_delayed_work has the potential to re-arm, switch to
disable_delayed_work.

Retaking the mutex before removing attributes could deadlock on store()
as well, so don't retake the mutex.

Fixes: a23f3497bf208c59ad ("HID: hid-lenovo-go-s: Add Lenovo Legion Go S Series HID Driver")
Cc: stable@vger.kernel.org
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index e979527ece8e..b27e82ec9b76 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -1445,9 +1445,9 @@ static int hid_gos_cfg_probe(struct hid_device *hdev,
 
 static void hid_gos_cfg_remove(struct hid_device *hdev)
 {
-	guard(mutex)(&drvdata.cfg_mutex);
-	cancel_delayed_work_sync(&drvdata.gos_cfg_setup);
+	disable_delayed_work_sync(&drvdata.gos_cfg_setup);
 	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);
+
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
 	hid_set_drvdata(hdev, NULL);
-- 
2.55.0

[PATCH v2 04/11] HID: hid-lenovo-go-s: Use pm_ptr for reset_resume callback

[Derek J. Clark]

Adds missing pm_ptr to the .reset_resume callback for hid-lenovo-go-s

Fixes: 3524900cc571bd922a ("HID: hid-lenovo-go-s: restore OS_TYPE after resume from s2idle")
Cc: stable@vger.kernel.org
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index b27e82ec9b76..a16733edad5d 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -1560,7 +1560,7 @@ static struct hid_driver hid_lenovo_go_s = {
 	.probe = hid_gos_probe,
 	.remove = hid_gos_remove,
 	.raw_event = hid_gos_raw_event,
-	.reset_resume = hid_gos_reset_resume,
+	.reset_resume = pm_ptr(hid_gos_reset_resume),
 };
 module_hid_driver(hid_lenovo_go_s);
 
-- 
2.55.0

[PATCH v2 05/11] HID: hid-lenovo-go-s: Add kobject_uevent notify during reset-resume

[Derek J. Clark]

Adds kobject_uevent notification after setting os_mode attribute on resume.

Fixes: 3524900cc571bd922a ("HID: hid-lenovo-go-s: restore OS_TYPE after resume from s2idle")
Cc: stable@vger.kernel.org
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index a16733edad5d..ed08200e3571 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -1471,6 +1471,7 @@ static int hid_gos_cfg_reset_resume(struct hid_device *hdev)
 	if (drvdata.os_mode != os_mode)
 		return -ENODEV;
 
+	kobject_uevent(&drvdata.hdev->dev.kobj, KOBJ_CHANGE);
 	return 0;
 }
 
-- 
2.55.0

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

[Derek J. Clark]

Currently a device can suspend while delayed work is pending or
executing. Cancel delayed work if it was pending and restart it if it
hasn't run yet. Gate access to show/store functions behind a bool to
prevent access prior to cfg_setup() being completed.

Fixes: a23f3497bf208c59ad ("HID: hid-lenovo-go-s: Add Lenovo Legion Go S Series HID Driver")
Cc: stable@vger.kernel.org
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 149 ++++++++++++++++++++++++++++++++++
 1 file changed, 149 insertions(+)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index ed08200e3571..14aeddd65704 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -39,6 +39,8 @@ static struct hid_gos_cfg {
 	struct hid_device *hdev;
 	bool orphan_ack_pending;
 	struct mutex cfg_mutex; /*ensure single synchronous output report*/
+	bool gp_registered;
+	bool rgb_registered;
 	int cmd_status;
 	u8 gp_auto_sleep_time;
 	u8 gp_dpad_mode;
@@ -507,10 +509,22 @@ static ssize_t gamepad_property_store(struct device *dev,
 				      const char *buf, size_t count,
 				      enum feature_status_index index)
 {
+	bool dev_registered;
 	size_t size = 1;
 	u8 val = 0;
 	int ret;
 
+	/* rgb_enabled is attached to led_classdev, not hid_device */
+	if (index == FEATURE_RGB_ENABLE)
+		/* Pairs with smp_store_release from gos_cfg_setup */
+		dev_registered = smp_load_acquire(&drvdata.rgb_registered);
+	else
+		/* Pairs with smp_store_release from gos_cfg_setup */
+		dev_registered = smp_load_acquire(&drvdata.gp_registered);
+
+	if (!dev_registered)
+		return -ENODEV;
+
 	switch (index) {
 	case FEATURE_GAMEPAD_MODE:
 		ret = sysfs_match_string(gamepad_mode_text, buf);
@@ -591,9 +605,21 @@ static ssize_t gamepad_property_show(struct device *dev,
 				     struct device_attribute *attr, char *buf,
 				     enum feature_status_index index)
 {
+	bool dev_registered;
 	ssize_t count = 0;
 	u8 i;
 
+	/* rgb_enabled is attached to led_classdev, not hid_device */
+	if (index == FEATURE_RGB_ENABLE)
+		/* Pairs with smp_store_release from gos_cfg_setup */
+		dev_registered = smp_load_acquire(&drvdata.rgb_registered);
+	else
+		/* Pairs with smp_store_release from gos_cfg_setup */
+		dev_registered = smp_load_acquire(&drvdata.gp_registered);
+
+	if (!dev_registered)
+		return -ENODEV;
+
 	count = mcu_property_out(drvdata.hdev, GET_GAMEPAD_CFG, index, NULL, 0);
 	if (count < 0)
 		return count;
@@ -729,10 +755,16 @@ static ssize_t touchpad_property_store(struct device *dev,
 				       const char *buf, size_t count,
 				       enum touchpad_config_index index)
 {
+	bool gp_registered;
 	size_t size = 1;
 	u8 val = 0;
 	int ret;
 
+	/* Pairs with smp_store_release from gos_cfg_setup */
+	gp_registered = smp_load_acquire(&drvdata.gp_registered);
+	if (!gp_registered)
+		return -ENODEV;
+
 	switch (index) {
 	case CFG_WINDOWS_MODE:
 		ret = sysfs_match_string(touchpad_mode_text, buf);
@@ -763,9 +795,15 @@ static ssize_t touchpad_property_show(struct device *dev,
 				      struct device_attribute *attr, char *buf,
 				      enum touchpad_config_index index)
 {
+	bool gp_registered;
 	int ret = 0;
 	u8 i;
 
+	/* Pairs with smp_store_release from gos_cfg_setup */
+	gp_registered = smp_load_acquire(&drvdata.gp_registered);
+	if (!gp_registered)
+		return -ENODEV;
+
 	ret = mcu_property_out(drvdata.hdev, GET_TP_PARAM, index, NULL, 0);
 	if (ret < 0)
 		return ret;
@@ -792,9 +830,15 @@ static ssize_t touchpad_property_options(struct device *dev,
 					 char *buf,
 					 enum touchpad_config_index index)
 {
+	bool gp_registered;
 	size_t count = 0;
 	unsigned int i;
 
+	/* Pairs with smp_store_release from gos_cfg_setup */
+	gp_registered = smp_load_acquire(&drvdata.gp_registered);
+	if (!gp_registered)
+		return -ENODEV;
+
 	switch (index) {
 	case CFG_WINDOWS_MODE:
 	case CFG_LINUX_MODE:
@@ -817,9 +861,15 @@ static ssize_t test_property_show(struct device *dev,
 				  struct device_attribute *attr, char *buf,
 				  enum test_command_index index)
 {
+	bool gp_registered;
 	size_t count = 0;
 	u8 i;
 
+	/* Pairs with smp_store_release from gos_cfg_setup */
+	gp_registered = smp_load_acquire(&drvdata.gp_registered);
+	if (!gp_registered)
+		return -ENODEV;
+
 	switch (index) {
 	case TEST_TP_MFR:
 		i = drvdata.tp_manufacturer;
@@ -877,9 +927,15 @@ static ssize_t rgb_effect_store(struct device *dev,
 {
 	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(drvdata.led_cdev);
 	enum rgb_config_index index;
+	bool rgb_registered;
 	u8 effect;
 	int ret;
 
+	/* Pairs with smp_store_release from gos_cfg_setup */
+	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
+	if (!rgb_registered)
+		return -ENODEV;
+
 	ret = sysfs_match_string(rgb_effect_text, buf);
 	if (ret < 0)
 		return ret;
@@ -904,8 +960,14 @@ static ssize_t rgb_effect_store(struct device *dev,
 static ssize_t rgb_effect_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
+	bool rgb_registered;
 	int ret;
 
+	/* Pairs with smp_store_release from gos_cfg_setup */
+	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
+	if (!rgb_registered)
+		return -ENODEV;
+
 	ret = rgb_attr_show();
 	if (ret)
 		return ret;
@@ -937,9 +999,15 @@ static ssize_t rgb_speed_store(struct device *dev,
 {
 	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(drvdata.led_cdev);
 	enum rgb_config_index index;
+	bool rgb_registered;
 	int val = 0;
 	int ret;
 
+	/* Pairs with smp_store_release from gos_cfg_setup */
+	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
+	if (!rgb_registered)
+		return -ENODEV;
+
 	ret = kstrtoint(buf, 10, &val);
 	if (ret)
 		return ret;
@@ -967,8 +1035,14 @@ static ssize_t rgb_speed_store(struct device *dev,
 static ssize_t rgb_speed_show(struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
+	bool rgb_registered;
 	int ret;
 
+	/* Pairs with smp_store_release from gos_cfg_setup */
+	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
+	if (!rgb_registered)
+		return -ENODEV;
+
 	ret = rgb_attr_show();
 	if (ret)
 		return ret;
@@ -988,9 +1062,15 @@ static ssize_t rgb_speed_range_show(struct device *dev,
 static ssize_t rgb_mode_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
 {
+	bool rgb_registered;
 	int ret;
 	u8 val;
 
+	/* Pairs with smp_store_release from gos_cfg_setup */
+	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
+	if (!rgb_registered)
+		return -ENODEV;
+
 	ret = sysfs_match_string(rgb_mode_text, buf);
 	if (ret <= 0)
 		return ret;
@@ -1010,8 +1090,14 @@ static ssize_t rgb_mode_store(struct device *dev, struct device_attribute *attr,
 static ssize_t rgb_mode_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
+	bool rgb_registered;
 	int ret;
 
+	/* Pairs with smp_store_release from gos_cfg_setup */
+	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
+	if (!rgb_registered)
+		return -ENODEV;
+
 	ret = rgb_cfg_call(drvdata.hdev, GET_RGB_CFG, LIGHT_MODE_SEL, NULL, 0);
 	if (ret)
 		return ret;
@@ -1041,10 +1127,16 @@ static ssize_t rgb_profile_store(struct device *dev,
 				 struct device_attribute *attr, const char *buf,
 				 size_t count)
 {
+	bool rgb_registered;
 	size_t size = 1;
 	int ret;
 	u8 val;
 
+	/* Pairs with smp_store_release from gos_cfg_setup */
+	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
+	if (!rgb_registered)
+		return -ENODEV;
+
 	ret = kstrtou8(buf, 10, &val);
 	if (ret < 0)
 		return ret;
@@ -1064,8 +1156,14 @@ static ssize_t rgb_profile_store(struct device *dev,
 static ssize_t rgb_profile_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
+	bool rgb_registered;
 	int ret;
 
+	/* Pairs with smp_store_release from gos_cfg_setup */
+	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
+	if (!rgb_registered)
+		return -ENODEV;
+
 	ret = rgb_cfg_call(drvdata.hdev, GET_RGB_CFG, LIGHT_PROFILE_SEL, NULL, 0);
 	if (ret)
 		return ret;
@@ -1087,8 +1185,14 @@ static void hid_gos_brightness_set(struct led_classdev *led_cdev,
 {
 	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(drvdata.led_cdev);
 	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);
+	if (!rgb_registered)
+		return;
+
 	if (brightness > led_cdev->max_brightness) {
 		dev_err(led_cdev->dev, "Invalid argument\n");
 		return;
@@ -1395,6 +1499,11 @@ static void cfg_setup(struct work_struct *work)
 			"Failed to retrieve OS Mode: %i\n", ret);
 		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);
 }
 
 static int hid_gos_cfg_probe(struct hid_device *hdev,
@@ -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);
 
-- 
2.55.0

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

[Derek J. Clark]

Currently this driver registers attributes before the MCU is ready to
accept commands. This 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. Gate creation to drvdata.gp_registered and
drvdata.rgb_registered so a resume before setup has run doesn't prevent
attribute creation or attempt to re-register the same attributes.

Query failures reschedule the setup work, up to 5 attempts 50 ms apart,
since the MCU may not be ready to accept commands immediately after
probe.

The registered flags are set before the matching registration steps and
cleared on failure, so a failed run leaves no state that blocks a later
run from retrying. The LED classdev is registered without devres because
the failure and remove paths unregister it manually.

Fixes: a23f3497bf208c59ad ("HID: hid-lenovo-go-s: Add Lenovo Legion Go S Series HID Driver")
Cc: stable@vger.kernel.org
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 134 ++++++++++++++++++++++++----------
 1 file changed, 95 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index 14aeddd65704..cefb243800b6 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 = {
 	.subled_info = gos_rgb_subled_info,
 };
 
-static void cfg_setup(struct work_struct *work)
+static int cfg_query_device(void)
 {
 	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;
+		dev_err(&drvdata.hdev->dev, "Failed to retrieve MCU ID: %i\n", ret);
+		return ret;
 	}
 
 	ret = mcu_property_out(drvdata.hdev, GET_VERSION, FEATURE_NONE, NULL, 0);
 	if (ret) {
 		dev_err(&drvdata.hdev->dev, "Failed to retrieve MCU Version: %i\n", ret);
-		return;
+		return ret;
 	}
 
 	ret = mcu_property_out(drvdata.hdev, GET_PL_TEST, TEST_TP_MFR, NULL, 0);
 	if (ret) {
 		dev_err(&drvdata.hdev->dev,
 			"Failed to retrieve Touchpad Manufacturer: %i\n", ret);
-		return;
+		return ret;
 	}
 
 	ret = mcu_property_out(drvdata.hdev, GET_PL_TEST, TEST_TP_VER, NULL, 0);
 	if (ret) {
 		dev_err(&drvdata.hdev->dev,
 			"Failed to retrieve Touchpad Firmware Version: %i\n", ret);
-		return;
+		return ret;
 	}
 
 	ret = mcu_property_out(drvdata.hdev, GET_PL_TEST, TEST_IMU_MFR, NULL, 0);
 	if (ret) {
 		dev_err(&drvdata.hdev->dev,
 			"Failed to retrieve IMU Manufacturer: %i\n", ret);
-		return;
+		return ret;
 	}
 
-	ret = mcu_property_out(drvdata.hdev, GET_GAMEPAD_CFG, FEATURE_OS_MODE,
-			       NULL, 0);
+	return mcu_property_out(drvdata.hdev, GET_GAMEPAD_CFG, FEATURE_OS_MODE,
+				 NULL, 0);
+}
+
+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;
+
+	if (!drvdata.gp_registered) {
+		/* Pairs with smp_load_acquire in attribute show/store functions */
+		smp_store_release(&drvdata.gp_registered, true);
+		gp_registered = true;
+
+		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);
+
+			/* Pairs with smp_load_acquire in attribute show/store functions */
+			smp_store_release(&drvdata.gp_registered, false);
+			gp_registered = false;
+		}
+	}
+
+	if (!drvdata.rgb_registered) {
+		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;
+
+		ret = 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);
+
+			/* Pairs with smp_load_acquire in attribute show/store functions */
+			smp_store_release(&drvdata.rgb_registered, false);
+			rgb_registered = false;
+		} else {
+			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);
+
+				led_classdev_multicolor_unregister(&gos_cdev_rgb);
+
+				/* Pairs with smp_load_acquire in attribute show/store functions */
+				smp_store_release(&drvdata.rgb_registered, false);
+				rgb_registered = false;
+			}
+		}
+	}
+
+	if (gp_registered)
+		kobject_uevent(&drvdata.hdev->dev.kobj, KOBJ_CHANGE);
+	if (rgb_registered)
+		kobject_uevent(&drvdata.led_cdev->dev->kobj, KOBJ_CHANGE);
 }
 
 static int hid_gos_cfg_probe(struct hid_device *hdev,
@@ -1513,30 +1584,8 @@ static int hid_gos_cfg_probe(struct hid_device *hdev,
 
 	hid_set_drvdata(hdev, &drvdata);
 	drvdata.hdev = hdev;
-	mutex_init(&drvdata.cfg_mutex);
-
-	ret = sysfs_create_groups(&hdev->dev.kobj, top_level_attr_groups);
-	if (ret) {
-		dev_err_probe(&hdev->dev, ret,
-			      "Failed to create gamepad configuration attributes\n");
-		return ret;
-	}
-
-	ret = devm_led_classdev_multicolor_register(&hdev->dev, &gos_cdev_rgb);
-	if (ret) {
-		dev_err_probe(&hdev->dev, ret, "Failed to create RGB device\n");
-		return ret;
-	}
-
-	ret = devm_device_add_group(gos_cdev_rgb.led_cdev.dev, &rgb_attr_group);
-	if (ret) {
-		dev_err_probe(&hdev->dev, ret,
-			      "Failed to create RGB configuration attributes\n");
-		return ret;
-	}
-
-	drvdata.led_cdev = &gos_cdev_rgb.led_cdev;
 
+	mutex_init(&drvdata.cfg_mutex);
 	init_completion(&drvdata.send_cmd_complete);
 
 	/* Executing calls prior to returning from probe will lock the MCU. Schedule
@@ -1554,9 +1603,16 @@ static int hid_gos_cfg_probe(struct hid_device *hdev,
 
 static void hid_gos_cfg_remove(struct hid_device *hdev)
 {
+	bool rgb_registered;
+
 	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);
+	if (rgb_registered)
+		led_classdev_multicolor_unregister(&gos_cdev_rgb);
+
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
 	hid_set_drvdata(hdev, NULL);
-- 
2.55.0

[PATCH v2 08/11] HID: hid-lenovo-go-s: Reorganize and rename hid_gos_cfg

[Derek J. Clark]

Renames hid_gos_cfg struct to gos_cfg_drvdata in preparation for later
fixes.

This is a prerequisite for later fixes. No functional change intended.

Fixes: a23f3497bf208c59ad ("HID: hid-lenovo-go-s: Add Lenovo Legion Go S Series HID Driver")
Cc: stable@vger.kernel.org
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 39 ++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index cefb243800b6..136372eb6e6b 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -32,37 +32,42 @@
 #define GO_S_CFG_INTF_IN	0x84
 #define GO_S_PACKET_SIZE	64
 
-static struct hid_gos_cfg {
-	struct delayed_work gos_cfg_setup;
+static struct gos_cfg_drvdata {
+	/* MCU General Variables */
 	struct completion send_cmd_complete;
-	struct led_classdev *led_cdev;
+	struct delayed_work gos_cfg_setup;
 	struct hid_device *hdev;
 	bool orphan_ack_pending;
 	struct mutex cfg_mutex; /*ensure single synchronous output report*/
-	bool gp_registered;
-	bool rgb_registered;
 	int cmd_status;
 	int setup_attempts;
+
+	/* Gamepad Variables */
 	u8 gp_auto_sleep_time;
-	u8 gp_dpad_mode;
-	u8 gp_mode;
-	u8 gp_poll_rate;
-	u8 imu_bypass_en;
 	u8 imu_manufacturer;
+	bool gp_registered;
+	u8 tp_windows_mode;
+	u8 tp_manufacturer;
+	u8 tp_linux_mode;
+	u8 imu_bypass_en;
 	u8 imu_sensor_en;
+	u8 gp_dpad_mode;
+	u8 gp_poll_rate;
 	u8 mcu_id[12];
 	u8 mouse_step;
+	u8 tp_version;
+	u8 gp_mode;
 	u8 os_mode;
-	u8 rgb_effect;
-	u8 rgb_en;
-	u8 rgb_mode;
+	u8 tp_en;
+
+	/* RGB Variables */
+	struct led_classdev *led_cdev;
+	bool rgb_registered;
 	u8 rgb_profile;
+	u8 rgb_effect;
 	u8 rgb_speed;
-	u8 tp_en;
-	u8 tp_linux_mode;
-	u8 tp_windows_mode;
-	u8 tp_version;
-	u8 tp_manufacturer;
+	u8 rgb_mode;
+	u8 rgb_en;
 } drvdata;
 
 struct gos_cfg_attr {
-- 
2.55.0

[PATCH v2 09/11] HID: hid-lenovo-go-s: Move static led_classdev_mc to drvdata struct

[Derek J. Clark]

In the next patch I will switch drvdata from static global to a
devm_kzalloc struct. In preparation for that, move the led_classdev_mc
to be a member of the gos_cfg_drvdata struct. As part of that effort,
provide a constant name when accessing it and access the led_classdev
through the led_classdev_mc.

Fixes: be6d7dbb368ba840d7 ("HID: hid-lenovo-go-s: Add RGB LED control interface")
Cc: stable@vger.kernel.org
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 77 +++++++++++++++++------------------
 1 file changed, 38 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index 136372eb6e6b..2e938dd5e77c 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -61,7 +61,7 @@ static struct gos_cfg_drvdata {
 	u8 tp_en;
 
 	/* RGB Variables */
-	struct led_classdev *led_cdev;
+	struct led_classdev_mc led_mc;
 	bool rgb_registered;
 	u8 rgb_profile;
 	u8 rgb_effect;
@@ -348,7 +348,7 @@ static int hid_gos_pl_test_event(struct command_report *cmd_rep)
 
 static int hid_gos_light_event(struct command_report *cmd_rep)
 {
-	struct led_classdev_mc *mc_cdev;
+	struct led_classdev_mc *led_mc;
 	int ret = 0;
 
 	switch (cmd_rep->sub_cmd) {
@@ -363,12 +363,12 @@ static int hid_gos_light_event(struct command_report *cmd_rep)
 	case USR_LIGHT_PROFILE_1:
 	case USR_LIGHT_PROFILE_2:
 	case USR_LIGHT_PROFILE_3:
-		mc_cdev = lcdev_to_mccdev(drvdata.led_cdev);
+		led_mc = &drvdata.led_mc;
 		drvdata.rgb_effect = cmd_rep->data[0];
-		mc_cdev->subled_info[0].intensity = cmd_rep->data[1];
-		mc_cdev->subled_info[1].intensity = cmd_rep->data[2];
-		mc_cdev->subled_info[2].intensity = cmd_rep->data[3];
-		drvdata.led_cdev->brightness = cmd_rep->data[4];
+		led_mc->subled_info[0].intensity = cmd_rep->data[1];
+		led_mc->subled_info[1].intensity = cmd_rep->data[2];
+		led_mc->subled_info[2].intensity = cmd_rep->data[3];
+		led_mc->led_cdev.brightness = cmd_rep->data[4];
 		drvdata.rgb_speed = cmd_rep->data[5];
 		ret = 0;
 		break;
@@ -931,7 +931,7 @@ static ssize_t rgb_effect_store(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)
 {
-	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(drvdata.led_cdev);
+	struct led_classdev_mc *led_mc = &drvdata.led_mc;
 	enum rgb_config_index index;
 	bool rgb_registered;
 	u8 effect;
@@ -949,10 +949,10 @@ static ssize_t rgb_effect_store(struct device *dev,
 	effect = ret;
 	index = drvdata.rgb_profile + 2;
 	u8 rgb_profile[6] = { effect,
-			      mc_cdev->subled_info[0].intensity,
-			      mc_cdev->subled_info[1].intensity,
-			      mc_cdev->subled_info[2].intensity,
-			      drvdata.led_cdev->brightness,
+			      led_mc->subled_info[0].intensity,
+			      led_mc->subled_info[1].intensity,
+			      led_mc->subled_info[2].intensity,
+			      led_mc->led_cdev.brightness,
 			      drvdata.rgb_speed };
 
 	ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, index, rgb_profile, 6);
@@ -1003,7 +1003,7 @@ static ssize_t rgb_speed_store(struct device *dev,
 			       struct device_attribute *attr, const char *buf,
 			       size_t count)
 {
-	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(drvdata.led_cdev);
+	struct led_classdev_mc *led_mc = &drvdata.led_mc;
 	enum rgb_config_index index;
 	bool rgb_registered;
 	int val = 0;
@@ -1023,10 +1023,10 @@ static ssize_t rgb_speed_store(struct device *dev,
 
 	index = drvdata.rgb_profile + 2;
 	u8 rgb_profile[6] = { drvdata.rgb_effect,
-			      mc_cdev->subled_info[0].intensity,
-			      mc_cdev->subled_info[1].intensity,
-			      mc_cdev->subled_info[2].intensity,
-			      drvdata.led_cdev->brightness,
+			      led_mc->subled_info[0].intensity,
+			      led_mc->subled_info[1].intensity,
+			      led_mc->subled_info[2].intensity,
+			      led_mc->led_cdev.brightness,
 			      val };
 
 	ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, index, rgb_profile, 6);
@@ -1189,7 +1189,7 @@ 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 *mc_cdev = lcdev_to_mccdev(drvdata.led_cdev);
+	struct led_classdev_mc *led_mc = &drvdata.led_mc;
 	enum rgb_config_index index;
 	bool rgb_registered;
 	int ret;
@@ -1206,9 +1206,9 @@ static void hid_gos_brightness_set(struct led_classdev *led_cdev,
 
 	index = drvdata.rgb_profile + 2;
 	u8 rgb_profile[6] = { drvdata.rgb_effect,
-			      mc_cdev->subled_info[0].intensity,
-			      mc_cdev->subled_info[1].intensity,
-			      mc_cdev->subled_info[2].intensity,
+			      led_mc->subled_info[0].intensity,
+			      led_mc->subled_info[1].intensity,
+			      led_mc->subled_info[2].intensity,
 			      brightness,
 			      drvdata.rgb_speed };
 
@@ -1448,17 +1448,6 @@ static struct mc_subled gos_rgb_subled_info[] = {
 	},
 };
 
-static struct led_classdev_mc gos_cdev_rgb = {
-	.led_cdev = {
-		.name = "go_s:rgb:joystick_rings",
-		.brightness = 0x50,
-		.max_brightness = 0x64,
-		.brightness_set = hid_gos_brightness_set,
-	},
-	.num_colors = ARRAY_SIZE(gos_rgb_subled_info),
-	.subled_info = gos_rgb_subled_info,
-};
-
 static int cfg_query_device(void)
 {
 	int ret;
@@ -1544,14 +1533,12 @@ static void cfg_setup(struct work_struct *work)
 	}
 
 	if (!drvdata.rgb_registered) {
-		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;
 
 		ret = led_classdev_multicolor_register(&drvdata.hdev->dev,
-						       &gos_cdev_rgb);
+						       &drvdata.led_mc);
 		if (ret) {
 			dev_err(&drvdata.hdev->dev, "Failed to create RGB device: %i\n",
 				ret);
@@ -1560,14 +1547,14 @@ static void cfg_setup(struct work_struct *work)
 			smp_store_release(&drvdata.rgb_registered, false);
 			rgb_registered = false;
 		} else {
-			ret = devm_device_add_group(gos_cdev_rgb.led_cdev.dev,
+			ret = devm_device_add_group(drvdata.led_mc.led_cdev.dev,
 						    &rgb_attr_group);
 			if (ret) {
 				dev_err(&drvdata.hdev->dev,
 					"Failed to create RGB configuration attributes: %i\n",
 					ret);
 
-				led_classdev_multicolor_unregister(&gos_cdev_rgb);
+				led_classdev_multicolor_unregister(&drvdata.led_mc);
 
 				/* Pairs with smp_load_acquire in attribute show/store functions */
 				smp_store_release(&drvdata.rgb_registered, false);
@@ -1579,7 +1566,7 @@ static void cfg_setup(struct work_struct *work)
 	if (gp_registered)
 		kobject_uevent(&drvdata.hdev->dev.kobj, KOBJ_CHANGE);
 	if (rgb_registered)
-		kobject_uevent(&drvdata.led_cdev->dev->kobj, KOBJ_CHANGE);
+		kobject_uevent(&drvdata.led_mc.led_cdev.dev->kobj, KOBJ_CHANGE);
 }
 
 static int hid_gos_cfg_probe(struct hid_device *hdev,
@@ -1593,6 +1580,18 @@ static int hid_gos_cfg_probe(struct hid_device *hdev,
 	mutex_init(&drvdata.cfg_mutex);
 	init_completion(&drvdata.send_cmd_complete);
 
+	/* Device is hardwired and name is guaranteed to be unique */
+	drvdata.led_mc.led_cdev.name = "go_s:rgb:joystick_rings";
+	drvdata.led_mc.led_cdev.brightness = 0x50;
+	drvdata.led_mc.led_cdev.max_brightness = 0x64;
+	drvdata.led_mc.led_cdev.color = LED_COLOR_ID_RGB;
+	drvdata.led_mc.led_cdev.brightness_set = hid_gos_brightness_set;
+	drvdata.led_mc.num_colors = 3;
+	drvdata.led_mc.subled_info = devm_kmemdup(&hdev->dev, gos_rgb_subled_info,
+						  sizeof(gos_rgb_subled_info), GFP_KERNEL);
+	if (!drvdata.led_mc.subled_info)
+		return -ENOMEM;
+
 	/* Executing calls prior to returning from probe will lock the MCU. Schedule
 	 * initial data call after probe has completed and MCU can accept calls.
 	 */
@@ -1616,7 +1615,7 @@ static void hid_gos_cfg_remove(struct hid_device *hdev)
 	/* Pairs with smp_store_release in cfg_setup */
 	rgb_registered = smp_load_acquire(&drvdata.rgb_registered);
 	if (rgb_registered)
-		led_classdev_multicolor_unregister(&gos_cdev_rgb);
+		led_classdev_multicolor_unregister(&drvdata.led_mc);
 
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
-- 
2.55.0

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

[Derek J. Clark]

Changes drvdata from static global to devm allocation per device.

Also fix stale references to the setup work function name in the
memory barrier pairing comments.

Fixes: a23f3497bf208c59ad ("HID: hid-lenovo-go-s: Add Lenovo Legion Go S Series HID Driver")
Cc: stable@vger.kernel.org
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 532 ++++++++++++++++++++--------------
 1 file changed, 315 insertions(+), 217 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index 2e938dd5e77c..1a2b28e1bed1 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -10,6 +10,7 @@
 #include <linux/array_size.h>
 #include <linux/cleanup.h>
 #include <linux/completion.h>
+#include <linux/container_of.h>
 #include <linux/delay.h>
 #include <linux/dev_printk.h>
 #include <linux/device.h>
@@ -32,7 +33,7 @@
 #define GO_S_CFG_INTF_IN	0x84
 #define GO_S_PACKET_SIZE	64
 
-static struct gos_cfg_drvdata {
+struct gos_cfg_drvdata {
 	/* MCU General Variables */
 	struct completion send_cmd_complete;
 	struct delayed_work gos_cfg_setup;
@@ -68,7 +69,7 @@ static struct gos_cfg_drvdata {
 	u8 rgb_speed;
 	u8 rgb_mode;
 	u8 rgb_en;
-} drvdata;
+};
 
 struct gos_cfg_attr {
 	u8 index;
@@ -244,56 +245,57 @@ static const char *const imu_manufacturer_text[] = {
 	[IMU_ST] = "ST",
 };
 
-static int hid_gos_version_event(u8 *data)
+static int hid_gos_version_event(struct gos_cfg_drvdata *drvdata, u8 *data)
 {
 	struct version_report *ver_rep = (struct version_report *)data;
 
-	drvdata.hdev->firmware_version = get_unaligned_le32(&ver_rep->version);
+	drvdata->hdev->firmware_version = get_unaligned_le32(&ver_rep->version);
 	return 0;
 }
 
-static int hid_gos_mcu_id_event(struct command_report *cmd_rep)
+static int hid_gos_mcu_id_event(struct gos_cfg_drvdata *drvdata, struct command_report *cmd_rep)
 {
-	drvdata.mcu_id[0] = cmd_rep->sub_cmd;
-	memcpy(&drvdata.mcu_id[1], cmd_rep->data, 11);
+	drvdata->mcu_id[0] = cmd_rep->sub_cmd;
+	memcpy(&drvdata->mcu_id[1], cmd_rep->data, 11);
 
 	return 0;
 }
 
-static int hid_gos_gamepad_cfg_event(struct command_report *cmd_rep)
+static int hid_gos_gamepad_cfg_event(struct gos_cfg_drvdata *drvdata,
+				     struct command_report *cmd_rep)
 {
 	int ret = 0;
 
 	switch (cmd_rep->sub_cmd) {
 	case FEATURE_GAMEPAD_MODE:
-		drvdata.gp_mode = cmd_rep->data[0];
+		drvdata->gp_mode = cmd_rep->data[0];
 		break;
 	case FEATURE_AUTO_SLEEP_TIME:
-		drvdata.gp_auto_sleep_time = cmd_rep->data[0];
+		drvdata->gp_auto_sleep_time = cmd_rep->data[0];
 		break;
 	case FEATURE_IMU_BYPASS:
-		drvdata.imu_bypass_en = cmd_rep->data[0];
+		drvdata->imu_bypass_en = cmd_rep->data[0];
 		break;
 	case FEATURE_RGB_ENABLE:
-		drvdata.rgb_en = cmd_rep->data[0];
+		drvdata->rgb_en = cmd_rep->data[0];
 		break;
 	case FEATURE_IMU_ENABLE:
-		drvdata.imu_sensor_en = cmd_rep->data[0];
+		drvdata->imu_sensor_en = cmd_rep->data[0];
 		break;
 	case FEATURE_TOUCHPAD_ENABLE:
-		drvdata.tp_en = cmd_rep->data[0];
+		drvdata->tp_en = cmd_rep->data[0];
 		break;
 	case FEATURE_OS_MODE:
-		drvdata.os_mode = cmd_rep->data[0];
+		drvdata->os_mode = cmd_rep->data[0];
 		break;
 	case FEATURE_POLL_RATE:
-		drvdata.gp_poll_rate = cmd_rep->data[0];
+		drvdata->gp_poll_rate = cmd_rep->data[0];
 		break;
 	case FEATURE_DPAD_MODE:
-		drvdata.gp_dpad_mode = cmd_rep->data[0];
+		drvdata->gp_dpad_mode = cmd_rep->data[0];
 		break;
 	case FEATURE_MOUSE_WHEEL_STEP:
-		drvdata.mouse_step = cmd_rep->data[0];
+		drvdata->mouse_step = cmd_rep->data[0];
 		break;
 	default:
 		ret = -EINVAL;
@@ -303,16 +305,16 @@ static int hid_gos_gamepad_cfg_event(struct command_report *cmd_rep)
 	return ret;
 }
 
-static int hid_gos_touchpad_event(struct command_report *cmd_rep)
+static int hid_gos_touchpad_event(struct gos_cfg_drvdata *drvdata, struct command_report *cmd_rep)
 {
 	int ret = 0;
 
 	switch (cmd_rep->sub_cmd) {
 	case CFG_LINUX_MODE:
-		drvdata.tp_linux_mode = cmd_rep->data[0];
+		drvdata->tp_linux_mode = cmd_rep->data[0];
 		break;
 	case CFG_WINDOWS_MODE:
-		drvdata.tp_windows_mode = cmd_rep->data[0];
+		drvdata->tp_windows_mode = cmd_rep->data[0];
 		break;
 	default:
 		ret = -EINVAL;
@@ -322,21 +324,21 @@ static int hid_gos_touchpad_event(struct command_report *cmd_rep)
 	return ret;
 }
 
-static int hid_gos_pl_test_event(struct command_report *cmd_rep)
+static int hid_gos_pl_test_event(struct gos_cfg_drvdata *drvdata, struct command_report *cmd_rep)
 {
 	int ret = 0;
 
 	switch (cmd_rep->sub_cmd) {
 	case TEST_TP_MFR:
-		drvdata.tp_manufacturer = cmd_rep->data[0];
+		drvdata->tp_manufacturer = cmd_rep->data[0];
 		ret = 0;
 		break;
 	case TEST_IMU_MFR:
-		drvdata.imu_manufacturer = cmd_rep->data[0];
+		drvdata->imu_manufacturer = cmd_rep->data[0];
 		ret = 0;
 		break;
 	case TEST_TP_VER:
-		drvdata.tp_version = cmd_rep->data[0];
+		drvdata->tp_version = cmd_rep->data[0];
 		ret = 0;
 		break;
 	default:
@@ -346,30 +348,30 @@ static int hid_gos_pl_test_event(struct command_report *cmd_rep)
 	return ret;
 }
 
-static int hid_gos_light_event(struct command_report *cmd_rep)
+static int hid_gos_light_event(struct gos_cfg_drvdata *drvdata, struct command_report *cmd_rep)
 {
 	struct led_classdev_mc *led_mc;
 	int ret = 0;
 
 	switch (cmd_rep->sub_cmd) {
 	case LIGHT_MODE_SEL:
-		drvdata.rgb_mode = cmd_rep->data[0];
+		drvdata->rgb_mode = cmd_rep->data[0];
 		ret = 0;
 		break;
 	case LIGHT_PROFILE_SEL:
-		drvdata.rgb_profile = cmd_rep->data[0];
+		drvdata->rgb_profile = cmd_rep->data[0];
 		ret = 0;
 		break;
 	case USR_LIGHT_PROFILE_1:
 	case USR_LIGHT_PROFILE_2:
 	case USR_LIGHT_PROFILE_3:
-		led_mc = &drvdata.led_mc;
-		drvdata.rgb_effect = cmd_rep->data[0];
+		led_mc = &drvdata->led_mc;
+		drvdata->rgb_effect = cmd_rep->data[0];
 		led_mc->subled_info[0].intensity = cmd_rep->data[1];
 		led_mc->subled_info[1].intensity = cmd_rep->data[2];
 		led_mc->subled_info[2].intensity = cmd_rep->data[3];
 		led_mc->led_cdev.brightness = cmd_rep->data[4];
-		drvdata.rgb_speed = cmd_rep->data[5];
+		drvdata->rgb_speed = cmd_rep->data[5];
 		ret = 0;
 		break;
 	default:
@@ -402,6 +404,7 @@ static int get_endpoint_address(struct hid_device *hdev)
 static int hid_gos_raw_event(struct hid_device *hdev, struct hid_report *report,
 			     u8 *data, int size)
 {
+	struct gos_cfg_drvdata *drvdata = hid_get_drvdata(hdev);
 	struct command_report *cmd_rep;
 	int ep, ret;
 
@@ -409,6 +412,9 @@ static int hid_gos_raw_event(struct hid_device *hdev, struct hid_report *report,
 	if (ep != GO_S_CFG_INTF_IN)
 		return 0;
 
+	if (!drvdata)
+		return -ENODEV;
+
 	if (size != GO_S_PACKET_SIZE)
 		return -EINVAL;
 
@@ -416,22 +422,22 @@ static int hid_gos_raw_event(struct hid_device *hdev, struct hid_report *report,
 
 	switch (cmd_rep->cmd) {
 	case GET_VERSION:
-		ret = hid_gos_version_event(data);
+		ret = hid_gos_version_event(drvdata, data);
 		break;
 	case GET_MCU_ID:
-		ret = hid_gos_mcu_id_event(cmd_rep);
+		ret = hid_gos_mcu_id_event(drvdata, cmd_rep);
 		break;
 	case GET_GAMEPAD_CFG:
-		ret = hid_gos_gamepad_cfg_event(cmd_rep);
+		ret = hid_gos_gamepad_cfg_event(drvdata, cmd_rep);
 		break;
 	case GET_TP_PARAM:
-		ret = hid_gos_touchpad_event(cmd_rep);
+		ret = hid_gos_touchpad_event(drvdata, cmd_rep);
 		break;
 	case GET_PL_TEST:
-		ret = hid_gos_pl_test_event(cmd_rep);
+		ret = hid_gos_pl_test_event(drvdata, cmd_rep);
 		break;
 	case GET_RGB_CFG:
-		ret = hid_gos_light_event(cmd_rep);
+		ret = hid_gos_light_event(drvdata, cmd_rep);
 		break;
 	case SET_GAMEPAD_CFG:
 	case SET_RGB_CFG:
@@ -445,8 +451,8 @@ static int hid_gos_raw_event(struct hid_device *hdev, struct hid_report *report,
 	dev_dbg(&hdev->dev, "Rx data as raw input report: [%*ph]\n",
 		GO_S_PACKET_SIZE, data);
 
-	drvdata.cmd_status = ret;
-	complete(&drvdata.send_cmd_complete);
+	drvdata->cmd_status = ret;
+	complete(&drvdata->send_cmd_complete);
 
 	return ret;
 }
@@ -454,28 +460,32 @@ static int hid_gos_raw_event(struct hid_device *hdev, struct hid_report *report,
 static int mcu_property_out(struct hid_device *hdev, u8 command, u8 index,
 			    u8 *data, size_t len)
 {
+	struct gos_cfg_drvdata *drvdata = hid_get_drvdata(hdev);
 	unsigned char *dmabuf __free(kfree) = NULL;
 	u8 header[] = { command, index };
 	size_t header_size = ARRAY_SIZE(header);
 	long t_remaining;
 	int timeout, ret;
 
+	if (!drvdata)
+		return -ENODEV;
+
 	if (header_size + len > GO_S_PACKET_SIZE)
 		return -EINVAL;
 
-	guard(mutex)(&drvdata.cfg_mutex);
+	guard(mutex)(&drvdata->cfg_mutex);
 
 	/*
 	 * A reply to the previous command may still be in flight. Give it a
 	 * short window to arrive and be consumed before this call reinits the
 	 * completion, so a late reply can't be mistaken for this command's.
 	 */
-	if (drvdata.orphan_ack_pending) {
-		wait_for_completion_timeout(&drvdata.send_cmd_complete, msecs_to_jiffies(25));
-		drvdata.orphan_ack_pending = false;
-		drvdata.cmd_status = -ETIMEDOUT;
+	if (drvdata->orphan_ack_pending) {
+		wait_for_completion_timeout(&drvdata->send_cmd_complete, msecs_to_jiffies(25));
+		drvdata->orphan_ack_pending = false;
+		drvdata->cmd_status = -ETIMEDOUT;
 	}
-	reinit_completion(&drvdata.send_cmd_complete);
+	reinit_completion(&drvdata->send_cmd_complete);
 
 	/* We can't use a devm_alloc reusable buffer without side effects during suspend */
 	dmabuf = kzalloc(GO_S_PACKET_SIZE, GFP_KERNEL);
@@ -498,14 +508,14 @@ static int mcu_property_out(struct hid_device *hdev, u8 command, u8 index,
 
 	/* PL_TEST commands can take longer because they go out to another device */
 	timeout = (command == GET_PL_TEST) ? 200 : 5;
-	t_remaining = wait_for_completion_interruptible_timeout(&drvdata.send_cmd_complete,
+	t_remaining = wait_for_completion_interruptible_timeout(&drvdata->send_cmd_complete,
 								msecs_to_jiffies(timeout));
 	if (t_remaining <= 0) {
 		/* A reply to this command may still be in flight. */
-		drvdata.orphan_ack_pending = true;
+		drvdata->orphan_ack_pending = true;
 	}
 
-	ret = t_remaining > 0 ? drvdata.cmd_status : t_remaining ?: -EBUSY;
+	ret = t_remaining > 0 ? drvdata->cmd_status : t_remaining ?: -EBUSY;
 
 	return ret;
 }
@@ -515,18 +525,30 @@ static ssize_t gamepad_property_store(struct device *dev,
 				      const char *buf, size_t count,
 				      enum feature_status_index index)
 {
+	struct gos_cfg_drvdata *drvdata;
+	struct led_classdev_mc *led_mc;
+	struct led_classdev *led_cdev;
+	struct hid_device *hdev;
 	bool dev_registered;
 	size_t size = 1;
 	u8 val = 0;
 	int ret;
 
 	/* rgb_enabled is attached to led_classdev, not hid_device */
-	if (index == FEATURE_RGB_ENABLE)
-		/* Pairs with smp_store_release from gos_cfg_setup */
-		dev_registered = smp_load_acquire(&drvdata.rgb_registered);
-	else
-		/* Pairs with smp_store_release from gos_cfg_setup */
-		dev_registered = smp_load_acquire(&drvdata.gp_registered);
+	if (index == FEATURE_RGB_ENABLE) {
+		led_cdev = dev_get_drvdata(dev);
+		led_mc = lcdev_to_mccdev(led_cdev);
+		drvdata = container_of(led_mc, struct gos_cfg_drvdata, led_mc);
+		hdev = drvdata->hdev;
+	/* Pairs with smp_store_release from cfg_setup */
+		dev_registered = smp_load_acquire(&drvdata->rgb_registered);
+
+	} else {
+		hdev = to_hid_device(dev);
+		drvdata = hid_get_drvdata(hdev);
+	/* Pairs with smp_store_release from cfg_setup */
+		dev_registered = smp_load_acquire(&drvdata->gp_registered);
+	}
 
 	if (!dev_registered)
 		return -ENODEV;
@@ -599,7 +621,7 @@ static ssize_t gamepad_property_store(struct device *dev,
 	if (!val)
 		size = 0;
 
-	ret = mcu_property_out(drvdata.hdev, SET_GAMEPAD_CFG, index, &val,
+	ret = mcu_property_out(hdev, SET_GAMEPAD_CFG, index, &val,
 			       size);
 	if (ret < 0)
 		return ret;
@@ -611,79 +633,92 @@ static ssize_t gamepad_property_show(struct device *dev,
 				     struct device_attribute *attr, char *buf,
 				     enum feature_status_index index)
 {
+	struct gos_cfg_drvdata *drvdata;
+	struct led_classdev_mc *led_mc;
+	struct led_classdev *led_cdev;
+	struct hid_device *hdev;
 	bool dev_registered;
+
 	ssize_t count = 0;
 	u8 i;
 
 	/* rgb_enabled is attached to led_classdev, not hid_device */
-	if (index == FEATURE_RGB_ENABLE)
-		/* Pairs with smp_store_release from gos_cfg_setup */
-		dev_registered = smp_load_acquire(&drvdata.rgb_registered);
-	else
-		/* Pairs with smp_store_release from gos_cfg_setup */
-		dev_registered = smp_load_acquire(&drvdata.gp_registered);
+	if (index == FEATURE_RGB_ENABLE) {
+		led_cdev = dev_get_drvdata(dev);
+		led_mc = lcdev_to_mccdev(led_cdev);
+		drvdata = container_of(led_mc, struct gos_cfg_drvdata, led_mc);
+		hdev = drvdata->hdev;
+	/* Pairs with smp_store_release from cfg_setup */
+		dev_registered = smp_load_acquire(&drvdata->rgb_registered);
+
+	} else {
+		hdev = to_hid_device(dev);
+		drvdata = hid_get_drvdata(hdev);
+	/* Pairs with smp_store_release from cfg_setup */
+		dev_registered = smp_load_acquire(&drvdata->gp_registered);
+	}
 
 	if (!dev_registered)
 		return -ENODEV;
 
-	count = mcu_property_out(drvdata.hdev, GET_GAMEPAD_CFG, index, NULL, 0);
+	count = mcu_property_out(hdev, GET_GAMEPAD_CFG, index, NULL, 0);
 	if (count < 0)
 		return count;
 
 	switch (index) {
 	case FEATURE_GAMEPAD_MODE:
-		i = drvdata.gp_mode;
+		i = drvdata->gp_mode;
 		if (i >= ARRAY_SIZE(gamepad_mode_text))
 			return -EINVAL;
 		count = sysfs_emit(buf, "%s\n", gamepad_mode_text[i]);
 		break;
 	case FEATURE_AUTO_SLEEP_TIME:
-		count = sysfs_emit(buf, "%u\n", drvdata.gp_auto_sleep_time);
+		count = sysfs_emit(buf, "%u\n", drvdata->gp_auto_sleep_time);
 		break;
 	case FEATURE_IMU_ENABLE:
-		i = drvdata.imu_sensor_en;
+		i = drvdata->imu_sensor_en;
 		if (i >= ARRAY_SIZE(feature_enabled_text))
 			return -EINVAL;
 		count = sysfs_emit(buf, "%s\n", feature_enabled_text[i]);
 		break;
 	case FEATURE_IMU_BYPASS:
-		i = drvdata.imu_bypass_en;
+		i = drvdata->imu_bypass_en;
 		if (i >= ARRAY_SIZE(feature_enabled_text))
 			return -EINVAL;
 		count = sysfs_emit(buf, "%s\n", feature_enabled_text[i]);
 		break;
 	case FEATURE_RGB_ENABLE:
-		i = drvdata.rgb_en;
+		i = drvdata->rgb_en;
 		if (i >= ARRAY_SIZE(feature_enabled_text))
 			return -EINVAL;
 		count = sysfs_emit(buf, "%s\n", feature_enabled_text[i]);
 		break;
 	case FEATURE_TOUCHPAD_ENABLE:
-		i = drvdata.tp_en;
+		i = drvdata->tp_en;
 		if (i >= ARRAY_SIZE(feature_enabled_text))
 			return -EINVAL;
 		count = sysfs_emit(buf, "%s\n", feature_enabled_text[i]);
 		break;
 	case FEATURE_OS_MODE:
-		i = drvdata.os_mode;
+		i = drvdata->os_mode;
 		if (i >= ARRAY_SIZE(os_type_text))
 			return -EINVAL;
 		count = sysfs_emit(buf, "%s\n", os_type_text[i]);
 		break;
 	case FEATURE_POLL_RATE:
-		i = drvdata.gp_poll_rate;
+		i = drvdata->gp_poll_rate;
 		if (i >= ARRAY_SIZE(poll_rate_text))
 			return -EINVAL;
 		count = sysfs_emit(buf, "%s\n", poll_rate_text[i]);
 		break;
 	case FEATURE_DPAD_MODE:
-		i = drvdata.gp_dpad_mode;
+		i = drvdata->gp_dpad_mode;
 		if (i >= ARRAY_SIZE(dpad_mode_text))
 			return -EINVAL;
 		count = sysfs_emit(buf, "%s\n", dpad_mode_text[i]);
 		break;
 	case FEATURE_MOUSE_WHEEL_STEP:
-		i = drvdata.mouse_step;
+		i = drvdata->mouse_step;
 		if (i < 1 || i > 127)
 			return -EINVAL;
 		count = sysfs_emit(buf, "%u\n", i);
@@ -761,13 +796,18 @@ static ssize_t touchpad_property_store(struct device *dev,
 				       const char *buf, size_t count,
 				       enum touchpad_config_index index)
 {
+	struct hid_device *hdev = to_hid_device(dev);
+	struct gos_cfg_drvdata *drvdata = hid_get_drvdata(hdev);
 	bool gp_registered;
 	size_t size = 1;
 	u8 val = 0;
 	int ret;
 
-	/* Pairs with smp_store_release from gos_cfg_setup */
-	gp_registered = smp_load_acquire(&drvdata.gp_registered);
+	if (!drvdata)
+		return -ENODEV;
+
+	/* Pairs with smp_store_release from cfg_setup */
+	gp_registered = smp_load_acquire(&drvdata->gp_registered);
 	if (!gp_registered)
 		return -ENODEV;
 
@@ -790,7 +830,7 @@ static ssize_t touchpad_property_store(struct device *dev,
 	if (!val)
 		size = 0;
 
-	ret = mcu_property_out(drvdata.hdev, SET_TP_PARAM, index, &val, size);
+	ret = mcu_property_out(hdev, SET_TP_PARAM, index, &val, size);
 	if (ret < 0)
 		return ret;
 
@@ -801,25 +841,30 @@ static ssize_t touchpad_property_show(struct device *dev,
 				      struct device_attribute *attr, char *buf,
 				      enum touchpad_config_index index)
 {
+	struct hid_device *hdev = to_hid_device(dev);
+	struct gos_cfg_drvdata *drvdata = hid_get_drvdata(hdev);
 	bool gp_registered;
 	int ret = 0;
 	u8 i;
 
-	/* Pairs with smp_store_release from gos_cfg_setup */
-	gp_registered = smp_load_acquire(&drvdata.gp_registered);
+	if (!drvdata)
+		return -ENODEV;
+
+	/* Pairs with smp_store_release from cfg_setup */
+	gp_registered = smp_load_acquire(&drvdata->gp_registered);
 	if (!gp_registered)
 		return -ENODEV;
 
-	ret = mcu_property_out(drvdata.hdev, GET_TP_PARAM, index, NULL, 0);
+	ret = mcu_property_out(hdev, GET_TP_PARAM, index, NULL, 0);
 	if (ret < 0)
 		return ret;
 
 	switch (index) {
 	case CFG_WINDOWS_MODE:
-		i = drvdata.tp_windows_mode;
+		i = drvdata->tp_windows_mode;
 		break;
 	case CFG_LINUX_MODE:
-		i = drvdata.tp_linux_mode;
+		i = drvdata->tp_linux_mode;
 		break;
 	default:
 		return -EINVAL;
@@ -836,15 +881,9 @@ static ssize_t touchpad_property_options(struct device *dev,
 					 char *buf,
 					 enum touchpad_config_index index)
 {
-	bool gp_registered;
 	size_t count = 0;
 	unsigned int i;
 
-	/* Pairs with smp_store_release from gos_cfg_setup */
-	gp_registered = smp_load_acquire(&drvdata.gp_registered);
-	if (!gp_registered)
-		return -ENODEV;
-
 	switch (index) {
 	case CFG_WINDOWS_MODE:
 	case CFG_LINUX_MODE:
@@ -867,30 +906,35 @@ static ssize_t test_property_show(struct device *dev,
 				  struct device_attribute *attr, char *buf,
 				  enum test_command_index index)
 {
+	struct hid_device *hdev = to_hid_device(dev);
+	struct gos_cfg_drvdata *drvdata = hid_get_drvdata(hdev);
 	bool gp_registered;
 	size_t count = 0;
 	u8 i;
 
-	/* Pairs with smp_store_release from gos_cfg_setup */
-	gp_registered = smp_load_acquire(&drvdata.gp_registered);
+	if (!drvdata)
+		return -ENODEV;
+
+	/* Pairs with smp_store_release from cfg_setup */
+	gp_registered = smp_load_acquire(&drvdata->gp_registered);
 	if (!gp_registered)
 		return -ENODEV;
 
 	switch (index) {
 	case TEST_TP_MFR:
-		i = drvdata.tp_manufacturer;
+		i = drvdata->tp_manufacturer;
 		if (i >= ARRAY_SIZE(touchpad_manufacturer_text))
 			return -EINVAL;
 		count = sysfs_emit(buf, "%s\n", touchpad_manufacturer_text[i]);
 		break;
 	case TEST_IMU_MFR:
-		i = drvdata.imu_manufacturer;
+		i = drvdata->imu_manufacturer;
 		if (i >= ARRAY_SIZE(imu_manufacturer_text))
 			return -EINVAL;
 		count = sysfs_emit(buf, "%s\n", imu_manufacturer_text[i]);
 		break;
 	case TEST_TP_VER:
-		count = sysfs_emit(buf, "%u\n", drvdata.tp_version);
+		count = sysfs_emit(buf, "%u\n", drvdata->tp_version);
 		break;
 	default:
 		count = -EINVAL;
@@ -903,7 +947,13 @@ static ssize_t test_property_show(struct device *dev,
 static ssize_t mcu_id_show(struct device *dev, struct device_attribute *attr,
 			   char *buf)
 {
-	return sysfs_emit(buf, "%*phN\n", 12, &drvdata.mcu_id);
+	struct hid_device *hdev = to_hid_device(dev);
+	struct gos_cfg_drvdata *drvdata = hid_get_drvdata(hdev);
+
+	if (!drvdata)
+		return -ENODEV;
+
+	return sysfs_emit(buf, "%*phN\n", 12, &drvdata->mcu_id);
 }
 
 static int rgb_cfg_call(struct hid_device *hdev, enum mcu_command_index cmd,
@@ -918,27 +968,29 @@ static int rgb_cfg_call(struct hid_device *hdev, enum mcu_command_index cmd,
 	return mcu_property_out(hdev, cmd, index, val, size);
 }
 
-static int rgb_attr_show(void)
+static int rgb_attr_show(struct gos_cfg_drvdata *drvdata)
 {
 	enum rgb_config_index index;
 
-	index = drvdata.rgb_profile + 2;
+	index = drvdata->rgb_profile + 2;
 
-	return rgb_cfg_call(drvdata.hdev, GET_RGB_CFG, index, NULL, 0);
+	return rgb_cfg_call(drvdata->hdev, GET_RGB_CFG, index, NULL, 0);
 };
 
 static ssize_t rgb_effect_store(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)
 {
-	struct led_classdev_mc *led_mc = &drvdata.led_mc;
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	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;
 	u8 effect;
 	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 -ENODEV;
 
@@ -947,41 +999,44 @@ static ssize_t rgb_effect_store(struct device *dev,
 		return ret;
 
 	effect = ret;
-	index = drvdata.rgb_profile + 2;
+	index = drvdata->rgb_profile + 2;
 	u8 rgb_profile[6] = { effect,
 			      led_mc->subled_info[0].intensity,
 			      led_mc->subled_info[1].intensity,
 			      led_mc->subled_info[2].intensity,
 			      led_mc->led_cdev.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);
 	if (ret)
 		return ret;
 
-	drvdata.rgb_effect = effect;
+	drvdata->rgb_effect = effect;
 	return count;
 };
 
 static ssize_t rgb_effect_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	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);
 	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 -ENODEV;
 
-	ret = rgb_attr_show();
+	ret = rgb_attr_show(drvdata);
 	if (ret)
 		return ret;
 
-	if (drvdata.rgb_effect >= ARRAY_SIZE(rgb_effect_text))
+	if (drvdata->rgb_effect >= ARRAY_SIZE(rgb_effect_text))
 		return -EINVAL;
 
-	return sysfs_emit(buf, "%s\n", rgb_effect_text[drvdata.rgb_effect]);
+	return sysfs_emit(buf, "%s\n", rgb_effect_text[drvdata->rgb_effect]);
 }
 
 static ssize_t rgb_effect_index_show(struct device *dev,
@@ -1003,14 +1058,16 @@ static ssize_t rgb_speed_store(struct device *dev,
 			       struct device_attribute *attr, const char *buf,
 			       size_t count)
 {
-	struct led_classdev_mc *led_mc = &drvdata.led_mc;
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	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 val = 0;
 	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 -ENODEV;
 
@@ -1021,19 +1078,19 @@ static ssize_t rgb_speed_store(struct device *dev,
 	if (val < 0 || val > 100)
 		return -EINVAL;
 
-	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,
 			      led_mc->led_cdev.brightness,
 			      val };
 
-	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);
 	if (ret)
 		return ret;
 
-	drvdata.rgb_speed = val;
+	drvdata->rgb_speed = val;
 
 	return count;
 };
@@ -1041,22 +1098,25 @@ static ssize_t rgb_speed_store(struct device *dev,
 static ssize_t rgb_speed_show(struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	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);
 	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 -ENODEV;
 
-	ret = rgb_attr_show();
+	ret = rgb_attr_show(drvdata);
 	if (ret)
 		return ret;
 
-	if (drvdata.rgb_speed > 100)
+	if (drvdata->rgb_speed > 100)
 		return -EINVAL;
 
-	return sysfs_emit(buf, "%hhu\n", drvdata.rgb_speed);
+	return sysfs_emit(buf, "%hhu\n", drvdata->rgb_speed);
 }
 
 static ssize_t rgb_speed_range_show(struct device *dev,
@@ -1068,12 +1128,15 @@ static ssize_t rgb_speed_range_show(struct device *dev,
 static ssize_t rgb_mode_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
 {
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	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);
 	bool rgb_registered;
 	int ret;
 	u8 val;
 
-	/* 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 -ENODEV;
 
@@ -1083,12 +1146,12 @@ static ssize_t rgb_mode_store(struct device *dev, struct device_attribute *attr,
 
 	val = ret;
 
-	ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, LIGHT_MODE_SEL, &val,
+	ret = rgb_cfg_call(drvdata->hdev, SET_RGB_CFG, LIGHT_MODE_SEL, &val,
 			   1);
 	if (ret)
 		return ret;
 
-	drvdata.rgb_mode = val;
+	drvdata->rgb_mode = val;
 
 	return count;
 };
@@ -1096,22 +1159,25 @@ static ssize_t rgb_mode_store(struct device *dev, struct device_attribute *attr,
 static ssize_t rgb_mode_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	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);
 	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 -ENODEV;
 
-	ret = rgb_cfg_call(drvdata.hdev, GET_RGB_CFG, LIGHT_MODE_SEL, NULL, 0);
+	ret = rgb_cfg_call(drvdata->hdev, GET_RGB_CFG, LIGHT_MODE_SEL, NULL, 0);
 	if (ret)
 		return ret;
 
-	if (drvdata.rgb_mode >= ARRAY_SIZE(rgb_mode_text))
+	if (drvdata->rgb_mode >= ARRAY_SIZE(rgb_mode_text))
 		return -EINVAL;
 
-	return sysfs_emit(buf, "%s\n", rgb_mode_text[drvdata.rgb_mode]);
+	return sysfs_emit(buf, "%s\n", rgb_mode_text[drvdata->rgb_mode]);
 };
 
 static ssize_t rgb_mode_index_show(struct device *dev,
@@ -1133,13 +1199,16 @@ static ssize_t rgb_profile_store(struct device *dev,
 				 struct device_attribute *attr, const char *buf,
 				 size_t count)
 {
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	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);
 	bool rgb_registered;
 	size_t size = 1;
 	int ret;
 	u8 val;
 
-	/* 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 -ENODEV;
 
@@ -1150,11 +1219,11 @@ static ssize_t rgb_profile_store(struct device *dev,
 	if (val < 1 || val > 3)
 		return -EINVAL;
 
-	ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, LIGHT_PROFILE_SEL, &val, size);
+	ret = rgb_cfg_call(drvdata->hdev, SET_RGB_CFG, LIGHT_PROFILE_SEL, &val, size);
 	if (ret)
 		return ret;
 
-	drvdata.rgb_profile = val;
+	drvdata->rgb_profile = val;
 
 	return count;
 };
@@ -1162,22 +1231,25 @@ static ssize_t rgb_profile_store(struct device *dev,
 static ssize_t rgb_profile_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	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);
 	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 -ENODEV;
 
-	ret = rgb_cfg_call(drvdata.hdev, GET_RGB_CFG, LIGHT_PROFILE_SEL, NULL, 0);
+	ret = rgb_cfg_call(drvdata->hdev, GET_RGB_CFG, LIGHT_PROFILE_SEL, NULL, 0);
 	if (ret)
 		return ret;
 
-	if (drvdata.rgb_profile < 1 || drvdata.rgb_profile > 3)
+	if (drvdata->rgb_profile < 1 || drvdata->rgb_profile > 3)
 		return -EINVAL;
 
-	return sysfs_emit(buf, "%hhu\n", drvdata.rgb_profile);
+	return sysfs_emit(buf, "%hhu\n", drvdata->rgb_profile);
 };
 
 static ssize_t rgb_profile_range_show(struct device *dev,
@@ -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);
 	switch (ret) {
 	case 0:
 		led_cdev->brightness = brightness;
@@ -1448,155 +1521,166 @@ static struct mc_subled gos_rgb_subled_info[] = {
 	},
 };
 
-static int cfg_query_device(void)
+static int cfg_query_device(struct gos_cfg_drvdata *drvdata)
 {
 	int ret;
 
 	/* MCU */
-	ret = mcu_property_out(drvdata.hdev, GET_MCU_ID, FEATURE_NONE, NULL, 0);
+	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);
+		dev_err(&drvdata->hdev->dev, "Failed to retrieve MCU ID: %i\n", ret);
 		return ret;
 	}
 
-	ret = mcu_property_out(drvdata.hdev, GET_VERSION, FEATURE_NONE, NULL, 0);
+	ret = mcu_property_out(drvdata->hdev, GET_VERSION, FEATURE_NONE, NULL, 0);
 	if (ret) {
-		dev_err(&drvdata.hdev->dev, "Failed to retrieve MCU Version: %i\n", ret);
+		dev_err(&drvdata->hdev->dev, "Failed to retrieve MCU Version: %i\n", ret);
 		return ret;
 	}
 
-	ret = mcu_property_out(drvdata.hdev, GET_PL_TEST, TEST_TP_MFR, NULL, 0);
+	ret = mcu_property_out(drvdata->hdev, GET_PL_TEST, TEST_TP_MFR, NULL, 0);
 	if (ret) {
-		dev_err(&drvdata.hdev->dev,
+		dev_err(&drvdata->hdev->dev,
 			"Failed to retrieve Touchpad Manufacturer: %i\n", ret);
 		return ret;
 	}
 
-	ret = mcu_property_out(drvdata.hdev, GET_PL_TEST, TEST_TP_VER, NULL, 0);
+	ret = mcu_property_out(drvdata->hdev, GET_PL_TEST, TEST_TP_VER, NULL, 0);
 	if (ret) {
-		dev_err(&drvdata.hdev->dev,
+		dev_err(&drvdata->hdev->dev,
 			"Failed to retrieve Touchpad Firmware Version: %i\n", ret);
 		return ret;
 	}
 
-	ret = mcu_property_out(drvdata.hdev, GET_PL_TEST, TEST_IMU_MFR, NULL, 0);
+	ret = mcu_property_out(drvdata->hdev, GET_PL_TEST, TEST_IMU_MFR, NULL, 0);
 	if (ret) {
-		dev_err(&drvdata.hdev->dev,
+		dev_err(&drvdata->hdev->dev,
 			"Failed to retrieve IMU Manufacturer: %i\n", ret);
 		return ret;
 	}
 
-	return mcu_property_out(drvdata.hdev, GET_GAMEPAD_CFG, FEATURE_OS_MODE,
+	return mcu_property_out(drvdata->hdev, GET_GAMEPAD_CFG, FEATURE_OS_MODE,
 				 NULL, 0);
 }
 
 static void cfg_setup(struct work_struct *work)
 {
+	struct delayed_work *dwork = container_of(work, struct delayed_work, work);
+	struct gos_cfg_drvdata *drvdata = container_of(dwork, struct gos_cfg_drvdata,
+						       gos_cfg_setup);
 	bool gp_registered = false;
 	bool rgb_registered = false;
 	int ret;
 
-	ret = cfg_query_device();
+	if (!drvdata)
+		return;
+
+	ret = cfg_query_device(drvdata);
 	if (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,
+		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);
+				drvdata->setup_attempts);
 			return;
 		}
 
-		schedule_delayed_work(&drvdata.gos_cfg_setup, msecs_to_jiffies(50));
+		schedule_delayed_work(&drvdata->gos_cfg_setup, msecs_to_jiffies(50));
 		return;
 	}
 
-	drvdata.setup_attempts = 0;
+	drvdata->setup_attempts = 0;
 
-	if (!drvdata.gp_registered) {
-		/* Pairs with smp_load_acquire in attribute show/store functions */
-		smp_store_release(&drvdata.gp_registered, true);
+	if (!drvdata->gp_registered) {
+		/* Pairs with smp_load_acquire in attribute show/store */
+		smp_store_release(&drvdata->gp_registered, true);
 		gp_registered = true;
 
-		ret = sysfs_create_groups(&drvdata.hdev->dev.kobj, top_level_attr_groups);
+		ret = sysfs_create_groups(&drvdata->hdev->dev.kobj, top_level_attr_groups);
 		if (ret) {
-			dev_err(&drvdata.hdev->dev,
+			dev_err(&drvdata->hdev->dev,
 				"Failed to create gamepad configuration attributes: %i\n",
 				ret);
 
-			/* Pairs with smp_load_acquire in attribute show/store functions */
-			smp_store_release(&drvdata.gp_registered, false);
+			/* Pairs with smp_load_acquire in attribute show/store */
+			smp_store_release(&drvdata->gp_registered, false);
 			gp_registered = false;
 		}
 	}
 
-	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;
 			}
 		}
 	}
 
 	if (gp_registered)
-		kobject_uevent(&drvdata.hdev->dev.kobj, KOBJ_CHANGE);
+		kobject_uevent(&drvdata->hdev->dev.kobj, KOBJ_CHANGE);
 	if (rgb_registered)
-		kobject_uevent(&drvdata.led_mc.led_cdev.dev->kobj, KOBJ_CHANGE);
+		kobject_uevent(&drvdata->led_mc.led_cdev.dev->kobj, KOBJ_CHANGE);
 }
 
 static int hid_gos_cfg_probe(struct hid_device *hdev,
 			     const struct hid_device_id *_id)
 {
+	struct gos_cfg_drvdata *drvdata;
 	int ret;
 
-	hid_set_drvdata(hdev, &drvdata);
-	drvdata.hdev = hdev;
+	drvdata = devm_kzalloc(&hdev->dev, sizeof(struct gos_cfg_drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	hid_set_drvdata(hdev, drvdata);
+	drvdata->hdev = hdev;
 
-	mutex_init(&drvdata.cfg_mutex);
-	init_completion(&drvdata.send_cmd_complete);
+	mutex_init(&drvdata->cfg_mutex);
+	init_completion(&drvdata->send_cmd_complete);
 
 	/* Device is hardwired and name is guaranteed to be unique */
-	drvdata.led_mc.led_cdev.name = "go_s:rgb:joystick_rings";
-	drvdata.led_mc.led_cdev.brightness = 0x50;
-	drvdata.led_mc.led_cdev.max_brightness = 0x64;
-	drvdata.led_mc.led_cdev.color = LED_COLOR_ID_RGB;
-	drvdata.led_mc.led_cdev.brightness_set = hid_gos_brightness_set;
-	drvdata.led_mc.num_colors = 3;
-	drvdata.led_mc.subled_info = devm_kmemdup(&hdev->dev, gos_rgb_subled_info,
-						  sizeof(gos_rgb_subled_info), GFP_KERNEL);
-	if (!drvdata.led_mc.subled_info)
+	drvdata->led_mc.led_cdev.name = "go_s:rgb:joystick_rings";
+	drvdata->led_mc.led_cdev.brightness = 0x50;
+	drvdata->led_mc.led_cdev.max_brightness = 0x64;
+	drvdata->led_mc.led_cdev.color = LED_COLOR_ID_RGB;
+	drvdata->led_mc.led_cdev.brightness_set = hid_gos_brightness_set;
+	drvdata->led_mc.num_colors = 3;
+	drvdata->led_mc.subled_info = devm_kmemdup(&hdev->dev, gos_rgb_subled_info,
+						   sizeof(gos_rgb_subled_info), GFP_KERNEL);
+	if (!drvdata->led_mc.subled_info)
 		return -ENOMEM;
 
 	/* Executing calls prior to returning from probe will lock the MCU. Schedule
 	 * initial data call after probe has completed and MCU can accept calls.
 	 */
-	INIT_DELAYED_WORK(&drvdata.gos_cfg_setup, &cfg_setup);
-	ret = schedule_delayed_work(&drvdata.gos_cfg_setup, msecs_to_jiffies(2));
+	INIT_DELAYED_WORK(&drvdata->gos_cfg_setup, &cfg_setup);
+	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;
@@ -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);
@@ -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;
@@ -1642,20 +1735,20 @@ static int hid_gos_cfg_reset_resume(struct hid_device *hdev)
 		return 0;
 	}
 
-	ret = mcu_property_out(drvdata.hdev, SET_GAMEPAD_CFG,
+	ret = mcu_property_out(drvdata->hdev, SET_GAMEPAD_CFG,
 			       FEATURE_OS_MODE, &os_mode, 1);
 	if (ret < 0)
 		return ret;
 
-	ret = mcu_property_out(drvdata.hdev, GET_GAMEPAD_CFG,
+	ret = mcu_property_out(drvdata->hdev, GET_GAMEPAD_CFG,
 			       FEATURE_OS_MODE, NULL, 0);
 	if (ret < 0)
 		return ret;
 
-	if (drvdata.os_mode != os_mode)
+	if (drvdata->os_mode != os_mode)
 		return -ENODEV;
 
-	kobject_uevent(&drvdata.hdev->dev.kobj, KOBJ_CHANGE);
+	kobject_uevent(&drvdata->hdev->dev.kobj, KOBJ_CHANGE);
 	return 0;
 }
 
@@ -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;
 }
-- 
2.55.0

[PATCH v2 11/11] HID: hid-lenovo-go-s: Stop and close device on probe failure

[Derek J. Clark]

If hid_gos_cfg_probe() fails, the HID device has already been started
and opened. Probe failure does not trigger the driver's remove
function, so the device is left open and running.

Close and stop the device on this error path.

Fixes: a23f3497bf208c59ad ("HID: hid-lenovo-go-s: Add Lenovo Legion Go S Series HID Driver")
Cc: stable@vger.kernel.org
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index 1a2b28e1bed1..694b329f30db 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -1786,8 +1786,11 @@ static int hid_gos_probe(struct hid_device *hdev,
 	}
 
 	ret = hid_gos_cfg_probe(hdev, id);
-	if (ret)
+	if (ret) {
 		dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface");
+		hid_hw_close(hdev);
+		hid_hw_stop(hdev);
+	}
 
 	dev_dbg(&hdev->dev, "Started interface %x as Go S configuration interface\n", ep);
 	return ret;
-- 
2.55.0

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

[sashiko-bot]

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

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

[sashiko-bot]

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

Re: [PATCH v2 09/11] HID: hid-lenovo-go-s: Move static led_classdev_mc to drvdata struct

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] HID: hid-lenovo-go-s: sleeping in atomic context in hid_gos_brightness_set()
--

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

HID: hid-lenovo-go-s: Move static led_classdev_mc to drvdata struct

This commit moves the static gos_cdev_rgb multicolor LED structure into
struct gos_cfg_drvdata and initializes it in hid_gos_cfg_probe() in
preparation for dynamically allocating drvdata.

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

[ ... ]

> @@ -1593,6 +1580,18 @@ static int hid_gos_cfg_probe(struct hid_device *hdev,
>  	mutex_init(&drvdata.cfg_mutex);
>  	init_completion(&drvdata.send_cmd_complete);
>  
> +	/* Device is hardwired and name is guaranteed to be unique */
> +	drvdata.led_mc.led_cdev.name = "go_s:rgb:joystick_rings";
> +	drvdata.led_mc.led_cdev.brightness = 0x50;
> +	drvdata.led_mc.led_cdev.max_brightness = 0x64;
> +	drvdata.led_mc.led_cdev.color = LED_COLOR_ID_RGB;
> +	drvdata.led_mc.led_cdev.brightness_set = hid_gos_brightness_set;
> +	drvdata.led_mc.num_colors = 3;
> +	drvdata.led_mc.subled_info = devm_kmemdup(&hdev->dev, gos_rgb_subled_info,
> +						  sizeof(gos_rgb_subled_info), GFP_KERNEL);
> +	if (!drvdata.led_mc.subled_info)
> +		return -ENOMEM;

[Severity: High]
Can registering hid_gos_brightness_set() as the non-blocking
brightness_set callback sleep in atomic context?

In the LED core, brightness_set is invoked directly from atomic contexts
such as software blink timers and LED triggers via:

  led_timer_function()
    -> led_set_brightness_nosleep()
      -> led_set_brightness_nopm()
        -> __led_set_brightness()
          -> led_cdev->brightness_set()

However, hid_gos_brightness_set() calls rgb_cfg_call() ->
mcu_property_out(), which sleeps:

  drivers/hid/hid-lenovo-go-s.c:mcu_property_out() {
      ...
      guard(mutex)(&drvdata.cfg_mutex);
      ...
      if (drvdata.orphan_ack_pending) {
          wait_for_completion_timeout(&drvdata.send_cmd_complete, msecs_to_jiffies(25));
      ...
      dmabuf = kzalloc(GO_S_PACKET_SIZE, GFP_KERNEL);
      ...
      ret = hid_hw_output_report(hdev, dmabuf, GO_S_PACKET_SIZE);
      ...
      t_remaining = wait_for_completion_interruptible_timeout(&drvdata.send_cmd_complete,
                                                              msecs_to_jiffies(timeout));
      ...
  }

Should hid_gos_brightness_set() be wired up to brightness_set_blocking
instead of brightness_set so that the LED core defers atomic updates to
set_brightness_work?

> +
>  	/* Executing calls prior to returning from probe will lock the MCU. Schedule
>  	 * initial data call after probe has completed and MCU can accept calls.
>  	 */

[ ... ]

>

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

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

[sashiko-bot]

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