Re: [PATCH v2 03/12] HID: asus: add gamepad configuration

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The device state (e.g., Xbox controller mode) is not restored during `hid_asus_ally_reset_resume()` after a device reset, leading to desynchronization between the hardware state and the cached sysfs state.
- [Low] Passing implicitly NULL sysfs group name to %s format specifier in error message.
--

--- Patch [3]: [PATCH v2 03/12] HID: asus: add gamepad configuration ---
commit 9e908c106ea4b31ecf33a0f67456da287b68ca3a
Author: Denis Benato <[email protected]>

HID: asus: add gamepad configuration

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

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 88b30726f6c67..900914d21ca46 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> +static const struct attribute_group ally_attr_groups[] = {
> +	{
> +		.attrs = ally_config_attrs,
> +	},
> +};
> +
> +/**
> + * ally_config_create() - Initialize configuration and create sysfs entries
> + * @hdev: HID device
> + * @ally: non-NULL ally device data with uninitialized config pointer
> + *
> + * Return: valid pointer on success, error pointer on failure
> + */
> +static struct ally_config *ally_config_create(struct hid_device *hdev, struct ally_handheld *ally)
> +{
> +	struct ally_config *cfg;
> +	int ret, sysfs_i;

[ ... ]

> +	for (sysfs_i = 0; sysfs_i < ARRAY_SIZE(ally_attr_groups); sysfs_i++) {
> +		ret = devm_device_add_group(&hdev->dev, &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);

[Severity: Low]
Is this going to print (null) in the error message? The ally_attr_groups
definition doesn't seem to have a .name field set for the group.

> +			goto ally_config_create_sysfs_err;
> +		}
> +	}

[ ... ]

> @@ -994,6 +1292,16 @@ static int hid_asus_ally_reset_resume(struct hid_device *hdev, struct ally_handh
>  	if (ep != HID_ALLY_INTF_CFG_IN)
>  		return 0;
>  
> +	/*
> +	 * This function assumes the asus-specific initialization
> +	 * to have been performed already at this point.
> +	 */
> +	ret = ally_gamepad_check_ready(ally, hdev);
> +	if (ret < 0) {
> +		hid_err(hdev, "ROG Ally device is not ready: %d\n", ret);
> +		return ret;
> +	}
> +
>  	ret = hid_asus_ally_init(hdev, ally);
>  	if (ret < 0)
>  		return ret;

[Severity: Medium]
Does this code need to restore the device configuration state, such as the Xbox
controller mode, after a device reset?

Since hid_asus_ally_init() only disables force-feedback, it looks like a system
suspend and resume cycle could cause the hardware to revert to defaults, while
the cached ally->config still holds the previous settings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3