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

["Derek J. Clark" <derekjohn.clark@gmail.com>]

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