Re: [PATCH v4 11/13] HID: asus: add support for gamepad mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The driver fails to restore the user-configured gamepad mode during a reset resume, leaving the hardware and software state out of sync.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the allocated `ally_config` structure is never assigned to `ally_drvdata.config`, causing all sysfs configuration attributes to immediately return `-ENODEV`.
--

commit 1fb61822e5f0cd86a15f0eb20ff193c1240573e5
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for gamepad mode

ROG Ally devices can emulate either a mouse+keyboard (desktop mode)
or an gamepad device (xbox360 controller in ROG ally and a custom
DInput device on newer models): add support for switching the current
controller mode.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 61330ae082b16..9ed6c605df888 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> +static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *ally = drvdata->rog_ally;
> +	struct ally_config *cfg;
> +	u8 mode_byte;
> +	int mode;
> +	int ret;
> +
> +	if (!ally)
> +		return -ENODEV;
> +
> +	cfg = ally_get_config(ally);
> +	if (!cfg)
> +		return -ENODEV;

[Severity: High]
This isn't a bug introduced by this patch, but will this !cfg check always
evaluate to true, blocking all sysfs functionality?

Every sysfs attribute relies on ally_get_config(), which attempts to return
ally->config. Because ally->config is implicitly initialized to NULL (via the
global ally_drvdata) and never populated with the allocated ally_cfg pointer,
all sysfs attributes immediately reject access with -ENODEV.

Looking at the probe path where the shared ally_drvdata state is populated
after a successful interface probe, the config pointer assignment is entirely
omitted:

drivers/hid/hid-asus.c:hid_asus_ally_probe() {
    ...
		ally_cfg = ally_config_create(hdev, &ally_drvdata);
		if (IS_ERR(ally_cfg)) {
			...
		}

		ret = hid_asus_ally_init(hdev, &ally_drvdata);
		if (ret < 0) {
			...
		}

		spin_lock_irqsave(&ally_data_lock, flags);
		ally_drvdata.cfg_hdev = hdev;
        /* missing: ally_drvdata.config = ally_cfg; */
		spin_unlock_irqrestore(&ally_data_lock, flags);
    ...
}

This disables the entirety of the newly added gamepad configuration features.

> +
> +	mode = sysfs_match_string(ally_gamepad_mode_text, buf);
> +	if (mode < 0) {
> +		hid_err(hdev, "Unknown gamepad mode\n");
> +		return mode;
> +	}

[ ... ]

> +/**
> + * ally_apply_gamepad_mode() - Re-send the recorded gamepad mode to the hardware
> + * @hdev: HID device
> + * @ally: ally handheld structure
> + * @cfg: ally config
> + *
> + * Re-send the gamepad mode currently recorded in the configuration to the
> + * hardware: during probe that is the default mode, while a reset resume
> + * restores whatever the user last wrote instead of overwriting it. The
> + * shared state is accessed under the lock, as sysfs is already live.
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +static int ally_apply_gamepad_mode(struct hid_device *hdev,
> +				   struct ally_handheld *ally,
> +				   struct ally_config *cfg)
> +{
> +	scoped_guard(mutex, &cfg->config_mutex)
> +		return ally_set_gamepad_mode(ally, hdev, cfg->gamepad_mode);
> +}
> +

[Severity: Medium]
Does the driver fail to restore the user-configured gamepad mode during a
reset resume?

The documentation for ally_apply_gamepad_mode() explicitly outlines its
design intent for restoring the mode after a reset resume, but it appears
unwired in hid_asus_ally_init(), which only restores the Xbox controller mode:

drivers/hid/hid-asus.c:hid_asus_ally_init() {
    ...
	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;
}

Without a call to ally_apply_gamepad_mode() here, if the system suspends,
the MCU powers off and loses its gamepad mode setting. On resume, the hardware
reverts to its default mode while the sysfs driver state (and cfg->gamepad_mode)
incorrectly claims the user's previously configured mode is still active.

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