Re: [PATCH v5 04/13] HID: asus: add gamepad configuration

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] State initialization and restoration logic errors corrupt user settings on re-probe.
- [Medium] Sysfs attributes are exposed to userspace before the device data is fully linked, causing spurious -ENODEV errors.
--

commit 396e8f687c336ca0b0f9ddcf54fbc96925c6f82f
Author: Denis Benato [off-list ref]

    HID: asus: add gamepad configuration
    
    Add the base configuration structures for the gamepad configuration,
    detect capabilities and initialize the device in a known state.

[ ... ]

[ ... ]

+	for (sysfs_i = 0; sysfs_i < ARRAY_SIZE(ally_attr_groups); sysfs_i++) {
+		ret = sysfs_create_group(&hdev->dev.kobj,
+					 &ally_attr_groups[sysfs_i]);
+		if (ret < 0) {
+			hid_err(hdev, "Failed to create sysfs group '%s': %d\n",
+				ally_attr_groups[sysfs_i].name ?: "", ret);
+			goto ally_config_create_sysfs_err;
+		}
+	}

[Severity: Medium]
Will this expose sysfs attributes to userspace before the device data is fully
linked?

Since ally_drvdata.config is not assigned until later in hid_asus_ally_probe(),
if a userspace tool like udev reads an attribute in this window,
ally_get_config() will return NULL, causing a spurious -ENODEV error.

+
+	/* So far the only hardware this is supported is the Ally 1 */
+	if (cfg->xbox_controller_support) {
+		ret = ally_set_xbox_controller(hdev, ally, cfg, true);
+		if (ret < 0)
+			hid_warn(hdev, "Failed to set default Xbox controller mode: %d\n",
+				ret);
+	}
+
+	cfg->initialized = true;

[Severity: High]
Does this code unconditionally overwrite the user's previously saved preference
back to true on every probe?

Because this block is outside the `if (!cfg->initialized)` check above, it
seems it will force the hardware and software state to true whenever the device
suspends/resumes or re-enumerates, destroying any saved settings.

[ ... ]

[Severity: High]
Will this silently fail to restore cached user settings to the hardware during
probe?

Because hid_asus_ally_init() is called before ally_drvdata.config is published
in hid_asus_ally_probe(), ally_get_config() will return NULL here, causing it
to bail out early.

+
+	/* The MCU may have just been reset: restore the cached state. */
+	guard(mutex)(&cfg->config_mutex);
+
+	if (cfg->xbox_controller_enabled) {
+		ret = ally_set_xbox_controller(hdev, ally, cfg, true);
+		if (ret < 0)
+			hid_warn(hdev, "Failed to restore Xbox controller mode: %d\n",
+				 ret);
+	}
+
 	return 0;
 }

[Severity: High]
If the hardware reset to true but the user had disabled the Xbox controller
(cfg->xbox_controller_enabled == false), does this incorrectly fail to restore
the state to false?

It looks like this code only restores the state if it was true, and hardcodes
the value passed to ally_set_xbox_controller() to true instead of passing
cfg->xbox_controller_enabled.

[ ... ]

[Severity: High]
This publishes the configuration pointer to the shared state after
hid_asus_ally_init() has already run, which hides it from the initialization
function above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904145845.184887-1-denis.benato@linux.dev?part=4